cgroups: fs: fix cgroup.Parent path sanitisation#451
cgroups: fs: fix cgroup.Parent path sanitisation#451LK4D4 merged 1 commit intoopencontainers:masterfrom cyphar:fix-infinite-recursion
Conversation
libcontainer/cgroups/fs/cpuset.go
Outdated
There was a problem hiding this comment.
In what case would parent equals to current? Did you hit this in real life?
There was a problem hiding this comment.
Yes. It causes Docker to crash in not-very-fun ways. But any path where filepath.Dir(path) == path (such as /) will cause this.
There was a problem hiding this comment.
Can you be more specific, like how can we reproduce this with docker? I still don't know how could filepath.Dir(path) == path and it's not returned by "filepath.Clean(parent) == root".
And I think it'll help to add more comments for such code.
There was a problem hiding this comment.
The case --cgroup-parent=../../../../../hello/this/is/a/crash will cause Docker to crash due to an infinite recursion error (take a look at the upstream vendor commit for a test case). While this is solved with my path cleaning code, it implies that there are base cases not accounted for.
root isn't /, it's just a regular path. So if path is a not a subdirectory of root, this will break. I can make it panic (or return an error) if we ever hit the case that root is a subdirectory of path, if that'll make you happier.
|
Mmm i think we should have test for edge case like this, maybe eventually runc should test itself instead of depending on the docker test suite. For now, can we add a test on the corresponding vendor update in docker ? |
|
@dqminh |
|
@hqhq I've made it emit an error (and have a slightly more descriptive comment). I don't think it makes sense to omit this check, even though the parent path stuff fixes the root cause (this incomplete base case causes lots of versions of Docker to be vulnerable to a DoS, and it's cheap to check the single case). PTAL |
libcontainer/cgroups/fs/apply_raw.go
Outdated
There was a problem hiding this comment.
If path = /a/b/c, absClean(path) will end up with a/b/c, seems not correct.
There was a problem hiding this comment.
Wait what? This isn't the latest version of the commit. One sec.
Properly sanitise the --cgroup-parent path, to avoid potential issues (as it starts creating directories and writing to files as root). In addition, fix an infinite recursion due to incomplete base cases. It might be a good idea to move pathClean to a separate library (which deals with path safety concerns, so all of runC and Docker can take advantage of it). Signed-off-by: Aleksa Sarai <asarai@suse.com>
|
Sorry for the wrong version of the patch. Check it again. /ping @dqminh @mrunalp @vishh @crosbymichael @hqhq |
|
LGTM |
1 similar comment
|
LGTM |
cgroups: fs: fix cgroup.Parent path sanitisation
…dependent specs-go/config: Drop platform-independent comment
Properly sanitise the --cgroup-parent path, to avoid potential issues
(as it starts creating directories and writing to files as root). In
addition, fix an infinite recursion due to incomplete base cases.
Signed-off-by: Aleksa Sarai asarai@suse.com
/cc @security-team @diogomonica