From a8b372fb365f4b69f0b06aa9c3e642e6aa022840 Mon Sep 17 00:00:00 2001 From: Filippo Valsorda Date: Tue, 21 Jan 2020 14:45:15 -0500 Subject: [release-branch.go1.12-security] crypto/x509: mitigate CVE-2020-0601 verification bypass on Windows An attacker can trick the Windows system verifier to use a poisoned set of elliptic curve parameters for a trusted root, allowing it to generate spoofed signatures. When this happens, the returned chain will present the unmodified original root, so the actual signatures won't verify (as they are invalid for the correct parameters). Simply double check them as a safety measure and mitigation. Windows users should still install the system security patch ASAP. This is the same mitigation adopted by Chromium: https://chromium-review.googlesource.com/c/chromium/src/+/1994434 Change-Id: I2c734f6fb2cb51d906c7fd77034318ffeeb3e146 Reviewed-on: https://go-review.googlesource.com/c/go/+/215905 Run-TryBot: Filippo Valsorda TryBot-Result: Gobot Gobot Reviewed-by: Ryan Sleevi Reviewed-by: Katie Hockman Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/647124 Reviewed-by: Filippo Valsorda --- src/crypto/x509/root_windows.go | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src/crypto/x509/root_windows.go b/src/crypto/x509/root_windows.go index 74d395df70..3da3d06e73 100644 --- a/src/crypto/x509/root_windows.go +++ b/src/crypto/x509/root_windows.go @@ -219,10 +219,26 @@ func (c *Certificate) systemVerify(opts *VerifyOptions) (chains [][]*Certificate if err != nil { return nil, err } + if len(chain) < 1 { + return nil, errors.New("x509: internal error: system verifier returned an empty chain") + } - chains = append(chains, chain) + // Mitigate CVE-2020-0601, where the Windows system verifier might be + // tricked into using custom curve parameters for a trusted root, by + // double-checking all ECDSA signatures. If the system was tricked into + // using spoofed parameters, the signature will be invalid for the correct + // ones we parsed. (We don't support custom curves ourselves.) + for i, parent := range chain[1:] { + if parent.PublicKeyAlgorithm != ECDSA { + continue + } + if err := parent.CheckSignature(chain[i].SignatureAlgorithm, + chain[i].RawTBSCertificate, chain[i].Signature); err != nil { + return nil, err + } + } - return chains, nil + return [][]*Certificate{chain}, nil } func loadSystemRoots() (*CertPool, error) { -- cgit v1.3 From 44bb3b4b5341f5bb373acb0b8130795f888c9ace Mon Sep 17 00:00:00 2001 From: Katie Hockman Date: Fri, 24 Jan 2020 15:29:12 -0500 Subject: [release-branch.go1.12-security] internal/x/crypto/cryptobyte: import security fix for 32-bit archs cryptobyte: fix panic due to malformed ASN.1 inputs on 32-bit archs When int is 32 bits wide (on 32-bit architectures like 386 and arm), an overflow could occur, causing a panic, due to malformed ASN.1 being passed to any of the ASN1 methods of String. Tested on linux/386 and darwin/amd64. This fixes CVE-2020-7919 and was found thanks to the Project Wycheproof test vectors. Change-Id: I8c9696a8bfad1b40ec877cd740dba3467d66ab54 Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/645211 Reviewed-by: Katie Hockman Reviewed-by: Adam Langley x/crypto/cryptobyte is used in crypto/x509 for parsing certificates. Malformed certificates might cause a panic during parsing on 32-bit architectures (like arm and 386). Change-Id: I3c619af508bacff84023be4d5a7c4992c2f20a56 Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/647483 Reviewed-by: Dmitri Shuralyov --- src/internal/x/crypto/cryptobyte/asn1.go | 5 +++-- src/internal/x/crypto/cryptobyte/asn1_test.go | 4 ++++ src/internal/x/crypto/cryptobyte/string.go | 7 +------ 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/internal/x/crypto/cryptobyte/asn1.go b/src/internal/x/crypto/cryptobyte/asn1.go index 2d40680ddd..758ac3a649 100644 --- a/src/internal/x/crypto/cryptobyte/asn1.go +++ b/src/internal/x/crypto/cryptobyte/asn1.go @@ -470,7 +470,8 @@ func (s *String) ReadASN1GeneralizedTime(out *time.Time) bool { // It reports whether the read was successful. func (s *String) ReadASN1BitString(out *encoding_asn1.BitString) bool { var bytes String - if !s.ReadASN1(&bytes, asn1.BIT_STRING) || len(bytes) == 0 { + if !s.ReadASN1(&bytes, asn1.BIT_STRING) || len(bytes) == 0 || + len(bytes)*8/8 != len(bytes) { return false } @@ -740,7 +741,7 @@ func (s *String) readASN1(out *String, outTag *asn1.Tag, skipHeader bool) bool { length = headerLen + len32 } - if uint32(int(length)) != length || !s.ReadBytes((*[]byte)(out), int(length)) { + if int(length) < 0 || !s.ReadBytes((*[]byte)(out), int(length)) { return false } if skipHeader && !out.Skip(int(headerLen)) { diff --git a/src/internal/x/crypto/cryptobyte/asn1_test.go b/src/internal/x/crypto/cryptobyte/asn1_test.go index ca28e3bcfb..f90d768edb 100644 --- a/src/internal/x/crypto/cryptobyte/asn1_test.go +++ b/src/internal/x/crypto/cryptobyte/asn1_test.go @@ -31,6 +31,10 @@ var readASN1TestData = []readASN1Test{ {"non-minimal length", append([]byte{0x30, 0x82, 0, 0x80}, make([]byte, 0x80)...), 0x30, false, nil}, {"invalid tag", []byte{0xa1, 3, 0x4, 1, 1}, 31, false, nil}, {"high tag", []byte{0x1f, 0x81, 0x80, 0x01, 2, 1, 2}, 0xff /* actually 0x4001, but tag is uint8 */, false, nil}, + {"2**31 - 1 length", []byte{0x30, 0x84, 0x7f, 0xff, 0xff, 0xff}, 0x30, false, nil}, + {"2**32 - 1 length", []byte{0x30, 0x84, 0xff, 0xff, 0xff, 0xff}, 0x30, false, nil}, + {"2**63 - 1 length", []byte{0x30, 0x88, 0x7f, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}, 0x30, false, nil}, + {"2**64 - 1 length", []byte{0x30, 0x88, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}, 0x30, false, nil}, } func TestReadASN1(t *testing.T) { diff --git a/src/internal/x/crypto/cryptobyte/string.go b/src/internal/x/crypto/cryptobyte/string.go index bd2ed2e207..a3ecf63828 100644 --- a/src/internal/x/crypto/cryptobyte/string.go +++ b/src/internal/x/crypto/cryptobyte/string.go @@ -24,7 +24,7 @@ type String []byte // read advances a String by n bytes and returns them. If less than n bytes // remain, it returns nil. func (s *String) read(n int) []byte { - if len(*s) < n { + if len(*s) < n || n < 0 { return nil } v := (*s)[:n] @@ -105,11 +105,6 @@ func (s *String) readLengthPrefixed(lenLen int, outChild *String) bool { length = length << 8 length = length | uint32(b) } - if int(length) < 0 { - // This currently cannot overflow because we read uint24 at most, but check - // anyway in case that changes in the future. - return false - } v := s.read(int(length)) if v == nil { return false -- cgit v1.3 From e60fc07b54375c9dcc5d6e28c9376926c450fd57 Mon Sep 17 00:00:00 2001 From: Katie Hockman Date: Mon, 27 Jan 2020 14:11:04 -0500 Subject: [release-branch.go1.12-security] doc: document Go 1.12.16 Change-Id: Ib8ac9bf5020d9ab126a8069378978d7dce3509dc Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/648870 Reviewed-by: Dmitri Shuralyov --- doc/devel/release.html | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/doc/devel/release.html b/doc/devel/release.html index 487367c35f..38a9040d5e 100644 --- a/doc/devel/release.html +++ b/doc/devel/release.html @@ -139,6 +139,13 @@ the Go +1.12.16 milestone on our issue tracker for details. +

+

go1.11 (released 2018/08/24)

-- cgit v1.3 From deac3221fc4cd365fb40d269dd56551e9d354356 Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Mon, 27 Jan 2020 16:36:40 -0500 Subject: [release-branch.go1.12-security] go1.12.16 Change-Id: Iea658e285670a897a45eca3756004f050763c64d Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/649301 Reviewed-by: Katie Hockman --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index b115ecfa6a..33bccfc8ee 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -go1.12.15 \ No newline at end of file +go1.12.16 \ No newline at end of file -- cgit v1.3