From 06f4cbbcda0739ae78c96c5f26e3e72b60f92a4d Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 24 Nov 2023 18:21:01 +0100 Subject: [PATCH 1/4] Add (temporary) fix for missing null bytes in Apple JWS signatures Apparently the Apple macOS (and iOS?) ACME client seems to omit leading null bytes from JWS signatures. The base64-url encoded bytes decode to a shorter byte slice than what the JOSE library expects (e.g. 63 bytes instead of 64 bytes for ES256), and then results in a `jose.ErrCryptoFailure`. This commit retries verification of the JWS in case the first verification fails with `jose.ErrCryptoFailure`. The signatures are checked to be of the correct length, and if not, null bytes are prepended to the signature. Then verification is retried, which might fail again, but for other reasons. On success, the payload is returned. Apple should fix this in their ACME client, but in the meantime this commit prevents some "bad request" error cases from happening. --- acme/api/middleware.go | 50 +++++++++++++++++++++++++- acme/api/middleware_test.go | 70 ++++++++++++++++++++++++++++++++++++- 2 files changed, 118 insertions(+), 2 deletions(-) diff --git a/acme/api/middleware.go b/acme/api/middleware.go index ab2ab908..2e8c5b91 100644 --- a/acme/api/middleware.go +++ b/acme/api/middleware.go @@ -1,6 +1,7 @@ package api import ( + "bytes" "context" "crypto/rsa" "errors" @@ -422,11 +423,20 @@ func verifyAndExtractJWSPayload(next nextHTTP) nextHTTP { render.Error(w, acme.NewError(acme.ErrorMalformedType, "verifier and signature algorithm do not match")) return } + payload, err := jws.Verify(jwk) - if err != nil { + switch { + case errors.Is(err, jose.ErrCryptoFailure): + payload, err = retryVerificationWithPatchedSignatures(jws, jwk) + if err != nil { + render.Error(w, acme.WrapError(acme.ErrorMalformedType, err, "error verifying jws with patched signature(s)")) + return + } + case err != nil: render.Error(w, acme.WrapError(acme.ErrorMalformedType, err, "error verifying jws")) return } + ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{ value: payload, isPostAsGet: len(payload) == 0, @@ -436,6 +446,44 @@ func verifyAndExtractJWSPayload(next nextHTTP) nextHTTP { } } +// retryVerificationWithPatchedSignatures retries verification of the JWS using +// the JWK by patching the JWS signatures if they're determined to be too short. +// +// Generally this shouldn't happen, but we've observed this to be the case with +// the macOS ACME client, which seems to omit (at least one) leading null byte(s). +// The error returned is `square/go-jose: error in cryptographic primitive`, which +// is a sential error that hides the details of the actual underlying error, which +// is as follows: `square/go-jose: invalid signature size, have 63 bytes, wanted 64`, +// for ES256. +func retryVerificationWithPatchedSignatures(jws *jose.JSONWebSignature, jwk *jose.JSONWebKey) ([]byte, error) { + patchSignatures(jws) + return jws.Verify(jwk) +} + +// patchSignatures patches the signatures in a JWS if it finds signatures that +// are too short for the signature algorithm. +func patchSignatures(jws *jose.JSONWebSignature) { + for i, s := range jws.Signatures { + var expectedSize int + alg := strings.ToUpper(s.Header.Algorithm) + switch alg { + case jose.ES256: + expectedSize = 64 + case jose.ES384: + expectedSize = 96 + case jose.ES512: + expectedSize = 132 + default: + // other cases are (currently) ignored; TODO(hs): need other cases too? + continue + } + if diff := expectedSize - len(s.Signature); diff > 0 { + s.Signature = append(bytes.Repeat([]byte{0x00}, diff), s.Signature...) + jws.Signatures[i] = s + } + } +} + // isPostAsGet asserts that the request is a PostAsGet (empty JWS payload). func isPostAsGet(next nextHTTP) nextHTTP { return func(w http.ResponseWriter, r *http.Request) { diff --git a/acme/api/middleware_test.go b/acme/api/middleware_test.go index 90190bc7..e0b04a1f 100644 --- a/acme/api/middleware_test.go +++ b/acme/api/middleware_test.go @@ -6,6 +6,7 @@ import ( "crypto" "encoding/base64" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -14,9 +15,10 @@ import ( "strings" "testing" - "github.com/pkg/errors" "github.com/smallstep/assert" "github.com/smallstep/certificates/acme" + tassert "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.step.sm/crypto/jose" "go.step.sm/crypto/keyutil" ) @@ -577,6 +579,38 @@ func TestHandler_verifyAndExtractJWSPayload(t *testing.T) { }, } }, + "ok/apple-acmeclient-omitting-leading-null-bytes-in-signature": func(t *testing.T) test { + appleNullByteCaseKey := []byte(`{ + "kid": "2eIRaNCmST4CwcOHyRSrGO-7k4TAr9fdNk0_2qw4-ps", + "crv": "P-256", + "alg": "ES256", + "kty": "EC", + "x": "v_Twh1khKHS32OJF7Bhzr3DVExbC54bRokOE9wdweRY", + "y": "-IWh-AXzmzh1Mg9OMuWfzje24wLPJmU6AwERo86zk3k" + }`) + appleNullByteCaseJWK := &jose.JSONWebKey{} + err = json.Unmarshal(appleNullByteCaseKey, appleNullByteCaseJWK) + require.NoError(t, err) + appleNullByteCaseBody := `{"protected":"eyJhbGciOiJFUzI1NiIsImp3ayI6eyJjcnYiOiJQLTI1NiIsImt0eSI6IkVDIiwieCI6InZfVHdoMWtoS0hTMzJPSkY3Qmh6cjNEVkV4YkM1NGJSb2tPRTl3ZHdlUlkiLCJ5IjoiLUlXaC1BWHptemgxTWc5T011V2Z6amUyNHdMUEptVTZBd0VSbzg2emszayJ9LCJub25jZSI6ImVERTRjMnhPVDIxQ1lUWnNjMjl4ZFhjNVNFcEdVM0ZRVkc5SGEzRjBSMVEiLCJ1cmwiOiJodHRwczovL2FhZTItMTYzLTE1OC00NS00OS5uZ3Jvay1mcmVlLmFwcC9hY21lL21kYS9uZXctYWNjb3VudCJ9","payload":"eyJ0ZXJtc09mU2VydmljZUFncmVlZCI6dHJ1ZX0","signature":"57RgynZOnBrkx8UIrXZD80Lfql1IitFjBqFoiz2YOew2_oCb8JLx2C2RFp1AGQmP-3p6KP8e3GKb4anZVrdf"}` + appleNullByteCaseJWS, err := jose.ParseJWS(appleNullByteCaseBody) + require.NoError(t, err) + ctx := context.WithValue(context.Background(), jwsContextKey, appleNullByteCaseJWS) + ctx = context.WithValue(ctx, jwkContextKey, appleNullByteCaseJWK) + return test{ + ctx: ctx, + statusCode: 200, + next: func(w http.ResponseWriter, r *http.Request) { + p, err := payloadFromContext(r.Context()) + tassert.NoError(t, err) + if tassert.NotNil(t, p) { + tassert.Equal(t, []byte(`{"termsOfServiceAgreed":true}`), p.value) + tassert.False(t, p.isPostAsGet) + tassert.False(t, p.isEmptyJSON) + } + w.Write(testBody) + }, + } + }, } for name, run := range tests { tc := run(t) @@ -1695,3 +1729,37 @@ func TestHandler_checkPrerequisites(t *testing.T) { }) } } + +func Test_patchSignatures(t *testing.T) { + key := []byte(`{ + "kid": "2eIRaNCmST4CwcOHyRSrGO-7k4TAr9fdNk0_2qw4-ps", + "crv": "P-256", + "alg": "ES256", + "kty": "EC", + "x": "v_Twh1khKHS32OJF7Bhzr3DVExbC54bRokOE9wdweRY", + "y": "-IWh-AXzmzh1Mg9OMuWfzje24wLPJmU6AwERo86zk3k" + }`) + jwk := &jose.JSONWebKey{} + err := json.Unmarshal(key, jwk) + require.NoError(t, err) + body := `{"protected":"eyJhbGciOiJFUzI1NiIsImp3ayI6eyJjcnYiOiJQLTI1NiIsImt0eSI6IkVDIiwieCI6InZfVHdoMWtoS0hTMzJPSkY3Qmh6cjNEVkV4YkM1NGJSb2tPRTl3ZHdlUlkiLCJ5IjoiLUlXaC1BWHptemgxTWc5T011V2Z6amUyNHdMUEptVTZBd0VSbzg2emszayJ9LCJub25jZSI6ImVERTRjMnhPVDIxQ1lUWnNjMjl4ZFhjNVNFcEdVM0ZRVkc5SGEzRjBSMVEiLCJ1cmwiOiJodHRwczovL2FhZTItMTYzLTE1OC00NS00OS5uZ3Jvay1mcmVlLmFwcC9hY21lL21kYS9uZXctYWNjb3VudCJ9","payload":"eyJ0ZXJtc09mU2VydmljZUFncmVlZCI6dHJ1ZX0","signature":"57RgynZOnBrkx8UIrXZD80Lfql1IitFjBqFoiz2YOew2_oCb8JLx2C2RFp1AGQmP-3p6KP8e3GKb4anZVrdf"}` + jws, err := jose.ParseJWS(body) + require.NoError(t, err) + tests := []struct { + name string + jws *jose.JSONWebSignature + jwk *jose.JSONWebKey + }{ + {"ok/patched", jws, jwk}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + patchSignatures(tt.jws) + tassert.Len(t, tt.jws.Signatures[0].Signature, 64) + + data, err := tt.jws.Verify(tt.jwk) + tassert.NoError(t, err) + tassert.Equal(t, []byte(`{"termsOfServiceAgreed":true}`), data) + }) + } +} From 113491e7afc42a1fbdffa2964e97712fa58a22f7 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 24 Nov 2023 18:29:22 +0100 Subject: [PATCH 2/4] Remove TODO for patching other algorithms for Apple ACME client --- acme/api/middleware.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acme/api/middleware.go b/acme/api/middleware.go index 2e8c5b91..5bdc2e86 100644 --- a/acme/api/middleware.go +++ b/acme/api/middleware.go @@ -474,7 +474,7 @@ func patchSignatures(jws *jose.JSONWebSignature) { case jose.ES512: expectedSize = 132 default: - // other cases are (currently) ignored; TODO(hs): need other cases too? + // other cases are (currently) ignored continue } if diff := expectedSize - len(s.Signature); diff > 0 { From 26a3bb3c11d021652a97788ef0e5cac5e3ba6514 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Wed, 29 Nov 2023 02:30:28 +0100 Subject: [PATCH 3/4] Make the Apple JWS fix more robust and catch more cases. --- acme/api/middleware.go | 94 ++++++++++++++++++++++----- acme/api/middleware_test.go | 124 +++++++++++++++++++++++++++++------- 2 files changed, 179 insertions(+), 39 deletions(-) diff --git a/acme/api/middleware.go b/acme/api/middleware.go index 5bdc2e86..257635f1 100644 --- a/acme/api/middleware.go +++ b/acme/api/middleware.go @@ -1,7 +1,6 @@ package api import ( - "bytes" "context" "crypto/rsa" "errors" @@ -452,20 +451,23 @@ func verifyAndExtractJWSPayload(next nextHTTP) nextHTTP { // Generally this shouldn't happen, but we've observed this to be the case with // the macOS ACME client, which seems to omit (at least one) leading null byte(s). // The error returned is `square/go-jose: error in cryptographic primitive`, which -// is a sential error that hides the details of the actual underlying error, which +// is a sentinel error that hides the details of the actual underlying error, which // is as follows: `square/go-jose: invalid signature size, have 63 bytes, wanted 64`, // for ES256. -func retryVerificationWithPatchedSignatures(jws *jose.JSONWebSignature, jwk *jose.JSONWebKey) ([]byte, error) { - patchSignatures(jws) - return jws.Verify(jwk) -} - -// patchSignatures patches the signatures in a JWS if it finds signatures that -// are too short for the signature algorithm. -func patchSignatures(jws *jose.JSONWebSignature) { - for i, s := range jws.Signatures { +func retryVerificationWithPatchedSignatures(jws *jose.JSONWebSignature, jwk *jose.JSONWebKey) (data []byte, err error) { + originalSignatureValues := make([][]byte, len(jws.Signatures)) + patched := false + defer func() { + if patched && err != nil { + for i, sig := range jws.Signatures { + sig.Signature = originalSignatureValues[i] + jws.Signatures[i] = sig + } + } + }() + for i, sig := range jws.Signatures { var expectedSize int - alg := strings.ToUpper(s.Header.Algorithm) + alg := strings.ToUpper(sig.Header.Algorithm) switch alg { case jose.ES256: expectedSize = 64 @@ -477,11 +479,73 @@ func patchSignatures(jws *jose.JSONWebSignature) { // other cases are (currently) ignored continue } - if diff := expectedSize - len(s.Signature); diff > 0 { - s.Signature = append(bytes.Repeat([]byte{0x00}, diff), s.Signature...) - jws.Signatures[i] = s + + switch diff := expectedSize - len(sig.Signature); diff { + case 0: + // expected length; nothing to do; will result in just doing the + // same verification (as done before calling this function) again, + // and thus an error will be returned. + continue + case 1: + patched = true + original := make([]byte, expectedSize-diff) + copy(original, sig.Signature) + originalSignatureValues[i] = original + + patchedR := make([]byte, expectedSize) + copy(patchedR[0:1], []byte{0x00}) + copy(patchedR[1:], original[0:expectedSize-diff]) + sig.Signature = patchedR + jws.Signatures[i] = sig + + // verify it with a patched R; return early if successful; continue + // with patching S if not. + data, err = jws.Verify(jwk) + if err == nil { + return + } + + patchedS := make([]byte, expectedSize) + halfSize := expectedSize / 2 + copy(patchedS[:halfSize], original[:halfSize]) + copy(patchedS[halfSize:expectedSize/2+1], []byte{0x00}) + copy(patchedS[halfSize+1:], original[halfSize:]) + sig.Signature = patchedS + jws.Signatures[i] = sig + case 2: + // assumption is currently the Apple case, in which only the + // first null byte of R and/or S are removed, and thus not a case in + // which two first bytes of either R or S are removed. + patched = true + original := make([]byte, expectedSize-diff) + copy(original, sig.Signature) + originalSignatureValues[i] = original + + patchedRS := make([]byte, expectedSize) + halfSize := expectedSize / 2 + copy(patchedRS[0:1], []byte{0x00}) + copy(patchedRS[1:halfSize], original[0:halfSize-1]) + copy(patchedRS[halfSize:halfSize+1], []byte{0x00}) + copy(patchedRS[halfSize+1:], original[halfSize-1:expectedSize-2]) + sig.Signature = patchedRS + jws.Signatures[i] = sig + default: + // Technically, there can be multiple null bytes in either R or S, + // so when the difference is larger than 2, there is more than one + // option to pick. Apple's ACME client seems to only cut off the + // first null byte of either R or S, so we don't do anything in this + // case. Will result in just doing the same verification (as done + // before calling this function) again, and thus an error will be + // returned. + // TODO(hs): log this specific case? It might mean some other ACME + // client is doing weird things. + continue } } + + data, err = jws.Verify(jwk) + + return } // isPostAsGet asserts that the request is a PostAsGet (empty JWS payload). diff --git a/acme/api/middleware_test.go b/acme/api/middleware_test.go index e0b04a1f..230041d5 100644 --- a/acme/api/middleware_test.go +++ b/acme/api/middleware_test.go @@ -471,7 +471,7 @@ func TestHandler_verifyAndExtractJWSPayload(t *testing.T) { err: acme.NewErrorISE("jwk expected in request context"), } }, - "fail/verify-jws-failure": func(t *testing.T) test { + "fail/verify-jws-failure-wrong-jwk": func(t *testing.T) test { _jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) _pub := _jwk.Public() @@ -483,6 +483,33 @@ func TestHandler_verifyAndExtractJWSPayload(t *testing.T) { err: acme.NewError(acme.ErrorMalformedType, "error verifying jws: square/go-jose: error in cryptographic primitive"), } }, + "fail/verify-jws-failure-too-many-signatures": func(t *testing.T) test { + newParsedJWS, err := jose.ParseJWS(raw) + assert.FatalError(t, err) + newParsedJWS.Signatures = append(newParsedJWS.Signatures, newParsedJWS.Signatures...) + ctx := context.WithValue(context.Background(), jwsContextKey, newParsedJWS) + ctx = context.WithValue(ctx, jwkContextKey, pub) + return test{ + ctx: ctx, + statusCode: 400, + err: acme.NewError(acme.ErrorMalformedType, "error verifying jws: square/go-jose: too many signatures in payload; expecting only one"), + } + }, + "fail/apple-acmeclient-omitting-leading-null-byte-in-signature-with-wrong-jwk": func(t *testing.T) test { + _jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) + assert.FatalError(t, err) + _pub := _jwk.Public() + appleNullByteCaseBody := `{"payload":"dGVzdC0xMTA1","protected":"eyJhbGciOiJFUzI1NiJ9","signature":"rQPYKYflfKnlgBKqDeWsJH2TJ6iHAnou7sFzXlmYD4ArXqLfYuqotWERKrna2wfzh0pu7USWO2gzlOqRK9qq"}` + appleNullByteCaseJWS, err := jose.ParseJWS(appleNullByteCaseBody) + require.NoError(t, err) + ctx := context.WithValue(context.Background(), jwsContextKey, appleNullByteCaseJWS) + ctx = context.WithValue(ctx, jwkContextKey, &_pub) + return test{ + ctx: ctx, + statusCode: 400, + err: acme.NewError(acme.ErrorMalformedType, "error verifying jws: square/go-jose: error in cryptographic primitive"), + } + }, "fail/algorithm-mismatch": func(t *testing.T) test { _pub := *pub clone := &_pub @@ -579,19 +606,19 @@ func TestHandler_verifyAndExtractJWSPayload(t *testing.T) { }, } }, - "ok/apple-acmeclient-omitting-leading-null-bytes-in-signature": func(t *testing.T) test { + "ok/apple-acmeclient-omitting-leading-null-byte-in-signature": func(t *testing.T) test { appleNullByteCaseKey := []byte(`{ - "kid": "2eIRaNCmST4CwcOHyRSrGO-7k4TAr9fdNk0_2qw4-ps", + "kid": "uioinbiTlJICL0MYsb6ar1totfRA2tiPqWgntF8xUdo", "crv": "P-256", "alg": "ES256", "kty": "EC", - "x": "v_Twh1khKHS32OJF7Bhzr3DVExbC54bRokOE9wdweRY", - "y": "-IWh-AXzmzh1Mg9OMuWfzje24wLPJmU6AwERo86zk3k" + "x": "wlz-Kv9X0h32fzLq-cogls9HxoZQqV-GuWxdb2MCeUY", + "y": "xzP6zRrg_jynYljZTxfJuql_QWtdQR6lpJ52q_6Vavg" }`) appleNullByteCaseJWK := &jose.JSONWebKey{} err = json.Unmarshal(appleNullByteCaseKey, appleNullByteCaseJWK) require.NoError(t, err) - appleNullByteCaseBody := `{"protected":"eyJhbGciOiJFUzI1NiIsImp3ayI6eyJjcnYiOiJQLTI1NiIsImt0eSI6IkVDIiwieCI6InZfVHdoMWtoS0hTMzJPSkY3Qmh6cjNEVkV4YkM1NGJSb2tPRTl3ZHdlUlkiLCJ5IjoiLUlXaC1BWHptemgxTWc5T011V2Z6amUyNHdMUEptVTZBd0VSbzg2emszayJ9LCJub25jZSI6ImVERTRjMnhPVDIxQ1lUWnNjMjl4ZFhjNVNFcEdVM0ZRVkc5SGEzRjBSMVEiLCJ1cmwiOiJodHRwczovL2FhZTItMTYzLTE1OC00NS00OS5uZ3Jvay1mcmVlLmFwcC9hY21lL21kYS9uZXctYWNjb3VudCJ9","payload":"eyJ0ZXJtc09mU2VydmljZUFncmVlZCI6dHJ1ZX0","signature":"57RgynZOnBrkx8UIrXZD80Lfql1IitFjBqFoiz2YOew2_oCb8JLx2C2RFp1AGQmP-3p6KP8e3GKb4anZVrdf"}` + appleNullByteCaseBody := `{"payload":"dGVzdC0xMTA1","protected":"eyJhbGciOiJFUzI1NiJ9","signature":"rQPYKYflfKnlgBKqDeWsJH2TJ6iHAnou7sFzXlmYD4ArXqLfYuqotWERKrna2wfzh0pu7USWO2gzlOqRK9qq"}` appleNullByteCaseJWS, err := jose.ParseJWS(appleNullByteCaseBody) require.NoError(t, err) ctx := context.WithValue(context.Background(), jwsContextKey, appleNullByteCaseJWS) @@ -603,7 +630,7 @@ func TestHandler_verifyAndExtractJWSPayload(t *testing.T) { p, err := payloadFromContext(r.Context()) tassert.NoError(t, err) if tassert.NotNil(t, p) { - tassert.Equal(t, []byte(`{"termsOfServiceAgreed":true}`), p.value) + tassert.Equal(t, []byte(`test-1105`), p.value) tassert.False(t, p.isPostAsGet) tassert.False(t, p.isEmptyJSON) } @@ -1730,36 +1757,85 @@ func TestHandler_checkPrerequisites(t *testing.T) { } } -func Test_patchSignatures(t *testing.T) { - key := []byte(`{ - "kid": "2eIRaNCmST4CwcOHyRSrGO-7k4TAr9fdNk0_2qw4-ps", +func Test_retryVerificationWithPatchedSignatures(t *testing.T) { + patchedRKey := []byte(`{ + "kid": "uioinbiTlJICL0MYsb6ar1totfRA2tiPqWgntF8xUdo", "crv": "P-256", "alg": "ES256", "kty": "EC", - "x": "v_Twh1khKHS32OJF7Bhzr3DVExbC54bRokOE9wdweRY", - "y": "-IWh-AXzmzh1Mg9OMuWfzje24wLPJmU6AwERo86zk3k" + "x": "wlz-Kv9X0h32fzLq-cogls9HxoZQqV-GuWxdb2MCeUY", + "y": "xzP6zRrg_jynYljZTxfJuql_QWtdQR6lpJ52q_6Vavg" }`) - jwk := &jose.JSONWebKey{} - err := json.Unmarshal(key, jwk) + patchedRJWK := &jose.JSONWebKey{} + err := json.Unmarshal(patchedRKey, patchedRJWK) require.NoError(t, err) - body := `{"protected":"eyJhbGciOiJFUzI1NiIsImp3ayI6eyJjcnYiOiJQLTI1NiIsImt0eSI6IkVDIiwieCI6InZfVHdoMWtoS0hTMzJPSkY3Qmh6cjNEVkV4YkM1NGJSb2tPRTl3ZHdlUlkiLCJ5IjoiLUlXaC1BWHptemgxTWc5T011V2Z6amUyNHdMUEptVTZBd0VSbzg2emszayJ9LCJub25jZSI6ImVERTRjMnhPVDIxQ1lUWnNjMjl4ZFhjNVNFcEdVM0ZRVkc5SGEzRjBSMVEiLCJ1cmwiOiJodHRwczovL2FhZTItMTYzLTE1OC00NS00OS5uZ3Jvay1mcmVlLmFwcC9hY21lL21kYS9uZXctYWNjb3VudCJ9","payload":"eyJ0ZXJtc09mU2VydmljZUFncmVlZCI6dHJ1ZX0","signature":"57RgynZOnBrkx8UIrXZD80Lfql1IitFjBqFoiz2YOew2_oCb8JLx2C2RFp1AGQmP-3p6KP8e3GKb4anZVrdf"}` - jws, err := jose.ParseJWS(body) + patchedRBody := `{"payload":"dGVzdC0xMTA1","protected":"eyJhbGciOiJFUzI1NiJ9","signature":"rQPYKYflfKnlgBKqDeWsJH2TJ6iHAnou7sFzXlmYD4ArXqLfYuqotWERKrna2wfzh0pu7USWO2gzlOqRK9qq"}` + patchedR, err := jose.ParseJWS(patchedRBody) require.NoError(t, err) + + patchedSKey := []byte(`{ + "kid": "PblXsnK59uTiF5k3mmAN2B6HDPPxqBL_4UGhEG8ZO6g", + "crv": "P-256", + "alg": "ES256", + "kty": "EC", + "x": "T5aM_TOSattXNeUkH1VHZXh8URzdjZTI2zLvVgI0cy0", + "y": "Lf8h8qZnURXIxm6OnQ69kxGC91YtTZRD2GAroEf1UA8" + }`) + patchedSJWK := &jose.JSONWebKey{} + err = json.Unmarshal(patchedSKey, patchedSJWK) + require.NoError(t, err) + patchedSBody := `{"payload":"dGVzdC02Ng","protected":"eyJhbGciOiJFUzI1NiJ9","signature":"krtSKSgVB04oqx6i9QLeal_wZSnjV1_PSIM3AubT0WRIxnhl_yYbVpa3i53p3dUW56TtP6_SUZboH6SvLHMz"}` + patchedS, err := jose.ParseJWS(patchedSBody) + require.NoError(t, err) + + patchedRSKey := []byte(`{ + "kid": "U8BmBVbZsNUawvhOomJQPa6uYj1rdxCPQWF_nOLVsc4", + "crv": "P-256", + "alg": "ES256", + "kty": "EC", + "x": "Ym0l3GMS6aHBLo-xe73Kub4kafnOBu_QAfOsx5y-bV0", + "y": "wKijX9Cu67HbK94StPcI18WulgRfIMbP2ZU7gQuf3-M" + }`) + patchedRSJWK := &jose.JSONWebKey{} + err = json.Unmarshal(patchedRSKey, patchedRSJWK) + require.NoError(t, err) + patchedRSBody := `{"payload":"dGVzdC05MDY3","protected":"eyJhbGciOiJFUzI1NiJ9","signature":"2r_My19oRg7mWf9I5JTkNYp8otfEMz-yXRA8ltZTAKZxyJLurpVEgicmNItu7lfcCrGrTgI3Obye_gSaIyc"}` + patchedRS, err := jose.ParseJWS(patchedRSBody) + require.NoError(t, err) + + patchedRWithWrongJWK, err := jose.ParseJWS(patchedRBody) + require.NoError(t, err) + tests := []struct { - name string - jws *jose.JSONWebSignature - jwk *jose.JSONWebKey + name string + jws *jose.JSONWebSignature + jwk *jose.JSONWebKey + expectedData []byte + expectedSignature string + expectedError error }{ - {"ok/patched", jws, jwk}, + {"ok/patched-r", patchedR, patchedRJWK, []byte(`test-1105`), `AK0D2CmH5Xyp5YASqg3lrCR9kyeohwJ6Lu7Bc15ZmA-AK16i32LqqLVhESq52tsH84dKbu1EljtoM5TqkSvaqg`, nil}, + {"ok/patched-s", patchedS, patchedSJWK, []byte(`test-66`), `krtSKSgVB04oqx6i9QLeal_wZSnjV1_PSIM3AubT0WQASMZ4Zf8mG1aWt4ud6d3VFuek7T-v0lGW6B-kryxzMw`, nil}, + {"ok/patched-rs", patchedRS, patchedRSJWK, []byte(`test-9067`), `ANq_zMtfaEYO5ln_SOSU5DWKfKLXxDM_sl0QPJbWUwAApnHIku6ulUSCJyY0i27uV9wKsatOAjc5vJ7-BJojJw`, nil}, + {"fail/patched-r-wrong-jwk", patchedRWithWrongJWK, patchedRSJWK, nil, `rQPYKYflfKnlgBKqDeWsJH2TJ6iHAnou7sFzXlmYD4ArXqLfYuqotWERKrna2wfzh0pu7USWO2gzlOqRK9qq`, errors.New("square/go-jose: error in cryptographic primitive")}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - patchSignatures(tt.jws) - tassert.Len(t, tt.jws.Signatures[0].Signature, 64) + expectedSignature, decodeErr := base64.RawURLEncoding.DecodeString(tt.expectedSignature) + require.NoError(t, decodeErr) + + data, err := retryVerificationWithPatchedSignatures(tt.jws, tt.jwk) + if tt.expectedError != nil { + tassert.EqualError(t, err, tt.expectedError.Error()) + tassert.Equal(t, expectedSignature, tt.jws.Signatures[0].Signature) + tassert.Empty(t, data) + return + } - data, err := tt.jws.Verify(tt.jwk) tassert.NoError(t, err) - tassert.Equal(t, []byte(`{"termsOfServiceAgreed":true}`), data) + tassert.Len(t, tt.jws.Signatures[0].Signature, 64) + tassert.Equal(t, expectedSignature, tt.jws.Signatures[0].Signature) + tassert.Equal(t, tt.expectedData, data) }) } } From 405aae798c14dc9f46f1b315113c3f447e55c482 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 30 Nov 2023 14:27:32 +0100 Subject: [PATCH 4/4] Simplify the `copy` logic used when patching JWS signature --- acme/api/middleware.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/acme/api/middleware.go b/acme/api/middleware.go index 257635f1..7bd6c0a9 100644 --- a/acme/api/middleware.go +++ b/acme/api/middleware.go @@ -493,8 +493,7 @@ func retryVerificationWithPatchedSignatures(jws *jose.JSONWebSignature, jwk *jos originalSignatureValues[i] = original patchedR := make([]byte, expectedSize) - copy(patchedR[0:1], []byte{0x00}) - copy(patchedR[1:], original[0:expectedSize-diff]) + copy(patchedR[1:], original) // [0x00, R.0:31, S.0:32], for expectedSize 64 sig.Signature = patchedR jws.Signatures[i] = sig @@ -507,9 +506,8 @@ func retryVerificationWithPatchedSignatures(jws *jose.JSONWebSignature, jwk *jos patchedS := make([]byte, expectedSize) halfSize := expectedSize / 2 - copy(patchedS[:halfSize], original[:halfSize]) - copy(patchedS[halfSize:expectedSize/2+1], []byte{0x00}) - copy(patchedS[halfSize+1:], original[halfSize:]) + copy(patchedS, original[:halfSize]) // [R.0:32], for expectedSize 64 + copy(patchedS[halfSize+1:], original[halfSize:]) // [R.0:32, 0x00, S.0:31] sig.Signature = patchedS jws.Signatures[i] = sig case 2: @@ -523,10 +521,8 @@ func retryVerificationWithPatchedSignatures(jws *jose.JSONWebSignature, jwk *jos patchedRS := make([]byte, expectedSize) halfSize := expectedSize / 2 - copy(patchedRS[0:1], []byte{0x00}) - copy(patchedRS[1:halfSize], original[0:halfSize-1]) - copy(patchedRS[halfSize:halfSize+1], []byte{0x00}) - copy(patchedRS[halfSize+1:], original[halfSize-1:expectedSize-2]) + copy(patchedRS[1:], original[:halfSize-1]) // [0x00, R.0:31], for expectedSize 64 + copy(patchedRS[halfSize+1:], original[halfSize-1:]) // [0x00, R.0:31, 0x00, S.0:31] sig.Signature = patchedRS jws.Signatures[i] = sig default: