diff --git a/acme/api/middleware.go b/acme/api/middleware.go index ab2ab908..7bd6c0a9 100644 --- a/acme/api/middleware.go +++ b/acme/api/middleware.go @@ -422,11 +422,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 +445,105 @@ 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 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) (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(sig.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 + continue + } + + 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[1:], original) // [0x00, R.0:31, S.0:32], for expectedSize 64 + 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, 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: + // 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[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: + // 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). 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..230041d5 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" ) @@ -469,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() @@ -481,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 @@ -577,6 +606,38 @@ func TestHandler_verifyAndExtractJWSPayload(t *testing.T) { }, } }, + "ok/apple-acmeclient-omitting-leading-null-byte-in-signature": func(t *testing.T) test { + appleNullByteCaseKey := []byte(`{ + "kid": "uioinbiTlJICL0MYsb6ar1totfRA2tiPqWgntF8xUdo", + "crv": "P-256", + "alg": "ES256", + "kty": "EC", + "x": "wlz-Kv9X0h32fzLq-cogls9HxoZQqV-GuWxdb2MCeUY", + "y": "xzP6zRrg_jynYljZTxfJuql_QWtdQR6lpJ52q_6Vavg" + }`) + appleNullByteCaseJWK := &jose.JSONWebKey{} + err = json.Unmarshal(appleNullByteCaseKey, appleNullByteCaseJWK) + require.NoError(t, err) + 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, 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(`test-1105`), 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 +1756,86 @@ func TestHandler_checkPrerequisites(t *testing.T) { }) } } + +func Test_retryVerificationWithPatchedSignatures(t *testing.T) { + patchedRKey := []byte(`{ + "kid": "uioinbiTlJICL0MYsb6ar1totfRA2tiPqWgntF8xUdo", + "crv": "P-256", + "alg": "ES256", + "kty": "EC", + "x": "wlz-Kv9X0h32fzLq-cogls9HxoZQqV-GuWxdb2MCeUY", + "y": "xzP6zRrg_jynYljZTxfJuql_QWtdQR6lpJ52q_6Vavg" + }`) + patchedRJWK := &jose.JSONWebKey{} + err := json.Unmarshal(patchedRKey, patchedRJWK) + require.NoError(t, err) + 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 + expectedData []byte + expectedSignature string + expectedError error + }{ + {"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) { + 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 + } + + tassert.NoError(t, err) + tassert.Len(t, tt.jws.Signatures[0].Signature, 64) + tassert.Equal(t, expectedSignature, tt.jws.Signatures[0].Signature) + tassert.Equal(t, tt.expectedData, data) + }) + } +}