[go] crypto/x509: allow parsing of certificates with unknown critical extensions.

[Adam Langley (Gerrit)]

Adam Langley uploaded a change:
https://go-review.googlesource.com/9390

crypto/x509: allow parsing of certificates with unknown critical extensions.

Previously, unknown critical extensions were a parse error. However, for
some cases one wishes to parse and use a certificate that may contain
these extensions. For example, when using a certificate in a TLS server:
it's the client's concern whether it understands the critical extensions
but the server still wishes to parse SNI values out of the certificate
etc.

This change moves the rejection of unknown critical extensions from
ParseCertificate to Certificate.Verify. The former will now record the
OIDs of unknown critical extensions in the Certificate and the latter
will fail to verify certificates with them. If a user of this package
wishes to handle any unknown critical extensions themselves, they can
extract the extensions from Certificate.Extensions, process them and
remove known OIDs from Certificate.UnknownCriticalExtensions.

See discussion at
https://groups.google.com/forum/#!msg/golang-nuts/IrzoZlwalTQ/qdK1k-ogeHIJ
and in the linked bug.

Fixes #10459

Change-Id: I762521a44c01160fa0901f990ba2f5d4977d7977
---
M src/crypto/x509/verify.go
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
3 files changed, 47 insertions(+), 11 deletions(-)



diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go
index 7226d0a..21b870c 100644
--- a/src/crypto/x509/verify.go
+++ b/src/crypto/x509/verify.go
@@ -215,6 +215,10 @@
return c.systemVerify(&opts)
}

+ if len(c.UnhandledCriticalExtensions) > 0 {
+ return nil, UnhandledCriticalExtension{}
+ }
+
if opts.Roots == nil {
opts.Roots = systemRootsPool()
if opts.Roots == nil {
diff --git a/src/crypto/x509/x509.go b/src/crypto/x509/x509.go
index 71b0804..1aefe44 100644
--- a/src/crypto/x509/x509.go
+++ b/src/crypto/x509/x509.go
@@ -488,6 +488,16 @@
// field is not populated when parsing certificates, see Extensions.
ExtraExtensions []pkix.Extension

+ // UnhandledCriticalExtensions contains a list of extension IDs that
+ // were not (fully) processed when parsing. Verify will fail if this
+ // slice is non-empty, unless verification is delegated to an OS
+ // library which understand all the critical extensions.
+ //
+ // Users can access these extensions using Extensions and can remove
+ // elements from this slice if they believe that they have been
+ // handled.
+ UnhandledCriticalExtensions []asn1.ObjectIdentifier
+
ExtKeyUsage []ExtKeyUsage // Sequence of extended key
usages.
UnknownExtKeyUsage []asn1.ObjectIdentifier // Encountered extended key
usages unknown to this package.

@@ -897,7 +907,7 @@

for _, e := range in.TBSCertificate.Extensions {
out.Extensions = append(out.Extensions, e)
- failIfCritical := false
+ unhandled := false

if len(e.Id) == 4 && e.Id[0] == 2 && e.Id[1] == 5 && e.Id[2] == 29 {
switch e.Id[3] {
@@ -936,7 +946,7 @@

if len(out.DNSNames) == 0 && len(out.EmailAddresses) == 0 &&
len(out.IPAddresses) == 0 {
// If we didn't parse anything then we do the critical check, below.
- failIfCritical = true
+ unhandled = true
}

case 30:
@@ -1054,8 +1064,8 @@
}

default:
- // Unknown extensions cause an error if marked as critical.
- failIfCritical = true
+ // Unknown extensions are recorded if critical.
+ unhandled = true
}
} else if e.Id.Equal(oidExtensionAuthorityInfoAccess) {
// RFC 5280 4.2.2.1: Authority Information Access
@@ -1076,12 +1086,12 @@
}
}
} else {
- // Unknown extensions cause an error if marked as critical.
- failIfCritical = true
+ // Unknown extensions are recorded if critical.
+ unhandled = true
}

- if e.Critical && failIfCritical {
- return out, UnhandledCriticalExtension{}
+ if e.Critical && unhandled {
+ out.UnhandledCriticalExtensions =
append(out.UnhandledCriticalExtensions, e.Id)
}
}

diff --git a/src/crypto/x509/x509_test.go b/src/crypto/x509/x509_test.go
index 95efaf3..86a8b16 100644
--- a/src/crypto/x509/x509_test.go
+++ b/src/crypto/x509/x509_test.go
@@ -509,7 +509,13 @@
CommonName: "foo",
},
NotBefore: time.Unix(1000, 0),
- NotAfter: time.Unix(100000, 0),
+ NotAfter: time.Now().AddDate(1, 0, 0),
+
+ BasicConstraintsValid: true,
+ IsCA: true,
+
+ KeyUsage: KeyUsageCertSign,
+ ExtKeyUsage: []ExtKeyUsage{ExtKeyUsageServerAuth},

ExtraExtensions: []pkix.Extension{
{
@@ -525,13 +531,29 @@
t.Fatalf("failed to create certificate: %s", err)
}

- _, err = ParseCertificate(derBytes)
+ cert, err := ParseCertificate(derBytes)
+ if err != nil {
+ t.Fatalf("Certificate with unknown critical extension was not
parsed: %s", err)
+ }
+
+ roots := NewCertPool()
+ roots.AddCert(cert)
+
+ // Setting Roots ensures that Verify won't delegate to the OS
+ // library and thus the correct error should always be
+ // returned.
+ _, err = cert.Verify(VerifyOptions{Roots: roots})
if err == nil {
- t.Fatalf("Certificate with critical extension was parsed without
error.")
+ t.Fatal("Certificate with unknown critical extension was verified
without error")
}
if _, ok := err.(UnhandledCriticalExtension); !ok {
t.Fatalf("Error was %#v, but wanted one of type
UnhandledCriticalExtension", err)
}
+
+ cert.UnhandledCriticalExtensions = nil
+ if _, err = cert.Verify(VerifyOptions{Roots: roots}); err != nil {
+ t.Errorf("Certificate failed to verify after unhandled critical
extensions were cleared: %s", err)
+ }
}
}


--
https://go-review.googlesource.com/9390

[Brad Fitzpatrick (Gerrit)]

Brad Fitzpatrick has posted comments on this change.

crypto/x509: allow parsing of certificates with unknown critical extensions.

Patch Set 1: Code-Review+2

(1 comment)

https://go-review.googlesource.com/#/c/9390/1/src/crypto/x509/verify.go
File src/crypto/x509/verify.go:

Line 219: return nil, UnhandledCriticalExtension{}
Guess we named this wrong long ago. (should end in Error) Oh well.


--
https://go-review.googlesource.com/9390
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-HasComments: Yes

[Nathan Youngman (Gerrit)]

Nathan Youngman has posted comments on this change.

crypto/x509: allow parsing of certificates with unknown critical extensions.

Patch Set 1:

(1 comment)

https://go-review.googlesource.com/#/c/9390/1/src/crypto/x509/verify.go
File src/crypto/x509/verify.go:

Line 219: return nil, UnhandledCriticalExtension{}
> Guess we named this wrong long ago. (should end in Error) Oh well.

Recently I ran into IncorrectPasswordError which should actually be
ErrIncorrectPassword as well.

Does it make sense to introduce a new exported type
UnhandledCriticalExtensionError that is equal to UnhandledCriticalExtension
(kept for backwards compatibility)?

--
https://go-review.googlesource.com/9390
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>

Gerrit-Reviewer: Nathan Youngman <he...@nathany.com>
Gerrit-HasComments: Yes

[Adam Langley (Gerrit)]

Adam Langley has posted comments on this change.

crypto/x509: allow parsing of certificates with unknown critical extensions.

Patch Set 1: Run-TryBot+1

(1 comment)

https://go-review.googlesource.com/#/c/9390/1/src/crypto/x509/verify.go
File src/crypto/x509/verify.go:

Line 219: return nil, UnhandledCriticalExtension{}

> Recently I ran into IncorrectPasswordError which should actually be
> ErrInco

I think these errors probably predated the time when we had a naming
standard for errors and then escaped clean up :(

Since people will typically be type casting from |error| to the specific
type, it's not obvious how to add an alias with a conforming name.

Maybe if the error was an interface. Since it doesn't contain anything in
this case that's possible, but it could still break code.


--
https://go-review.googlesource.com/9390
Gerrit-Reviewer: Adam Langley <a...@golang.org>

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

crypto/x509: allow parsing of certificates with unknown critical extensions.

Patch Set 1:

TryBots beginning. Status page: http://farmer.golang.org/try?commit=30d20daa

--
https://go-review.googlesource.com/9390
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>

Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-Reviewer: Nathan Youngman <he...@nathany.com>
Gerrit-HasComments: No

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

crypto/x509: allow parsing of certificates with unknown critical extensions.

Patch Set 1:

This change failed on freebsd-amd64-gce101:
See
https://storage.googleapis.com/go-build-log/30d20daa/freebsd-amd64-gce101_fedfefb6.log

Consult https://build.golang.org/ to see whether it's a new failure.

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

crypto/x509: allow parsing of certificates with unknown critical extensions.

Patch Set 1:

This change failed on freebsd-386-gce101:
See
https://storage.googleapis.com/go-build-log/30d20daa/freebsd-386-gce101_bf137701.log

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

crypto/x509: allow parsing of certificates with unknown critical extensions.

Patch Set 1:

This change failed on windows-amd64-gce:
See
https://storage.googleapis.com/go-build-log/30d20daa/windows-amd64-gce_41c36c7b.log

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

crypto/x509: allow parsing of certificates with unknown critical extensions.

Patch Set 1:

This change failed on linux-amd64-race:
See
https://storage.googleapis.com/go-build-log/30d20daa/linux-amd64-race_c28328d0.log

[Adam Langley (Gerrit)]

Reviewers: Brad Fitzpatrick

Adam Langley uploaded a new patch set:

[Adam Langley (Gerrit)]

Adam Langley has submitted this change and it was merged.

crypto/x509: allow parsing of certificates with unknown critical extensions.

Previously, unknown critical extensions were a parse error. However, for
some cases one wishes to parse and use a certificate that may contain
these extensions. For example, when using a certificate in a TLS server:
it's the client's concern whether it understands the critical extensions
but the server still wishes to parse SNI values out of the certificate
etc.

This change moves the rejection of unknown critical extensions from
ParseCertificate to Certificate.Verify. The former will now record the
OIDs of unknown critical extensions in the Certificate and the latter
will fail to verify certificates with them. If a user of this package
wishes to handle any unknown critical extensions themselves, they can
extract the extensions from Certificate.Extensions, process them and
remove known OIDs from Certificate.UnknownCriticalExtensions.

See discussion at
https://groups.google.com/forum/#!msg/golang-nuts/IrzoZlwalTQ/qdK1k-ogeHIJ
and in the linked bug.

Fixes #10459

Change-Id: I762521a44c01160fa0901f990ba2f5d4977d7977

Reviewed-on: https://go-review.googlesource.com/9390
Reviewed-by: Brad Fitzpatrick <brad...@golang.org>

---
M src/crypto/x509/verify.go
M src/crypto/x509/x509.go
M src/crypto/x509/x509_test.go
3 files changed, 47 insertions(+), 11 deletions(-)

Approvals:
Brad Fitzpatrick: Looks good to me, approved

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

crypto/x509: allow parsing of certificates with unknown critical extensions.

Patch Set 1:

4 of 12 TryBots failed: freebsd-amd64-gce101, freebsd-386-gce101,
windows-amd64-gce, linux-amd64-race

--
https://go-review.googlesource.com/9390
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-Reviewer: Nathan Youngman <he...@nathany.com>

Gerrit-HasComments: No

[Nathan Youngman (Gerrit)]

Nathan Youngman has posted comments on this change.

crypto/x509: allow parsing of certificates with unknown critical extensions.

Patch Set 1:

(1 comment)

I tested this by loading an iPhone Software Development Signing certificate
as from #10459. It worked without error (using tls.X509KeyPair).

I haven't figured out how to use Verify on the cert yet, but just wanted to
let you know that it solves the issue I reported. Thanks.

https://go-review.googlesource.com/#/c/9390/1/src/crypto/x509/x509.go
File src/crypto/x509/x509.go:

Line 494: understand
understands

--
https://go-review.googlesource.com/9390
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-Reviewer: Nathan Youngman <he...@nathany.com>

Gerrit-HasComments: Yes