From 0df44e5d903b7b8cb375cc2dfd849d72df3c183c Mon Sep 17 00:00:00 2001 From: Jeff Hodges Date: Tue, 6 Oct 2015 00:19:10 -0700 Subject: [PATCH] clean up CSRs with capitalized letters This change lowercases domains before they are stored in the database and makes policy.WillingToIssue reject any domains with uppercase letters. Fixes #927. --- ca/certificate-authority.go | 12 ++-- ca/certificate-authority_test.go | 33 +++++++++- ca/testdata/capitalized_cn_and_san.der.csr | Bin 0 -> 716 bytes ca/testdata/testcsr.go | 69 +++++++++++++++++++++ core/util.go | 7 ++- core/util_test.go | 7 +++ policy/policy-authority.go | 8 +-- policy/policy-authority_test.go | 3 + ra/registration-authority.go | 6 +- ra/registration-authority_test.go | 19 ++++++ 10 files changed, 147 insertions(+), 17 deletions(-) create mode 100644 ca/testdata/capitalized_cn_and_san.der.csr create mode 100644 ca/testdata/testcsr.go diff --git a/ca/certificate-authority.go b/ca/certificate-authority.go index 34f5a23b2..c7706bdc1 100644 --- a/ca/certificate-authority.go +++ b/ca/certificate-authority.go @@ -16,6 +16,7 @@ import ( "fmt" "io/ioutil" "math/big" + "strings" "time" "github.com/letsencrypt/boulder/Godeps/_workspace/src/github.com/jmhodges/clock" @@ -223,7 +224,8 @@ func (ca *CertificateAuthorityImpl) RevokeCertificate(serial string, reasonCode } // IssueCertificate attempts to convert a CSR into a signed Certificate, while -// enforcing all policies. +// enforcing all policies. Names (domains) in the CertificateRequest will be +// lowercased before storage. func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest, regID int64) (core.Certificate, error) { emptyCert := core.Certificate{} var err error @@ -253,10 +255,10 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest hostNames := make([]string, len(csr.DNSNames)) copy(hostNames, csr.DNSNames) if len(csr.Subject.CommonName) > 0 { - commonName = csr.Subject.CommonName - hostNames = append(hostNames, csr.Subject.CommonName) + commonName = strings.ToLower(csr.Subject.CommonName) + hostNames = append(hostNames, commonName) } else if len(hostNames) > 0 { - commonName = hostNames[0] + commonName = strings.ToLower(hostNames[0]) } else { err = core.MalformedRequestError("Cannot issue a certificate without a hostname.") // AUDIT[ Certificate Requests ] 11917fa4-10ef-4e0d-9105-bacbe7836a3c @@ -265,7 +267,7 @@ func (ca *CertificateAuthorityImpl) IssueCertificate(csr x509.CertificateRequest } // Collapse any duplicate names. Note that this operation may re-order the names - hostNames = core.UniqueNames(hostNames) + hostNames = core.UniqueLowerNames(hostNames) if ca.MaxNames > 0 && len(hostNames) > ca.MaxNames { err = core.MalformedRequestError(fmt.Sprintf("Certificate request has %d > %d names", len(hostNames), ca.MaxNames)) ca.log.WarningErr(err) diff --git a/ca/certificate-authority_test.go b/ca/certificate-authority_test.go index 0d9873b69..c2de71d52 100644 --- a/ca/certificate-authority_test.go +++ b/ca/certificate-authority_test.go @@ -11,6 +11,7 @@ import ( "encoding/asn1" "fmt" "io/ioutil" + "sort" "testing" "time" @@ -64,7 +65,7 @@ var ( DupeNameCSR = mustRead("./testdata/dupe_name.der.csr") // CSR generated by Go: - // * Random pulic key + // * Random public key // * CN = [none] // * DNSNames = not-example.com, www.not-example.com, mail.example.com TooManyNameCSR = mustRead("./testdata/too_many_names.der.csr") @@ -81,7 +82,14 @@ var ( // * DNSNames = not-example.com, www.not-example.com, mail.not-example.com // * Signature algorithm: SHA1WithRSA BadAlgorithmCSR = mustRead("./testdata/bad_algorithm.der.csr") - log = mocks.UseMockLog() + + // CSR generated by Go: + // * Random public key + // * CN = CapiTalizedLetters.com + // * DNSNames = moreCAPs.com, morecaps.com, evenMOREcaps.com, Capitalizedletters.COM + CapitalizedCSR = mustRead("./testdata/capitalized_cn_and_san.der.csr") + + log = mocks.UseMockLog() ) // CFSSL config @@ -421,3 +429,24 @@ func TestRejectBadAlgorithm(t *testing.T) { _, ok := err.(core.MalformedRequestError) test.Assert(t, ok, "Incorrect error type returned") } + +func TestCapitalizedLetters(t *testing.T) { + ctx := setup(t) + defer ctx.cleanUp() + ctx.caConfig.MaxNames = 3 + ca, err := NewCertificateAuthorityImpl(ctx.caConfig, ctx.fc, caCertFile) + ca.Publisher = &mocks.Publisher{} + ca.PA = ctx.pa + ca.SA = ctx.sa + + csr, _ := x509.ParseCertificateRequest(CapitalizedCSR) + cert, err := ca.IssueCertificate(*csr, ctx.reg.ID) + test.AssertNotError(t, err, "Failed to gracefully handle a CSR with capitalized names") + + parsedCert, err := x509.ParseCertificate(cert.DER) + test.AssertNotError(t, err, "Error parsing certificate produced by CA") + test.AssertEquals(t, "capitalizedletters.com", parsedCert.Subject.CommonName) + sort.Strings(parsedCert.DNSNames) + expected := []string{"capitalizedletters.com", "evenmorecaps.com", "morecaps.com"} + test.AssertDeepEquals(t, expected, parsedCert.DNSNames) +} diff --git a/ca/testdata/capitalized_cn_and_san.der.csr b/ca/testdata/capitalized_cn_and_san.der.csr new file mode 100644 index 0000000000000000000000000000000000000000..a638dc09a222e4b0c192dc52d6fdf153e16b4eba GIT binary patch literal 716 zcmXqLVme{a#JGWpk-8bFQA7%v@{A>MR-aq4G-VU!vRwtRw z4+xph?Em!fsr3K15BYY9Y~#H!<(9V1tuIw4<{jz0DiLn~Jb>ep&%cuj-qNSOzo}tp zVBIzGL}spBP@pv4s`WYf9()@DAr~NMkGOt=F z?@oGJDX??r>SV`*ev35SCjKaOE6co-RK0pWSJuKub{S9aM?FcNed*5T-zUFX`U=Dx zQEB@W&#m<&*MRFuuAy#!@e@9?M@#l!ny~BU?c+NSUsRahVQS38%*epFAju#B9!8ve zhQS7b2L8Zsl@(<1FmP+)$;~fHb#@E@hbV-XoLB(i38a>#=K1;uxxz)jF;N1Ei5zH5 zIQ#n|#{xGv78n`!EEk;Le=pjyMDp_Vi#}hK*cs!Z^!j-pE|~wXv@+|*esdG8wI)te zdG1tve6sP4|Na9Jm9`C0PmP@G7CgB3x!K}oHn-wIb{(B{pZTKa?^(95(l_*oBA4o` z?Vk+g+2SRpHCJc;lbcj4YdB%2ub127&AIol2Q&*$6{}g}n4#I|^>62=e_5B;&o@{a zBV3o!nDy^reBP7oo}4SsioX*xnKyO$ft35YlD8&V|N6XG{;a9wxexrt!R*(gExvtp zRoZRYaIE~l-F7``UvEo(aeen@=kh&Q)!wNTx!m3}d+w$cDl8%@i|U^nwQrrWQPtd` ZaN~rmiS3D#x&6}ezwA`iJ$gvZ5CHPdHU$6x literal 0 HcmV?d00001 diff --git a/ca/testdata/testcsr.go b/ca/testdata/testcsr.go new file mode 100644 index 000000000..367c8d485 --- /dev/null +++ b/ca/testdata/testcsr.go @@ -0,0 +1,69 @@ +// Hack up the x509.CertificateRequest in here, run `go run testcsr.go`, and a +// DER-encoded CertificateRequest will be printed to stdout. +package main + +import ( + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "log" + "os" +) + +// A 2048-bit RSA private key +var pemPrivateKey = `-----BEGIN RSA PRIVATE KEY----- +MIIEowIBAAKCAQEA5cpXqfCaUDD+hf93j5jxbrhK4jrJAzfAEjeZj/Lx5Rv/7eEO +uhS2DdCU2is82vR6yJ7EidUYVz/nUAjSTP7JIEsbyvfsfACABbqRyGltHlJnULVH +y/EMjt9xKZf17T8tOLHVUEAJTxsvjKn4TMIQJTNrAqm/lNrUXmCIR41Go+3RBGC6 +YdAKEwcZMCzrjQGF06mC6/6xMmYMSMd6+VQRFIPpuPK/6BBp1Tgju2LleRC5uatj +QcFOoilGkfh1RnZp3GJ7q58KaqHiPmjl31rkY5vS3LP7yfU5TRBcxCSG8l8LKuRt +MArkbTEtj3PkDjbipL/SkLrZ28e5w9Egl4g1MwIDAQABAoIBABZqY5zPPK5f6SQ3 +JHmciMitL5jb9SncMV9VjyRMpa4cyh1xW9dpF81HMI4Ls7cELEoPuspbQDGaqTzU +b3dVT1dYHFDzWF1MSzDD3162cg+IKE3mMSfCzt/NCiPtj+7hv86NAmr+pCnUVBIb +rn4GXD7UwjaTSn4Bzr+aGREpxd9Nr0JdNQwxVHZ75A92vTihCfaXyMCjhW3JEpF9 +N89XehgidoGgtUxxeeb+WsO3nvVBpLv/HDxMTx/IDzvSA5nLlYMcqVzb7IJoeAQu +og0WJKlniYzvIdoQ6/hGydAW5sKd0qWh0JPYs7uLKAWrdAWvrFAp7//fYKVamalU +8pUu/WkCgYEA+tcTQ3qTnVh41O9YeM/7NULpIkuCAlR+PBRky294zho9nGQIPdaW +VNvyqqjLaHaXJVokYHbU4hDk6RbrhoWVd4Po/5g9cUkT1f6nrdZGRkg4XOCzHWvV +Yrqh3eYYX4bdiH5EhB78m0rrbjHfd7SF3cdYNzOUS2kJvCInYC6zPx8CgYEA6oRr +UhZFuoqRsEb28ELM8sHvdIMA/C3aWCu+nUGQ4gHSEb4uvuOD/7tQNuCaBioiXVPM +/4hjk9jHJcjYf5l33ANqIP7JiYAt4rzTWXF3iS6kQOhQhjksSlSnWqw0Uu1DtlpG +rzeG1ZkBuwH7Bx0yj4sGSz5sAvyF44aRsE6AC20CgYEArafWO0ISDb1hMbFdo44B +ELd45Pg3UluiZP+NZFWQ4cbC3pFWL1FvE+KNll5zK6fmLcLBKlM6QCOIBmKKvb+f +YXVeCg0ghFweMmkxNqUAU8nN02bwOa8ctFQWmaOhPgkFN2iLEJjPMsdkRA6c8ad1 +gbtvNBAuWyKlzawrbGgISesCgYBkGEjGLINubx5noqJbQee/5U6S6CdPezKqV2Fw +NT/ldul2cTn6d5krWYOPKKYU437vXokst8XooKm/Us41CAfEfCCcHKNgcLklAXsj +ve5LOwEYQw+7ekORJjiX1tAuZN51wmpQ9t4x5LB8ZQgDrU6bPbdd/jKTw7xRtGoS +Wi8EsQKBgG8iGy3+kVBIjKHxrN5jVs3vj/l/fQL0WRMLCMmVuDBfsKyy3f9n8R1B +/KdwoyQFwsLOyr5vAjiDgpFurXQbVyH4GDFiJGS1gb6MNcinwSTpsbOLLV7zgibX +A2NgiQ+UeWMia16dZVd6gGDlY3lQpeyLdsdDd+YppNfy9vedjbvT +-----END RSA PRIVATE KEY-----` + +func main() { + block, _ := pem.Decode([]byte(pemPrivateKey)) + rsaPriv, err := x509.ParsePKCS1PrivateKey(block.Bytes) + if err != nil { + log.Fatalf("Failed to parse private key: %s", err) + } + + req := &x509.CertificateRequest{ + Subject: pkix.Name{ + CommonName: "CapiTalizedLetters.com", + }, + DNSNames: []string{ + "moreCAPs.com", + "morecaps.com", + "evenMOREcaps.com", + "Capitalizedletters.COM", + }, + } + csr, err := x509.CreateCertificateRequest(rand.Reader, req, rsaPriv) + if err != nil { + log.Fatalf("unable to create CSR: %s", err) + } + _, err = os.Stdout.Write(csr) + if err != nil { + log.Fatalf("unable to write to stdout: %s", err) + } +} diff --git a/core/util.go b/core/util.go index 224d035cf..cc3027283 100644 --- a/core/util.go +++ b/core/util.go @@ -472,11 +472,12 @@ func GetBuildHost() (retID string) { return } -// UniqueNames returns the set of all unique names in the input. -func UniqueNames(names []string) (unique []string) { +// UniqueLowerNames returns the set of all unique names in the input after all +// of them are lowercased. The returned names will be in their lowercased form. +func UniqueLowerNames(names []string) (unique []string) { nameMap := make(map[string]int, len(names)) for _, name := range names { - nameMap[name] = 1 + nameMap[strings.ToLower(name)] = 1 } unique = make([]string, 0, len(nameMap)) diff --git a/core/util_test.go b/core/util_test.go index b3c5b71bb..6d26fa081 100644 --- a/core/util_test.go +++ b/core/util_test.go @@ -11,6 +11,7 @@ import ( "math" "math/big" "net/url" + "sort" "testing" "github.com/letsencrypt/boulder/Godeps/_workspace/src/github.com/letsencrypt/go-jose" @@ -113,3 +114,9 @@ func TestAcmeURL(t *testing.T) { a := (*AcmeURL)(u) test.AssertEquals(t, s, a.String()) } + +func TestUniqueLowerNames(t *testing.T) { + u := UniqueLowerNames([]string{"foobar.com", "fooBAR.com", "baz.com", "foobar.com", "bar.com", "bar.com"}) + sort.Strings(u) + test.AssertDeepEquals(t, []string{"bar.com", "baz.com", "foobar.com"}, u) +} diff --git a/policy/policy-authority.go b/policy/policy-authority.go index a5a08c07d..78c29429c 100644 --- a/policy/policy-authority.go +++ b/policy/policy-authority.go @@ -64,7 +64,7 @@ const ( whitelistedPartnerRegID = -1 ) -var dnsLabelRegexp = regexp.MustCompile("^[a-zA-Z0-9][a-zA-Z0-9-]{0,62}$") +var dnsLabelRegexp = regexp.MustCompile("^[a-z0-9][a-z0-9-]{0,62}$") var punycodeRegexp = regexp.MustCompile("^xn--") func isDNSCharacter(ch byte) bool { @@ -112,7 +112,8 @@ func (e SyntaxError) Error() string { return "Syntax error" } func (e NonPublicError) Error() string { return "Name does not end in a public suffix" } // WillingToIssue determines whether the CA is willing to issue for the provided -// identifier. +// identifier. It expects domains in id to be lowercase to prevent mismatched +// cases breaking queries. // // We place several criteria on identifiers we are willing to issue for: // @@ -132,8 +133,6 @@ func (e NonPublicError) Error() string { return "Name does not end in a // XXX: Is there any need for this method to be constant-time? We're // going to refuse to issue anyway, but timing could leak whether // names are on the blacklist. -// -// XXX: We should probably fold everything to lower-case somehow. func (pa PolicyAuthorityImpl) WillingToIssue(id core.AcmeIdentifier, regID int64) error { if id.Type != core.IdentifierDNS { return InvalidIdentifierError{} @@ -146,7 +145,6 @@ func (pa PolicyAuthorityImpl) WillingToIssue(id core.AcmeIdentifier, regID int64 } } - domain = strings.ToLower(domain) if len(domain) > 255 { return SyntaxError{} } diff --git a/policy/policy-authority_test.go b/policy/policy-authority_test.go index c3cd284f7..b74d6796a 100644 --- a/policy/policy-authority_test.go +++ b/policy/policy-authority_test.go @@ -86,6 +86,9 @@ func TestWillingToIssue(t *testing.T) { `zombocom`, `localhost`, `mail`, + + // disallow capitalized letters for #927 + `CapitalizedLetters.com`, } shouldBeNonPublic := []string{ diff --git a/ra/registration-authority.go b/ra/registration-authority.go index 5c8f161c9..455b8b2bc 100644 --- a/ra/registration-authority.go +++ b/ra/registration-authority.go @@ -186,7 +186,8 @@ func validateContacts(contacts []*core.AcmeURL, resolver core.DNSResolver, stats return } -// NewAuthorization constuct a new Authz from a request. +// NewAuthorization constuct a new Authz from a request. Values (domains) in +// request.Identifier will be lowercased before storage. func (ra *RegistrationAuthorityImpl) NewAuthorization(request core.Authorization, regID int64) (authz core.Authorization, err error) { reg, err := ra.SA.GetRegistration(regID) if err != nil { @@ -195,6 +196,7 @@ func (ra *RegistrationAuthorityImpl) NewAuthorization(request core.Authorization } identifier := request.Identifier + identifier.Value = strings.ToLower(identifier.Value) // Check that the identifier is present and appropriate if err = ra.PA.WillingToIssue(identifier, regID); err != nil { @@ -275,7 +277,7 @@ func (ra *RegistrationAuthorityImpl) MatchesCSR( if len(csr.Subject.CommonName) > 0 { hostNames = append(hostNames, csr.Subject.CommonName) } - hostNames = core.UniqueNames(hostNames) + hostNames = core.UniqueLowerNames(hostNames) if !core.KeyDigestEquals(parsedCertificate.PublicKey, csr.PublicKey) { err = core.InternalServerError("Generated certificate public key doesn't match CSR public key") diff --git a/ra/registration-authority_test.go b/ra/registration-authority_test.go index 77959a3a9..7ca53f869 100644 --- a/ra/registration-authority_test.go +++ b/ra/registration-authority_test.go @@ -403,6 +403,25 @@ func TestNewAuthorization(t *testing.T) { t.Log("DONE TestNewAuthorization") } +func TestNewAuthorizationCapitalLetters(t *testing.T) { + _, sa, ra, _, cleanUp := initAuthorities(t) + defer cleanUp() + + authzReq := core.Authorization{ + Identifier: core.AcmeIdentifier{ + Type: core.IdentifierDNS, + Value: "NOT-example.COM", + }, + } + authz, err := ra.NewAuthorization(authzReq, Registration.ID) + test.AssertNotError(t, err, "NewAuthorization failed") + test.AssertEquals(t, "not-example.com", authz.Identifier.Value) + + dbAuthz, err := sa.GetAuthorization(authz.ID) + test.AssertNotError(t, err, "Could not fetch authorization from database") + assertAuthzEqual(t, authz, dbAuthz) +} + func TestUpdateAuthorization(t *testing.T) { va, sa, ra, _, cleanUp := initAuthorities(t) defer cleanUp()