crypto/x509: Certificate.Verify crash on macOS with Go 1.18

[bradfitz]

On macOS, on an M1 Mac running macOS 12.3 and Go 1.18, crypto/x509.(*Certificate).Verify crashes:

SIGTRAP: trace trap
PC=0x197f52664 m=13 sigcode=0

goroutine 0 [idle]:
crypto/x509/internal/macos.syscall(0x14000200480?, 0x140002329f0?, 0x1400024ca68?, 0x1001f770c?, 0x1400024ca38?, 0x100341f84?, 0x10091c100?)
	/Users/bradfitz/sdk/go1.18/src/runtime/sys_darwin.go:99 +0x40 fp=0x1400024c9d0 sp=0x1400024c970 pc=0x100228880
crypto/x509/internal/macos.CFRelease(0x140002329f0?)
	/Users/bradfitz/sdk/go1.18/src/crypto/x509/internal/macos/corefoundation.go:152 +0x44 fp=0x1400024ca20 sp=0x1400024c9d0 pc=0x100341bb4
crypto/x509/internal/macos.ReleaseCFArray(0x1400024caa8?)
	/Users/bradfitz/sdk/go1.18/src/crypto/x509/internal/macos/corefoundation.go:204 +0x34 fp=0x1400024ca50 sp=0x1400024ca20 pc=0x100341e24
crypto/x509.(*Certificate).systemVerify.func1()
	/Users/bradfitz/sdk/go1.18/src/crypto/x509/root_darwin.go:14 +0x2c fp=0x1400024ca70 sp=0x1400024ca50 pc=0x10034b58c
runtime.deferreturn()
	/Users/bradfitz/sdk/go1.18/src/runtime/panic.go:436 +0x38 fp=0x1400024cab0 sp=0x1400024ca70 pc=0x1001f7718
crypto/x509.(*Certificate).systemVerify(0x14000474000, 0x1400024ce80)
	/Users/bradfitz/sdk/go1.18/src/crypto/x509/root_darwin.go:35 +0x30c fp=0x1400024cd10 sp=0x1400024cab0 pc=0x10034affc
crypto/x509.(*Certificate).Verify(0x14000474000, {{0x14000036408, 0x14}, 0x140005e4750, 0x0, {0x0, 0x0, 0x0}, {0x0, 0x0, ...}, ...})
	/Users/bradfitz/sdk/go1.18/src/crypto/x509/verify.go:747 +0x478 fp=0x1400024ce70 sp=0x1400024cd10 pc=0x10034efd8
tailscale.com/net/tlsdial.Config.func1({0x304, 0x0, 0x0, 0x1301, {0x0, 0x0}, 0x1, {0x14000036408, 0x14}, {0x140002382e0, ...}, ...})
	/Users/bradfitz/src/tailscale.com/net/tlsdial/tlsdial.go:79 +0x168 fp=0x1400024cfb0 sp=0x1400024ce70 pc=0x1004894c8
crypto/tls.(*Conn).verifyServerCertificate(0x1400017ce00, {0x1400010fec0, 0x4, 0x4})
	/Users/bradfitz/sdk/go1.18/src/crypto/tls/handshake_client.go:893 +0x3cc fp=0x1400024d240 sp=0x1400024cfb0 pc=0x10036763c
crypto/tls.(*clientHandshakeStateTLS13).readServerCertificate(0x1400024d648)
	/Users/bradfitz/sdk/go1.18/src/crypto/tls/handshake_client_tls13.go:457 +0x280 fp=0x1400024d450 sp=0x1400024d240 pc=0x100369d60
crypto/tls.(*clientHandshakeStateTLS13).handshake(0x1400024d648)
	/Users/bradfitz/sdk/go1.18/src/crypto/tls/handshake_client_tls13.go:87 +0x1c0 fp=0x1400024d490 sp=0x1400024d450 pc=0x100368160
crypto/tls.(*Conn).clientHandshake(0x1400017ce00, {0x1009bb0a0, 0x1400036e600})
	/Users/bradfitz/sdk/go1.18/src/crypto/tls/handshake_client.go:219 +0x444 fp=0x1400024d720 sp=0x1400024d490 pc=0x100363cb4
crypto/tls.(*Conn).clientHandshake-fm({0x1009bb0a0?, 0x1400036e600?})
	<autogenerated>:1 +0x40 fp=0x1400024d750 sp=0x1400024d720 pc=0x100386640
crypto/tls.(*Conn).handshakeContext(0x1400017ce00, {0x1009bb0d8, 0x1400003c080})
	/Users/bradfitz/sdk/go1.18/src/crypto/tls/conn.go:1452 +0x3cc fp=0x1400024d830 sp=0x1400024d750 pc=0x10036232c
crypto/tls.(*Conn).HandshakeContext(...)
	/Users/bradfitz/sdk/go1.18/src/crypto/tls/conn.go:1402
crypto/tls.(*Conn).Handshake(...)
	/Users/bradfitz/sdk/go1.18/src/crypto/tls/conn.go:1386
tailscale.com/derp/derphttp.(*Client).connect(0x14000470000, {0x1009bb0a0, 0x140003de340}, {0x10073b067, 0x17})
	/Users/bradfitz/src/tailscale.com/derp/derphttp/derphttp_client.go:374 +0xe78 fp=0x1400024df90 sp=0x1400024d830 pc=0x10048b218
tailscale.com/derp/derphttp.(*Client).Connect(...)
	/Users/bradfitz/src/tailscale.com/derp/derphttp/derphttp_client.go:131
tailscale.com/wgengine/magicsock.(*Conn).derpWriteChanOfAddr.func2()
	/Users/bradfitz/src/tailscale.com/wgengine/magicsock/magicsock.go:1421 +0x44 fp=0x1400024dfd0 sp=0x1400024df90 pc=0x1005d5674
runtime.goexit()
	/Users/bradfitz/sdk/go1.18/src/runtime/asm_arm64.s:1259 +0x4 fp=0x1400024dfd0 sp=0x1400024dfd0 pc=0x10022b9a4
created by tailscale.com/wgengine/magicsock.(*Conn).derpWriteChanOfAddr
	/Users/bradfitz/src/tailscale.com/wgengine/magicsock/magicsock.go:1420 +0xc1c

/cc @rolandshoemaker @FiloSottile @ianlancetaylor @josharian

[bradfitz]

Minimal repro:

package main

import (
	"log"
	"net/http"
	"os"
)

func main() {
	res, err := http.Get("https://derp10.tailscale.com")
	if err != nil {
		log.Fatal(err)
	}
	res.Write(os.Stdout)
}

[bradfitz]

Interestingly, https://google.com works. So something about the cert? Lack of OCSP stapling? (it was made with autocert)

[AGWA]

I see that https://derp10.tailscale.com is serving an extra self-signed certificate at the end of its chain with a subject of CN = derpkey8dc58100b2493614ee1692831a461f3f4dd3f9b3b088e244f887f81b4906ac26 - maybe that's triggering the crash?

[bradfitz]

@AGWA, ah, right! We send that along to clients to save an RTT.

[moderation]

Same here

SIGTRAP: trace trap
PC=0x1a8f12768 m=0 sigcode=0

goroutine 0 [idle]:
crypto/x509/internal/macos.syscall(0x104f1dc00?, 0x1400000ea20?, 0x14000716a68?, 0x10438770c?, 0x14000716a38?, 0x1044d1f84?, 0x104aac100?)
	/Users/moderation/Applications/go/src/runtime/sys_darwin.go:99 +0x40 fp=0x140007169d0 sp=0x14000716970 pc=0x1043b8880
crypto/x509/internal/macos.CFRelease(0x1400000ea20?)
	/Users/moderation/Applications/go/src/crypto/x509/internal/macos/corefoundation.go:152 +0x44 fp=0x14000716a20 sp=0x140007169d0 pc=0x1044d1bb4
crypto/x509/internal/macos.ReleaseCFArray(0x14000716aa8?)
	/Users/moderation/Applications/go/src/crypto/x509/internal/macos/corefoundation.go:204 +0x34 fp=0x14000716a50 sp=0x14000716a20 pc=0x1044d1e24
crypto/x509.(*Certificate).systemVerify.func1()
	/Users/moderation/Applications/go/src/crypto/x509/root_darwin.go:14 +0x2c fp=0x14000716a70 sp=0x14000716a50 pc=0x1044db58c
runtime.deferreturn()
	/Users/moderation/Applications/go/src/runtime/panic.go:436 +0x38 fp=0x14000716ab0 sp=0x14000716a70 pc=0x104387718
crypto/x509.(*Certificate).systemVerify(0x140000e6100, 0x14000716e80)
	/Users/moderation/Applications/go/src/crypto/x509/root_darwin.go:35 +0x30c fp=0x14000716d10 sp=0x14000716ab0 pc=0x1044daffc
crypto/x509.(*Certificate).Verify(0x140000e6100, {{0x140002dc6d8, 0x13}, 0x1400049ad80, 0x0, {0x0, 0x0, 0x0}, {0x0, 0x0, ...}, ...})
	/Users/moderation/Applications/go/src/crypto/x509/verify.go:747 +0x478 fp=0x14000716e70 sp=0x14000716d10 pc=0x1044defd8
tailscale.com/net/tlsdial.Config.func1({0x304, 0x0, 0x0, 0x1301, {0x0, 0x0}, 0x1, {0x140002dc6d8, 0x13}, {0x140001e6220, ...}, ...})
	/Users/moderation/Applications/tailscale/net/tlsdial/tlsdial.go:79 +0x168 fp=0x14000716fb0 sp=0x14000716e70 pc=0x1046194c8
crypto/tls.(*Conn).verifyServerCertificate(0x14000533500, {0x1400024cc60, 0x4, 0x4})
	/Users/moderation/Applications/go/src/crypto/tls/handshake_client.go:893 +0x3cc fp=0x14000717240 sp=0x14000716fb0 pc=0x1044f763c
crypto/tls.(*clientHandshakeStateTLS13).readServerCertificate(0x14000717648)
	/Users/moderation/Applications/go/src/crypto/tls/handshake_client_tls13.go:457 +0x280 fp=0x14000717450 sp=0x14000717240 pc=0x1044f9d60
crypto/tls.(*clientHandshakeStateTLS13).handshake(0x14000717648)
	/Users/moderation/Applications/go/src/crypto/tls/handshake_client_tls13.go:87 +0x1c0 fp=0x14000717490 sp=0x14000717450 pc=0x1044f8160
crypto/tls.(*Conn).clientHandshake(0x14000533500, {0x104b4b0a0, 0x1400017a300})
	/Users/moderation/Applications/go/src/crypto/tls/handshake_client.go:219 +0x444 fp=0x14000717720 sp=0x14000717490 pc=0x1044f3cb4
crypto/tls.(*Conn).clientHandshake-fm({0x104b4b0a0?, 0x1400017a300?})
	<autogenerated>:1 +0x40 fp=0x14000717750 sp=0x14000717720 pc=0x104516640
crypto/tls.(*Conn).handshakeContext(0x14000533500, {0x104b4b0d8, 0x14000038090})
	/Users/moderation/Applications/go/src/crypto/tls/conn.go:1452 +0x3cc fp=0x14000717830 sp=0x14000717750 pc=0x1044f232c
crypto/tls.(*Conn).HandshakeContext(...)
	/Users/moderation/Applications/go/src/crypto/tls/conn.go:1402
crypto/tls.(*Conn).Handshake(...)
	/Users/moderation/Applications/go/src/crypto/tls/conn.go:1386
tailscale.com/derp/derphttp.(*Client).connect(0x1400068e000, {0x104b4b0a0, 0x1400048a240}, {0x1048cb067, 0x17})
	/Users/moderation/Applications/tailscale/derp/derphttp/derphttp_client.go:374 +0xe78 fp=0x14000717f90 sp=0x14000717830 pc=0x10461b218
tailscale.com/derp/derphttp.(*Client).Connect(...)
	/Users/moderation/Applications/tailscale/derp/derphttp/derphttp_client.go:131
tailscale.com/wgengine/magicsock.(*Conn).derpWriteChanOfAddr.func2()
	/Users/moderation/Applications/tailscale/wgengine/magicsock/magicsock.go:1421 +0x44 fp=0x14000717fd0 sp=0x14000717f90 pc=0x104765674
runtime.goexit()
	/Users/moderation/Applications/go/src/runtime/asm_arm64.s:1259 +0x4 fp=0x14000717fd0 sp=0x14000717fd0 pc=0x1043bb9a4
created by tailscale.com/wgengine/magicsock.(*Conn).derpWriteChanOfAddr
	/Users/moderation/Applications/tailscale/wgengine/magicsock/magicsock.go:1420 +0xc1c

[josharian]

Brad and I have a diagnosis, working on a fix and test. They'll be ready soon.

[gopherbot]

Change https://go.dev/cl/393655 mentions this issue: crypto/x509: fix Certificate.Verify crash

[josharian]

@bradfitz

diff --git a/src/crypto/x509/root_darwin.go b/src/crypto/x509/root_darwin.go
index 1ef9c0f71e..ad365f577e 100644
--- a/src/crypto/x509/root_darwin.go
+++ b/src/crypto/x509/root_darwin.go
@@ -13,6 +13,9 @@ func (c *Certificate) systemVerify(opts *VerifyOptions) (chains [][]*Certificate
 	certs := macOS.CFArrayCreateMutable()
 	defer macOS.ReleaseCFArray(certs)
 	leaf := macOS.SecCertificateCreateWithData(c.Raw)
+	if leaf == 0 {
+		return nil, errors.New("invalid leaf certificate")
+	}
 	macOS.CFArrayAppendValue(certs, leaf)
 	if opts.Intermediates != nil {
 		for _, lc := range opts.Intermediates.lazyCerts {
@@ -21,7 +24,9 @@ func (c *Certificate) systemVerify(opts *VerifyOptions) (chains [][]*Certificate
 				return nil, err
 			}
 			sc := macOS.SecCertificateCreateWithData(c.Raw)
-			macOS.CFArrayAppendValue(certs, sc)
+			if sc != 0 {
+				macOS.CFArrayAppendValue(certs, sc)
+			}
 		}
 	}
 
diff --git a/src/crypto/x509/verify_test.go b/src/crypto/x509/verify_test.go
index f4ea08bbf5..100a8ff0f9 100644
--- a/src/crypto/x509/verify_test.go
+++ b/src/crypto/x509/verify_test.go
@@ -1876,3 +1876,37 @@ func TestSystemRootsErrorUnwrap(t *testing.T) {
 		t.Error("errors.Is failed, wanted success")
 	}
 }
+
+func TestIssue51759(t *testing.T) {
+	// badCertData contains a cert that we parse as valid
+	// but that macOS SecCertificateCreateWithData rejects.
+	const badCertData = "0\x82\x01U0\x82\x01\a\xa0\x03\x02\x01\x02\x02\x01\x020\x05\x06\x03+ep0R1P0N\x06\x03U\x04\x03\x13Gderpkey8dc58100b2493614ee1692831a461f3f4dd3f9b3b088e244f887f81b4906ac260\x1e\x17\r220112235755Z\x17\r220313235755Z0R1P0N\x06\x03U\x04\x03\x13Gderpkey8dc58100b2493614ee1692831a461f3f4dd3f9b3b088e244f887f81b4906ac260*0\x05\x06\x03+ep\x03!\x00bA\xd8e\xadW\xcb\xefZ\x89\xb5\"\x1eR\x9d\xba\x0e:\x1042Q@\u007f\xbd\xfb{ks\x04\xd1£\x020\x000\x05\x06\x03+ep\x03A\x00[\xa7\x06y\x86(\x94\x97\x9eLwA\x00\x01x\xaa\xbc\xbd Ê]\n(΅!ف0\xf5\x9a%I\x19<\xffo\xf1\xeaaf@\xb1\xa7\xaf\xfd\xe9R\xc7\x0f\x8d&\xd5\xfc\x0f;Ϙ\x82\x84a\xbc\r"
+	badCert, err := ParseCertificate([]byte(badCertData))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	t.Run("leaf", func(t *testing.T) {
+		opts := VerifyOptions{}
+		_, err = badCert.Verify(opts)
+		if err == nil {
+			t.Fatal("expected error")
+		}
+	})
+
+	goodCert, err := certificateFromPEM(googleLeaf)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	t.Run("intermediate", func(t *testing.T) {
+		opts := VerifyOptions{
+			Intermediates: NewCertPool(),
+		}
+		opts.Intermediates.AddCert(badCert)
+		_, err = goodCert.Verify(opts)
+		if err == nil {
+			t.Fatal("expected error")
+		}
+	})
+}

[bradfitz]

@gopherbot please consider this for backport to 1.18, it's a regression