sec(dind): gate --security-opt seccomp/apparmor=unconfined behind allow_privileged - #174
Merged
Conversation
…ow_privileged HostConfig.SecurityOpt was applied by a loop that ran regardless of dind.allow_privileged, so a sibling container could strip its own seccomp filter with `docker run --security-opt seccomp=unconfined` on a host configured to deny the elevation stack. Same for apparmor=unconfined. The gate checked Privileged and CapAdd only; the later loop applied a field the gate never looked at. Extend the existing gate rather than adding a second one. elevatingSecurityOpt is now the single answer to "does this --security-opt ask for elevation?" and both checkPrivilegedGate and the applier (securityOptSpecOpts) read it, so refusing and applying cannot drift apart again. Both the key=value and the legacy key:value spellings are covered because the applier accepted both. Behaviour with allow_privileged = true is unchanged: the options still reach the OCI spec, which is what setup-buildx's container driver needs. Every other --security-opt value is still ignored rather than refused. The 403 names the option, the sandbox it removes, and the config knob that governs it, and it still runs after request-shape validation so a malformed create keeps its 400. Fixes #172
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #172. Live on the fleet today with
dind.allow_privileged = false, on nodes running public-repo CI.The bug
checkPrivilegedGateinspected onlyPrivilegedandCapAdd. TheSecurityOptloop ran unconditionally insideif req.HostConfig != nil, mappingseccomp=unconfined→oci.WithSeccompUnconfinedandapparmor=…:unconfined→oci.WithApparmorProfile(""). Nothing further down re-checked it, andcheckPrivilegedGateis the only elevation gate on the create path —checkWindowsSiblingGateis a platform check that runs after.So a sibling could strip its own seccomp filter while the host was configured to deny the elevation stack.
The fix — one predicate, so gate and applier cannot diverge
That divergence was the bug: a gate checking one field while a later loop applied another. So:
elevatingSecurityOpt(opt) (mechanism string, elevating bool)— the single answer to "does this ask for elevation?", handling both=and legacy:spellings.checkPrivilegedGatescanshc.SecurityOptthrough it, so there is still exactly one gate.securityOptSpecOpts(), switching on the same predicate's output.403 body matches the existing privileged-gate shape — what, why, which knob:
With
allow_privileged = true, behaviour is byte-identical to before, including which values are ignored.The audit — and the reassuring part
The issue was found by inspection, so the obvious worry was more of the same. There is not.
hostConfigdecodes only 11 fields. Everything else in Docker'sHostConfigJSON is discarded at decode time —Devices,Sysctls,Ulimits,PidMode,IpcMode,UsernsMode,CapDrop,MaskedPaths,ReadonlyPaths,GroupAdd,CgroupParent,ShmSize,Runtime,OomScoreAdjare not fields on the struct and cannot reach the OCI spec at all. The severity of #172 stands as filed; it does not escalate.Field by field:
PrivilegedCapAddWithAddedCapabilitiesSecurityOptBindsbuildBindMountsTmpfswithTmpfsMountCgroupnsModeWithNamespacedCgroupforprivateNetworkModehost/none/container:<id>not honoured; the sibling always gets its own netns. Nooci.WithHostNamespaceanywhere inpkg/dind. Fail-safe.PortBindingsRestartPolicy,InitAdjacent paths checked and clean:
execCreateRequestdoes not decodePrivileged, sodocker exec --privilegedis a silent no-op;pkg/buildkit/server.go:233setsEntitlements: nil, soRUN --security=insecureandnetwork.hostare refused by BuildKit itself.Found, not fixed
--tmpfsoptions replace thenosuid,nodevdefault wholesale —--tmpfs /x:rw,exec,suid,devyields a mount with neither. Matches real Docker's semantics and impact is capped by the deny-all device cgroup, but it is a user-controlled reduction of a sandbox property on an unprivileged container. Low severity; worth its own issue.User: {UID:0, GID:0}and nilCapabilities. Filed separately as it is unverified and needs a Linux host to settle.--device,--pid=host,--sysctl,--cap-drop,--security-opt no-new-privileges=true, …) all succeed and do nothing. Fail-safe in the security direction — including the cases where the job asked for more hardening — but confusing.Tests
+253 lines in
pkg/dind/privileged_gate_test.go, following the house pattern. Both keys × both spellings, offender not at index 0, 10 unrelated-value cases (label=disable,no-new-privileges=true,seccomp=/path/profile.json,apparmor=docker-default,seccomp=unconfined-ish, empty), open-gate still-works, and a malformed-request test proving 400 still beats 403.Proven non-vacuous by four mutations, each reverted:
SecurityOptcase "seccomp"never matchesAppliesElevationWhenAskedFAILs — the "still works when open" assertion is real, not just a status checkkey:valuefallbackgo test -count=1 ./pkg/dind/... ./pkg/config/...ok on Windows.GOOS=linux go build ./...andGOOS=linux go vet ./pkg/dind/...exit 0 (which typechecks the linux-only test files); Linux execution not available on this host.Merge note
Overlaps #173 in
pkg/dind/containers.goandpkg/dind/dind.go. The diff was deliberately kept inside the gate region (~lines 210–290) and clear of the bind-staging code, so the two should merge without a fight.