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: > > > 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: > > > > > > > On Wed, May 29, 2024 at 12:35:24PM +1000, David Gibson wrote: > > > > > 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 > > > > > > Reported-by: Richard W.M. Jones > > > > > > Signed-off-by: Stefano Brivio > > > > > > > > > > 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? > > > > > > Yes, please, that, or a patch :) > > > > While I was testing this, I found we do seem to check it: > > > > https://passt.top/passt/tree/conf.c#n1446 > > 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. -- 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