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.
6 months ago · 06f4cbbcda
parent cf6e189d7c
commit 06f4cbbcda
2 changed files with 118 additions and 2 deletions
--- 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) {
--- 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)
+		})
+	}
+}