[go] crypto/x509: fix certificate policy marshaling
44 views
Skip to first unread message
Roland Shoemaker (Gerrit)
Nov 2, 2023, 1:13:33 PM11/2/23
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Roland Shoemaker has uploaded this change for review.
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.
Change-Id: I0fcbd3ceaab7a376e7e991ff8b37e2145ffb4a61
---
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
2 files changed, 37 insertions(+), 3 deletions(-)
diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go
index c710655..fa2785a 100644
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -1186,7 +1186,7 @@
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 @@
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 @@
// - PermittedIPRanges
// - PermittedURIDomains
// - PolicyIdentifiers
+// - Policies
// - SerialNumber
// - SignatureAlgorithm
// - Subject
@@ -1557,6 +1571,9 @@
//
// 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 bdc0321..f32c390 100644
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -3929,6 +3929,7 @@
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 @@
}
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 @@
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)
+ }
+}
To view, visit change 539297. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Nov 2, 2023, 1:14:18 PM11/2/23
to goph...@pubsubhelper.golang.org, Mateusz Poliwczak, Damien Neil, Filippo Valsorda, golang-co...@googlegroups.com
Attention is currently required from: Damien Neil, Mateusz Poliwczak.
Patch set 1:Commit-Queue +1
Roland Shoemaker (Gerrit)
Nov 2, 2023, 1:17:00 PM11/2/23
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Damien Neil, Mateusz Poliwczak.
Roland Shoemaker uploaded patch set #2 to this change.
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
---
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
2 files changed, 37 insertions(+), 3 deletions(-)
To view, visit change 539297. To unsubscribe, or for help writing mail filters, visit settings.
Mateusz Poliwczak (Gerrit)
Nov 2, 2023, 1:27:24 PM11/2/23
to Roland Shoemaker, goph...@pubsubhelper.golang.org, Go LUCI, Damien Neil, Filippo Valsorda, golang-co...@googlegroups.com
Attention is currently required from: Damien Neil, Roland Shoemaker.
Patch set 2:Run-TryBot +1
1 comment:
Patchset:
Good catch!
To view, visit change 539297. To unsubscribe, or for help writing mail filters, visit settings.
Mateusz Poliwczak (Gerrit)
Nov 2, 2023, 1:27:33 PM11/2/23
to Roland Shoemaker, goph...@pubsubhelper.golang.org, Go LUCI, Damien Neil, Filippo Valsorda, golang-co...@googlegroups.com
Attention is currently required from: Damien Neil, Roland Shoemaker.
Patch set 2:-Run-TryBotCode-Review +1
Damien Neil (Gerrit)
Nov 2, 2023, 4:15:22 PM11/2/23
to Roland Shoemaker, goph...@pubsubhelper.golang.org, Mateusz Poliwczak, Go LUCI, Filippo Valsorda, golang-co...@googlegroups.com
Attention is currently required from: Roland Shoemaker.
Patch set 2:Code-Review +2
Roland Shoemaker (Gerrit)
Nov 3, 2023, 11:20:39 AM11/3/23
to goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Damien Neil, Mateusz Poliwczak, Go LUCI, Filippo Valsorda, golang-co...@googlegroups.com
Roland Shoemaker submitted this change.
Approvals:
Mateusz Poliwczak: Looks good to me, but someone else must approve
Go LUCI: TryBots succeeded
Damien Neil: Looks good to me, approved
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 <mpoliw...@gmail.com>
Reviewed-by: Damien Neil <dn...@google.com>
LUCI-TryBot-Result: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>
---
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
2 files changed, 37 insertions(+), 3 deletions(-)
To view, visit change 539297. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages