Add depth cap to KDL parser - #554
Merged
Merged
Conversation
The KDL parser is mutually recursive over children blocks:
parseDocument -> parseNode -> parseNodeEntries -> parseDocument
Each { } block added a set of call frames with no depth counter and no
limit, so deeply nested input overflowed the 1GB goroutine stack. A stack
overflow is a fatal error that recover() cannot intercept, so any caller
passing untrusted KDL through kdlReader.Read() could be crashed.
KDL was missed when 4c91d0d added the same caps to the JSON and XML
readers. It now enforces the same 10,000 level limit and returns
ErrKDLMaxDepthExceeded.
Both recursion sites into parseDocument are covered - the children block
and the slashdashed children block, which is reachable via "node /-{ "
and was equally exploitable.
GHSA-v72w-jh9m-fv4r
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 GHSA-v72w-jh9m-fv4r (high, CVSS 7.5, CWE-674). Reported by @vnykmshr.
Problem
The KDL parser is mutually recursive over children blocks:
Each
{ }block added a set of call frames with no depth counter and no limit, so deeply nested input overflowed the 1GB goroutine stack. A stack overflow is a fatal error thatrecover()cannot intercept, so any caller passing untrusted KDL throughkdlReader.Read()orinternal.Parse()could be crashed. No auth or preconditions beyond supplying KDL input.KDL was missed when 4c91d0d added the same depth caps to the JSON and XML readers.
Affected: v3.10.0 (KDL support) through v3.11.2.
Two recursion sites, not one
The advisory named the children block. There are actually two call sites recursing into
parseDocument, and the second is separately reachable:parser.go:156)node {x 2Mfatal error: stack overflowparser.go:193)node /-{x 2Mfatal error: stack overflowCapping only the site named in the advisory would have left the slashdash path as a bypass. Both are capped here, and both have regression tests.
Fix
depthis threaded throughparseDocument->parseNode->parseNodeEntries, with the guard at the top ofparseDocument. This mirrorsdecodeObject/decodeArrayin the JSON reader andparseElementin the XML reader — same 10,000 limit, same error shape.ErrKDLMaxDepthExceededis re-exported fromparsing/kdlbecause the parser lives in aninternalpackage that callers cannot import.Verification
Reproduced both crashes on
masterfirst, with stack traces showing the expectedparseDocumentframes. After the fix both returnkdl nesting depth exceeded, and 10,000 levels still parses.Tests cover: 10,001 children blocks (error), 10,001 slashdashed children blocks (error), exactly 10,000 (succeeds), and ordinary nesting (succeeds).
Adjacent recursion checked
nodesToValue/nodeToValuein the reader andwriteDocument/writeNodein the generator are also mutually recursive, so I checked whether they are separately reachable with deep input. They are not — every reader caps at 10,000: JSON and XML explicitly, YAML via its library (yaml: exceeded max depth of 10000, confirmed by piping 200,000-deep YAML to KDL). Capping the parser bounds the tree, so no further changes were needed.go build,go vet, and the full suite are clean.