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" <rjones(a)redhat.com> wrote: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()).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.On 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: > 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?Yes, please, that, or a patch :)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