From 06f4cbbcda0739ae78c96c5f26e3e72b60f92a4d Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 24 Nov 2023 18:21:01 +0100 Subject: [PATCH] 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) + }) + } +}