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 <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: rolandshoemaker

Diff for: 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 for: src/crypto/x509/x509_test.go

@@ -3929,23 +3929,24 @@ 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}),
 		},
 	}
 
 	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)
+	}
+}