Repository navigation
fix: deny attach and refuse protocol upgrades in all three (#76) - #79
Merged
Merged
Conversation
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.
Description
Closes #76.
Security fix. Through the Go proxy, a client could write to any container's stdin with a websocket attach:
GET /containers/{name}/attach/ws?stdin=1&stream=1plusUpgrade: websocket. The router treated it as a read (GET passthrough), andhttputil.ReverseProxyrelayed the101 Switching Protocols. With a shell as the container's process, that is equivalent to exec. It worked with and without--readonly. Rust and TS forwarded the request too, but in testing no stdin got through (Rust's connection broke after relaying101; TS timed out).What changes
attachas a whole segment after/containers/{name}, for every method, withattach is not allowed. That covers/attach,/attach/wsand version-prefixed forms. A container namedattach-runneris unaffected; a container literally namedattachcan no longer be addressed by name (the same trade-off asexec, documented).Upgradeheader, or aConnectionheader containing theupgradetoken (case-insensitive, comma-separated, across repeated headers), gets 403protocol upgrade not allowedand an audit entry. No endpoint can be turned into a raw bidirectional stream. The Docker CLI upgrades only for attach, exec and BuildKit sessions, all of which are denied, so legitimate use is unaffected;logs -fandeventsstream without upgrading.'upgrade'listener, so Node delivers upgrade requests to the normal request handler; a real-http.Servertest pins that.spec/router.qntgainsALLOW_ATTACH,isAttachPath, the independently written propertyattachNeverAllowed, rows inrouter, androuter_pre76(attach allowed) to show the old gap.attachandwsjoinWORDS; existing instances are unchanged.spec/docker_socket_policy.qnt's attach rows becomemethod: "*"plus/containers/:name/attach/ws. The upgrade refusal is header-level and not modelled (noted inspec/README.md, like the query string).Review notes
bump=patch tag=v0.3.9./_ping,/version,/info,/eventsallow any method;CONNECThandled unevenly). Predates this PR; not a tunnel.Verification
routerwithALLOW_ATTACH = truefailsattachSoundTestand the GET, HEAD and named-container rows.attachPostDeniedstill passes because POST attach was already default-denied; it is documented as a pin.d110b8a), the new tests fail; andmain's Go code with this branch's integration suite:HTTP/1.1 403 Forbidden(protocol upgrade not allowed), and nothing reaches the container. Before the fix, Go delivered the frame.make test-allGo 110, Rust 142, TS 160 (1 skipped);make lint-all,make test-spec(8 runs),make verifypass; integrationALL 53 TESTS PASSEDfor Go, Rust and TS; socket suiteALL 15 TESTS PASSED.Type of change
Implementation(s) changed
Testing
make test-all)make verify,make test-spec)Checklist