net/http TestTransportBodyAltRewind fails with backend: panic: cipher: incorrect additional data length given to GCM TLS

[dagood]

Trying to integrate db597f1 into microsoft/go, I hit this error in the standard library tests:

$ GOEXPERIMENT=opensslcrypto ../bin/go test net/http
panic: cipher: incorrect additional data length given to GCM TLS

goroutine 151 [running]:
vendor/github.com/golang-fips/openssl/v2.(*cipherGCM).Seal(0xc0000be708?, {0xc000010c00?, 0x1?, 0x8e0a40?}, {0xc0002f62c0?, 0x6601fc?, 0xd1b3c0?}, {0xc000010c05, 0x11, 0xfb}, ...)
        /home/dagood/git/go/go/src/vendor/github.com/golang-fips/openssl/v2/cipher.go:408 +0x3cb
crypto/tls.(*xorNonceAEAD).Seal(0xc0002f62c0, {0xc000010c00?, 0xc000014460?, 0xc000014456?}, {0xc0000045d8, 0x8, 0xc0001a5620?}, {0xc000010c05, 0x11, 0xfb}, ...)
        /home/dagood/git/go/go/src/crypto/tls/cipher_suites.go:507 +0xd1
crypto/tls.(*halfConn).encrypt(0xc000004598, {0xc000010c00, 0x5, 0x100}, {0xc000014460, 0x10, 0x10}, {0xa137e0, 0xa0ee48})
        /home/dagood/git/go/go/src/crypto/tls/conn.go:529 +0x764
crypto/tls.(*Conn).writeRecordLocked(0xc000004388, 0x16, {0xc000014460, 0x10, 0x10})
        /home/dagood/git/go/go/src/crypto/tls/conn.go:1022 +0x64e
crypto/tls.(*Conn).writeHandshakeRecord(0xc000004388, {0xa150b8?, 0xc000180230?}, {0x7f3a9a179a90, 0xc00020e450})
        /home/dagood/git/go/go/src/crypto/tls/conn.go:1057 +0x146
crypto/tls.(*serverHandshakeStateTLS13).sendServerParameters(0xc0000bec00)
        /home/dagood/git/go/go/src/crypto/tls/handshake_server_tls13.go:665 +0x88b
crypto/tls.(*serverHandshakeStateTLS13).handshake(0xc000207c00)
        /home/dagood/git/go/go/src/crypto/tls/handshake_server_tls13.go:60 +0x72
crypto/tls.(*Conn).serverHandshake(0xc000004388, {0xa163d8, 0xc000000000})
        /home/dagood/git/go/go/src/crypto/tls/handshake_server.go:53 +0x16a
crypto/tls.(*Conn).handshakeContext(0xc000004388, {0xa161e0, 0xd99ca0})
        /home/dagood/git/go/go/src/crypto/tls/conn.go:1553 +0x3cb
crypto/tls.(*Conn).HandshakeContext(...)
        /home/dagood/git/go/go/src/crypto/tls/conn.go:1493
crypto/tls.(*Conn).Handshake(...)
        /home/dagood/git/go/go/src/crypto/tls/conn.go:1477
net/http.TestTransportBodyAltRewind.func1()
        /home/dagood/git/go/go/src/net/http/transport_internal_test.go:217 +0x1c5
created by net/http.TestTransportBodyAltRewind in goroutine 150
        /home/dagood/git/go/go/src/net/http/transport_internal_test.go:206 +0x287
FAIL    net/http        0.138s
FAIL

This is because additionalData is []uint8 len: 5, cap: 128, [23,3,3,0,33], gcmTlsAddSize is 13:

openssl/cipher.go

Lines 406 to 410 in db597f1

	if g.tls != cipherGCMTLSNone {
	if len(additionalData) != gcmTlsAddSize {
	panic("cipher: incorrect additional data length given to GCM TLS")
	}
	counter := binary.BigEndian.Uint64(nonce[gcmTlsFixedNonceSize:])

• Removing the panic entirely makes the net/http tests pass.
• This conditional also makes the tests pass: if g.tls == cipherGCMTLS12 && len(additionalData) != gcmTlsAddSize { panic(...) }.

I've been trying to find this reason for this panic and haven't spotted anything yet. If there's something similar we need for TLS 1.3, I think knowing the reason for this panic will help.

(I'm not sure if I've checked everything thoroughly--I figured someone else might be better at finding/remembering. 🙂)

I didn't think about running the actual stdlib tests before submitting/merging NewGCMTLS13, #132. This is a followup issue for that PR.

/cc @qmuntal @gdams

[ueno]

In TLS 1.2, the additional data is constructed as:

      additional_data = seq_num(8) + TLSCompressed.type(1) +
                        TLSCompressed.version(2) + TLSCompressed.length(2);

while in TLS 1.3 it is:

      additional_data = TLSCiphertext.opaque_type(1) ||
                        TLSCiphertext.legacy_record_version(2) ||
                        TLSCiphertext.length(2)

so the length being 5 bytes is actually expected in TLS 1.3. Sorry for not spotting this during the review.

[dagood]

Aha, thanks! I'll submit a new PR for a version-specific check.