diff --git a/handshake_messages.go b/handshake_messages.go index c382b3346a..883838b6aa 100644 --- a/handshake_messages.go +++ b/handshake_messages.go @@ -1499,6 +1499,7 @@ func (m *certificateMsg) unmarshal(data []byte) bool { } type certificateMsgTLS13 struct { + original []byte // [uTLS] certificate Certificate ocspStapling bool scts bool @@ -1561,7 +1562,7 @@ func marshalCertificate(b *cryptobyte.Builder, certificate Certificate) { } func (m *certificateMsgTLS13) unmarshal(data []byte) bool { - *m = certificateMsgTLS13{} + *m = certificateMsgTLS13{original: data} // [uTLS] s := cryptobyte.String(data) var context cryptobyte.String @@ -1578,6 +1579,22 @@ func (m *certificateMsgTLS13) unmarshal(data []byte) bool { return true } +// [UTLS SECTION BEGINS] +// originalBytes lets transcriptMsg hash this Certificate message exactly as it +// was received, instead of a re-marshal via marshalCertificate(). The re-marshal +// is not guaranteed to be byte-identical to the peer's encoding (only leaf +// OCSP/SCT are re-emitted, in a fixed order; other per-certificate extensions +// and non-canonical length encodings are lost), which diverges the TLS 1.3 +// handshake transcript and causes CertificateVerify to fail with +// "crypto/rsa: verification error". clientHelloMsg, serverHelloMsg and +// certificateRequestMsgTLS13 already preserve their original bytes for the same +// reason. +func (m *certificateMsgTLS13) originalBytes() []byte { + return m.original +} + +// [UTLS SECTION ENDS] + func unmarshalCertificate(s *cryptobyte.String, certificate *Certificate) bool { var certList cryptobyte.String if !s.ReadUint24LengthPrefixed(&certList) { diff --git a/handshake_messages_test.go b/handshake_messages_test.go index b71d97ca26..aa916c6b48 100644 --- a/handshake_messages_test.go +++ b/handshake_messages_test.go @@ -100,6 +100,8 @@ func TestMarshalUnmarshal(t *testing.T) { t.original = nil case *certificateRequestMsgTLS13: // [UTLS] t.original = nil // [UTLS] + case *certificateMsgTLS13: // [UTLS] + t.original = nil // [UTLS] } if !reflect.DeepEqual(m1, m) { diff --git a/u_certificate_originalbytes_test.go b/u_certificate_originalbytes_test.go new file mode 100644 index 0000000000..2fd21edddc --- /dev/null +++ b/u_certificate_originalbytes_test.go @@ -0,0 +1,82 @@ +package tls + +import ( + "bytes" + "crypto/sha256" + "testing" + + "golang.org/x/crypto/cryptobyte" +) + +// buildTLS13CertificateWithLeafExtension builds a valid TLS 1.3 Certificate +// handshake message whose leaf certificate carries an extension that +// marshalCertificate does not re-emit (it only re-emits OCSP and SCT). +// Re-marshaling therefore drops the extension, so a transcript computed from +// the re-marshal diverges from the peer's — which makes CertificateVerify fail +// with "crypto/rsa: verification error". +func buildTLS13CertificateWithLeafExtension() []byte { + var b cryptobyte.Builder + b.AddUint8(typeCertificate) + b.AddUint24LengthPrefixed(func(b *cryptobyte.Builder) { + b.AddUint8(0) // empty certificate_request_context + b.AddUint24LengthPrefixed(func(b *cryptobyte.Builder) { + // single leaf certificate + b.AddUint24LengthPrefixed(func(b *cryptobyte.Builder) { + b.AddBytes([]byte("dummy-leaf-der")) + }) + // leaf extensions: one extension that unmarshal ignores and + // marshalCertificate never re-emits. + b.AddUint16LengthPrefixed(func(b *cryptobyte.Builder) { + b.AddUint16(0x1234) // unknown extension type + b.AddUint16LengthPrefixed(func(b *cryptobyte.Builder) { + b.AddBytes([]byte("ext-payload")) + }) + }) + }) + }) + out, err := b.Bytes() + if err != nil { + panic(err) + } + return out +} + +// TestCertificateMsgTLS13PreservesOriginalBytes ensures certificateMsgTLS13 +// feeds transcriptMsg the exact bytes it was unmarshaled from, not a +// (potentially lossy) re-marshal. +func TestCertificateMsgTLS13PreservesOriginalBytes(t *testing.T) { + wire := buildTLS13CertificateWithLeafExtension() + + var m certificateMsgTLS13 + if !m.unmarshal(wire) { + t.Fatal("unmarshal failed") + } + + wm, ok := handshakeMessage(&m).(handshakeMessageWithOriginalBytes) + if !ok { + t.Fatal("certificateMsgTLS13 does not implement handshakeMessageWithOriginalBytes") + } + if !bytes.Equal(wm.originalBytes(), wire) { + t.Fatalf("originalBytes() != wire\n orig=%x\n wire=%x", wm.originalBytes(), wire) + } + + // Guard: the re-marshal must actually diverge from the wire, otherwise the + // test is no longer exercising the bug. + remarshaled, err := m.marshal() + if err != nil { + t.Fatalf("marshal: %v", err) + } + if bytes.Equal(remarshaled, wire) { + t.Fatal("re-marshal unexpectedly equals wire; test no longer exercises the divergence") + } + + // transcriptMsg must hash the original wire bytes, not the re-marshal. + h := sha256.New() + if err := transcriptMsg(&m, h); err != nil { + t.Fatalf("transcriptMsg: %v", err) + } + want := sha256.Sum256(wire) + if got := h.Sum(nil); !bytes.Equal(got, want[:]) { + t.Fatal("transcriptMsg hashed the re-marshal instead of the original wire bytes") + } +}