From f218e314f8cb5914b78f516f6dcd39b7aca4f2a5 Mon Sep 17 00:00:00 2001 From: Hugo Landau Date: Fri, 18 Dec 2015 19:48:11 +0000 Subject: [PATCH] Add good key testing for ECDSA. --- ca/certificate-authority.go | 5 +- ca/certificate-authority_test.go | 42 ++++--- cmd/boulder-ca/main.go | 3 +- cmd/boulder-ra/main.go | 2 +- cmd/boulder-wfe/main.go | 4 +- cmd/config.go | 17 +++ core/good_key.go | 182 +++++++++++++++++++++++++----- core/good_key_test.go | 142 ++++++++++++++++++++--- ra/registration-authority.go | 6 +- ra/registration-authority_test.go | 11 +- test/boulder-config.json | 9 +- wfe/web-front-end.go | 10 +- wfe/web-front-end_test.go | 11 +- 13 files changed, 366 insertions(+), 78 deletions(-) diff --git a/ca/certificate-authority.go b/ca/certificate-authority.go index 96694aa621e..900ad34d0af 100644 --- a/ca/certificate-authority.go +++ b/ca/certificate-authority.go @@ -65,6 +65,7 @@ type CertificateAuthorityImpl struct { SA core.StorageAuthority PA core.PolicyAuthority Publisher core.Publisher + keyPolicy core.KeyPolicy clk clock.Clock // TODO(jmhodges): should be private, like log log *blog.AuditLogger stats statsd.Statter @@ -90,6 +91,7 @@ func NewCertificateAuthorityImpl( stats statsd.Statter, issuer *x509.Certificate, privateKey crypto.Signer, + keyPolicy core.KeyPolicy, ) (*CertificateAuthorityImpl, error) { var ca *CertificateAuthorityImpl var err error @@ -142,6 +144,7 @@ func NewCertificateAuthorityImpl( stats: stats, notAfter: issuer.NotAfter, hsmFaultTimeout: config.HSMFaultTimeout.Duration, + keyPolicy: keyPolicy, } if config.Expiry == "" { @@ -236,7 +239,7 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest ca.log.AuditErr(err) return emptyCert, err } - if err = core.GoodKey(key); err != nil { + if err = ca.keyPolicy.GoodKey(key); err != nil { err = core.MalformedRequestError(fmt.Sprintf("Invalid public key in CSR: %s", err.Error())) // AUDIT[ Certificate Requests ] 11917fa4-10ef-4e0d-9105-bacbe7836a3c ca.log.AuditErr(err) diff --git a/ca/certificate-authority_test.go b/ca/certificate-authority_test.go index 081f730711c..2341c605c91 100644 --- a/ca/certificate-authority_test.go +++ b/ca/certificate-authority_test.go @@ -109,13 +109,14 @@ func mustRead(path string) []byte { } type testCtx struct { - sa core.StorageAuthority - caConfig cmd.CAConfig - reg core.Registration - pa core.PolicyAuthority - fc clock.FakeClock - stats *mocks.Statter - cleanUp func() + sa core.StorageAuthority + caConfig cmd.CAConfig + reg core.Registration + pa core.PolicyAuthority + keyPolicy core.KeyPolicy + fc clock.FakeClock + stats *mocks.Statter + cleanUp func() } var caKey crypto.Signer @@ -207,11 +208,18 @@ func setup(t *testing.T) *testCtx { stats := mocks.NewStatter() + keyPolicy := core.KeyPolicy{ + AllowRSA: true, + AllowECDSANISTP256: true, + AllowECDSANISTP384: true, + } + return &testCtx{ ssa, caConfig, reg, pa, + keyPolicy, fc, &stats, cleanUp, @@ -223,14 +231,14 @@ func TestFailNoSerial(t *testing.T) { defer ctx.cleanUp() ctx.caConfig.SerialPrefix = 0 - _, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey) + _, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey, ctx.keyPolicy) test.AssertError(t, err, "CA should have failed with no SerialPrefix") } func TestIssueCertificate(t *testing.T) { ctx := setup(t) defer ctx.cleanUp() - ca, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey) + ca, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey, ctx.keyPolicy) test.AssertNotError(t, err, "Failed to create CA") ca.Publisher = &mocks.Publisher{} ca.PA = ctx.pa @@ -307,7 +315,7 @@ func TestIssueCertificate(t *testing.T) { func TestRejectNoName(t *testing.T) { ctx := setup(t) defer ctx.cleanUp() - ca, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey) + ca, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey, ctx.keyPolicy) test.AssertNotError(t, err, "Failed to create CA") ca.Publisher = &mocks.Publisher{} ca.PA = ctx.pa @@ -324,7 +332,7 @@ func TestRejectNoName(t *testing.T) { func TestRejectTooManyNames(t *testing.T) { ctx := setup(t) defer ctx.cleanUp() - ca, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey) + ca, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey, ctx.keyPolicy) test.AssertNotError(t, err, "Failed to create CA") ca.Publisher = &mocks.Publisher{} ca.PA = ctx.pa @@ -341,7 +349,7 @@ func TestRejectTooManyNames(t *testing.T) { func TestDeduplication(t *testing.T) { ctx := setup(t) defer ctx.cleanUp() - ca, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey) + ca, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey, ctx.keyPolicy) test.AssertNotError(t, err, "Failed to create CA") ca.Publisher = &mocks.Publisher{} ca.PA = ctx.pa @@ -365,7 +373,7 @@ func TestDeduplication(t *testing.T) { func TestRejectValidityTooLong(t *testing.T) { ctx := setup(t) defer ctx.cleanUp() - ca, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey) + ca, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey, ctx.keyPolicy) test.AssertNotError(t, err, "Failed to create CA") ca.Publisher = &mocks.Publisher{} ca.PA = ctx.pa @@ -383,7 +391,7 @@ func TestRejectValidityTooLong(t *testing.T) { func TestShortKey(t *testing.T) { ctx := setup(t) defer ctx.cleanUp() - ca, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey) + ca, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey, ctx.keyPolicy) ca.Publisher = &mocks.Publisher{} ca.PA = ctx.pa ca.SA = ctx.sa @@ -399,7 +407,7 @@ func TestShortKey(t *testing.T) { func TestRejectBadAlgorithm(t *testing.T) { ctx := setup(t) defer ctx.cleanUp() - ca, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey) + ca, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey, ctx.keyPolicy) ca.Publisher = &mocks.Publisher{} ca.PA = ctx.pa ca.SA = ctx.sa @@ -416,7 +424,7 @@ func TestCapitalizedLetters(t *testing.T) { ctx := setup(t) defer ctx.cleanUp() ctx.caConfig.MaxNames = 3 - ca, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey) + ca, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey, ctx.keyPolicy) ca.Publisher = &mocks.Publisher{} ca.PA = ctx.pa ca.SA = ctx.sa @@ -437,7 +445,7 @@ func TestHSMFaultTimeout(t *testing.T) { ctx := setup(t) defer ctx.cleanUp() - ca, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey) + ca, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, ctx.stats, caCert, caKey, ctx.keyPolicy) ca.Publisher = &mocks.Publisher{} ca.PA = ctx.pa ca.SA = ctx.sa diff --git a/cmd/boulder-ca/main.go b/cmd/boulder-ca/main.go index e0481299873..a42d4666a55 100644 --- a/cmd/boulder-ca/main.go +++ b/cmd/boulder-ca/main.go @@ -91,7 +91,8 @@ func main() { clock.Default(), stats, issuer, - priv) + priv, + c.KeyPolicy()) cmd.FailOnError(err, "Failed to create CA impl") cai.PA = pa diff --git a/cmd/boulder-ra/main.go b/cmd/boulder-ra/main.go index f64940157b3..4f44cfb5ff9 100644 --- a/cmd/boulder-ra/main.go +++ b/cmd/boulder-ra/main.go @@ -59,7 +59,7 @@ func main() { } rai := ra.NewRegistrationAuthorityImpl(clock.Default(), auditlogger, stats, - dc, rateLimitPolicies, c.RA.MaxContactsPerRegistration) + dc, rateLimitPolicies, c.RA.MaxContactsPerRegistration, c.KeyPolicy()) rai.PA = pa raDNSTimeout, err := time.ParseDuration(c.Common.DNSTimeout) cmd.FailOnError(err, "Couldn't parse RA DNS timeout") diff --git a/cmd/boulder-wfe/main.go b/cmd/boulder-wfe/main.go index 9b4c79c1e1f..d7124d1b97a 100644 --- a/cmd/boulder-wfe/main.go +++ b/cmd/boulder-wfe/main.go @@ -53,7 +53,7 @@ func main() { app.Action = func(c cmd.Config, stats statsd.Statter, auditlogger *blog.AuditLogger) { go cmd.DebugServer(c.WFE.DebugAddr) - wfe, err := wfe.NewWebFrontEndImpl(stats, clock.Default()) + wfe, err := wfe.NewWebFrontEndImpl(stats, clock.Default(), c.KeyPolicy()) cmd.FailOnError(err, "Unable to create WFE") rac, sac := setupWFE(c, auditlogger, stats) wfe.RA = rac @@ -79,6 +79,8 @@ func main() { wfe.IssuerCert, err = cmd.LoadCert(c.Common.IssuerCert) cmd.FailOnError(err, fmt.Sprintf("Couldn't read issuer cert [%s]", c.Common.IssuerCert)) + auditlogger.Info(fmt.Sprintf("WFE using key policy: %#v", c.KeyPolicy())) + go cmd.ProfileCmd("WFE", stats) // Set up paths diff --git a/cmd/config.go b/cmd/config.go index 0e251fc62de..e9a1ac15d31 100644 --- a/cmd/config.go +++ b/cmd/config.go @@ -187,9 +187,26 @@ type Config struct { ReportDirectoryPath string } + AllowedSigningAlgos struct { + RSA bool + ECDSANISTP256 bool + ECDSANISTP384 bool + ECDSANISTP521 bool + } + SubscriberAgreementURL string } +// KeyPolicy returns a KeyPolicy reflecting the Boulder configuration. +func (config *Config) KeyPolicy() core.KeyPolicy { + return core.KeyPolicy{ + AllowRSA: config.AllowedSigningAlgos.RSA, + AllowECDSANISTP256: config.AllowedSigningAlgos.ECDSANISTP256, + AllowECDSANISTP384: config.AllowedSigningAlgos.ECDSANISTP384, + AllowECDSANISTP521: config.AllowedSigningAlgos.ECDSANISTP521, + } +} + // ServiceConfig contains config items that are common to all our services, to // be embedded in other config structs. type ServiceConfig struct { diff --git a/core/good_key.go b/core/good_key.go index 6d65ffe9fd1..d5b32ca559b 100644 --- a/core/good_key.go +++ b/core/good_key.go @@ -8,9 +8,9 @@ package core import ( "crypto" "crypto/ecdsa" + "crypto/elliptic" "crypto/rsa" "fmt" - blog "github.com/letsencrypt/boulder/log" "math/big" "reflect" "sync" @@ -38,53 +38,163 @@ var ( smallPrimes []*big.Int ) +// KeyPolicy etermines which types of key may be used with various boulder +// operations. +type KeyPolicy struct { + AllowRSA bool // Whether RSA keys should be allowed. + AllowECDSANISTP256 bool // Whether ECDSA NISTP256 keys should be allowed. + AllowECDSANISTP384 bool // Whether ECDSA NISTP384 keys should be allowed. + AllowECDSANISTP521 bool // Whether ECDSA NISTP521 keys should be allowed. +} + // GoodKey returns true iff the key is acceptable for both TLS use and account // key use (our requirements are the same for either one), according to basic // strength and algorithm checking. // TODO: Support JsonWebKeys once go-jose migration is done. -func GoodKey(key crypto.PublicKey) error { - log := blog.GetAuditLogger() +func (policy *KeyPolicy) GoodKey(key crypto.PublicKey) error { switch t := key.(type) { case rsa.PublicKey: - return GoodKeyRSA(t) + return policy.goodKeyRSA(t) case *rsa.PublicKey: - return GoodKeyRSA(*t) + return policy.goodKeyRSA(*t) case ecdsa.PublicKey: - return GoodKeyECDSA(t) + return policy.goodKeyECDSA(t) case *ecdsa.PublicKey: - return GoodKeyECDSA(*t) + return policy.goodKeyECDSA(*t) default: - err := MalformedRequestError(fmt.Sprintf("Unknown key type %s", reflect.TypeOf(key))) - log.Debug(err.Error()) - return err + return MalformedRequestError(fmt.Sprintf("Unknown key type %s", reflect.TypeOf(key))) } } // GoodKeyECDSA determines if an ECDSA pubkey meets our requirements -func GoodKeyECDSA(key ecdsa.PublicKey) (err error) { - log := blog.GetAuditLogger() - err = NotSupportedError("ECDSA keys not yet supported") - log.Debug(err.Error()) - return +func (policy *KeyPolicy) goodKeyECDSA(key ecdsa.PublicKey) (err error) { + // Check the curve. + // + // The validity of the curve is an assumption for all following tests. + err = policy.goodCurve(key.Curve) + if err != nil { + return err + } + + // Key validation routine adapted from NIST SP800-56A § 5.6.2.3.2. + // + // + // Assuming a prime field since a) we are only allowing such curves and b) + // crypto/elliptic only supports prime curves. Where this assumption + // simplifies the code below, it is explicitly stated and explained. If ever + // adapting this code to support non-prime curves, refer to NIST SP800-56A § + // 5.6.2.3.2 and adapt this code appropriately. + params := key.Params() + + // SP800-56A § 5.6.2.3.2 Step 1. + // Partial check of the public key for an invalid range in the EC group: + // Verify that key is not the point at infinity O. + // 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 MalformedRequestError("Key x, y must not be the point at infinity") + } + + // SP800-56A § 5.6.2.3.2 Step 2. + // "Verify that x_Q and y_Q are integers in the interval [0,p-1] in the + // case that q is an odd prime p, or that x_Q and y_Q are bit strings + // of length m bits in the case that q = 2**m." + // + // Prove prime field: ASSUMED. + // Prove q != 2: ASSUMED. (Curve parameter. No supported curve has q == 2.) + // Prime field && q != 2 => q is an odd prime p + // Therefore "verify that x, y are in [0, p-1]" satisfies step 2. + // + // Therefore verify that both x and y of the public key point have the unique + // 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 MalformedRequestError("Key x, y must not be negative") + } + + if key.X.Cmp(params.P) >= 0 || key.Y.Cmp(params.P) >= 0 { + return MalformedRequestError("Key x, y must not exceed P-1") + } + + // SP800-56A § 5.6.2.3.2 Step 3. + // "If q is an odd prime p, verify that (y_Q)**2 === (x_Q)***3 + a*x_Q + b (mod p). + // If q = 2**m, verify that (y_Q)**2 + (x_Q)*(y_Q) == (x_Q)**3 + a*(x_Q)*2 + b in + // the finite field of size 2**m. + // (Ensures that the public key is on the correct elliptic curve.)" + // + // q is an odd prime p: proven/assumed above. + // a = -3 for all supported curves. + // + // Therefore step 3 is satisfied simply by showing that + // y**2 === x**3 - 3*x + B (mod P). + // + // 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 MalformedRequestError("Key point is not on the curve") + } + + // SP800-56A § 5.6.2.3.2 Step 4. + // "Verify that n*Q == O. + // (Ensures that the public key has the correct order. Along with check 1, + // ensures that the public key is in the correct range in the correct EC + // subgroup, that is, it is in the correct EC subgroup and is not the + // identity element.)" + // + // Ensure that public key has the correct order: + // verify that n*Q = O. + // + // n*Q = O 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 MalformedRequestError("Public key does not have correct order") + } + + // End of SP800-56A § 5.6.2.3.2 Public Key Validation Routine. + // Key is valid. + return nil +} + +// Returns true iff the point (x,y) on NIST P-256, NIST P-384 or NIST P-521 is +// the point at infinity. These curves all have the same point at infinity +// (0,0). This function must ONLY be used on points on curves verified to have +// (0,0) as their point at infinity. +func isPointAtInfinityNISTP(x, y *big.Int) bool { + return x.Sign() == 0 && y.Sign() == 0 +} + +// GoodCurve determines if an elliptic curve meets our requirements. +func (policy *KeyPolicy) goodCurve(c elliptic.Curve) (err error) { + // Simply use a whitelist for now. + params := c.Params() + switch { + case policy.AllowECDSANISTP256 && params == elliptic.P256().Params(): + return nil + case policy.AllowECDSANISTP384 && params == elliptic.P384().Params(): + return nil + case policy.AllowECDSANISTP521 && params == elliptic.P521().Params(): + return nil + default: + return MalformedRequestError(fmt.Sprintf("ECDSA curve %v not allowed", params.Name)) + } } // GoodKeyRSA determines if a RSA pubkey meets our requirements -func GoodKeyRSA(key rsa.PublicKey) (err error) { - log := blog.GetAuditLogger() +func (policy *KeyPolicy) goodKeyRSA(key rsa.PublicKey) (err error) { + if !policy.AllowRSA { + return MalformedRequestError("RSA keys are not allowed") + } + // Baseline Requirements Appendix A // Modulus must be >= 2048 bits and <= 4096 bits modulus := key.N modulusBitLen := modulus.BitLen() const maxKeySize = 4096 if modulusBitLen < 2048 { - err = MalformedRequestError(fmt.Sprintf("Key too small: %d", modulusBitLen)) - log.Debug(err.Error()) - return err + return MalformedRequestError(fmt.Sprintf("Key too small: %d", modulusBitLen)) } if modulusBitLen > maxKeySize { - err = MalformedRequestError(fmt.Sprintf("Key too large: %d > %d", modulusBitLen, maxKeySize)) - log.Debug(err.Error()) - return err + return MalformedRequestError(fmt.Sprintf("Key too large: %d > %d", modulusBitLen, maxKeySize)) } // The CA SHALL confirm that the value of the public exponent is an // odd number equal to 3 or more. Additionally, the public exponent @@ -93,26 +203,36 @@ func GoodKeyRSA(key rsa.PublicKey) (err error) { // 2^32 - 1 or 2^64 - 1, because it stores E as an integer. So we // don't need to check the upper bound. if (key.E%2) == 0 || key.E < ((1<<16)+1) { - err = MalformedRequestError(fmt.Sprintf("Key exponent should be odd and >2^16: %d", key.E)) - log.Debug(err.Error()) - return err + return MalformedRequestError(fmt.Sprintf("Key exponent should be odd and >2^16: %d", key.E)) } // 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 MalformedRequestError("Key divisible by small prime") + } + + return nil +} + +// Returns true iff integer i is divisible by any of the primes in smallPrimes. +// +// Short circuits; execution time is dependent on i. Do not use this on secret +// values. +func checkSmallPrimes(i *big.Int) bool { smallPrimesSingleton.Do(func() { for _, prime := range smallPrimeInts { smallPrimes = append(smallPrimes, big.NewInt(prime)) } }) + for _, prime := range smallPrimes { var result big.Int - result.Mod(modulus, prime) + result.Mod(i, prime) if result.Sign() == 0 { - err = MalformedRequestError(fmt.Sprintf("Key divisible by small prime: %d", prime)) - log.Debug(err.Error()) - return err + return true } } - return nil + + return false } diff --git a/core/good_key_test.go b/core/good_key_test.go index 33f6ded8a22..6a0cfb4ac25 100644 --- a/core/good_key_test.go +++ b/core/good_key_test.go @@ -7,6 +7,7 @@ package core import ( "crypto/ecdsa" + "crypto/elliptic" "crypto/rand" "crypto/rsa" "math/big" @@ -15,29 +16,29 @@ import ( "github.com/letsencrypt/boulder/test" ) -func TestUnknownKeyType(t *testing.T) { - notAKey := struct{}{} - test.AssertError(t, GoodKey(notAKey), "Should have rejected a key of unknown type") +var testingPolicy = &KeyPolicy{ + AllowRSA: true, + AllowECDSANISTP256: true, + AllowECDSANISTP384: true, } -func TestWrongKeyType(t *testing.T) { - ecdsaKey := ecdsa.PublicKey{} - test.AssertError(t, GoodKey(&ecdsaKey), "Should have rejected ECDSA key.") - test.AssertError(t, GoodKey(ecdsaKey), "Should have rejected ECDSA key.") +func TestUnknownKeyType(t *testing.T) { + notAKey := struct{}{} + test.AssertError(t, testingPolicy.GoodKey(notAKey), "Should have rejected a key of unknown type") } func TestSmallModulus(t *testing.T) { private, err := rsa.GenerateKey(rand.Reader, 2040) test.AssertNotError(t, err, "Error generating key") - test.AssertError(t, GoodKey(&private.PublicKey), "Should have rejected too-short key.") - test.AssertError(t, GoodKey(private.PublicKey), "Should have rejected too-short key.") + test.AssertError(t, testingPolicy.GoodKey(&private.PublicKey), "Should have rejected too-short key.") + test.AssertError(t, testingPolicy.GoodKey(private.PublicKey), "Should have rejected too-short key.") } func TestLargeModulus(t *testing.T) { private, err := rsa.GenerateKey(rand.Reader, 4097) test.AssertNotError(t, err, "Error generating key") - test.AssertError(t, GoodKey(&private.PublicKey), "Should have rejected too-long key.") - test.AssertError(t, GoodKey(private.PublicKey), "Should have rejected too-long key.") + test.AssertError(t, testingPolicy.GoodKey(&private.PublicKey), "Should have rejected too-long key.") + test.AssertError(t, testingPolicy.GoodKey(private.PublicKey), "Should have rejected too-long key.") } func TestSmallExponent(t *testing.T) { @@ -46,7 +47,7 @@ func TestSmallExponent(t *testing.T) { N: bigOne.Lsh(bigOne, 2048), E: 5, } - test.AssertError(t, GoodKey(&key), "Should have rejected small exponent.") + test.AssertError(t, testingPolicy.GoodKey(&key), "Should have rejected small exponent.") } func TestEvenExponent(t *testing.T) { @@ -55,7 +56,7 @@ func TestEvenExponent(t *testing.T) { N: bigOne.Lsh(bigOne, 2048), E: 1 << 17, } - test.AssertError(t, GoodKey(&key), "Should have rejected even exponent.") + test.AssertError(t, testingPolicy.GoodKey(&key), "Should have rejected even exponent.") } func TestEvenModulus(t *testing.T) { @@ -64,7 +65,7 @@ func TestEvenModulus(t *testing.T) { N: bigOne.Lsh(bigOne, 2048), E: (1 << 17) + 1, } - test.AssertError(t, GoodKey(&key), "Should have rejected even modulus.") + test.AssertError(t, testingPolicy.GoodKey(&key), "Should have rejected even modulus.") } func TestModulusDivisibleBy752(t *testing.T) { @@ -76,11 +77,120 @@ func TestModulusDivisibleBy752(t *testing.T) { N: N, E: (1 << 17) + 1, } - test.AssertError(t, GoodKey(&key), "Should have rejected modulus divisible by 751.") + test.AssertError(t, testingPolicy.GoodKey(&key), "Should have rejected modulus divisible by 751.") } func TestGoodKey(t *testing.T) { private, err := rsa.GenerateKey(rand.Reader, 2048) test.AssertNotError(t, err, "Error generating key") - test.AssertNotError(t, GoodKey(&private.PublicKey), "Should have accepted good key.") + test.AssertNotError(t, testingPolicy.GoodKey(&private.PublicKey), "Should have accepted good key.") +} + +func TestECDSABadCurve(t *testing.T) { + for _, curve := range invalidCurves { + private, err := ecdsa.GenerateKey(curve, rand.Reader) + test.AssertNotError(t, err, "Error generating key") + test.AssertError(t, testingPolicy.GoodKey(&private.PublicKey), "Should have rejected key with unsupported curve.") + test.AssertError(t, testingPolicy.GoodKey(private.PublicKey), "Should have rejected key with unsupported curve.") + } +} + +var invalidCurves = []elliptic.Curve{ + elliptic.P224(), + elliptic.P521(), +} + +var validCurves = []elliptic.Curve{ + elliptic.P256(), + elliptic.P384(), +} + +func TestECDSAGoodKey(t *testing.T) { + for _, curve := range validCurves { + private, err := ecdsa.GenerateKey(curve, rand.Reader) + test.AssertNotError(t, err, "Error generating key") + test.AssertNotError(t, testingPolicy.GoodKey(&private.PublicKey), "Should have accepted good key.") + test.AssertNotError(t, testingPolicy.GoodKey(private.PublicKey), "Should have accepted good key.") + } +} + +func TestECDSANotOnCurveX(t *testing.T) { + for _, curve := range validCurves { + // Change a public key so that it is no longer on the curve. + private, err := ecdsa.GenerateKey(curve, rand.Reader) + test.AssertNotError(t, err, "Error generating key") + + private.X.Add(private.X, big.NewInt(1)) + test.AssertError(t, testingPolicy.GoodKey(&private.PublicKey), "Should not have accepted key not on the curve.") + test.AssertError(t, testingPolicy.GoodKey(private.PublicKey), "Should not have accepted key not on the curve.") + } +} + +func TestECDSANotOnCurveY(t *testing.T) { + for _, curve := range validCurves { + // Again with Y. + private, err := ecdsa.GenerateKey(curve, rand.Reader) + test.AssertNotError(t, err, "Error generating key") + + // Change the public key so that it is no longer on the curve. + private.Y.Add(private.Y, big.NewInt(1)) + test.AssertError(t, testingPolicy.GoodKey(&private.PublicKey), "Should not have accepted key not on the curve.") + test.AssertError(t, testingPolicy.GoodKey(private.PublicKey), "Should not have accepted key not on the curve.") + } +} + +func TestECDSANegative(t *testing.T) { + for _, curve := range validCurves { + // Check that negative X is not accepted. + private, err := ecdsa.GenerateKey(curve, rand.Reader) + test.AssertNotError(t, err, "Error generating key") + + private.X.Neg(private.X) + test.AssertError(t, testingPolicy.GoodKey(&private.PublicKey), "Should not have accepted key with negative X.") + test.AssertError(t, testingPolicy.GoodKey(private.PublicKey), "Should not have accepted key with negative X.") + + // Check that negative Y is not accepted. + private.X.Neg(private.X) + private.Y.Neg(private.Y) + test.AssertError(t, testingPolicy.GoodKey(&private.PublicKey), "Should not have accepted key with negative Y.") + test.AssertError(t, testingPolicy.GoodKey(private.PublicKey), "Should not have accepted key with negative Y.") + } +} + +func TestECDSANegativeUnmodulatedX(t *testing.T) { + for _, curve := range validCurves { + // Check that unmodulated X is not accepted. + private, err := ecdsa.GenerateKey(curve, rand.Reader) + test.AssertNotError(t, err, "Error generating key") + + private.X.Mul(private.X, private.Curve.Params().P) + test.AssertError(t, testingPolicy.GoodKey(&private.PublicKey), "Should not have accepted key with unmodulated X.") + test.AssertError(t, testingPolicy.GoodKey(private.PublicKey), "Should not have accepted key with unmodulated X.") + } +} + +func TestECDSANegativeUnmodulatedY(t *testing.T) { + for _, curve := range validCurves { + // Check that unmodulated Y is not accepted. + private, err := ecdsa.GenerateKey(curve, rand.Reader) + test.AssertNotError(t, err, "Error generating key") + + private.X.Mul(private.Y, private.Curve.Params().P) + test.AssertError(t, testingPolicy.GoodKey(&private.PublicKey), "Should not have accepted key with unmodulated Y.") + test.AssertError(t, testingPolicy.GoodKey(private.PublicKey), "Should not have accepted key with unmodulated Y.") + } +} + +func TestECDSAIdentity(t *testing.T) { + for _, curve := range validCurves { + // The point at infinity is 0,0, it should not be accepted. + public := ecdsa.PublicKey{ + Curve: curve, + X: big.NewInt(0), + Y: big.NewInt(0), + } + + test.AssertError(t, testingPolicy.GoodKey(&public), "Should not have accepted key with point at infinity.") + test.AssertError(t, testingPolicy.GoodKey(public), "Should not have accepted key with point at infinity.") + } } diff --git a/ra/registration-authority.go b/ra/registration-authority.go index 8a4ffded6bc..95202c6f469 100644 --- a/ra/registration-authority.go +++ b/ra/registration-authority.go @@ -55,6 +55,7 @@ type RegistrationAuthorityImpl struct { clk clock.Clock log *blog.AuditLogger dc *DomainCheck + keyPolicy core.KeyPolicy // How long before a newly created authorization expires. authorizationLifetime time.Duration pendingAuthorizationLifetime time.Duration @@ -71,7 +72,7 @@ type RegistrationAuthorityImpl struct { } // NewRegistrationAuthorityImpl constructs a new RA object. -func NewRegistrationAuthorityImpl(clk clock.Clock, logger *blog.AuditLogger, stats statsd.Statter, dc *DomainCheck, policies cmd.RateLimitConfig, maxContactsPerReg int) *RegistrationAuthorityImpl { +func NewRegistrationAuthorityImpl(clk clock.Clock, logger *blog.AuditLogger, stats statsd.Statter, dc *DomainCheck, policies cmd.RateLimitConfig, maxContactsPerReg int, keyPolicy core.KeyPolicy) *RegistrationAuthorityImpl { // TODO(jmhodges): making RA take a "RA" stats.Scope, not Statter scope := metrics.NewStatsdScope(stats, "RA") ra := &RegistrationAuthorityImpl{ @@ -84,6 +85,7 @@ func NewRegistrationAuthorityImpl(clk clock.Clock, logger *blog.AuditLogger, sta rlPolicies: policies, tiMu: new(sync.RWMutex), maxContactsPerReg: maxContactsPerReg, + keyPolicy: keyPolicy, regByIPStats: scope.NewScope("RA", "RateLimit", "RegistrationsByIP"), pendAuthByRegIDStats: scope.NewScope("RA", "RateLimit", "PendingAuthorizationsByRegID"), @@ -210,7 +212,7 @@ func (ra *RegistrationAuthorityImpl) checkRegistrationLimit(ip net.IP) error { // NewRegistration constructs a new Registration from a request. func (ra *RegistrationAuthorityImpl) NewRegistration(init core.Registration) (reg core.Registration, err error) { - if err = core.GoodKey(init.Key.Key); err != nil { + if err = ra.keyPolicy.GoodKey(init.Key.Key); err != nil { return core.Registration{}, core.MalformedRequestError(fmt.Sprintf("Invalid public key: %s", err.Error())) } if err = ra.checkRegistrationLimit(init.InitialIP); err != nil { diff --git a/ra/registration-authority_test.go b/ra/registration-authority_test.go index 2678d6117b2..5e56289c941 100644 --- a/ra/registration-authority_test.go +++ b/ra/registration-authority_test.go @@ -146,6 +146,12 @@ func makeResponse(ch core.Challenge) (out core.Challenge, err error) { return } +var testKeyPolicy = core.KeyPolicy{ + AllowRSA: true, + AllowECDSANISTP256: true, + AllowECDSANISTP384: true, +} + func initAuthorities(t *testing.T) (*DummyValidationAuthority, *sa.SQLStorageAuthority, *RegistrationAuthorityImpl, clock.FakeClock, func()) { err := json.Unmarshal(AccountKeyJSONA, &AccountKeyA) test.AssertNotError(t, err, "Failed to unmarshal public JWK") @@ -215,7 +221,8 @@ func initAuthorities(t *testing.T) (*DummyValidationAuthority, *sa.SQLStorageAut fc, stats, caCert, - caKey) + caKey, + testKeyPolicy) test.AssertNotError(t, err, "Couldn't create CA") ca.SA = ssa ca.PA = pa @@ -242,7 +249,7 @@ func initAuthorities(t *testing.T) (*DummyValidationAuthority, *sa.SQLStorageAut Threshold: 100, Window: cmd.ConfigDuration{Duration: 24 * 90 * time.Hour}, }, - }, 1) + }, 1, testKeyPolicy) ra.SA = ssa ra.VA = va ra.CA = ca diff --git a/test/boulder-config.json b/test/boulder-config.json index 72d802d8998..b2799fcc308 100644 --- a/test/boulder-config.json +++ b/test/boulder-config.json @@ -306,5 +306,12 @@ "dbConnectFile": "test/secrets/cert_checker_dburl" }, - "subscriberAgreementURL": "http://127.0.0.1:4001/terms/v1" + "subscriberAgreementURL": "http://127.0.0.1:4001/terms/v1", + + "allowedSigningAlgos": { + "rsa": true, + "ecdsanistp256": true, + "ecdsanistp384": true, + "ecdsanistp521": false + } } diff --git a/wfe/web-front-end.go b/wfe/web-front-end.go index e0dfb803d79..04ab62b6fd4 100644 --- a/wfe/web-front-end.go +++ b/wfe/web-front-end.go @@ -75,6 +75,9 @@ type WebFrontEndImpl struct { // Register of anti-replay nonces nonceService *core.NonceService + // Key policy. + keyPolicy core.KeyPolicy + // Cache settings CertCacheDuration time.Duration CertNoCacheExpirationWindow time.Duration @@ -90,7 +93,7 @@ type WebFrontEndImpl struct { } // NewWebFrontEndImpl constructs a web service for Boulder -func NewWebFrontEndImpl(stats statsd.Statter, clk clock.Clock) (WebFrontEndImpl, error) { +func NewWebFrontEndImpl(stats statsd.Statter, clk clock.Clock, keyPolicy core.KeyPolicy) (WebFrontEndImpl, error) { logger := blog.GetAuditLogger() logger.Notice("Web Front End Starting") @@ -104,6 +107,7 @@ func NewWebFrontEndImpl(stats statsd.Statter, clk clock.Clock) (WebFrontEndImpl, clk: clk, nonceService: nonceService, stats: stats, + keyPolicy: keyPolicy, }, nil } @@ -355,7 +359,7 @@ func (wfe *WebFrontEndImpl) verifyPOST(logEvent *requestEvent, request *http.Req // When looking up keys from the registrations DB, we can be confident they // are "good". But when we are verifying against any submitted key, we want // to check its quality before doing the verify. - if err = core.GoodKey(submittedKey.Key); err != nil { + if err = wfe.keyPolicy.GoodKey(submittedKey.Key); err != nil { wfe.stats.Inc("WFE.Errors.JWKRejectedByGoodKey", 1, 1.0) logEvent.AddError("JWK in request was rejected by GoodKey: %s", err) return nil, nil, reg, probs.Malformed(err.Error()) @@ -719,7 +723,7 @@ func (wfe *WebFrontEndImpl) NewCertificate(logEvent *requestEvent, response http // bytes on the wire, and (b) the CA logs all rejections as audit events, but // a bad key from the client is just a malformed request and doesn't need to // be audited. - if err := core.GoodKey(certificateRequest.CSR.PublicKey); err != nil { + if err := wfe.keyPolicy.GoodKey(certificateRequest.CSR.PublicKey); err != nil { logEvent.AddError("CSR public key failed GoodKey: %s", err) wfe.sendError(response, logEvent, probs.Malformed("Invalid key in certificate request :: %s", err), err) return diff --git a/wfe/web-front-end_test.go b/wfe/web-front-end_test.go index 88f22712b84..a2cffa5e1b9 100644 --- a/wfe/web-front-end_test.go +++ b/wfe/web-front-end_test.go @@ -197,10 +197,17 @@ func signRequest(t *testing.T, req string, nonceService *core.NonceService) stri return ret } +var testKeyPolicy = core.KeyPolicy{ + AllowRSA: true, + AllowECDSANISTP256: true, + AllowECDSANISTP384: true, +} + func setupWFE(t *testing.T) (WebFrontEndImpl, clock.FakeClock) { fc := clock.NewFake() stats, _ := statsd.NewNoopClient() - wfe, err := NewWebFrontEndImpl(stats, fc) + + wfe, err := NewWebFrontEndImpl(stats, fc, testKeyPolicy) test.AssertNotError(t, err, "Unable to create WFE") wfe.NewReg = wfe.BaseURL + NewRegPath @@ -546,7 +553,7 @@ func TestIssueCertificate(t *testing.T) { // TODO: Use a mock RA so we can test various conditions of authorized, not // authorized, etc. stats, _ := statsd.NewNoopClient(nil) - ra := ra.NewRegistrationAuthorityImpl(fc, wfe.log, stats, nil, cmd.RateLimitConfig{}, 0) + ra := ra.NewRegistrationAuthorityImpl(fc, wfe.log, stats, nil, cmd.RateLimitConfig{}, 0, testKeyPolicy) ra.SA = mocks.NewStorageAuthority(fc) ra.CA = &MockCA{} ra.PA = &MockPA{}