libcontainer: cgroups: fs: fix innerPath#552
libcontainer: cgroups: fs: fix innerPath#552crosbymichael merged 3 commits intoopencontainers:masterfrom cyphar:fix-cgroup-path
Conversation
|
Right, so it turns out that the patch I'm fixing also introduced a security vulnerability. I'll include it in this PR. |
|
The Docker suite passes once you apply this PR. Please merge ASAP. Thanks guys. :D |
|
Looks like I forgot to call the cleaning path routine in my last refactoring. Thanks for spotting it! Could you also add a unit test for it within runc? This would avoid having to wait on vendoring into docker to find future regression if any. |
|
Yes, can you add some unit tests for this path logic? |
|
@crosbymichael I've added some (pretty exhaustive IMO) unit tests to make sure we don't run into this again. :P |
| } | ||
|
|
||
| config := &configs.Cgroup{ | ||
| Parent: "../../../../../../../../../../some/path", |
There was a problem hiding this comment.
Missing the initial / to make it absolute
There was a problem hiding this comment.
Nit addressed (in TestInvalidAbsoluteCgroupParent).
Fix m.Path legacy code to actually work. Signed-off-by: Aleksa Sarai <asarai@suse.com>
In order to avoid problems with security regressions going unnoticed, add some unit tests that should make sure security regressions in cgroup path safety cause tests to fail in runC. Signed-off-by: Aleksa Sarai <asarai@suse.com>
|
LGTM |
|
/ping @crosbymichael @mlaventure @LK4D4 |
| cgName := libcontainerUtils.CleanPath(c.Name) | ||
|
|
||
| innerPath := cgPath | ||
| if innerPath == "" { |
There was a problem hiding this comment.
maybe just rename innerPath to cgPath to avoid temp variable
|
LGTM |
libcontainer: cgroups: fs: fix innerPath
improve validate usage message
Fix m.Path legacy code to actually work. This means
we'll be able to finally vendor to Docker. It'd be nice
to merge this ASAP so we can finally merge a bunch
of stuff in Docker.
Fixes #551
Signed-off-by: Aleksa Sarai asarai@suse.com
/cc @crosbymichael @LK4D4