From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id 91A125A0082 for ; Tue, 14 Feb 2023 17:22:55 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1676391774; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=q5Ae0nJP1Xe9YqS9qRRtPDLzyXJJCwj9hvOG6TmYBIE=; b=STETrZUSxTJMykaeK/blNHSGAEYbzfHj6MjkrWjmPwnbHJ5KyPxr0RbzGmlHZX5OHiijjk jRPkjQP0IAxYqO5od3fu/iS7ZDYg1GuTsDah97rzhqDR6Ar+hEcisGs2nYQMsbFHFEvUeC ugJ+NOUPyf9+te4SjFSnc5HflHc7lls= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-656-O8mZAYQYPvywkCbxcKMeGQ-1; Tue, 14 Feb 2023 11:22:53 -0500 X-MC-Unique: O8mZAYQYPvywkCbxcKMeGQ-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id D80F7811E9C for ; Tue, 14 Feb 2023 16:22:52 +0000 (UTC) Received: from maya.cloud.tilaa.com (unknown [10.33.32.3]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8C6511121318; Tue, 14 Feb 2023 16:22:52 +0000 (UTC) Date: Tue, 14 Feb 2023 17:22:39 +0100 From: Stefano Brivio To: Michal =?UTF-8?B?UHLDrXZvem7DrWs=?= Subject: Re: [PATCH 4/4] qemu_passt: Don't let passt fork off Message-ID: <20230214172239.269f1b63@elisabeth> In-Reply-To: <56000087-8df0-294c-6f42-57983e15d383@redhat.com> References: <5abfc412e4692a38e980c8dc600e1bfbd03ddcfd.1676374699.git.mprivozn@redhat.com> <20230214140253.49bbc13a@elisabeth> <56000087-8df0-294c-6f42-57983e15d383@redhat.com> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: KWWAIP2JDLJRR6L2AAQKU2R6NIWN22QT X-Message-ID-Hash: KWWAIP2JDLJRR6L2AAQKU2R6NIWN22QT X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: libvir-list@redhat.com, passt-dev@passt.top X-Mailman-Version: 3.3.3 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Tue, 14 Feb 2023 16:30:17 +0100 Michal Pr=C3=ADvozn=C3=ADk wrote: > On 2/14/23 14:02, Stefano Brivio wrote: > > On Tue, 14 Feb 2023 12:51:22 +0100 > > Michal Privoznik wrote: > > =20 > >> When passt starts it tries to do some security measures to > >> restrict itself. For instance, it creates its own namespaces, > >> umounts basically everything, drops capabilities, forks off to > >> further restrict itself (the child is where all interesting work > >> takes place now). This is sound, except it's causing two > >> problems: > >> > >> 1) the PID file FD, which we leak into the passt process, gets > >> closed (and thus our virPidFile*() helpers see unlocked PID > >> file, which makes them think the process is gone), =20 > >=20 > > I didn't realise this was the case, but giving passt write (unless I'm > > missing something) access to a file created by libvirtd doesn't look > > desirable to me. =20 >=20 > That's not what's happening here. In fact, the opposite. We let libvirt > create the file, lock and write it. Okay, but the outcome is exactly what I'm saying. You're using advisory locking, what prevents passt from writing (a lot of bytes) to it? I just followed the path from open() to fcntl(), maybe I'm missing something. > Then the file's FD is leaked into > passt process. This way, we can learn whether the PID file is still > valid: if we just try to lock it and: >=20 > a) fail, then the file is still locked, i.e. passt is still running, > b) succeed, then the file is no longer locket by passt, i.e. it's no > longer running. >=20 > Thing is, passt doesn't even know about the FD (unless it starts closing > FDs "randomly", e.g. via closefrom(3) or an alternative). But it can also use it for write(2), as long as the attacker knows (quite obvious information) that libvirt is starting passt. The number of that file descriptor is deterministic, too. > This is the > reason we can't use passt's --pid, because it writes the PID file > without any locking dance. Therefore, libvirt can't know whether the PID > file is still valid. If it did just read the file and killed PID from > there it might kill a random process that was unfortunate to be assigned > the same PID. I don't think that's enough, passt could pass a copy of that file descriptor to another process (via e.g. SCM_RIGHTS), which then locks it. It's enough to cover the unfortunate random process, but not something done intentionally. That's the reason why pidfd_open(2) and CLONE_PIDFD were introduced. > BTW, there are two other issues with PID files in passt: > 1) no locking means, that the file is very easily overwritten: >=20 > $ passt --pid /tmp/passt.pid 2>/dev/null & \ > passt --pid /tmp/passt.pid 2>/dev/null; \ > pgrep passt ; cat /tmp/passt.pid > [1] 21029 > [1]+ Done passt --pid /tmp/passt.pid 2> /dev/null > 21034 > 21035 > 21035 It can be overwritten even with advisory locking, and mandatory locking isn't implemented in recent kernels. I'm not aware of any way to prevent this (and I wouldn't want to give the illusion that this is a "secure" mechanism, either). > 2) with --daemonize, only the PID of the parent process is written which > is pretty much useless, as the parent quits shortly after clone(). No, that's the PID of the child (of course!), see passt's __daemon() (util.c). It's written by the parent, sure, because the child doesn't know its own "real" PID. > Now, libvirt's virPidFile* machinery (src/util/virpidfile.c) ensures > that case 1) won't happen (yes, we have internal APIs that read pid > files unlocked, but those are not used from this code we're talking about= ). >=20 > And this patch tries to fix the case 2) by instructing passt to not > clone(). If it means that it can't use PID namespace, then so be it. > It's better to not let processes behind. Even though 2) is nothing you need to fix, I'd still suggest to *not* use that option *for this purpose*, because it's prone to races. > >> 2) the PID file no longer reflects true PID of the process. > >> > >> Worse, the child calls setsid() so we can't even kill the whole > >> process group. I mean, we can but it won't be any good. > >> > >> Fortunately, passt has '--foreground' argument, which causes it > >> to undergo the same security measures but without forking off the > >> child. =20 > >=20 > > They're not the same -- unfortunately they can't be, because, on Linux, > > you can't change the PID of an existing process, so there's no way to > > enter a new PID namespace without clone(). > >=20 > > If passt remains in the same PID namespace, it's still able to see PIDs > > of other processes, which is not desirable from a security perspective.= =20 >=20 > I understand that, but it's already confined in plenty other ways. It's > way worse to leave a process running than being able to see other PIDs. Not if somebody manages to find a creative way to attack other processes by arbitrary code execution in passt. In any case, there's no need to leave any process running. > > Again from a security perspective, this is probably a small impact, so > > I guess it's fine if there's no other way around it. But I see a lot of > > ways around it... >=20 > Is there? Yes, see my email on the other thread, archived at: https://listman.redhat.com/archives/libvir-list/2023-February/237741.html That is: just connect() and close() the socket if qemu doesn't do it. That's not more or less reliable than expecting passt to do the right thing when you give --foreground. > If we have a helper process that fork()-s then the only way we > can 'track' its PIDs (and kill them) is by placing them into a CGroup. > Until we do that we have to trust helper processes to behave properly. > But that's something I'd like to avoid. By the way, you need in any case to rely on --one-off as qemu terminates, because passt should run even without libvirtd running, as long as the guest is connected (for details about this reasoning, see https://bugzilla.redhat.com/show_bug.cgi?id=3D1597326). --=20 Stefano