install does not handle the creation of "drwxr-s---" correctly

[firasuke]

Hey there,

I am unable to create directories with the following permissions drwxr-s---. Normally I run install -dm 02750 directory with coreutils version of install and I get the correct permissions, but with toybox's install I am getting the following permissions instead drwxr-xr-x.

Any ideas what might be the issue here?

[landley]

Hmmm, the -m argument is handled in the install_node() dirtree callback but -d and -D are handled in install_main(). In part because "install -dm +x" hasn't got a previous mode to delta from, so... vs 777? Or vs umask?

Bit sleep deprived to fix this at the moment but I'll try to work out the correct thing to do in the morning. (And add a test.)

[firasuke]

Hmmm, the -m argument is handled in the install_node() dirtree callback but -d and -D are handled in install_main(). In part because "install -dm +x" hasn't got a previous mode to delta from, so... vs 777? Or vs umask?

I see, so they're handled separately.

Bit sleep deprived to fix this at the moment but I'll try to work out the correct thing to do in the morning. (And add a test.)

Yeah, no worries. I can help with the testing if needed.

[landley]

Oops, sorry, got distracted. I'll try to get this in tonight.

[landley]

Did you know that the linux mkdirat() syscall strips permission bits outside 01777? Meaning seting the set group ID bit on a directory (so newly created files in the directory inherit the directory's group rather than the users) is inherently racy.

I can open it (with the no follow symlink bit), stat it to confirm it's a directory (albeit not necessarily the one you just created, or somebody may have done a chown on it in the race window), and then do an fchmod() on that filehandle... except this needs to interact with -g and -o correctly, I think...?

[landley]

Eh, the chown's already racy if you're being security paranoid, and it has to go after the chown because that would reset suid bits anyway. (I tested by hand that "sudo ./toybox install -dm 1750 -g root directory" is both T and root, meaning the chown doesn't strip the sticky bit, but didn't add it to scripts/install.test because it would require root to test.)

Commit 39dea7710fa4.

[firasuke]

Eh, the chown's already racy if you're being security paranoid, and it has to go after the chown because that would reset suid bits anyway. (I tested by hand that "sudo ./toybox install -dm 1750 -g root directory" is both T and root, meaning the chown doesn't strip the sticky bit, but didn't add it to scripts/install.test because it would require root to test.)

Commit 39dea77.

Upon further testing, the issue appears to have been fixed. Thanks!