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) + }) + } +}