From: Roland Shoemaker Date: Thu, 2 Nov 2023 17:04:21 +0000 (-0700) Subject: crypto/x509: fix certificate policy marshaling X-Git-Tag: go1.22rc1~434 X-Git-Url: http://www.git.cypherpunks.ru/?a=commitdiff_plain;h=e2d9574b14b3db044331da0c6fadeb62315c644a;p=gostls13.git 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 Reviewed-by: Damien Neil LUCI-TryBot-Result: Go LUCI --- diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go index c710655304..fa2785aace 100644 --- 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 bdc03216bc..f32c390900 100644 --- 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) + } +}