If container will run as non root user, drop permitted, effective caps early#36587
If container will run as non root user, drop permitted, effective caps early#36587vdemeester merged 1 commit intomoby:masterfrom
Conversation
160252d to
5a2cdb1
Compare
|
Could you add a test for this? Looks like something that's easy to miss if the behaviour changes |
|
Yeah planning to add a test but in the airport so thought i would open the
PR first (the test is going to be a bit messy).
…On 14 Mar 2018 10:12, "Sebastiaan van Stijn" ***@***.***> wrote:
Could you add a test for this? Looks like something that's easy to miss if
the behaviour changes
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#36587 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAdcPKHIPl_VK_pFv3r1m5cvoEXUdzhOks5teO0jgaJpZM4SqI8m>
.
|
daemon/oci_linux.go
Outdated
There was a problem hiding this comment.
So when the user specifies --cap-(add|drop) on the CLI, this is available in the bounding set? I thought that we also wanted the effective set to remain consistent with what was requested from the end-user across the "user context" switch.
There was a problem hiding this comment.
Ah nevermind, I had to read more carefully CAPABILITIES(7). Yes, this seems like the behavior we should have had from the start.
|
LGTM This fixes also all the duplicates of: #27590 I guess |
|
LGTM |
|
windows failure (from here): 06:05:40 FAIL: docker_cli_run_test.go:4345: DockerSuite.TestSlowStdinClosing I hope to fix that one in #36571 (commit 5043639) which was merged recently. @justincormack could you please rebase (for the sake of checking if this one goes away)? |
…s early As soon as the initial executable in the container is executed as a non root user, permitted and effective capabilities are dropped. Drop them earlier than this, so that they are dropped before executing the file. The main effect of this is that if `CAP_DAC_OVERRIDE` is set (the default) the user will not be able to execute files they do not have permission to execute, which previously they could. The old behaviour was somewhat surprising and the new one is definitely correct, but it is not in any meaningful way exploitable, and I do not think it is necessary to backport this fix. It is unlikely to have any negative effects as almost all executables have world execute permission anyway. Use the bounding set not the effective set as the canonical set of capabilities, as effective will now vary. Signed-off-by: Justin Cormack <justin.cormack@docker.com>
5a2cdb1 to
15ff093
Compare
Codecov Report
@@ Coverage Diff @@
## master #36587 +/- ##
=========================================
Coverage ? 34.7%
=========================================
Files ? 612
Lines ? 45413
Branches ? 0
=========================================
Hits ? 15761
Misses ? 27590
Partials ? 2062 |
|
@kolyshkin rebased |
|
SGTM |
|
hmmm... 4 tests failed in CI: None of those seem like related to this change, though. |
|
experimental failure unrelated 👼 let's get this merged 👼 |
Make the same changes for exec as for run in moby#36587. This means that capabilities are set early to how they will be set after exec. This largely affects ability to exec a binary that would not be allowed by a user if `CAP_DAC_OVERRIDE` is not set. Signed-off-by: Justin Cormack <justin.cormack@docker.com>
|
This (with the upgrade to Docker How are you supposed to be able to add capabilities to non-root containers now? I really don't see the relevance of following |
|
@nfnty could you open an issue with details, and a minimal example that broke with this change? |
|
Sure, but maybe there's something I'm missing. Would be great to get an answer to how non-root containers are supposed to handle capabilities now. |
|
IIRC this fix just requires the containers entrypoint to have the executable bit set, whereas previously those permissions were not respected (ie, a binary without executable bit set would still run as if it had the bit set); setting the correct permissions on the executable is now needed (just as would be the case in a regular shell) |
|
No, |
|
ping @justincormack |
|
Hmm, I didn't think non root capabilities worked previously (other than for suid executables); that what ambient capabilities are for which we have not added support for yet. Let me experiment a bit. |
|
@nfnty I am quite confused about how you might be getting this to work, testing on an older Docker I get: Note the |
|
@justincormack Indeed, a combination of I'm currently using |
|
Hmm a minimal simple example with filesystem caps works for me on Docker 18.05.0-ce Dockerfile: So that is running with uid 100 and |
|
(I wouldn't expect this change to affect filesystem capabilities; it is possible some other change has affected round tripping on filesystem capabilities though, are you sure they are in the image at runtime? At one point only some graph drivers would round trip filesystem capabilities, and we also made some changes to the |
|
Thanks. I've pinpointed the problem to |
|
This patch combined with |
|
Yes |
As soon as the initial executable in the container is executed as a non root user,
permitted and effective capabilities are dropped. Drop them earlier than this, so
that they are dropped before executing the file. The main effect of this is that
if
CAP_DAC_OVERRIDEis set (the default) the user will not be able to executefiles they do not have permission to execute, which previously they could.
The old behaviour was somewhat surprising and the new one is definitely correct,
but it is not in any meaningful way exploitable, and I do not think it is
necessary to backport this fix. It is unlikely to have any negative effects as
almost all executables have world execute permission anyway.
Use the bounding set not the effective set as the canonical set of capabilities, as
effective will now vary.
Signed-off-by: Justin Cormack justin.cormack@docker.com