Merge pull request #1631 from smallstep/herman/fix-apple-acmeclient-invalid-signatures

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) {

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