Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crypto/tls: SCTs increment by one byte on resume during tls 1.0, tls 1.1, and 1.2 BoringSSL OnResume tests #68516

Open
monkwire opened this issue Jul 18, 2024 · 9 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@monkwire
Copy link

Go version

go version go1.23 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/clide/Library/Caches/go-build'
GOENV='/Users/clide/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/clide/Users/clide/programming/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/clide/Users/clide/programming/'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/clide/programming/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/clide/programming/go/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='devel go1.23-40cc90f1a3 Wed Jul 17 13:28:55 2024 -0400'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/clide/Library/Application Support/go/telemetry'
GCCGO='gccgo'
GOARM64='v8.0'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/3c/h4fmpgcs75l129kxndj1nsk00000gn/T/go-build1365661743=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

When running TestBogoSuite with the flags -enable-signed-cert-timestamps and -expect-signed-cert-timestamps, the last byte of an SCT increments by one during SendSCTListOnResume- tests for connections using TLS 1.0, TLS 1.1, and TLS 1.2. This does not happen when TLS 1.3 is used.

Here are the changes I made to see this:

diff --git a/src/crypto/tls/bogo_shim_test.go b/src/crypto/tls/bogo_shim_test.go
index ce01852aee..e22af8e216 100644
--- a/src/crypto/tls/bogo_shim_test.go
+++ b/src/crypto/tls/bogo_shim_test.go
@@ -44,6 +44,9 @@ var (
 
 	requireAnyClientCertificate = flag.Bool("require-any-client-certificate", false, "")
 
+	_                          = flag.Bool("enable-signed-cert-timestamps", false, "") // always enabled
+	expectSignedCertTimestamps = flag.String("expect-signed-cert-timestamps", "", "")
+
 	shimWritesFirst = flag.Bool("shim-writes-first", false, "")
 
 	resumeCount = flag.Int("resume-count", 0, "")
@@ -284,6 +287,9 @@ func bogoShim() {
 		}
 
 		cs := tlsConn.ConnectionState()
+		if len(cs.SignedCertificateTimestamps) != 0 {
+			log.Printf("tlsversion: %#v, resumeCount: %d, SCTs  %#v", VersionName(cs.Version), i, cs.SignedCertificateTimestamps)
+		}
 		if cs.HandshakeComplete {
 			if *expectALPN != "" && cs.NegotiatedProtocol != *expectALPN {
 				log.Fatalf("unexpected protocol negotiated: want %q, got %q", *expectALPN, cs.NegotiatedProtocol)

What did you see happen?

The SCTs change from [][]uint8{[]uint8{0x5, 0x6, 0x7, 0x8}} to [][]uint8{[]uint8{0x5, 0x6, 0x7, 0x9}} when resume count is 1 (only during OnResume tests).

go test ./crypto/tls -run "TestBogoSuite/(SignedCertificateTimestampList|SendSCTListOnResume)" -v produces:

=== RUN   TestBogoSuite
=== RUN   TestBogoSuite/SignedCertificateTimestampList-Client-TLS-TLS11
    bogo_shim_test.go:442: unexpected error output:
        2024/07/18 14:38:21 tlsversion: "TLS 1.1", resumeCount: 0, SCTs  [][]uint8{[]uint8{0x5, 0x6, 0x7, 0x8}}
        2024/07/18 14:38:21 tlsversion: "TLS 1.1", resumeCount: 1, SCTs  [][]uint8{[]uint8{0x5, 0x6, 0x7, 0x8}}
        
        
=== RUN   TestBogoSuite/SignedCertificateTimestampList-Server-TLS-TLS11
    bogo_shim_test.go:449: 
=== RUN   TestBogoSuite/SignedCertificateTimestampListEmptySCT-Client-TLS-TLS1
=== RUN   TestBogoSuite/SignedCertificateTimestampListEmpty-Client-TLS-TLS13
=== RUN   TestBogoSuite/SendSCTListOnResume-TLS-TLS12
    bogo_shim_test.go:442: unexpected error output:
        2024/07/18 14:38:21 tlsversion: "TLS 1.2", resumeCount: 0, SCTs  [][]uint8{[]uint8{0x5, 0x6, 0x7, 0x8}}
        2024/07/18 14:38:21 tlsversion: "TLS 1.2", resumeCount: 1, SCTs  [][]uint8{[]uint8{0x5, 0x6, 0x7, 0x9}}
        
        
=== RUN   TestBogoSuite/SignedCertificateTimestampListEmptySCT-Client-TLS-TLS11
=== RUN   TestBogoSuite/SendSCTListOnResume-TLS-TLS1
    bogo_shim_test.go:442: unexpected error output:
        2024/07/18 14:38:21 tlsversion: "TLS 1.0", resumeCount: 0, SCTs  [][]uint8{[]uint8{0x5, 0x6, 0x7, 0x8}}
        2024/07/18 14:38:21 tlsversion: "TLS 1.0", resumeCount: 1, SCTs  [][]uint8{[]uint8{0x5, 0x6, 0x7, 0x9}}
        
        
=== RUN   TestBogoSuite/SendSCTListOnResume-TLS-TLS11
    bogo_shim_test.go:442: unexpected error output:
        2024/07/18 14:38:21 tlsversion: "TLS 1.1", resumeCount: 0, SCTs  [][]uint8{[]uint8{0x5, 0x6, 0x7, 0x8}}
        2024/07/18 14:38:21 tlsversion: "TLS 1.1", resumeCount: 1, SCTs  [][]uint8{[]uint8{0x5, 0x6, 0x7, 0x9}}
        
        
=== RUN   TestBogoSuite/SignedCertificateTimestampList-Client-TLS-TLS12
    bogo_shim_test.go:442: unexpected error output:
        2024/07/18 14:38:21 tlsversion: "TLS 1.2", resumeCount: 0, SCTs  [][]uint8{[]uint8{0x5, 0x6, 0x7, 0x8}}
        2024/07/18 14:38:21 tlsversion: "TLS 1.2", resumeCount: 1, SCTs  [][]uint8{[]uint8{0x5, 0x6, 0x7, 0x8}}
        
        
=== RUN   TestBogoSuite/SendSCTListOnResume-TLS-TLS13
    bogo_shim_test.go:442: unexpected error output:
        2024/07/18 14:38:22 tlsversion: "TLS 1.3", resumeCount: 0, SCTs  [][]uint8{[]uint8{0x5, 0x6, 0x7, 0x8}}
        2024/07/18 14:38:22 tlsversion: "TLS 1.3", resumeCount: 1, SCTs  [][]uint8{[]uint8{0x5, 0x6, 0x7, 0x8}}
        
        
=== RUN   TestBogoSuite/SignedCertificateTimestampList-Server-TLS-TLS1
    bogo_shim_test.go:449: 
=== RUN   TestBogoSuite/SignedCertificateTimestampListEmpty-Client-TLS-TLS1
=== RUN   TestBogoSuite/SignedCertificateTimestampList-Client-TLS-TLS1
    bogo_shim_test.go:442: unexpected error output:
        2024/07/18 14:38:21 tlsversion: "TLS 1.0", resumeCount: 0, SCTs  [][]uint8{[]uint8{0x5, 0x6, 0x7, 0x8}}
        2024/07/18 14:38:21 tlsversion: "TLS 1.0", resumeCount: 1, SCTs  [][]uint8{[]uint8{0x5, 0x6, 0x7, 0x8}}
        
        
=== RUN   TestBogoSuite/SignedCertificateTimestampListEmptySCT-Client-TLS-TLS12
=== RUN   TestBogoSuite/SignedCertificateTimestampList-Server-TLS-TLS13
    bogo_shim_test.go:449: 
=== RUN   TestBogoSuite/SignedCertificateTimestampList-Server-TLS-TLS12
    bogo_shim_test.go:449: 
=== RUN   TestBogoSuite/SignedCertificateTimestampList-Client-TLS-TLS13
    bogo_shim_test.go:442: unexpected error output:
        2024/07/18 14:38:22 tlsversion: "TLS 1.3", resumeCount: 0, SCTs  [][]uint8{[]uint8{0x5, 0x6, 0x7, 0x8}}
        2024/07/18 14:38:22 tlsversion: "TLS 1.3", resumeCount: 1, SCTs  [][]uint8{[]uint8{0x5, 0x6, 0x7, 0x8}}
        
        
=== RUN   TestBogoSuite/SignedCertificateTimestampListEmpty-Client-TLS-TLS12
=== RUN   TestBogoSuite/SignedCertificateTimestampListInvalid-Server
    bogo_shim_test.go:449: 
=== RUN   TestBogoSuite/SignedCertificateTimestampListEmpty-Client-TLS-TLS11
=== RUN   TestBogoSuite/SignedCertificateTimestampListEmptySCT-Client-TLS-TLS13
--- FAIL: TestBogoSuite (6.14s)
    --- FAIL: TestBogoSuite/SignedCertificateTimestampList-Client-TLS-TLS11 (0.00s)
    --- SKIP: TestBogoSuite/SignedCertificateTimestampList-Server-TLS-TLS11 (0.00s)
    --- PASS: TestBogoSuite/SignedCertificateTimestampListEmptySCT-Client-TLS-TLS1 (0.00s)
    --- PASS: TestBogoSuite/SignedCertificateTimestampListEmpty-Client-TLS-TLS13 (0.00s)
    --- FAIL: TestBogoSuite/SendSCTListOnResume-TLS-TLS12 (0.00s)
    --- PASS: TestBogoSuite/SignedCertificateTimestampListEmptySCT-Client-TLS-TLS11 (0.00s)
    --- FAIL: TestBogoSuite/SendSCTListOnResume-TLS-TLS1 (0.00s)
    --- FAIL: TestBogoSuite/SendSCTListOnResume-TLS-TLS11 (0.00s)
    --- FAIL: TestBogoSuite/SignedCertificateTimestampList-Client-TLS-TLS12 (0.00s)
    --- FAIL: TestBogoSuite/SendSCTListOnResume-TLS-TLS13 (0.00s)
    --- SKIP: TestBogoSuite/SignedCertificateTimestampList-Server-TLS-TLS1 (0.00s)
    --- PASS: TestBogoSuite/SignedCertificateTimestampListEmpty-Client-TLS-TLS1 (0.00s)
    --- FAIL: TestBogoSuite/SignedCertificateTimestampList-Client-TLS-TLS1 (0.00s)
    --- PASS: TestBogoSuite/SignedCertificateTimestampListEmptySCT-Client-TLS-TLS12 (0.00s)
    --- SKIP: TestBogoSuite/SignedCertificateTimestampList-Server-TLS-TLS13 (0.00s)
    --- SKIP: TestBogoSuite/SignedCertificateTimestampList-Server-TLS-TLS12 (0.00s)
    --- FAIL: TestBogoSuite/SignedCertificateTimestampList-Client-TLS-TLS13 (0.00s)
    --- PASS: TestBogoSuite/SignedCertificateTimestampListEmpty-Client-TLS-TLS12 (0.00s)
    --- SKIP: TestBogoSuite/SignedCertificateTimestampListInvalid-Server (0.00s)
    --- PASS: TestBogoSuite/SignedCertificateTimestampListEmpty-Client-TLS-TLS11 (0.00s)
    --- PASS: TestBogoSuite/SignedCertificateTimestampListEmptySCT-Client-TLS-TLS13 (0.00s)
FAIL
FAIL	crypto/tls	6.389s
FAIL

What did you expect to see?

I expect the SCTs not to change on resume.

Additionally, I expect the behavior of TLS 1.0, 1.1, and 1.2 to be the the same as behavior for TLS 1.3.

@gabyhelp
Copy link

Related Issues and Documentation

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@rolandshoemaker
Copy link
Member

I believe the change on resumption is actually expected, see https://boringssl.googlesource.com/boringssl/+/refs/heads/master/ssl/test/runner/runner.go#8517.

Why this doesn't happen on 1.3 is not immediately clear to me, but may be because we are storing the state in the session ticket and restoring it, but not doing that consistently?

@rolandshoemaker
Copy link
Member

Ah, yup, in <1.3 we allow the server to override the stored SCTs: https://go.googlesource.com/go/+/refs/heads/master/src/crypto/tls/handshake_client.go#925, but in 1.3 we do not: https://go.googlesource.com/go/+/refs/heads/master/src/crypto/tls/handshake_client_tls13.go#477.

That should probably be consistent, I have no memory of why it is this way.

@rolandshoemaker
Copy link
Member

rolandshoemaker commented Jul 18, 2024

Oh it's because SCTs are not sent in the hello in 1.3, which I documented in the CL description when we implemented this https://go-review.googlesource.com/c/go/+/234237.

@monkwire
Copy link
Author

monkwire commented Jul 18, 2024

Ahh I see. Since BoringSSL always passes in an expected SCT value as a flag argument, how should OnResume tests be handled in bogo_shim_test? Does it make sense to explicitly allow for SCTs to be either what BoringSSL passes in ([][]uint8{[]uint8{0x5, 0x6, 0x7, 0x8}}) or what BoringSSL passes in with the last byte incremented ([][]uint8{[]uint8{0x5, 0x6, 0x7, 0x9}} ?

BoringSSL OnResume Tests:

                        var testSCTList = []byte{0, 6, 0, 4, 5, 6, 7, 8}
                        
                        // ...


			// The SCT extension did not specify that it must only be sent on resumption as it
			// should have, so test that we tolerate but ignore it.
			testCases = append(testCases, testCase{
				protocol: protocol,
				name:     "SendSCTListOnResume-" + suffix,
				config: Config{
					MaxVersion: ver.version,
					Credential: rsaCertificate.WithSCTList(testSCTList),
					Bugs: ProtocolBugs{
						SendSCTListOnResume: differentSCTList,
					},
				},
				flags: []string{
					"-enable-signed-cert-timestamps",
					"-expect-signed-cert-timestamps",
					base64FlagValue(testSCTList),
				},
				resumeSession: true,
			})

@rolandshoemaker
Copy link
Member

Probably reasonable to match the boringssl behavior and ignore the SCT extension on resume. That change should be a separate CL for crypto/tls.

@davidben the runner.go comment seems confusing, did you mean that the SCT extension should not be sent on resumption, rather than only sent on resumption?

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/599615 mentions this issue: crypto/tls: support signed cert timestamps flags in bogo_shim_test

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/597195 mentions this issue: crypto/tls: support signed cert timestamps flags in bogo_shim_test

@davidben
Copy link
Contributor

Whoops, yeah, I think I collided "must only be sent on full handshakes" and "must not be sent on resumption" in that comment.

@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 19, 2024
@cherrymui cherrymui added this to the Backlog milestone Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants