From 1b914ecf43e875a5134e7c36a5042822b7f890f8 Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Thu, 18 Jan 2024 12:51:13 -0800 Subject: [PATCH 1/4] [release-branch.go1.21] crypto/x509: make sure pub key is non-nil before interface conversion alreadyInChain assumes all keys fit a interface which contains the Equal method (which they do), but this ignores that certificates may have a nil key when PublicKeyAlgorithm is UnknownPublicKeyAlgorithm. In this case alreadyInChain panics. Check that the key is non-nil as part of considerCandidate (we are never going to build a chain containing UnknownPublicKeyAlgorithm anyway). For #65390 Fixes #65392 Fixes CVE-2024-24783 Change-Id: Ibdccc0a487e3368b6812be35daad2512220243f3 Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/2137282 Reviewed-by: Damien Neil Run-TryBot: Roland Shoemaker Reviewed-by: Tatiana Bradley Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/2173774 Reviewed-by: Roland Shoemaker Reviewed-by: Carlos Amedee Reviewed-on: https://go-review.googlesource.com/c/go/+/569238 Auto-Submit: Michael Knyszek LUCI-TryBot-Result: Go LUCI Reviewed-by: Carlos Amedee --- src/crypto/x509/verify.go | 4 ++++ src/crypto/x509/verify_test.go | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/src/crypto/x509/verify.go b/src/crypto/x509/verify.go index 9ef11466a47..b808f7d96b9 100644 --- a/src/crypto/x509/verify.go +++ b/src/crypto/x509/verify.go @@ -819,6 +819,10 @@ func (c *Certificate) buildChains(cache map[*Certificate][][]*Certificate, curre ) considerCandidate := func(certType int, candidate *Certificate) { + if candidate.PublicKey == nil { + return + } + for _, cert := range currentChain { if cert.Equal(candidate) { return diff --git a/src/crypto/x509/verify_test.go b/src/crypto/x509/verify_test.go index 9954a670da0..9da39ca94f6 100644 --- a/src/crypto/x509/verify_test.go +++ b/src/crypto/x509/verify_test.go @@ -1968,3 +1968,22 @@ func TestSystemRootsErrorUnwrap(t *testing.T) { t.Error("errors.Is failed, wanted success") } } + +func TestVerifyNilPubKey(t *testing.T) { + c := &Certificate{ + RawIssuer: []byte{1, 2, 3}, + AuthorityKeyId: []byte{1, 2, 3}, + } + opts := &VerifyOptions{} + opts.Roots = NewCertPool() + r := &Certificate{ + RawSubject: []byte{1, 2, 3}, + SubjectKeyId: []byte{1, 2, 3}, + } + opts.Roots.AddCert(r) + + _, err := c.buildChains(nil, []*Certificate{r}, nil, opts) + if _, ok := err.(UnknownAuthorityError); !ok { + t.Fatalf("buildChains returned unexpected error, got: %v, want %v", err, UnknownAuthorityError{}) + } +} -- 2.33.0