From 175fc26450d2d0df5cf43c6c47017e1e0b68e517 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Tue, 5 May 2015 19:03:06 -0700 Subject: [PATCH 1/4] Add good_key.go. --- core/good_key.go | 51 +++++++++++++++++++++++++++++++++++++++++++ core/good_key_test.go | 23 +++++++++++++++++++ 2 files changed, 74 insertions(+) create mode 100644 core/good_key.go create mode 100644 core/good_key_test.go diff --git a/core/good_key.go b/core/good_key.go new file mode 100644 index 000000000..f80dd9310 --- /dev/null +++ b/core/good_key.go @@ -0,0 +1,51 @@ +// Copyright 2014 ISRG. All rights reserved +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +package core + +import ( + "crypto" + "crypto/rsa" + "fmt" + blog "github.com/letsencrypt/boulder/log" +) + +// 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. +func GoodKey(key crypto.PublicKey) bool { + log := blog.GetAuditLogger() + rsaKey, ok := key.(rsa.PublicKey) + if !ok { + log.Debug("Non-RSA keys not yet supported.") + return false + } + // Baseline Requirements Appendix A + // Modulus must be >= 2048 bits + modulus := rsaKey.N + if modulus.BitLen() < 2048 { + log.Debug(fmt.Sprintf("Key too small: %d", modulus.BitLen())) + return false + } + // The CA SHALL confirm that the value of the public exponent + // is an odd number equal to 3 or more + if rsaKey.E % 2 == 0 { + log.Debug(fmt.Sprintf("Key exponent is an even number: %d", rsaKey.E)) + return false + } + // Additionally, the public exponent SHOULD be in the range between + // 2^16 + 1 and 2^256-1. + // NOTE: rsa.PublicKey cannot represent an exponent part greater than + // 2^256 - 1, because it stores E as an integer. So we don't check the upper + // bound. + if rsaKey.E < ((1 << 6) + 1) { + log.Debug(fmt.Sprintf("Key exponent is too small: %d", rsaKey.E)) + return false + } + // TODO: The modulus SHOULD also have the following + // characteristics: an odd number, not the power of a prime, + // and have no factors smaller than 752. + return true +} diff --git a/core/good_key_test.go b/core/good_key_test.go new file mode 100644 index 000000000..aeb2a6387 --- /dev/null +++ b/core/good_key_test.go @@ -0,0 +1,23 @@ +// Copyright 2014 ISRG. All rights reserved +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +package core + +import ( + "testing" + "crypto/ecdsa" + "github.com/letsencrypt/boulder/test" + //"crypto/rsa" +) + +func TestWrongKeyType(t *testing.T) { + ecdsaKey := ecdsa.PublicKey{} + test.Assert(t, !GoodKey(ecdsaKey), "Should have rejected ECDSA key.") +} + +func TestWrongKeyType(t *testing.T) { + ecdsaKey := ecdsa.PublicKey{} + test.Assert(t, !GoodKey(ecdsaKey), "Should have rejected ECDSA key.") +} From f778ba12dee913d54ce4c3bb8b7fc7950f1d6ccf Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Wed, 6 May 2015 10:25:30 -0700 Subject: [PATCH 2/4] Implement key checking in RA and CA. --- ca/certificate-authority.go | 32 ++++++++++++++++++----------- core/good_key.go | 14 ++++++++++--- core/good_key_test.go | 40 ++++++++++++++++++++++++++++++------ ra/registration-authority.go | 8 ++++++-- test/js/test.js | 1 - 5 files changed, 71 insertions(+), 24 deletions(-) diff --git a/ca/certificate-authority.go b/ca/certificate-authority.go index 8fd815f70..1d8f433dd 100644 --- a/ca/certificate-authority.go +++ b/ca/certificate-authority.go @@ -181,6 +181,14 @@ func (ca *CertificateAuthorityImpl) RevokeCertificate(serial string) (err error) // IssueCertificate attempts to convert a CSR into a signed Certificate, while // enforcing all policies. func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest) (cert core.Certificate, err error) { + key, ok := csr.PublicKey.(crypto.PublicKey) + if !ok { + return core.Certificate{}, fmt.Errorf("Invalid public key in CSR.") + } + if !core.GoodKey(key) { + return core.Certificate{}, fmt.Errorf("Invalid public key in CSR.") + } + // XXX Take in authorizations and verify that union covers CSR? // Pull hostnames from CSR hostNames := csr.DNSNames // DNSNames + CN from CSR @@ -190,9 +198,9 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest } else if len(hostNames) > 0 { commonName = hostNames[0] } else { - err = errors.New("Cannot issue a certificate without a hostname.") + err = fmt.Errorf("Cannot issue a certificate without a hostname.") ca.log.WarningErr(err) - return + return core.Certificate{}, err } if len(hostNames) == 0 { @@ -201,16 +209,16 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest identifier := core.AcmeIdentifier{Type: core.IdentifierDNS, Value: commonName} if err = ca.PA.WillingToIssue(identifier); err != nil { - err = errors.New("Policy forbids issuing for name " + commonName) + err = fmt.Errorf("Policy forbids issuing for name %s", commonName) ca.log.AuditErr(err) - return + return core.Certificate{}, err } for _, name := range hostNames { identifier = core.AcmeIdentifier{Type: core.IdentifierDNS, Value: name} if err = ca.PA.WillingToIssue(identifier); err != nil { - err = errors.New("Policy forbids issuing for name " + name) + err = fmt.Errorf("Policy forbids issuing for name %s", name) ca.log.AuditErr(err) - return + return core.Certificate{}, err } } @@ -224,7 +232,7 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest ca.DB.Begin() serialDec, err := ca.DB.IncrementAndGetSerial() if err != nil { - return + return core.Certificate{}, err } serialHex := fmt.Sprintf("%02X%014X", ca.Prefix, serialDec) @@ -242,14 +250,14 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest certPEM, err := ca.Signer.Sign(req) if err != nil { ca.DB.Rollback() - return + return core.Certificate{}, err } if len(certPEM) == 0 { err = errors.New("No certificate returned by server") ca.log.WarningErr(err) ca.DB.Rollback() - return + return core.Certificate{}, err } block, _ := pem.Decode(certPEM) @@ -257,7 +265,7 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest err = errors.New("Invalid certificate value returned") ca.log.WarningErr(err) ca.DB.Rollback() - return + return core.Certificate{}, err } certDER := block.Bytes @@ -273,9 +281,9 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest _, err = ca.SA.AddCertificate(certDER) if err != nil { ca.DB.Rollback() - return + return core.Certificate{}, err } ca.DB.Commit() - return + return cert, err } diff --git a/core/good_key.go b/core/good_key.go index f80dd9310..5749f2c5e 100644 --- a/core/good_key.go +++ b/core/good_key.go @@ -9,7 +9,10 @@ import ( "crypto" "crypto/rsa" "fmt" + "reflect" + blog "github.com/letsencrypt/boulder/log" + "github.com/letsencrypt/boulder/jose" ) // GoodKey returns true iff the key is acceptable for both TLS use and account @@ -17,10 +20,15 @@ import ( // strength and algorithm checking. func GoodKey(key crypto.PublicKey) bool { log := blog.GetAuditLogger() - rsaKey, ok := key.(rsa.PublicKey) + rsaKey, ok := key.(*rsa.PublicKey) if !ok { - log.Debug("Non-RSA keys not yet supported.") - return false + if jwk, ok := key.(jose.JsonWebKey); ok && jwk.Rsa != nil { + rsaKey = jwk.Rsa + } else { + log.Debug(fmt.Sprintf("Non-RSA keys not yet supported, got %s", + reflect.TypeOf(key))) + return false + } } // Baseline Requirements Appendix A // Modulus must be >= 2048 bits diff --git a/core/good_key_test.go b/core/good_key_test.go index aeb2a6387..d7d3c9c8d 100644 --- a/core/good_key_test.go +++ b/core/good_key_test.go @@ -6,18 +6,46 @@ package core import ( - "testing" "crypto/ecdsa" + "crypto/rand" + "crypto/rsa" + "math/big" + "testing" + "github.com/letsencrypt/boulder/test" - //"crypto/rsa" ) func TestWrongKeyType(t *testing.T) { ecdsaKey := ecdsa.PublicKey{} - test.Assert(t, !GoodKey(ecdsaKey), "Should have rejected ECDSA key.") + test.Assert(t, !GoodKey(&ecdsaKey), "Should have rejected ECDSA key.") } -func TestWrongKeyType(t *testing.T) { - ecdsaKey := ecdsa.PublicKey{} - test.Assert(t, !GoodKey(ecdsaKey), "Should have rejected ECDSA key.") +func TestSmallModulus(t *testing.T) { + private, err := rsa.GenerateKey(rand.Reader, 2040) + test.AssertNotError(t, err, "Error generating key") + test.Assert(t, !GoodKey(&private.PublicKey), "Should have rejected too-short key.") +} + +func TestSmallExponent(t *testing.T) { + bigOne := big.NewInt(1) + key := rsa.PublicKey{ + N: bigOne.Lsh(bigOne, 2048), + E: 5, + } + test.Assert(t, !GoodKey(&key), "Should have rejected small exponent.") +} + +func TestEvenExponent(t *testing.T) { + bigOne := big.NewInt(1) + key := rsa.PublicKey{ + N: bigOne.Lsh(bigOne, 2048), + E: 1 << 17, + } + test.Assert(t, !GoodKey(&key), "Should have rejected even exponent.") +} + +func TestGoodKey(t *testing.T) { + private, err := rsa.GenerateKey(rand.Reader, 2048) + test.AssertNotError(t, err, "Error generating key") + test.Assert(t, GoodKey(&private.PublicKey), "Should have accepted good key.") } diff --git a/ra/registration-authority.go b/ra/registration-authority.go index 0ac2e68c3..e7ee73895 100644 --- a/ra/registration-authority.go +++ b/ra/registration-authority.go @@ -47,9 +47,13 @@ func lastPathSegment(url core.AcmeURL) string { } func (ra *RegistrationAuthorityImpl) NewRegistration(init core.Registration, key jose.JsonWebKey) (reg core.Registration, err error) { + if !core.GoodKey(key) { + return core.Registration{}, fmt.Errorf("Invalid public key.") + } + regID, err := ra.SA.NewRegistration() if err != nil { - return + return core.Registration{}, err } reg = core.Registration{ @@ -61,7 +65,7 @@ func (ra *RegistrationAuthorityImpl) NewRegistration(init core.Registration, key // Store the authorization object, then return it err = ra.SA.UpdateRegistration(reg) - return + return reg, err } func (ra *RegistrationAuthorityImpl) NewAuthorization(request core.Authorization, key jose.JsonWebKey) (authz core.Authorization, err error) { diff --git a/test/js/test.js b/test/js/test.js index 67dac282f..f983bd281 100644 --- a/test/js/test.js +++ b/test/js/test.js @@ -314,7 +314,6 @@ function getReadyToValidate(err, resp, body) { var challenge = simpleHttps[0]; var path = crypto.randomString(8) + ".txt"; var challengePath = ".well-known/acme-challenge/" + path; - fs.writeFileSync(challengePath, challenge.token); state.responseURL = challenge["uri"]; state.path = path; From 02421fefd991838855f607cfd24384b51c05481a Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Wed, 6 May 2015 16:10:00 -0700 Subject: [PATCH 3/4] Add tests. --- ca/certificate-authority_test.go | 24 ++++++++++ ca/shortkey-csr.der | Bin 0 -> 207 bytes core/util.go | 5 ++ ra/registration-authority_test.go | 75 ++++++++++++++++++++++++++++-- 4 files changed, 99 insertions(+), 5 deletions(-) create mode 100644 ca/shortkey-csr.der diff --git a/ca/certificate-authority_test.go b/ca/certificate-authority_test.go index f2cfa07c6..e8f50e10d 100644 --- a/ca/certificate-authority_test.go +++ b/ca/certificate-authority_test.go @@ -427,6 +427,14 @@ func TestIssueCertificate(t *testing.T) { test.Assert(t, certStatus.Status == core.OCSPStatusGood, "Certificate status was not good") test.Assert(t, certStatus.SubscriberApproved == false, "Subscriber shouldn't have approved cert yet.") } +} + + +func TestRejectNoName(t *testing.T) { + cadb, storageAuthority, caConfig := setup(t) + ca, err := NewCertificateAuthorityImpl(cadb, caConfig) + test.AssertNotError(t, err, "Failed to create CA") + ca.SA = storageAuthority // Test that the CA rejects CSRs with no names csrDER, _ := hex.DecodeString(NO_NAME_CSR_HEX) @@ -436,3 +444,19 @@ func TestIssueCertificate(t *testing.T) { t.Errorf("CA improperly agreed to create a certificate with no name") } } + +func TestShortKey(t *testing.T) { + cadb, storageAuthority, caConfig := setup(t) + ca, err := NewCertificateAuthorityImpl(cadb, caConfig) + ca.SA = storageAuthority + + csrDER, err := ioutil.ReadFile("shortkey-csr.der") + if err != nil { + t.Errorf("Failed to read shortkey-csr.der") + } + csr, _ := x509.ParseCertificateRequest(csrDER) + _, err = ca.IssueCertificate(*csr) + if err == nil { + t.Errorf("CA improperly created a certificate with short key.") + } +} diff --git a/ca/shortkey-csr.der b/ca/shortkey-csr.der new file mode 100644 index 0000000000000000000000000000000000000000..aa0ef8d7e3ac36dd0d24f4c06e755efccd6f2b21 GIT binary patch literal 207 zcmXqLJY!J7#K>SEY$#~J&&C|e!py@_oRMEtlAT&<5M#j0#;Mij(e|B}k&%&=f!UkE zz=O$=VcWO4OT3i4?K1hkLfnDx zZNQ9%Q!XCU^ECSZh;l#N{Pgs>R3>Id2F3*p$fk1xO?PCN 0 && len(segments[0]) == 0 { diff --git a/ra/registration-authority_test.go b/ra/registration-authority_test.go index 0c66bbb36..1d89687d8 100644 --- a/ra/registration-authority_test.go +++ b/ra/registration-authority_test.go @@ -63,13 +63,21 @@ func (cadb *MockCADatabase) IncrementAndGetSerial() (int, error) { var ( // These values we simulate from the client AccountKeyJSON = []byte(`{ - "kty": "EC", - "crv": "P-521", - "x": "AHKZLLOsCOzz5cY97ewNUajB957y-C-U88c3v13nmGZx6sYl_oJXu9A5RkTKqjqvjyekWF-7ytDyRXYgCF5cj0Kt", - "y": "AdymlHvOiLxXkEhayXQnNCvDX4h9htZaCJN34kfmC6pV5OhQHiraVySsUdaQkAgDPrwQrJmbnX9cwlGfP-HqHZR1" - }`) + "e": "AQAB", + "kty": "RSA", + "n": "tSwgy3ORGvc7YJI9B2qqkelZRUC6F1S5NwXFvM4w5-M0TsxbFsH5UH6adigV0jzsDJ5imAechcSoOhAh9POceCbPN1sTNwLpNbOLiQQ7RD5mY_pSUHWXNmS9R4NZ3t2fQAzPeW7jOfF0LKuJRGkekx6tXP1uSnNibgpJULNc4208dgBaCHo3mvaE2HV2GmVl1yxwWX5QZZkGQGjNDZYnjFfa2DKVvFs0QbAk21ROm594kAxlRlMMrvqlf24Eq4ERO0ptzpZgm_3j_e4hGRD39gJS7kAzK-j2cacFQ5Qi2Y6wZI2p-FCq_wiYsfEAIkATPBiLKl_6d_Jfcvs_impcXQ" + }`) + AccountKey = jose.JsonWebKey{} + ShortKeyJSON = []byte(`{ + "e": "AQAB", + "kty": "RSA", + "n": "tSwgy3ORGvc7YJI9B2qqkelZRUC6F1S5NwXFvM4w5-M0TsxbFsH5UH6adigV0jzsDJ5imAechcSoOhAh9POceCbPN1sTNwLpNbOLiQQ7RD5mY_" + }`) + + ShortKey = jose.JsonWebKey{} + AuthzRequest = core.Authorization{ Identifier: core.AcmeIdentifier{ Type: core.IdentifierDNS, @@ -96,6 +104,9 @@ func initAuthorities(t *testing.T) (core.CertificateAuthority, *DummyValidationA err := json.Unmarshal(AccountKeyJSON, &AccountKey) test.AssertNotError(t, err, "Failed to unmarshall JWK") + err = json.Unmarshal(ShortKeyJSON, &ShortKey) + test.AssertNotError(t, err, "Failed to unmarshall JWK") + sa, err := sa.NewSQLStorageAuthority("sqlite3", ":memory:") test.AssertNotError(t, err, "Failed to create SA") sa.InitTables() @@ -131,6 +142,60 @@ func assertAuthzEqual(t *testing.T, a1, a2 core.Authorization) { // Not testing: Contact, Challenges } +func TestNewRegistration(t *testing.T) { + _, _, sa, ra := initAuthorities(t) + mailto, _ := url.Parse("mailto:foo@bar.com") + input := core.Registration{ + Contact: []core.AcmeURL{core.AcmeURL(*mailto)}, + } + + result, err := ra.NewRegistration(input, AccountKey) + test.AssertNotError(t, err, "Could not create new registration") + + test.Assert(t, result.Key.Equals(AccountKey), "Key didn't match") + test.Assert(t, len(result.Contact) == 1, "Wrong number of contacts") + test.Assert(t, mailto.String() == result.Contact[0].String(), + "Contact didn't match") + test.Assert(t, result.Agreement == "", "Agreement didn't default empty") + test.Assert(t, result.RecoveryToken != "", "Recovery token not filled") + + reg, err := sa.GetRegistration(result.ID) + test.AssertNotError(t, err, "Failed to retrieve registration") + test.Assert(t, reg.Key.Equals(AccountKey), "Retrieved registration differed.") +} + +func TestNewRegistrationNoFieldOverwrite(t *testing.T) { + _, _, _, ra := initAuthorities(t) + mailto, _ := url.Parse("mailto:foo@bar.com") + input := core.Registration{ + ID: "hi", + Key: ShortKey, + RecoveryToken: "RecoverMe", + Contact: []core.AcmeURL{core.AcmeURL(*mailto)}, + Agreement: "I agreed", + } + + result, err := ra.NewRegistration(input, AccountKey) + test.AssertNotError(t, err, "Could not create new registration") + + test.Assert(t, result.ID != "hi", "ID shouldn't be overwritten") + test.Assert(t, !result.Key.Equals(ShortKey), "Key shouldn't be overwritten") + // TODO: Enable this test case once we validate terms agreement. + // test.Assert(t, result.Agreement != "I agreed", "Agreement shouldn't be overwritten with invalid URL") + test.Assert(t, result.RecoveryToken != "RecoverMe", "Recovery token shouldn't be overwritten") +} + +func TestNewRegistrationBadKey(t *testing.T) { + _, _, _, ra := initAuthorities(t) + mailto, _ := url.Parse("mailto:foo@bar.com") + input := core.Registration{ + Contact: []core.AcmeURL{core.AcmeURL(*mailto)}, + } + + _, err := ra.NewRegistration(input, ShortKey) + test.AssertError(t, err, "Should have rejected authorization with short key") +} + func TestNewAuthorization(t *testing.T) { _, _, sa, ra := initAuthorities(t) From 0882e34ec6d750fa55c2ad9f5fef5697713a5c54 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Wed, 6 May 2015 16:51:13 -0700 Subject: [PATCH 4/4] Add testing for small prime divisibility. --- core/good_key.go | 20 +++++++++++++++++--- core/good_key_test.go | 21 +++++++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/core/good_key.go b/core/good_key.go index 5749f2c5e..c3776020b 100644 --- a/core/good_key.go +++ b/core/good_key.go @@ -10,11 +10,17 @@ import ( "crypto/rsa" "fmt" "reflect" + "math/big" blog "github.com/letsencrypt/boulder/log" "github.com/letsencrypt/boulder/jose" ) +// To generate, run: primes 2 752 | tr '\n' , +var smallPrimes = []int64{ + 2,3,5,7,11,13,17,19,23,29,31,37,41,43,47,53,59,61,67,71,73,79,83,89,97,101,103,107,109,113,127,131,137,139,149,151,157,163,167,173,179,181,191,193,197,199,211,223,227,229,233,239,241,251,257,263,269,271,277,281,283,293,307,311,313,317,331,337,347,349,353,359,367,373,379,383,389,397,401,409,419,421,431,433,439,443,449,457,461,463,467,479,487,491,499,503,509,521,523,541,547,557,563,569,571,577,587,593,599,601,607,613,617,619,631,641,643,647,653,659,661,673,677,683,691,701,709,719,727,733,739,743,751, +} + // 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. @@ -52,8 +58,16 @@ func GoodKey(key crypto.PublicKey) bool { log.Debug(fmt.Sprintf("Key exponent is too small: %d", rsaKey.E)) return false } - // TODO: The modulus SHOULD also have the following - // characteristics: an odd number, not the power of a prime, - // and have no factors smaller than 752. + // 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." + for _, prime := range smallPrimes { + var result big.Int + result.Mod(modulus, big.NewInt(prime)) + if result.Sign() == 0 { + log.Debug(fmt.Sprintf("Key divisible by small prime: %d", prime)) + return false + } + } return true } diff --git a/core/good_key_test.go b/core/good_key_test.go index d7d3c9c8d..7bd859002 100644 --- a/core/good_key_test.go +++ b/core/good_key_test.go @@ -44,6 +44,27 @@ func TestEvenExponent(t *testing.T) { test.Assert(t, !GoodKey(&key), "Should have rejected even exponent.") } +func TestEvenModulus(t *testing.T) { + bigOne := big.NewInt(1) + key := rsa.PublicKey{ + N: bigOne.Lsh(bigOne, 2048), + E: (1 << 17) + 1, + } + test.Assert(t, !GoodKey(&key), "Should have rejected even exponent.") +} + +func TestModulusDivisibleBy752(t *testing.T) { + N := big.NewInt(1) + N.Lsh(N, 2048) + N.Add(N, big.NewInt(1)) + N.Mul(N, big.NewInt(751)) + key := rsa.PublicKey{ + N: N, + E: (1 << 17) + 1, + } + test.Assert(t, !GoodKey(&key), "Should have rejected even exponent.") +} + func TestGoodKey(t *testing.T) { private, err := rsa.GenerateKey(rand.Reader, 2048) test.AssertNotError(t, err, "Error generating key")