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.129.124]) by passt.top (Postfix) with ESMTP id BA2D45A0262 for ; Tue, 13 Jun 2023 05:12:54 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1686625973; 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=n8ZNWhb6z/wwKwjA12hryQbC9+/PVOk2/kLIQ8y/8RE=; b=Z6FG5scE+Gr6ruMja5SF6RM6ub2iIzFQjsA73DO0p0Y426nfQo8jC41oGLdIidJG1sY1sW tfdsvNNipzsC7It4IbFJ/KyFDMe/9L95e+i6MjyOLLLG3csHoEOXP3kwE0DoQnUT99UFII XwBPAR/+Fs/MSDKX2+gHbYktP7EZAMc= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-55-m_rnPFXaMxSpliVNJmk36g-1; Mon, 12 Jun 2023 23:12:52 -0400 X-MC-Unique: m_rnPFXaMxSpliVNJmk36g-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 0DAEC1C068EB for ; Tue, 13 Jun 2023 03:12:52 +0000 (UTC) Received: from elisabeth (unknown [10.39.208.7]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E6FB92956; Tue, 13 Jun 2023 03:12:50 +0000 (UTC) Date: Tue, 13 Jun 2023 05:12:47 +0200 From: Stefano Brivio To: Michal =?UTF-8?B?UHLDrXZvem7DrWs=?= Subject: Re: [PATCH 0/2] Introduce --log-fd option Message-ID: <20230613051247.58707e32@elisabeth> In-Reply-To: <70692bd7-4d5f-5c83-105a-f1110108cdc4@redhat.com> References: <20230606225836.63aecebe@elisabeth> <39b5445f-3126-db92-01cd-60efb2dc25d1@redhat.com> <20230607164212.58afd4d8@elisabeth> <70692bd7-4d5f-5c83-105a-f1110108cdc4@redhat.com> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.5 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: 4LTGTCG6BHDW43VQOAYE5IWL6C3O3FEX X-Message-ID-Hash: 4LTGTCG6BHDW43VQOAYE5IWL6C3O3FEX 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: passt-dev@passt.top X-Mailman-Version: 3.3.8 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 Thu, 8 Jun 2023 13:51:44 +0200 Michal Pr=C3=ADvozn=C3=ADk wrote: > On 6/7/23 16:42, Stefano Brivio wrote: > > On Wed, 7 Jun 2023 13:38:12 +0200 > > Michal Pr=C3=ADvozn=C3=ADk wrote: > > =20 > >> On 6/6/23 22:58, Stefano Brivio wrote: =20 > >>> Hi, > >>> > >>> On Tue, 6 Jun 2023 13:41:28 +0200 > >>> Michal Privoznik wrote: > >>> =20 > >>>> The idea behind this is so that libvirt can create the log file, set= all > >>>> DAC/SELinux labels and just pass pre-opened FD to passt. > >>>> > >>>> You can view my WIP patches for libvirt here: > >>>> > >>>> https://gitlab.com/MichalPrivoznik/libvirt/-/commit/5bbe40087d6888a7= c4031a2224731523e117c072 =20 > >>> > >>> Thanks for the implementation... and also for taking care of the > >>> details! > >>> > >>> I'm not really enthusiastic about the security implications of this > >>> approach, but _if it's the only reasonable way to solve this_, I won'= t > >>> certainly stand in the way of progress. The series looks mostly good = to > >>> me, I have only a few nits, reported in the single replies. > >>> > >>> This adds a further interfacing mode between passt and the parent > >>> process, which makes me a bit uncomfortable in general. > >>> > >>> Specifically, if the parent process runs as root, this gives a rogue > >>> passt process a way to write potentially unlimited amounts of data to > >>> essentially any place (minus _some_ checks implemented by Linux Secur= ity > >>> Modules). And a rogue passt process doesn't necessarily imply a rogue > >>> parent, so this is additional surface. =20 > >> > >> Well, is it any different to when libvirt would create the file, set > >> perms and then let passt open() it? =20 > >=20 > > Yes -- I think it would be appropriate that the file is not opened as > > root, because access control (also on behalf of LSMs) logically happens > > on open(). It's passt using that file, so it should own it and be > > accounted for it (also in terms of disk quota), not libvirt. On the > > other hand: > > =20 > >> In fact, I find passing FD safer because libvirt doesn't need to set > >> up perms/owner and can leave the log file be owned by root:root with > >> 0600 mode, =20 > >=20 > > ...also true, even though: > > =20 > >> or any other user that libvirtd runs under. =20 > >=20 > > in that case, the file would be already, naturally, owned by that > > (non-root) user and created with 0600 permissions anyway. > >=20 > > The issue here in some sense is that libvirtd is commonly (?) running > > as root. To keep it simple, in the Podman integration (for pasta), we > > only enabled pasta (same binary as passt) to be used if Podman also > > runs "rootless". But, oops: > > https://github.com/containers/podman/issues/17840 > >=20 > > This makes me realise another point though: if libvirtd runs as root, > > at least in the current integration, or at least by default (?), passt > > will run with the same user as QEMU (usually "qemu") -- not "nobody". > > And at this point I'm not entirely sure that having a log file owned by > > root is much preferable to having it owned by that "qemu" user. > > =20 > >>> Oh, and by the way of LSMs, we kind of bypass stuff like this. > >>> > >>> For a non-rogue passt process, if the parent runs as root, I don't se= e > >>> any additional attack scenario -- the parent could do anything it wan= ts > >>> anyway. But if the parent runs as regular user, there are a few > >>> additional ways to cause passt to misbehave by passing in a file > >>> descriptor that doesn't correspond to a file, or that's opened by oth= er > >>> processes, without any mediation by the filesystem (which is generall= y > >>> speaking not under control of unprivileged users). =20 > >> > >> So an attacker can cause libvirtd to pass an FD that doesn't belong to > >> the log file opened by libvirtd? =20 > >=20 > > No, no, by "[not] a file" I really meant [not] a regular file (say, a > > TTY), and not necessarily libvirtd -- or a rogue libvirtd. All I'm > > saying is that if you have control over the log file descriptor, > > outside passt, as regular user, you might also get control over the > > behaviour of passt, without any mediation by the filesystem. > > =20 > >> Interesting, I though that's impossible. =20 > >=20 > > Well, strictly speaking it's not, just use a tracer, but then at that > > point one would gain nothing additional from it. > > =20 > >> I mean, it's sort of a goal we're working towards with QEMU > >> - libvirt opens FDs and then just pass them to QEMU. So if it's really > >> unsafe, we should re-evaluate our goal. =20 > >=20 > > To me that also poses the problem that if a LSM policy or VFS-based > > access control forbids QEMU to access a resource, you are effectively > > bypassing that -- and you're also bypassing whatever flag QEMU would > > normally use on open(). But I don't know in which cases this is being > > done and if it's actually a problem (or a bigger problem than the > > solution it offers). > > =20 > >>> I'd rather much prefer the more common approach of defaulting, or > >>> suggesting to the user, to write to a temporary filesystem, available > >>> under most distributions under /run or /var/run. Is that really > >>> unfeasible? =20 > >> > >> It if feasible. I just thought that when users want their logs to resi= de > >> under some special path that libvirtd has access to, but not passt the= n > >> we might use FD passing. =20 > >=20 > > On the other hand (and sure, from a user perspective this is different) > > we don't allow passt to save its PID file or create its socket in > > whatever place. But I see the usability point you're making and I think > > it has its value too. > >=20 > > Long story short, if you think (you know better) that users would > > commonly run libvirtd as root and request that passt writes its log > > file to an arbitrary location, even if we offer a different default > > (including a relative path) or somehow recommend something else, I thin= k > > we should go ahead with this. Otherwise I'm mildly against it. =20 >=20 > I don't think that passt advantage shows when running libvirtd as root. > There are other (better?) ways to provide network connectivity in that > case. Well, the default option is a bridge that offers quite little network isolation. While libvirt's default firewalling rules should prevent any spoofing, they are added on top, instead of being there by design. Same with preventing arbitrary IP (not TCP, not UDP, not ICMP) traffic -- on the other hand that might be a reason to use the bridge, if desired. There are a few other advantages using passt: you don't need NAT, which avoids the need for NDP/ARP proxying (a bit cumbersome with WiFi), and it might be the only way to run some workloads. Performance-wise, the default tap device gets a single queue, and with a few filtering rules (for the bridged tap) passt actually gives better throughput from some quick tests. We're also working on vhost-user support, which should improve the situation further. So, sure, there are valid reasons to use a bridged tap device, but they don't necessarily apply whenever libvirtd (for other reasons) needs to run as root. > The main advantage shows when running unprivileged in which case > the log file is going to be owned by current user (which is the same > user that QEMU will run under). >=20 > > =20 > >>> I'm thinking that libvirt already needs a specific directory for pass= t > >>> to use (socket and PID files). What if logFile, other than an absolut= e > >>> path, supported a relative path? This logic might perhaps apply to > >>> other helpers or external programs too. =20 > >> > >> That's tangent to this problem. =20 > >=20 > > Why? I think this also reflects on usability, and might have some weigh= t > > on the previous point. =20 >=20 > Well, libvirt would firstly need to expose the domain specific path > somewhere, so that users know the prefix that the relative path refers > to. Secondly, libvirt must then NOT remove the temporary path, so that > the log file is preserved even after domain is shut off (e.g. somebody > might be interested in log messages that happen when QEMU disconnects). Right. My only slightly informed opinion would suggest that this is actually more elegant/fitting, but I don't know how complicated it is to implement (especially the first point). > IOW, it is tangent. >=20 > >>> By the way, passt ships with AppArmor and SELinux example policies, > >>> which are also included in packages. They would need at least a quick > >>> review, probably some edits, and some basic tests. Thinking of those,= a > >>> relative path would also simplify things. =20 > >> > >> Ah, completely forgot about those. Yeah, they might need tweaking even > >> if we decide to go this route. =20 > >=20 > > Either way, do you plan to take care of those? I can, but not right > > away. >=20 > Nah, let's discard this. I didn't realize that you want the extra guard > of open(). So I'll just make libvirt create an empty file, set perms on > it and continue using --log-file. Sorry for the extra work, but yes, I think forcing passt to open() the file is quite relevant. > Thanks anyway! And thanks for the tentative patches! --=20 Stefano