libcontainer: cgroups: fs: fix innerPath #552
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 <[email protected]>
Ensure that path safety is maintained, this essentially reapplies c0cad6a ("cgroups: fs: fix cgroup.Parent path sanitisation"), which was accidentally removed in 256f3a8 ("Add support for CgroupsPath field"). Signed-off-by: Aleksa Sarai <[email protected]>
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 <[email protected]>
|
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
5 participants
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 [email protected]
/cc @crosbymichael @LK4D4