utils: switch to securejoin.MkdirAllHandle #4393
Conversation
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
This includes the MkdirAll and OpenInRoot implementations which are actually secure against races. Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
filepath-securejoin has a bunch of extra hardening features and is very well-tested, so we should use it instead of our own homebrew solution. A lot of rootfs_linux.go callers pass a SecureJoin'd path, which means we need to keep the wrapper helpers in utils, but at least the core logic is no longer in runc. In future we will want to remove this dodgy logic and just use file handles for everything (using libpathrs, ideally). Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
|
@kolyshkin WDYT? This is the simplest way of switching without rewriting a lot of |
| // different umask, we are already locked and there's nothing for us to | ||
| // do -- and if not then it doesn't matter which thread we run on and | ||
| // there's nothing for us to do). | ||
| expectedMode = uint32(unix.S_IFDIR | (mode &^ getUmask())) |
There was a problem hiding this comment.
This may causes a regression for dind now. @cyphar
@lifubang ➜ ~/ubuntu $ sudo /workspaces/runc/runc create test
ERRO[0000] runc create failed: unable to start container process: error during container init: error mounting "proc" to rootfs at "/proc": possible attack detected: newly created directory "/home/codespace/ubuntu/rootfs/proc" has incorrect mode 0o42755 (expected 0o40755)
This is from github codespace online webide, I think it looks like a dind, and it worked before.
There was a problem hiding this comment.
Maybe we should consider the SUID bit.
There was a problem hiding this comment.
Yeah, I noticed this when working on it but assumed that os.MkdirAll had the same issue because it seems suid (and sgid) bits aren't set if you pass them to mkdirat. Let me take a look.
This check isn't the problem though (unless you want to silently ignore suid bits being dropped) -- we pass the requested mode to mkdirat directly so the issue is that mkdirat doesn't set them.
There was a problem hiding this comment.
(Also I don't get why you'd want to set the mode of /proc that way. That seems very strange.)
There was a problem hiding this comment.
No, it’s the default action, I didn’t set anything. It’s also has an error for normal bind mount, if the destination dir is not existing in the container rootfs.
There was a problem hiding this comment.
See from the result, I think ‘mkdirat’ has honored the suid bit even though there is no suid bit passed in. But I don’t know where is the default suid bit come from, maybe from the parent dir?
There was a problem hiding this comment.
Yeah, it seems that only the sgid bit is inherited. TIL. I'll write a patch.
(We also need to fix a proc issue I just noticed...)
There was a problem hiding this comment.
cyphar/filepath-securejoin#25 should fix the issue, I'll send a PR with it applied.
filepath-securejoin has a bunch of extra hardening features and is very
well-tested, so we should use it instead of our own homebrew solution.
A lot of
rootfs_linux.gocallers pass a SecureJoin'd path, which meanswe need to keep the wrapper helpers in utils, but at least the core
logic is no longer in runc. In future we will want to remove this dodgy
logic and just use file handles for everything (using libpathrs,
ideally).
Signed-off-by: Aleksa Sarai cyphar@cyphar.com