Skip to content

Commit

Permalink
crypto/x509: parse all names in an RDN.
Browse files Browse the repository at this point in the history
The Subject and Issuer names in a certificate look like they should be a
list of key-value pairs. However, they're actually a list of lists of
key-value pairs. Previously we only looked at the first element of each
sublist and the vast majority of certificates only have one element per
sublist.

However, it's possible to have multiple elements and some 360
certificates from the “Pilot” log are so constructed.

This change causes all elements of the sublists to be processed.

Fixes #16836.

Change-Id: Ie0a5159135b08226ec517fcf251aa17aada37857
Reviewed-on: https://go-review.googlesource.com/30810
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
  • Loading branch information
agl committed Oct 11, 2016
1 parent c536812 commit 809a1de
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 27 deletions.
56 changes: 29 additions & 27 deletions src/crypto/x509/pkix/pkix.go
Expand Up @@ -64,34 +64,36 @@ func (n *Name) FillFromRDNSequence(rdns *RDNSequence) {
if len(rdn) == 0 {
continue
}
atv := rdn[0]
n.Names = append(n.Names, atv)
value, ok := atv.Value.(string)
if !ok {
continue
}

t := atv.Type
if len(t) == 4 && t[0] == 2 && t[1] == 5 && t[2] == 4 {
switch t[3] {
case 3:
n.CommonName = value
case 5:
n.SerialNumber = value
case 6:
n.Country = append(n.Country, value)
case 7:
n.Locality = append(n.Locality, value)
case 8:
n.Province = append(n.Province, value)
case 9:
n.StreetAddress = append(n.StreetAddress, value)
case 10:
n.Organization = append(n.Organization, value)
case 11:
n.OrganizationalUnit = append(n.OrganizationalUnit, value)
case 17:
n.PostalCode = append(n.PostalCode, value)
for _, atv := range rdn {
n.Names = append(n.Names, atv)
value, ok := atv.Value.(string)
if !ok {
continue
}

t := atv.Type
if len(t) == 4 && t[0] == 2 && t[1] == 5 && t[2] == 4 {
switch t[3] {
case 3:
n.CommonName = value
case 5:
n.SerialNumber = value
case 6:
n.Country = append(n.Country, value)
case 7:
n.Locality = append(n.Locality, value)
case 8:
n.Province = append(n.Province, value)
case 9:
n.StreetAddress = append(n.StreetAddress, value)
case 10:
n.Organization = append(n.Organization, value)
case 11:
n.OrganizationalUnit = append(n.OrganizationalUnit, value)
case 17:
n.PostalCode = append(n.PostalCode, value)
}
}
}
}
Expand Down
52 changes: 52 additions & 0 deletions src/crypto/x509/x509_test.go
Expand Up @@ -1405,3 +1405,55 @@ func TestISOOIDInCertificate(t *testing.T) {
t.Errorf("ISO OID not recognised in certificate")
}
}

// certMultipleRDN contains a RelativeDistinguishedName with two elements (the
// common name and serial number). This particular certificate was the first
// such certificate in the “Pilot” Certificate Transparency log.
const certMultipleRDN = `
-----BEGIN CERTIFICATE-----
MIIFRzCCBC+gAwIBAgIEOl59NTANBgkqhkiG9w0BAQUFADA9MQswCQYDVQQGEwJz
aTEbMBkGA1UEChMSc3RhdGUtaW5zdGl0dXRpb25zMREwDwYDVQQLEwhzaWdvdi1j
YTAeFw0xMjExMTYxMDUyNTdaFw0xNzExMTYxMjQ5MDVaMIGLMQswCQYDVQQGEwJz
aTEbMBkGA1UEChMSc3RhdGUtaW5zdGl0dXRpb25zMRkwFwYDVQQLExB3ZWItY2Vy
dGlmaWNhdGVzMRAwDgYDVQQLEwdTZXJ2ZXJzMTIwFAYDVQQFEw0xMjM2NDg0MDEw
MDEwMBoGA1UEAxMTZXBvcnRhbC5tc3MuZWR1cy5zaTCCASIwDQYJKoZIhvcNAQEB
BQADggEPADCCAQoCggEBAMrNkZH9MPuBTjMGNk3sJX8V+CkFx/4ru7RTlLS6dlYM
098dtSfJ3s2w0p/1NB9UmR8j0yS0Kg6yoZ3ShsSO4DWBtcQD8820a6BYwqxxQTNf
HSRZOc+N/4TQrvmK6t4k9Aw+YEYTMrWOU4UTeyhDeCcUsBdh7HjfWsVaqNky+2sv
oic3zP5gF+2QfPkvOoHT3FLR8olNhViIE6Kk3eFIEs4dkq/ZzlYdLb8pHQoj/sGI
zFmA5AFvm1HURqOmJriFjBwaCtn8AVEYOtQrnUCzJYu1ex8azyS2ZgYMX0u8A5Z/
y2aMS/B2W+H79WcgLpK28vPwe7vam0oFrVytAd+u65ECAwEAAaOCAf4wggH6MA4G
A1UdDwEB/wQEAwIFoDBABgNVHSAEOTA3MDUGCisGAQQBr1kBAwMwJzAlBggrBgEF
BQcCARYZaHR0cDovL3d3dy5jYS5nb3Yuc2kvY3BzLzAfBgNVHREEGDAWgRRwb2Rw
b3JhLm1pemtzQGdvdi5zaTCB8QYDVR0fBIHpMIHmMFWgU6BRpE8wTTELMAkGA1UE
BhMCc2kxGzAZBgNVBAoTEnN0YXRlLWluc3RpdHV0aW9uczERMA8GA1UECxMIc2ln
b3YtY2ExDjAMBgNVBAMTBUNSTDM5MIGMoIGJoIGGhldsZGFwOi8veDUwMC5nb3Yu
c2kvb3U9c2lnb3YtY2Esbz1zdGF0ZS1pbnN0aXR1dGlvbnMsYz1zaT9jZXJ0aWZp
Y2F0ZVJldm9jYXRpb25MaXN0P2Jhc2WGK2h0dHA6Ly93d3cuc2lnb3YtY2EuZ292
LnNpL2NybC9zaWdvdi1jYS5jcmwwKwYDVR0QBCQwIoAPMjAxMjExMTYxMDUyNTda
gQ8yMDE3MTExNjEyNDkwNVowHwYDVR0jBBgwFoAUHvjUU2uzgwbpBAZXAvmlv8ZY
PHIwHQYDVR0OBBYEFGI1Duuu+wTGDZka/xHNbwcbM69ZMAkGA1UdEwQCMAAwGQYJ
KoZIhvZ9B0EABAwwChsEVjcuMQMCA6gwDQYJKoZIhvcNAQEFBQADggEBAHny0K1y
BQznrzDu3DDpBcGYguKU0dvU9rqsV1ua4nxkriSMWjgsX6XJFDdDW60I3P4VWab5
ag5fZzbGqi8kva/CzGgZh+CES0aWCPy+4Gb8lwOTt+854/laaJvd6kgKTER7z7U9
9C86Ch2y4sXNwwwPJ1A9dmrZJZOcJjS/WYZgwaafY2Hdxub5jqPE5nehwYUPVu9R
uH6/skk4OEKcfOtN0hCnISOVuKYyS4ANARWRG5VGHIH06z3lGUVARFRJ61gtAprd
La+fgSS+LVZ+kU2TkeoWAKvGq8MAgDq4D4Xqwekg7WKFeuyusi/NI5rm40XgjBMF
DF72IUofoVt7wo0=
-----END CERTIFICATE-----`

func TestMultipleRDN(t *testing.T) {
block, _ := pem.Decode([]byte(certMultipleRDN))
cert, err := ParseCertificate(block.Bytes)
if err != nil {
t.Fatalf("certificate with two elements in an RDN failed to parse: %v", err)
}

if want := "eportal.mss.edus.si"; cert.Subject.CommonName != want {
t.Errorf("got common name of %q, but want %q", cert.Subject.CommonName, want)
}

if want := "1236484010010"; cert.Subject.SerialNumber != want {
t.Errorf("got serial number of %q, but want %q", cert.Subject.SerialNumber, want)
}
}

1 comment on commit 809a1de

@ramoas
Copy link

@ramoas ramoas commented on 809a1de Nov 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agl While this code successfully parses a multi-value RDN, it strips the set grouping. That is, ToRDNSequence after parsing such a certificate will not result in a multi-value RDN.

Without a breaking change to pkix.Name or the introduction of new fields/methods, I do not see an obvious way to preserve the multi-value nature of the RDN.

While multi-valued RDNs may not be common, they are clearly part of the RDN standard in RFC 5280, not erroneous certificate constructions like Bug #16836 seems to suggest.

If you decide to keep the parsing logic, I would at least add a note documenting the loss of information about the multi-value nature of an RDN.

Please sign in to comment.