Re: [PATCH 7/8] conf, passt, tap: Open socket and PID files before switching UID/GID

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