Fix/vuln reports#290
Merged
Merged
Conversation
… DoS ExpandSpec/ExpandSchema* inline every $ref with no memoization and no global budget. A self-contained spec where each of N definitions references the next one twice (via allOf/anyOf/oneOf/properties/items) drives 2^N schema expansions from O(N) bytes of input. A ~1.5 KB document expands into tens of GB of heap and >30 s of CPU, letting an unauthenticated caller exhaust resources in any service that expands untrusted specs. All refs can be fragment-only, so a restrictive PathLoader is not a mitigation. The expanded output itself is exponential (expansion inlines by value), so a memoization cache would bound CPU but not memory. The only guard that bounds memory is refusing to build the tree past a budget. Add ExpandOptions.MaxExpansionNodes, a per-call cap on the number of expanded schema nodes, enforced by a counter in the shared resolverContext incremented at the top of expandSchema: 0 (zero value): DefaultMaxExpansionNodes (500,000) -- every caller protected <0: unbounded (trusted specs only) >0: explicit cap 500,000 leaves ~10x headroom over the largest real spec we test (the full Kubernetes API expands to ~47,000 nodes) while stopping the attack well before it hurts. The metric is nodes, not $ref resolutions: measurement showed resolution counts do not grow with the attack (document-level caching collapses them), so a resolution cap would miss the blow-up entirely. Exceeding the budget returns ErrExpandTooManyNodes. This is a resource-exhaustion safeguard, so it is terminal even under ContinueOnError, rather than leaving the caller with a silently truncated spec. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
Add ExpandOptions.PathLoaderWithOptions, a document loader matching the func(string, ...loading.Option) (json.RawMessage, error) signature now used by the go-openapi/swag/loading and go-openapi/loads loaders. When set, it takes precedence over PathLoader, which in turn precedes the package-level default. This lets a caller inject an options-aware loader -- for example one confined to a directory with loading.WithRoot to safely resolve $ref targets from an untrusted spec -- directly, without wrapping it in a func(string) adapter closure. The injected loader carries its own loading options; the expander invokes it without adding any, keeping the plumbing free of forwarding logic. The change is additive: existing callers using PathLoader (or the default) are unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
IsValidURI resolved an absolute-URL $ref by issuing http.Get(v) on http.DefaultClient, which has no timeout and no cancellation. On an attacker-controlled URL whose server accepts the connection but never responds, the call blocks indefinitely, leaking goroutines, sockets, and file descriptors. Because go-openapi/validate calls IsValidURI on every $ref of a spec (in validateReferencesValid), an untrusted document full of stalling full-URL refs can exhaust resources during validation (CWE-400/CWE-770). The bare GET to an arbitrary, caller-supplied URL is also an SSRF vector against internal addresses. A method named IsValidURI should validate the reference, not probe remote reachability over the network: that made validation depend on network availability and non-deterministic. Resolving and fetching remote references is the expander's job, through its configurable (and now confinable) document loader. Treat a well-formed absolute URL as a valid URI without any network request. Local file references are still checked on disk. No spec or validate test depended on the probe (validate's fixtures use fragment-only refs). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
Expanding or resolving a specification loads referenced documents through a
pluggable loader that, by default, is unconfined. A "$ref" in an untrusted spec
can therefore read local files ("file:///etc/passwd", "../../secret.json" —
arbitrary file read / path traversal) or reach internal addresses via a remote
"$ref" (SSRF). This is inherent to the default configuration; it is not fixed by
a code change but by using a confined loader.
The mitigation already exists: inject an option-aware loader via
ExpandOptions.PathLoaderWithOptions built with go-openapi/swag/loading options
(loading.WithRoot to confine local reads, loading.WithHTTPClient to restrict
remote fetches), or — recommended — use the restricted loaders from
go-openapi/loads (SpecRestricted / SetRestrictedLoaders), which confine local
reads and reject loopback/private/link-local remote addresses, and whose
confinement applies to every "$ref" resolved during expansion.
Document this in a package-level "Security" section and add pointers to it from
ExpandSpec, ExpandSchemaWithBasePath and ExpandOptions. Also cross-reference
ExpandOptions.MaxExpansionNodes for the related $ref amplification vector.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
swag/loading v0.27.2 makes loading.WithRoot resolve absolute in-root paths (rebasing them onto the root) instead of rejecting every absolute path. Because spec normalizes every $ref to an absolute path against the spec's base, this is what makes a WithRoot-confined loader usable to safely expand an untrusted spec: legitimate in-root references resolve, while file:// and ../ references that escape the root are rejected. Add a consumer-side test that expands an untrusted spec through a PathLoaderWithOptions loader bound to loading.WithRoot, with an absolute RelativeBase: it asserts the in-root reference expands, the escaping references stay unexpanded, and no byte of the out-of-root file leaks into the result. go mod tidy also drops the now-unused swag/jsonname dependency. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
Expanding a spec with the default loader fetches any remote "$ref" through http.DefaultClient with no address filtering, so a "$ref" to a cloud metadata endpoint (e.g. AWS IMDS at 169.254.169.254), a private address, or localhost is an SSRF vector. This is documented in the package "Security" section; the mitigation is to inject an option-aware loader bound to a restricted HTTP client via ExpandOptions.PathLoaderWithOptions (or use the restricted loaders from go-openapi/loads). Add a regression test proving that posture end to end: a loader bound to a client whose DialContext refuses loopback/private/link-local/unspecified addresses causes ExpandSpec to refuse the IMDS reference at dial time, before any connection is made. The loader selection is shared by every expansion and resolution entry point, so this covers ExpandSpec, ExpandSchemaWithBasePath, ExpandResponse, ExpandParameter and the Resolve* functions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
Supersedes the v0.27.2 bump. swag/loading v0.27.3 fixes WithRoot on Windows:
spec normalizes references to a file-URL path form ("/C:/dir/file"), which
v0.27.2 did not recognize as absolute, so os.Root rejected legitimate in-root
targets and TestExpand_ConfinedLoader failed on Windows CI. v0.27.3 normalizes
that form before confinement, so a WithRoot-confined loader resolves in-root
references on Windows too, while still rejecting file:// and ../ escapes.
All go-openapi/swag submodules are moved to v0.27.3 together (stringutils, which
had no v0.27.2 tag, is included). No spec code change is required.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Frederic BIDON <fredbi@yahoo.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #290 +/- ##
==========================================
+ Coverage 67.08% 68.45% +1.37%
==========================================
Files 30 30
Lines 2415 2435 +20
==========================================
+ Hits 1620 1667 +47
+ Misses 626 599 -27
Partials 169 169 ☔ View full report in Codecov by Harness. |
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
1 participant
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.
Change type
Please select: 🆕 New feature or enhancement|🔧 Bug fix'|📃 Documentation update
Short description
Fixes
Full description
Checklist