fix: nil pointer dereference DoS via invalid DPoP htu URL#4112
fix: nil pointer dereference DoS via invalid DPoP htu URL#4112
Conversation
reinkrul
requested review from
gerardsn,
stevenvegt and
woutslakhorst
as code owners
2 new issues
|
…URLs url.Parse returns (nil, error) for strings with invalid percent-encoded sequences (e.g. "%zz"). strip() ignored the error and dereferenced the nil pointer, causing a server panic. Two-layer fix in crypto/dpop/dpop.go: - Parse() now validates the htu claim is a parseable URL, so tokens with a malformed htu are rejected early with a clear error message. - strip() signature changed to (string, error) so any remaining parse failure is propagated through Match() rather than causing a nil deref. Adds a unit test (TestParseDPoP/invalid_htu_claim_URL) that covers the regression: a DPoP token with htu="%zz" is rejected at Parse() time. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
reinkrul
force-pushed
the
fix/dpop-url-parse-nil-deref
branch
from
65c2686 to
64ea568
Compare
|
Coverage Impact ⬇️ Merging this pull request will decrease total coverage on Modified Files with Diff Coverage (1)
🤖 Increase coverage with AI coding...🚦 See full report on Qlty Cloud » 🛟 Help
|
There was a problem hiding this comment.
Pull request overview
This PR hardens DPoP parsing/matching against malformed htu values that previously could trigger a nil-pointer dereference panic via url.Parse, preventing a DoS on the validation endpoint.
Changes:
- Add
htuURL validation duringdpop.Parse()to reject invalid percent-encoding early. - Change
strip()to return an error and propagate it throughDPoP.Match()instead of ignoring parse failures. - Add a regression test covering invalid
htuURL input.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
crypto/dpop/dpop.go |
Validates htu URL in Parse(), makes strip() return (string, error), and handles parse errors in Match(). |
crypto/dpop/dpop_test.go |
Adds a regression test for invalid htu percent-encoding (%zz). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
A non-string htu or htm claim (e.g. a JSON number or object) would pass the empty-string check but then panic on the v.(string) type assertion, reintroducing a nil/panic vector. Validate the type first so any non-string value is rejected with a clear error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
A non-string htu or htm claim (e.g. a JSON number or object) would pass the empty-string check but then panic on the v.(string) type assertion, reintroducing a nil/panic vector. Validate the type first so any non-string value is rejected with a clear error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… validation (#4117) * Initial plan * refactor: rename shadowing s variables to htu/htm in Parse() Co-authored-by: reinkrul <1481228+reinkrul@users.noreply.github.com> Agent-Logs-Url: https://github.com/nuts-foundation/nuts-node/sessions/a3558c07-9536-44cd-b10a-74d497ac71f4 --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: reinkrul <1481228+reinkrul@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
crypto/dpop/dpop.go—url.Parsereturns(nil, error)for inputs with invalid percent-encoded sequences (e.g.%zz).strip()ignored the error and immediately wrote to the nil pointer, causing a server panic. Fixed by validating thehtuclaim inParse()(reject early) and changingstrip()to propagate the error throughMatch().Attack vector (before fix)
An attacker with access to the internal API crafts a DPoP JWT with
htu: "%zz", signs it with their own key, and POSTs it toPOST /internal/auth/v2/dpop/validate.dpop.Parse()accepted the token (the onlyhtuguard was!= ""), and the subsequent call toMatch()→strip()→url.Parse("%zz")→nil→ panic. The connection was reset; the server survived due tonet/http's per-connection recover, but the endpoint could be degraded by repeated calls.🤖 Generated with Claude Code