From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id DC1EB5A004F for ; Fri, 21 Jun 2024 03:02:48 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1718931765; bh=192Mm9cI65yGx+/luGhm5a5CPH7ZkH1u+Uj/jbh1h68=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=WNHMTNcgzKLLJ1ieBmD/vb0cgQFSUHiMcr915kuMAbuAmVaUC5OoUXCAZw4btv6ft HPF7nLxYPXW40IdKSM/2jiGWx05TmAOSK57c8utdzUONpCn5tOVKukRgwXlBjb6h2o IpyxpZdJ3U/2XvLT+Iv3X6gmeHOoD/JYSCGMx4I/Vxq9QdBJ0Q7Ahy/HzorIH+2qt3 jcUnyFLXb9DqmeFmD/GwYD1cNm2CANikIk9pMKaib3zQI+GDrLm6squi3TlzKBLPnT AdwRRTH88GFmD6S4SJnW1vw/4DxHNYOkJ+NYlfavxuCiSsXDro3voTdH8ePHg4i+3F 9OSQPmmrJf+bw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4W4zc545k7z4wc5; Fri, 21 Jun 2024 11:02:45 +1000 (AEST) Date: Fri, 21 Jun 2024 11:02:38 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 7/8] conf, passt, tap: Open socket and PID files before switching UID/GID Message-ID: References: <20240522205911.261325-1-sbrivio@redhat.com> <20240522205911.261325-8-sbrivio@redhat.com> <20240620113054.GB1450@redhat.com> <20240620141253.7e6d3855@elisabeth> <20240620124730.GC1450@redhat.com> <20240620162212.09ccc2f9@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="pV1Gf8dxr7tYD0Ts" Content-Disposition: inline In-Reply-To: <20240620162212.09ccc2f9@elisabeth> Message-ID-Hash: L5RB55WSVA55CHN4VAN5YINOK7N3W7EJ X-Message-ID-Hash: L5RB55WSVA55CHN4VAN5YINOK7N3W7EJ X-MailFrom: dgibson@gandalf.ozlabs.org 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: "Richard W.M. Jones" , passt-dev@passt.top, Minxi Hou 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: --pV1Gf8dxr7tYD0Ts Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jun 20, 2024 at 04:22:12PM +0200, Stefano Brivio wrote: > On Thu, 20 Jun 2024 13:47:31 +0100 > "Richard W.M. Jones" wrote: >=20 > > On Thu, Jun 20, 2024 at 02:12:53PM +0200, Stefano Brivio wrote: > > > On Thu, 20 Jun 2024 12:30:54 +0100 > > > "Richard W.M. Jones" wrote: > > > =20 > > > > On Wed, May 29, 2024 at 12:35:24PM +1000, David Gibson wrote: =20 > > > > > On Wed, May 22, 2024 at 10:59:10PM +0200, Stefano Brivio wrote: = =20 > > > > > > Otherwise, if the user runs us as root, and gives us paths that= are > > > > > > only accessible by root, we'll fail to open them, which might i= n turn > > > > > > encourage users to change permissions or ownerships: definitely= a bad > > > > > > idea in terms of security. > > > > > >=20 > > > > > > Reported-by: Minxi Hou > > > > > > Reported-by: Richard W.M. Jones > > > > > > Signed-off-by: Stefano Brivio =20 > > > > >=20 > > > > > Looking at this I did notice a pre-existing, well, maybe not bug > > > > > exactly, but possibly surprising behaviour, which makes me a > > > > > bit more nervous now that we can invoke it as root. > > > > >=20 > > > > > tap_sock_unix_open() will silently truncate the given socket path= to > > > > > the maximum length for a Unix socket. Which means we could bind(= ), > > > > > but also unlink() a path that's not exactly the same as the one t= he > > > > > one the user requested. I don't immediately see a way to exploit > > > > > that, but it's the sort of thing that makes me nervous. I think = we > > > > > should instead outright fail if the given socket path is too long= =2E =20 > > > >=20 > > > > Yes, agreed. > > > >=20 > > > > It seems as if the latest passt code still does this. Do you want = me > > > > to open a bug about it? =20 > > >=20 > > > Yes, please, that, or a patch :) =20 > >=20 > > While I was testing this, I found we do seem to check it: > >=20 > > https://passt.top/passt/tree/conf.c#n1446 >=20 > Oh, I thought David was referring to the loop in tap_sock_unix_open(), > where we try paths in the form "/tmp/passt_%i.socket". But even there, > we can't exceed UNIX_PATH_MAX. Actually I was referring to the memcpy() from sock_path to path in tap_sock_unix_open(). I hadn't thought to check if we'd previously verified the length of sock_path. So, yeah we seem to be ok here (although it's not obvious that's the case from within the code of tap_sock_unix_open()). > One minor issue remains, though: in conf(), we refuse paths that are > longer than UNIX_SOCK_MAX (100). That's the maximum index for the > "/tmp/passt_%i.socket", it happens to be a sane value, but we should use > UNIX_PATH_MAX (108) instead. I'll fix it, but wait for David's feedback > first. Well, apart from that, yes. --=20 David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson --pV1Gf8dxr7tYD0Ts Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmZ00S0ACgkQzQJF27ox 2GeitA//boFwOH1ylPJFjBo6YFr8HARfoWpmPW8Gu7tSbCGKeUBcl2KMQBo/wMit lz118jgJjHjCOicuRrx0qX8JYjege/d/PcEtTriMWkUmlTXbxosrmLl09nZ9dXx5 wKscSu32OmE49NJqXqPMYQAJPr48taw4xePtPEQg9i5fjjSY3vIOAAKmQRGqRI00 syifpYRq6IpnQ19tLQh2ro5nepAxBinXxIgSBSz1BcSJgNZXtS5Yzp4ntc2zyR6c JeoW4Wb9CSPrDGG53iyYABe7DRzmd7NJdAhwd95L+5y0taa5NOF9/CtQqTtlmWn3 tG/PTw+/NduKMyOUj0N2nE2jxp5FKCS9L+AZmIyIhOC/Lb+jQdvyuq4VWMtAi+cx Yyr6VCAGN5cuUehRNvfjtsbw/wZJCEqofwPGIRfvRl//CmTfBJkcndSssEFoggrN bFlFyNdxNL6Q6C/2h5TL0oblMV6oAKSLdG7X4npIypLwt0Cr9zSeP3SA9fBbTpWD augRsGzgKwYDhN5LDefYqJtWfJHK494XaZwTc+MBU3FL8PLIenKA+wU7YAdq/q/4 Y62mkaiIBdJ+AO6ABY+l47OPcL8y+H0AN2aalBvCMHSRFDAcK0L4d9JphzxVEbMs okI6NbvsM1YgGUfQAuudjBEUbIKhubip1Rn3Qc0PaQPQPuU2hSU= =A+wp -----END PGP SIGNATURE----- --pV1Gf8dxr7tYD0Ts--