mkdirall regression when default ACL is set

[lifubang]

If we use a bundle dir in tmp fs to start a container, it will fail when we mount a dir to the container.
The error msg is:

ERRO[0000] runc run failed: unable to start container process: error during container init: error mounting "/opt" to rootfs at "/foo": create mountpoint for /foo mount: possible attack detected: newly created directory "/tmp/ubuntu/rootfs/foo" has incorrect mode 0o40754 (expected 0o40755)

The /tmp mount options:

mount | grep /tmp
/dev/sda1 on /tmp type ext4 (rw,relatime)

[lifubang]

@cyphar PTAL

[cyphar]

Hmm, I can't reproduce this. Can you share the config you used? What is your umask? If you manually do mkdir -p in your tmpfs, what modes do you get?

We could probably loosen the checks to permit the created directory to be more strict than what we requested (i.e. the mode has fewer bits set than the expected set) but I'd like to know where the issue is coming from.

[cyphar]

Though it should be noted that if we mkdirall on vfat you will get errors either way. Maybe we should just skip checking the mode -- the owner checks are probably the main thing we care about in practice.

[lifubang]

You can use GitHub codespace online webide, then run ‘bats . /tests/integration/run.bats’, it can reproduce easily.

[kolyshkin]

You can use GitHub codespace online webide, then run ‘bats . /tests/integration/run.bats’, it can reproduce easily.

Reproduced.

It's not tmpfs in there, it's ext4.

$ sudo bats tests/integration/run.bats 
...
 ✗ runc run [execve error]
   (in test file tests/integration/run.bats, line 276)
     `[[ ${lines[0]} = "exec /run.sh: no such file or directory" ]]' failed
   runc spec (status=0):
   
   runc run test_hello (status=1):
   time="2024-09-22T19:23:04Z" level=error msg="runc run failed: unable to start container process: error during container init: error mounting \"proc\" to rootfs at \"/proc\": possible attack detected: newly created directory \"/tmp/bats-run-Uf8Lsc/runc.YGDOUv/bundle/rootfs/proc\" has incorrect mode 0o40754 (expected 0o40755)"
...
$ umask
0022
$ grep /tmp /proc/self/mountinfo 
762 753 8:1 /containerTmp /tmp rw,relatime - ext4 /dev/sda1 rw
$ ls -ld /tmp
drwxr-xrwt+ 4 root root 4096 Sep 22 19:29 /tmp
$ lsattr -d /tmp
--------------e----- /tmp
$ getfacl  /tmp
getfacl: Removing leading '/' from absolute path names
# file: tmp
# owner: root
# group: root
# flags: --t
user::rwx
group::r-x
other::rwx
default:user::rwx
default:group::r-x
default:other::rw-

I believe this is because of default:other::rw- ACL entry. If I remove it, tests run just fine:

$ sudo setfacl -k /tmp
$ sudo bats tests/integration/run.bats 
run.bats
 ✓ runc run
 ✓ runc run --keep
 ✓ runc run --keep (check cgroup exists)
 ✓ runc run [hostname domainname]
 ✓ runc run with tmpfs
 ✓ runc run with tmpfs perms
 ✓ RUNC_DMZ=true runc run [runc-dmz]
 ✓ RUNC_DMZ=true runc run [cap_sys_ptrace -> /proc/self/exe clone]
 ✓ runc run [/proc/self/exe clone]
 ✓ runc run [joining existing container namespaces]
 ✓ RUNC_DMZ=true runc run [exec error]
 ✓ runc run [execve error]

12 tests, 0 failures

[kolyshkin]

To reproduce locally, run sudo setfacl -m default:other::rw- /tmp

[cyphar]

Grr, ACLs... Yeah, we should probably just skip the mode checks given that this is the second bug we've had. I'm currently travelling, I'll write a patch next week.

[kolyshkin]

We could probably loosen the checks to permit the created directory to be more strict than what we requested (i.e. the mode has fewer bits set than the expected set)

Yes, I think this is the way to go

Maybe we should just skip checking the mode

Or downgrade it to a warning.

[cyphar]

We can also drop the umask logic entirely if we only care if there are extra bits set.