letsencrypt
/
boulder
mirror of https://github.com/letsencrypt/boulder.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix error types emitted by good_key.go (#4932) 

The `KeyPolicy.GoodKey` method is used to validate both public keys
used to sign JWK messages, and public keys contained inside CSR
messages.

According to RFC8555 section 6.7, validation failure in the former
case should result in `badPublicKey`, while validation failure in
the latter case should result in `badCSR`. In either case, a failure
due to reasons other than the key itself should result in
`serverInternal`.

However, the GoodKey method returns a variety of different errors
which are not all applicable depending on the context in which it is
called. In addition, the `csr.VerifyCSR` method passes these errors
through verbatim, resulting in ACME clients receiving confusing and
incorrect error message types.

This change causes the GoodKey method to always return either a
generic error or a KeyError. Calling methods should treat a `KeyError`
as either a `badPublicKey` or a `badCSR` depending on their context,
and may treat a generic error however they choose (though likely as a
serverInternal error).

Fixes #4930
This commit is contained in:
Aaron Gable 2020-07-06 10:06:10 -07:00 committed by GitHub
parent 35c19c2e08
commit 4a85abf25a
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 46 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
ca/ca_test.go
View File

@ -600,7 +600,7 @@ func TestInvalidCSRs(t *testing.T) {
// * Random public key -- 512 bits long
// * CN = (none)
// * DNSNames = not-example.com, www.not-example.com, mail.not-example.com
{"RejectShortKey", "./testdata/short_key.der.csr", nil, "Issued a certificate with too short a key.", berrors.BadPublicKey},
{"RejectShortKey", "./testdata/short_key.der.csr", nil, "Issued a certificate with too short a key.", berrors.BadCSR},
// CSR generated by Go:
// * Random RSA public key.

8
csr/csr.go
View File

@ -4,6 +4,7 @@ import (
"context"
"crypto"
"crypto/x509"
"errors"
"strings"
"github.com/letsencrypt/boulder/core"
@ -33,7 +34,7 @@ var goodSignatureAlgorithms = map[x509.SignatureAlgorithm]bool{
}
var (
invalidPubKey = berrors.BadPublicKeyError("invalid public key in CSR")
invalidPubKey = berrors.BadCSRError("invalid public key in CSR")
unsupportedSigAlg = berrors.BadCSRError("signature algorithm not supported")
invalidSig = berrors.BadCSRError("invalid signature on CSR")
invalidEmailPresent = berrors.BadCSRError("CSR contains one or more email address fields")
@ -52,7 +53,10 @@ func VerifyCSR(ctx context.Context, csr *x509.CertificateRequest, maxNames int,
return invalidPubKey
}
if err := keyPolicy.GoodKey(ctx, key); err != nil {
return berrors.BadPublicKeyError("invalid public key in CSR: %s", err)
if errors.Is(err, goodkey.ErrBadKey) {
return berrors.BadCSRError("invalid public key in CSR: %s", err)
}
return berrors.InternalServerError("error checking key validity: %s", err)
}
if !goodSignatureAlgorithms[csr.SignatureAlgorithm] {
return unsupportedSigAlg

4
goodkey/blocked_test.go
View File

@ -3,12 +3,12 @@ package goodkey
import (
"context"
"crypto"
"errors"
"io/ioutil"
"os"
"testing"
"github.com/letsencrypt/boulder/core"
berrors "github.com/letsencrypt/boulder/errors"
"github.com/letsencrypt/boulder/test"
"github.com/letsencrypt/boulder/web"
yaml "gopkg.in/yaml.v2"
@ -98,6 +98,6 @@ func TestBlockedKeys(t *testing.T) {
for _, k := range blockedKeys {
err := testingPolicy.GoodKey(context.Background(), k)
test.AssertError(t, err, "test key was not blocked by key policy with block list")
test.Assert(t, berrors.Is(err, berrors.BadPublicKey), "err was not BadPublicKey error")
test.Assert(t, errors.Is(err, ErrBadKey), "err was not goodkey.ErrBadKey")
}
}

55
goodkey/good_key.go
View File

@ -6,6 +6,8 @@ import (
"crypto/ecdsa"
"crypto/elliptic"
"crypto/rsa"
"errors"
"fmt"
"math/big"
"sync"
@ -39,6 +41,16 @@ var (
smallPrimesProduct *big.Int
)
// ErrBadKey represents an error with a key. It is distinct from the various
// ways in which an ACME request can have an erroneous key (BadPublicKeyError,
// BadCSRError) because this library is used to check both JWS signing keys and
// keys in CSRs.
var ErrBadKey = errors.New("")
func badKey(msg string, args ...interface{}) error {
return fmt.Errorf("%w%s", ErrBadKey, fmt.Errorf(msg, args...))
}
// BlockedKeyCheckFunc is used to pass in the sa.BlockedKey method to KeyPolicy,
// rather than storing a full sa.SQLStorageAuthority. This makes testing
// significantly simpler.
@ -97,7 +109,7 @@ func (policy *KeyPolicy) GoodKey(ctx context.Context, key crypto.PublicKey) erro
case *rsa.PublicKey, *ecdsa.PublicKey:
break
default:
return berrors.MalformedError("unsupported key type %T", t)
return badKey("unsupported key type %T", t)
}
// If there is a blocked list configured then check if the public key is one
// that has been administratively blocked.
@ -105,20 +117,19 @@ func (policy *KeyPolicy) GoodKey(ctx context.Context, key crypto.PublicKey) erro
if blocked, err := policy.blockedList.blocked(key); err != nil {
return berrors.InternalServerError("error checking blocklist for key: %v", key)
} else if blocked {
return berrors.BadPublicKeyError("public key is forbidden")
return badKey("public key is forbidden")
}
}
if policy.dbCheck != nil {
digest, err := core.KeyDigest(key)
if err != nil {
return err
return badKey("%w", err)
}
exists, err := policy.dbCheck(ctx, &sapb.KeyBlockedRequest{KeyHash: digest[:]})
if err != nil {
return err
}
if *exists.Exists {
return berrors.BadPublicKeyError("public key is forbidden")
} else if *exists.Exists {
return badKey("public key is forbidden")
}
}
switch t := key.(type) {
@ -127,7 +138,7 @@ func (policy *KeyPolicy) GoodKey(ctx context.Context, key crypto.PublicKey) erro
case *ecdsa.PublicKey:
return policy.goodKeyECDSA(t)
default:
return berrors.MalformedError("unsupported key type %T", key)
return badKey("unsupported key type %T", key)
}
}
@ -157,7 +168,7 @@ func (policy *KeyPolicy) goodKeyECDSA(key *ecdsa.PublicKey) (err error) {
// This code assumes that the point at infinity is (0,0), which is the
// case for all supported curves.
if isPointAtInfinityNISTP(key.X, key.Y) {
return berrors.MalformedError("key x, y must not be the point at infinity")
return badKey("key x, y must not be the point at infinity")
}
// SP800-56A § 5.6.2.3.2 Step 2.
@ -174,11 +185,11 @@ func (policy *KeyPolicy) goodKeyECDSA(key *ecdsa.PublicKey) (err error) {
// correct representation of an element in the underlying field by verifying
// that x and y are integers in [0, p-1].
if key.X.Sign() < 0 || key.Y.Sign() < 0 {
return berrors.MalformedError("key x, y must not be negative")
return badKey("key x, y must not be negative")
}
if key.X.Cmp(params.P) >= 0 || key.Y.Cmp(params.P) >= 0 {
return berrors.MalformedError("key x, y must not exceed P-1")
return badKey("key x, y must not exceed P-1")
}
// SP800-56A § 5.6.2.3.2 Step 3.
@ -196,7 +207,7 @@ func (policy *KeyPolicy) goodKeyECDSA(key *ecdsa.PublicKey) (err error) {
// This proves that the public key is on the correct elliptic curve.
// But in practice, this test is provided by crypto/elliptic, so use that.
if !key.Curve.IsOnCurve(key.X, key.Y) {
return berrors.MalformedError("key point is not on the curve")
return badKey("key point is not on the curve")
}
// SP800-56A § 5.6.2.3.2 Step 4.
@ -212,7 +223,7 @@ func (policy *KeyPolicy) goodKeyECDSA(key *ecdsa.PublicKey) (err error) {
// n*Q = Ø iff n*Q is the point at infinity (see step 1).
ox, oy := key.Curve.ScalarMult(key.X, key.Y, params.N.Bytes())
if !isPointAtInfinityNISTP(ox, oy) {
return berrors.MalformedError("public key does not have correct order")
return badKey("public key does not have correct order")
}
// End of SP800-56A § 5.6.2.3.2 Public Key Validation Routine.
@ -238,7 +249,7 @@ func (policy *KeyPolicy) goodCurve(c elliptic.Curve) (err error) {
case policy.AllowECDSANISTP384 && params == elliptic.P384().Params():
return nil
default:
return berrors.MalformedError("ECDSA curve %v not allowed", params.Name)
return badKey("ECDSA curve %v not allowed", params.Name)
}
}
@ -251,10 +262,10 @@ var acceptableRSAKeySizes = map[int]bool{
// GoodKeyRSA determines if a RSA pubkey meets our requirements
func (policy *KeyPolicy) goodKeyRSA(key *rsa.PublicKey) (err error) {
if !policy.AllowRSA {
return berrors.MalformedError("RSA keys are not allowed")
return badKey("RSA keys are not allowed")
}
if policy.weakRSAList != nil && policy.weakRSAList.Known(key) {
return berrors.MalformedError("key is on a known weak RSA key list")
return badKey("key is on a known weak RSA key list")
}
// Baseline Requirements Appendix A
@ -263,20 +274,20 @@ func (policy *KeyPolicy) goodKeyRSA(key *rsa.PublicKey) (err error) {
modulusBitLen := modulus.BitLen()
if features.Enabled(features.RestrictRSAKeySizes) {
if !acceptableRSAKeySizes[modulusBitLen] {
return berrors.MalformedError("key size not supported: %d", modulusBitLen)
return badKey("key size not supported: %d", modulusBitLen)
}
} else {
const maxKeySize = 4096
if modulusBitLen < 2048 {
return berrors.MalformedError("key too small: %d", modulusBitLen)
return badKey("key too small: %d", modulusBitLen)
}
if modulusBitLen > maxKeySize {
return berrors.MalformedError("key too large: %d > %d", modulusBitLen, maxKeySize)
return badKey("key too large: %d > %d", modulusBitLen, maxKeySize)
}
// Bit lengths that are not a multiple of 8 may cause problems on some
// client implementations.
if modulusBitLen%8 != 0 {
return berrors.MalformedError("key length wasn't a multiple of 8: %d", modulusBitLen)
return badKey("key length wasn't a multiple of 8: %d", modulusBitLen)
}
}
@ -294,19 +305,19 @@ func (policy *KeyPolicy) goodKeyRSA(key *rsa.PublicKey) (err error) {
// By only allowing one exponent, which fits these constraints, we satisfy
// these requirements.
if key.E != 65537 {
return berrors.MalformedError("key exponent must be 65537")
return badKey("key exponent must be 65537")
}
// The modulus SHOULD also have the following characteristics: an odd
// number, not the power of a prime, and have no factors smaller than 752.
// TODO: We don't yet check for "power of a prime."
if checkSmallPrimes(modulus) {
return berrors.MalformedError("key divisible by small prime")
return badKey("key divisible by small prime")
}
// Check for weak keys generated by Infineon hardware
// (see https://crocs.fi.muni.cz/public/papers/rsa_ccs17)
if rocacheck.IsWeak(key) {
return berrors.MalformedError("key generated by vulnerable Infineon-based hardware")
return badKey("key generated by vulnerable Infineon-based hardware")
}
return nil

4
goodkey/good_key_test.go
View File

@ -6,11 +6,11 @@ import (
"crypto/elliptic"
"crypto/rand"
"crypto/rsa"
"errors"
"fmt"
"math/big"
"testing"
berrors "github.com/letsencrypt/boulder/errors"
"github.com/letsencrypt/boulder/features"
sapb "github.com/letsencrypt/boulder/sa/proto"
"github.com/letsencrypt/boulder/test"
@ -275,7 +275,7 @@ func TestDBBlacklist(t *testing.T) {
exists = true
err = policy.GoodKey(context.Background(), k.Public())
test.AssertError(t, err, "GoodKey didn't fail with a blocked key")
test.Assert(t, berrors.Is(err, berrors.BadPublicKey), "returned error is wrong type")
test.Assert(t, errors.Is(err, ErrBadKey), "returned error is wrong type")
test.AssertEquals(t, err.Error(), "public key is forbidden")
}

2
test/v2_integration.py
View File

@ -1393,7 +1393,7 @@ def test_blocked_key_cert():
try:
order = client.poll_and_finalize(order)
except acme_errors.Error as e:
if e.typ != "urn:ietf:params:acme:error:badPublicKey":
if e.typ != "urn:ietf:params:acme:error:badCSR":
raise(Exception("problem did not have correct error type, had {0}".format(e.typ)))
if e.detail != "Error finalizing order :: invalid public key in CSR: public key is forbidden":
raise(Exception("problem did not have correct error detail, had {0}".format(e.detail)))

2
wfe/wfe.go
View File

@ -535,7 +535,7 @@ func (wfe *WebFrontEndImpl) verifyPOST(ctx context.Context, logEvent *web.Reques
// to check its quality before doing the verify.
if err = wfe.keyPolicy.GoodKey(ctx, submittedKey.Key); err != nil {
wfe.joseErrorCounter.WithLabelValues("JWKRejectedByGoodKey").Inc()
return nil, nil, reg, probs.Malformed(err.Error())
return nil, nil, reg, probs.BadPublicKey(err.Error())
}
key = submittedKey
} else if err != nil {