Skip to content

Commit 65c2686

Browse files
reinkrulclaude
andcommitted
fix: handle nil return from url.Parse in DPoP strip() and related sites
url.Parse returns (nil, error) for inputs containing invalid percent-encoded sequences (e.g. "%zz"). Three call-sites ignored the error and immediately used the returned pointer, causing a nil pointer dereference panic. crypto/dpop/dpop.go (strip): - Validate htu claim as a parseable URL in Parse() so malformed tokens are rejected early with a clear error, before Match() is reached. - Change strip() signature to (string, error) and propagate the error through Match() instead of dereferencing a nil *url.URL. auth/api/iam/session.go (CreateRedirectURI / redirectURI): - Added error checks; redirect URIs are validated before being stored so this is a defence-in-depth guard rather than a reachable crash path. vcr/revocation/statuslist2021_issuer.go (statusListURL): - Added nil guard with a clear panic message; the base URL is the node's own public URL validated at startup so the guard will never fire in practice. echo/dpop_strip_dos_test.go: - Regression test that starts a real Nuts node and submits a DPoP proof with an invalid htu URL, verifying the server returns a graceful rejection (HTTP 200, valid=false) instead of panicking and resetting the connection. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent f1a608a commit 65c2686

4 files changed

Lines changed: 146 additions & 12 deletions

File tree

auth/api/iam/session.go

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -175,12 +175,20 @@ type RedirectSession struct {
175175
}
176176

177177
func (s OAuthSession) CreateRedirectURI(params map[string]string) string {
178-
redirectURI, _ := url.Parse(s.RedirectURI)
178+
redirectURI, err := url.Parse(s.RedirectURI)
179+
if err != nil {
180+
// Redirect URIs are validated before being stored in the session, so this should never happen.
181+
return s.RedirectURI
182+
}
179183
r := http.AddQueryParams(*redirectURI, params)
180184
return r.String()
181185
}
182186

183187
func (s OAuthSession) redirectURI() *url.URL {
184-
redirectURL, _ := url.Parse(s.RedirectURI)
188+
redirectURL, err := url.Parse(s.RedirectURI)
189+
if err != nil {
190+
// Redirect URIs are validated before being stored in the session, so this should never happen.
191+
return nil
192+
}
185193
return redirectURL
186194
}

crypto/dpop/dpop.go

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,8 @@ func Parse(s string) (*DPoP, error) {
158158
}
159159
if v, ok := token.Get(HTUKey); !ok || v == "" {
160160
return nil, fmt.Errorf("%w: missing htu claim", ErrInvalidDPoP)
161+
} else if _, err := url.Parse(v.(string)); err != nil {
162+
return nil, fmt.Errorf("%w: invalid htu claim: %w", ErrInvalidDPoP, err)
161163
}
162164
if v, ok := token.Get(HTMKey); !ok || v == "" {
163165
return nil, fmt.Errorf("%w: missing htm claim", ErrInvalidDPoP)
@@ -220,22 +222,31 @@ func (t DPoP) Match(jkt string, method string, url string) (bool, error) {
220222
if method != t.HTM() {
221223
return false, fmt.Errorf("method mismatch, token: %s, given: %s", t.HTM(), method)
222224
}
223-
urlLeft := strip(t.HTU())
224-
urlRight := strip(url)
225+
urlLeft, err := strip(t.HTU())
226+
if err != nil {
227+
return false, fmt.Errorf("invalid htu claim: %w", err)
228+
}
229+
urlRight, err := strip(url)
230+
if err != nil {
231+
return false, fmt.Errorf("invalid url: %w", err)
232+
}
225233
if urlLeft != urlRight {
226234
return false, fmt.Errorf("url mismatch, token: %s, given: %s", urlLeft, urlRight)
227235
}
228236

229237
return true, nil
230238
}
231239

232-
func strip(raw string) string {
233-
url, _ := url.Parse(raw)
234-
url.Scheme = "https"
235-
url.Host = strings.Split(url.Host, ":")[0]
236-
url.RawQuery = ""
237-
url.Fragment = ""
238-
return url.String()
240+
func strip(raw string) (string, error) {
241+
u, err := url.Parse(raw)
242+
if err != nil {
243+
return "", err
244+
}
245+
u.Scheme = "https"
246+
u.Host = strings.Split(u.Host, ":")[0]
247+
u.RawQuery = ""
248+
u.Fragment = ""
249+
return u.String(), nil
239250
}
240251

241252
func (t DPoP) MarshalJSON() ([]byte, error) {

echo/dpop_strip_dos_test.go

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
/*
2+
* Nuts node
3+
* Copyright (C) 2024 Nuts community
4+
*
5+
* This program is free software: you can redistribute it and/or modify
6+
* it under the terms of the GNU General Public License as published by
7+
* the Free Software Foundation, either version 3 of the License, or
8+
* (at your option) any later version.
9+
*
10+
* This program is distributed in the hope that it will be useful,
11+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
12+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13+
* GNU General Public License for more details.
14+
*
15+
* You should have received a copy of the GNU General Public License
16+
* along with this program. If not, see <https://www.gnu.org/licenses/>.
17+
*/
18+
19+
package echo
20+
21+
// TestDPoP_StripNilDerefDoS is a regression test for a nil pointer dereference in
22+
// crypto/dpop.strip() when the DPoP token's htu claim contains an invalid
23+
// percent-encoded sequence (e.g. "%zz"). url.Parse returns (nil, err) for such
24+
// input; without the fix the function dereferenced the nil pointer, causing a panic.
25+
//
26+
// Attack path (before fix):
27+
// Attacker crafts a DPoP JWT with htu="%zz", signs it with their own key,
28+
// and POSTs it to POST /internal/auth/v2/dpop/validate.
29+
// dpop.Parse() accepted the token (htu is non-empty → passes the only check).
30+
// dpopToken.Match() called strip(token.HTU()) → url.Parse("%zz") → nil → PANIC.
31+
//
32+
// Expected outcome (after fix):
33+
// dpop.Parse() rejects the token because the htu value is not a valid URL.
34+
// ValidateDPoPProof returns 200 with Valid=false and a reason string.
35+
36+
import (
37+
"bytes"
38+
"crypto"
39+
"crypto/ecdsa"
40+
"crypto/elliptic"
41+
"crypto/rand"
42+
"encoding/base64"
43+
"encoding/json"
44+
"net/http"
45+
"testing"
46+
47+
"github.com/lestrrat-go/jwx/v2/jwa"
48+
"github.com/lestrrat-go/jwx/v2/jwk"
49+
"github.com/nuts-foundation/nuts-node/crypto/dpop"
50+
"github.com/nuts-foundation/nuts-node/test/node"
51+
"github.com/stretchr/testify/assert"
52+
"github.com/stretchr/testify/require"
53+
)
54+
55+
func TestDPoP_StripNilDerefDoS(t *testing.T) {
56+
internalBaseURL, _, _ := node.StartServer(t)
57+
58+
// Generate an EC key pair. The DPoP spec requires the public key in the JWT header,
59+
// so we need a real key to produce a valid signature that passes dpop.Parse().
60+
keyPair, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
61+
require.NoError(t, err)
62+
63+
// Build a syntactically valid DPoP token using a placeholder URL, then overwrite
64+
// the htu claim with an invalid percent-encoded string. "%zz" is invalid because
65+
// 'z' is not a hexadecimal digit, so url.Parse will return (nil, error).
66+
req, err := http.NewRequest(http.MethodPost, "https://example.com/token", nil)
67+
require.NoError(t, err)
68+
69+
token := dpop.New(*req)
70+
// Overwrite htu with a malformed percent-encoded value.
71+
// dpop.Parse() only checks !ok || v == "", so "%zz" passes that guard.
72+
require.NoError(t, token.Token.Set(dpop.HTUKey, "%zz"))
73+
74+
dpopProof, err := token.Sign("kid", keyPair, jwa.ES256)
75+
require.NoError(t, err)
76+
77+
// Compute the JWK thumbprint that the server will verify. Match() checks the
78+
// thumbprint first; an incorrect value short-circuits before strip() is reached.
79+
pubJWK, err := jwk.FromRaw(keyPair.Public())
80+
require.NoError(t, err)
81+
thumbprintBytes, err := pubJWK.Thumbprint(crypto.SHA256)
82+
require.NoError(t, err)
83+
thumbprint := base64.RawURLEncoding.EncodeToString(thumbprintBytes)
84+
85+
// POST to the internal validate endpoint. Any authenticated caller of the
86+
// internal API (e.g. a resource server) can reach this endpoint.
87+
body, err := json.Marshal(map[string]string{
88+
"dpop_proof": dpopProof,
89+
"method": http.MethodPost,
90+
"thumbprint": thumbprint,
91+
"token": "dummy-access-token",
92+
"url": "https://example.com/token",
93+
})
94+
require.NoError(t, err)
95+
96+
resp, err := http.Post(
97+
internalBaseURL+"/internal/auth/v2/dpop/validate",
98+
"application/json",
99+
bytes.NewReader(body),
100+
)
101+
require.NoError(t, err, "server should not close the connection (no panic)")
102+
defer resp.Body.Close()
103+
104+
// The fix rejects the token at dpop.Parse() time (invalid htu URL), so the
105+
// server returns 200 with Valid=false rather than panicking.
106+
require.Equal(t, http.StatusOK, resp.StatusCode)
107+
var result map[string]interface{}
108+
require.NoError(t, json.NewDecoder(resp.Body).Decode(&result))
109+
assert.Equal(t, false, result["valid"], "valid should be false for an invalid DPoP token")
110+
assert.NotEmpty(t, result["reason"], "a human-readable rejection reason should be present")
111+
}

vcr/revocation/statuslist2021_issuer.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -487,7 +487,11 @@ func (cs *StatusList2021) GetRevocation(credentialID ssi.URI) (*Revocation, erro
487487

488488
func (cs *StatusList2021) statusListURL(issuer did.DID, page int) string {
489489
// https://example.com/statuslist/<did>/page
490-
result, _ := url.Parse(cs.baseURL)
490+
result, err := url.Parse(cs.baseURL)
491+
if err != nil {
492+
// baseURL is the node's own public URL, validated at startup. This should never happen.
493+
panic("invalid statuslist base URL: " + err.Error())
494+
}
491495
return result.JoinPath("statuslist", issuer.String(), strconv.Itoa(page)).String()
492496
}
493497

0 commit comments

Comments
 (0)