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": "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": "v_Twh1khKHS32OJF7Bhzr3DVExbC54bRokOE9wdweRY", - "y": "-IWh-AXzmzh1Mg9OMuWfzje24wLPJmU6AwERo86zk3k" + "x": "T5aM_TOSattXNeUkH1VHZXh8URzdjZTI2zLvVgI0cy0", + "y": "Lf8h8qZnURXIxm6OnQ69kxGC91YtTZRD2GAroEf1UA8" }`) - jwk := &jose.JSONWebKey{} - err := json.Unmarshal(key, jwk) + patchedSJWK := &jose.JSONWebKey{} + err = json.Unmarshal(patchedSKey, patchedSJWK) require.NoError(t, err) - body := `{"protected":"eyJhbGciOiJFUzI1NiIsImp3ayI6eyJjcnYiOiJQLTI1NiIsImt0eSI6IkVDIiwieCI6InZfVHdoMWtoS0hTMzJPSkY3Qmh6cjNEVkV4YkM1NGJSb2tPRTl3ZHdlUlkiLCJ5IjoiLUlXaC1BWHptemgxTWc5T011V2Z6amUyNHdMUEptVTZBd0VSbzg2emszayJ9LCJub25jZSI6ImVERTRjMnhPVDIxQ1lUWnNjMjl4ZFhjNVNFcEdVM0ZRVkc5SGEzRjBSMVEiLCJ1cmwiOiJodHRwczovL2FhZTItMTYzLTE1OC00NS00OS5uZ3Jvay1mcmVlLmFwcC9hY21lL21kYS9uZXctYWNjb3VudCJ9","payload":"eyJ0ZXJtc09mU2VydmljZUFncmVlZCI6dHJ1ZX0","signature":"57RgynZOnBrkx8UIrXZD80Lfql1IitFjBqFoiz2YOew2_oCb8JLx2C2RFp1AGQmP-3p6KP8e3GKb4anZVrdf"}` - jws, err := jose.ParseJWS(body) + 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) }) } }