crypto/x509: fix certificate policy marshaling

CL 520535 added the new OID type, and the Certificate field Policies to
replace PolicyIdentifiers. During review I missed three problems: (1)
the marshaling of Certificate didn't take into account the case where
both fields were populated with the same OIDs (which would be the case
if you parsed a certificate and used it as a template), (2)
buildCertExtensions only generated the certificate policies extension if
PolicyIdentifiers was populated, and (3) how we would marshal an empty
OID (i.e. OID{}).

This change makes marshaling a certificate with an empty OID an error,
and only adds a single copy of any OID that appears in both Policies and
PolicyIdentifiers to the certificate policies extension. This should
make the round trip behavior for certificates reasonable.

Additionally this change documents that CreateCertificate uses the
Policies field from the template, and fixes buildCertExtensions to
populate the certificate policies extension if _either_
PolicyIdentifiers or Policies is populated, not just PolicyIdentifiers.

Fixes #63909

Change-Id: I0fcbd3ceaab7a376e7e991ff8b37e2145ffb4a61
Reviewed-on: https://go-review.googlesource.com/c/go/+/539297
Reviewed-by: Mateusz Poliwczak <mpoliwczak34@gmail.com>
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go
index c7106553043b15643313f444e9d00708de8b16c8..fa2785aacecc3bbb9f14bec5fc2c17e5ecb8445f 100644 (file)
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -1186,7 +1186,7 @@ func buildCertExtensions(template *Certificate, subjectIsEmpty bool, authorityKe
                n++
        }
 
-       if len(template.PolicyIdentifiers) > 0 &&
+       if (len(template.PolicyIdentifiers) > 0 || len(template.Policies) > 0) &&
                !oidInExtensions(oidExtensionCertificatePolicies, template.ExtraExtensions) {
                ret[n], err = marshalCertificatePolicies(template.Policies, template.PolicyIdentifiers)
                if err != nil {
@@ -1378,14 +1378,27 @@ func marshalCertificatePolicies(policies []OID, policyIdentifiers []asn1.ObjectI
 
        b := cryptobyte.NewBuilder(make([]byte, 0, 128))
        b.AddASN1(cryptobyte_asn1.SEQUENCE, func(child *cryptobyte.Builder) {
+               // added is used to track OIDs which are duplicated in both Policies and PolicyIdentifiers
+               // so they can be skipped. Note that this explicitly doesn't check for duplicate OIDs in
+               // Policies or in PolicyIdentifiers themselves, as this would be considered breaking behavior.
+               added := map[string]bool{}
                for _, v := range policies {
                        child.AddASN1(cryptobyte_asn1.SEQUENCE, func(child *cryptobyte.Builder) {
                                child.AddASN1(cryptobyte_asn1.OBJECT_IDENTIFIER, func(child *cryptobyte.Builder) {
+                                       oidStr := v.String()
+                                       added[oidStr] = true
+                                       if len(v.der) == 0 {
+                                               child.SetError(errors.New("invalid policy object identifier"))
+                                               return
+                                       }
                                        child.AddBytes(v.der)
                                })
                        })
                }
                for _, v := range policyIdentifiers {
+                       if added[v.String()] {
+                               continue
+                       }
                        child.AddASN1(cryptobyte_asn1.SEQUENCE, func(child *cryptobyte.Builder) {
                                child.AddASN1ObjectIdentifier(v)
                        })
@@ -1534,6 +1547,7 @@ var emptyASN1Subject = []byte{0x30, 0}
 //   - PermittedIPRanges
 //   - PermittedURIDomains
 //   - PolicyIdentifiers
+//   - Policies
 //   - SerialNumber
 //   - SignatureAlgorithm
 //   - Subject
@@ -1557,6 +1571,9 @@ var emptyASN1Subject = []byte{0x30, 0}
 //
 // If SubjectKeyId from template is empty and the template is a CA, SubjectKeyId
 // will be generated from the hash of the public key.
+//
+// If both PolicyIdentifiers and Policies are populated, any OID which appears
+// in both slices will only be added to the certificate policies extension once.
 func CreateCertificate(rand io.Reader, template, parent *Certificate, pub, priv any) ([]byte, error) {
        key, ok := priv.(crypto.Signer)
        if !ok {

diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index bdc03216bcbc625d6ceed4c9d9b799f8e50a47cd..f32c39090023a9b66f4b4d587f34fec44fa0410a 100644 (file)
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -3929,6 +3929,7 @@ func TestCertificateOIDPolicies(t *testing.T) {
                NotAfter:          time.Unix(100000, 0),
                PolicyIdentifiers: []asn1.ObjectIdentifier{[]int{1, 2, 3}},
                Policies: []OID{
+                       mustNewOIDFromInts(t, []uint64{1, 2, 3}),
                        mustNewOIDFromInts(t, []uint64{1, 2, 3, 4, 5}),
                        mustNewOIDFromInts(t, []uint64{1, 2, 3, math.MaxInt32}),
                        mustNewOIDFromInts(t, []uint64{1, 2, 3, math.MaxUint32, math.MaxUint64}),
@@ -3936,16 +3937,16 @@ func TestCertificateOIDPolicies(t *testing.T) {
        }
 
        var expectPolicyIdentifiers = []asn1.ObjectIdentifier{
+               []int{1, 2, 3},
                []int{1, 2, 3, 4, 5},
                []int{1, 2, 3, math.MaxInt32},
-               []int{1, 2, 3},
        }
 
        var expectPolicies = []OID{
+               mustNewOIDFromInts(t, []uint64{1, 2, 3}),
                mustNewOIDFromInts(t, []uint64{1, 2, 3, 4, 5}),
                mustNewOIDFromInts(t, []uint64{1, 2, 3, math.MaxInt32}),
                mustNewOIDFromInts(t, []uint64{1, 2, 3, math.MaxUint32, math.MaxUint64}),
-               mustNewOIDFromInts(t, []uint64{1, 2, 3}),
        }
 
        certDER, err := CreateCertificate(rand.Reader, &template, &template, rsaPrivateKey.Public(), rsaPrivateKey)
@@ -3966,3 +3967,19 @@ func TestCertificateOIDPolicies(t *testing.T) {
                t.Errorf("cert.Policies = %v, want: %v", cert.Policies, expectPolicies)
        }
 }
+
+func TestInvalidPolicyOID(t *testing.T) {
+       template := Certificate{
+               SerialNumber:      big.NewInt(1),
+               Subject:           pkix.Name{CommonName: "Cert"},
+               NotBefore:         time.Now(),
+               NotAfter:          time.Now().Add(time.Hour),
+               PolicyIdentifiers: []asn1.ObjectIdentifier{[]int{1, 2, 3}},
+               Policies:          []OID{OID{}},
+       }
+       _, err := CreateCertificate(rand.Reader, &template, &template, rsaPrivateKey.Public(), rsaPrivateKey)
+       expected := "invalid policy object identifier"
+       if err.Error() != expected {
+               t.Fatalf("CreateCertificate() unexpected error: %v, want: %v", err, expected)
+       }
+}