On Thu, 20 Jun 2024 13:47:31 +0100 "Richard W.M. Jones" <rjones(a)redhat.com> wrote:On Thu, Jun 20, 2024 at 02:12:53PM +0200, Stefano Brivio wrote: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. 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. -- StefanoOn Thu, 20 Jun 2024 12:30:54 +0100 "Richard W.M. Jones" <rjones(a)redhat.com> wrote:While I was testing this, I found we do seem to check it: https://passt.top/passt/tree/conf.c#n1446On Wed, May 29, 2024 at 12:35:24PM +1000, David Gibson wrote:Yes, please, that, or a patch :)On Wed, May 22, 2024 at 10:59:10PM +0200, Stefano Brivio wrote: > 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 in turn > encourage users to change permissions or ownerships: definitely a bad > idea in terms of security. > > Reported-by: Minxi Hou <mhou(a)redhat.com> > Reported-by: Richard W.M. Jones <rjones(a)redhat.com> > Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> 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. 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 the 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.Yes, agreed. It seems as if the latest passt code still does this. Do you want me to open a bug about it?