From e84435fe937c3ea48bdb9c8c39548d5e36295405 Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Wed, 7 Oct 2026 10:19:48 -0700 Subject: [PATCH] Fix security review findings - Conn.Read: reject zero-length handshake records with unexpected_message instead of panicking with an out-of-range index. Any client could trigger the panic after a successful ECH handshake because ECH configs are public. Also guard the same index in inspectWrite and use its record argument. - processEncryptedClientHello: stop trial decryption after the first key that succeeds. Previously, a later key with the same config_id and cipher suite reused the advanced HPKE context, failed, and cleared the decrypted ClientHelloInner, silently rejecting ECH. - NewConn: evaluate err when the deferred alert conversion runs, not when the defer statement executes. Alerts were never sent and the connection was never closed on error. - Transport: update the NewTransport doc to reflect that plaintext HTTP is allowed unless RequireECH is set. - dns: limit decoded names to 255 octets and labels to 63 octets (RFC 1035 3.1) to bound memory use from compression pointer chains. Co-Authored-By: Claude Opus 5.5 --- dns/message.go | 10 ++++ dns/message_test.go | 44 ++++++++++++++++ ech.go | 34 +++++++++---- ech_test.go | 119 ++++++++++++++++++++++++++++++++++++++++++++ transport.go | 10 ++-- 5 files changed, 203 insertions(+), 14 deletions(-) diff --git a/dns/message.go b/dns/message.go index 291e1bd..3f691bc 100644 --- a/dns/message.go +++ b/dns/message.go @@ -493,6 +493,10 @@ func (d decoder) name(s *cryptobyte.String) (string, error) { func (d decoder) nameLabels(s *cryptobyte.String) ([]string, error) { var labels []string + // The total length of a domain name in wire format, including the + // length octets and the terminating zero-length label, is limited to + // 255 octets. RFC 1035 Section 3.1 + size := 1 for { for !s.Empty() && (*s)[0]&0xc0 == 0xc0 { // pointer current := uintptr(unsafe.Pointer(&(*s)[0])) @@ -514,6 +518,12 @@ func (d decoder) nameLabels(s *cryptobyte.String) ([]string, error) { if len(name) == 0 { break } + if len(name) > 63 { + return nil, ErrDecodeError + } + if size += len(name) + 1; size > 255 { + return nil, ErrDecodeError + } labels = append(labels, string(name)) } return labels, nil diff --git a/dns/message_test.go b/dns/message_test.go index 6c628b9..df596ad 100644 --- a/dns/message_test.go +++ b/dns/message_test.go @@ -568,3 +568,47 @@ func TestResponseCode(t *testing.T) { t.Errorf("ResponseCode() = %d, want %d", got, want) } } + +func TestNameTooLong(t *testing.T) { + header := []byte{0x00, 0x00, 0x81, 0x80, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00} + label := func(n int) []byte { + return append([]byte{byte(n)}, strings.Repeat("a", n)...) + } + question := func(labels ...[]byte) []byte { + var b []byte + for _, l := range labels { + b = append(b, l...) + } + return append(b, 0x00, 0x00, 0x01, 0x00, 0x01) + } + cat := func(parts ...[]byte) []byte { + var b []byte + for _, p := range parts { + b = append(b, p...) + } + return b + } + + // 3*(63+1) + (61+1) + 1 = 255 octets. + if _, err := DecodeMessage(cat(header, question(label(63), label(63), label(63), label(61)))); err != nil { + t.Errorf("DecodeMessage(255 octets) = %v, want nil", err) + } + // 3*(63+1) + (62+1) + 1 = 256 octets. + if _, err := DecodeMessage(cat(header, question(label(63), label(63), label(63), label(62)))); err != ErrDecodeError { + t.Errorf("DecodeMessage(256 octets) = %v, want ErrDecodeError", err) + } + // A label longer than 63 octets. + if _, err := DecodeMessage(cat(header, question(label(64)))); err != ErrDecodeError { + t.Errorf("DecodeMessage(64-octet label) = %v, want ErrDecodeError", err) + } + + // The question name is 3*(63+1) + 1 = 193 octets. The answer's name is + // a 63-octet label followed by a pointer to the question name, for a + // total of 64 + 193 = 257 octets. + m := cat(header, question(label(63), label(63), label(63))) + m[7] = 1 // anCount + m = cat(m, label(63), []byte{0xc0, 0x0c}, []byte{0x00, 0x01, 0x00, 0x01, 0x00, 0x00, 0x01, 0x28, 0x00, 0x04, 1, 2, 3, 4}) + if _, err := DecodeMessage(m); err != ErrDecodeError { + t.Errorf("DecodeMessage(compressed 257 octets) = %v, want ErrDecodeError", err) + } +} diff --git a/ech.go b/ech.go index 0f0288f..d0e87e9 100644 --- a/ech.go +++ b/ech.go @@ -48,7 +48,9 @@ func WithDebug(f func(format string, arg ...any)) Option { // The ctx is used while reading the initial ClientHello only. It is not used // after New returns. func NewConn(ctx context.Context, conn net.Conn, options ...Option) (outConn *Conn, err error) { - defer convertErrorsToAlerts(conn, err) + defer func() { + convertErrorsToAlerts(conn, err) + }() done := make(chan struct{}) defer close(done) go func() { @@ -237,6 +239,7 @@ func (c *Conn) processEncryptedClientHello(h *clientHello, isRetry bool) (*clien if string(cfg.PublicName) != h.ServerName { return nil, ErrIllegalParameter } + break } if innerBytes == nil { // Section 7.1.1, regarding a retried ClientHello: @@ -318,12 +321,20 @@ func (c *Conn) processEncryptedClientHello(h *clientHello, isRetry bool) (*clien func (c *Conn) Read(b []byte) (int, error) { if !c.readPassthrough && len(c.readBuf) == 0 && c.readErr == nil { r, err := readRecord(c.Conn) - if len(r) >= 5 { - if r[0] == 22 { - c.debugf("Read %s(%d) %s\n", contentType(r[0]), r[0], handshakeMessageTypes[r[5]]) - } else { - c.debugf("Read %s(%d)\n", contentType(r[0]), r[0]) - } + switch { + case len(r) > 5 && r[0] == 22: + c.debugf("Read %s(%d) %s\n", contentType(r[0]), r[0], handshakeMessageTypes[r[5]]) + case len(r) >= 5: + c.debugf("Read %s(%d)\n", contentType(r[0]), r[0]) + } + if err == nil && r[0] == 22 && len(r) == 5 { + // RFC 8446, Section 5.1 + // Implementations MUST NOT send zero-length fragments of + // Handshake types. + err = fmt.Errorf("%w: zero-length handshake record", ErrUnexpectedMessage) + c.readErr = err + convertErrorsToAlerts(c, err) + return 0, err } switch { case err != nil: @@ -388,8 +399,11 @@ func (c *Conn) Write(b []byte) (int, error) { } func (c *Conn) inspectWrite(record []byte) error { - recType := c.writeBuf[0] - msgType := c.writeBuf[5] + recType := record[0] + var msgType uint8 + if len(record) > 5 { + msgType = record[5] + } if recType == 22 { c.debugf("Write %s(%d) %s\n", contentType(recType), recType, handshakeMessageTypes[msgType]) } else { @@ -399,7 +413,7 @@ func (c *Conn) inspectWrite(record []byte) error { case recType == 23: c.writePassthrough = true case recType == 22 && msgType == 2: // Handshake / ServerHello - h, err := parseServerHello(c.writeBuf[5:]) + h, err := parseServerHello(record[5:]) if err != nil { return fmt.Errorf("%w: parseServerHello: %v\n", ErrDecodeError, err) } diff --git a/ech_test.go b/ech_test.go index c4779a7..8da1240 100644 --- a/ech_test.go +++ b/ech_test.go @@ -716,3 +716,122 @@ func TestValidInnerAES(t *testing.T) { }) } } + +// TestZeroLengthHandshakeRecord verifies that a zero-length handshake record +// received after an accepted ClientHello is rejected without panicking. +func TestZeroLengthHandshakeRecord(t *testing.T) { + privKey, config, err := NewConfig(1, []byte("public.example.com")) + if err != nil { + t.Fatalf("NewConfig: %v", err) + } + pubKey := privKey.PublicKey() + keys := []Key{{Config: config, PrivateKey: privKey.Bytes()}} + + inner := newClientHello("private", "echExtInner", "tls1.3") + outer := newClientHello("public", "tls1.3", config, pubKey, inner) + c := newFakeConn(append(outer.bytes(), 22, 3, 3, 0, 0)) + + conn, err := NewConn(t.Context(), c, WithKeys(keys), WithDebug(t.Logf)) + if err != nil { + t.Fatalf("NewConn: %v", err) + } + if !conn.ECHAccepted() { + t.Fatal("ECHAccepted = false, want true") + } + if _, err := readRecord(conn); err != nil { + t.Fatalf("ClientHello: %v", err) + } + if _, err := conn.Read(make([]byte, 1024)); !errors.Is(err, ErrUnexpectedMessage) { + t.Fatalf("Read: %v, want ErrUnexpectedMessage", err) + } + if got, want := c.Writer.(*bytes.Buffer).Bytes(), []byte{0x15, 0x03, 0x03, 0x00, 0x02, 2, 10}; !bytes.Equal(got, want) { + t.Errorf("Alert = %v, want %v", got, want) + } +} + +// TestZeroLengthHandshakeRecordWrite verifies that writing a zero-length +// handshake record doesn't panic. +func TestZeroLengthHandshakeRecordWrite(t *testing.T) { + privKey, config, err := NewConfig(1, []byte("public.example.com")) + if err != nil { + t.Fatalf("NewConfig: %v", err) + } + pubKey := privKey.PublicKey() + keys := []Key{{Config: config, PrivateKey: privKey.Bytes()}} + + inner := newClientHello("private", "echExtInner", "tls1.3") + outer := newClientHello("public", "tls1.3", config, pubKey, inner) + c := newFakeConn(outer.bytes()) + + conn, err := NewConn(t.Context(), c, WithKeys(keys)) + if err != nil { + t.Fatalf("NewConn: %v", err) + } + if n, err := conn.Write([]byte{22, 3, 3, 0, 0}); err != nil || n != 5 { + t.Fatalf("Write: %d, %v", n, err) + } +} + +// TestKeysWithSameConfigID verifies that ECH is accepted when multiple keys +// share the same config ID, regardless of the order of the keys. +func TestKeysWithSameConfigID(t *testing.T) { + privKeyA, configA, err := NewConfig(1, []byte("public.example.com")) + if err != nil { + t.Fatalf("NewConfig: %v", err) + } + privKeyB, configB, err := NewConfig(1, []byte("public.example.com")) + if err != nil { + t.Fatalf("NewConfig: %v", err) + } + keyA := Key{Config: configA, PrivateKey: privKeyA.Bytes()} + keyB := Key{Config: configB, PrivateKey: privKeyB.Bytes()} + + for _, tc := range []struct { + name string + keys []Key + }{ + {"A,B", []Key{keyA, keyB}}, + {"B,A", []Key{keyB, keyA}}, + } { + t.Run(tc.name, func(t *testing.T) { + inner := newClientHello("private", "echExtInner", "tls1.3") + outer := newClientHello("public", "tls1.3", configA, privKeyA.PublicKey(), inner) + c := newFakeConn(outer.bytes()) + + conn, err := NewConn(t.Context(), c, WithKeys(tc.keys)) + if err != nil { + t.Fatalf("NewConn: %v", err) + } + if !conn.ECHAccepted() { + t.Fatal("ECHAccepted = false, want true") + } + if buf, err := readRecord(conn); err != nil { + t.Fatalf("ClientHello: %v", err) + } else if got, want := buf, inner.bytes(); !bytes.Equal(got, want) { + t.Fatalf("ClientHello = %v, want %v", got, want) + } + }) + } +} + +// TestNewConnSendsAlert verifies that NewConn sends an alert when it returns +// an error. +func TestNewConnSendsAlert(t *testing.T) { + privKey, config, err := NewConfig(1, []byte("public.example.com")) + if err != nil { + t.Fatalf("NewConfig: %v", err) + } + pubKey := privKey.PublicKey() + keys := []Key{{Config: config, PrivateKey: privKey.Bytes()}} + + inner := newClientHello("private", "echExtInner", "tls1.3") + outer := newClientHello("private", "tls1.3", config, pubKey, inner) + c := newFakeConn(outer.bytes()) + + if _, err := NewConn(t.Context(), c, WithKeys(keys)); !errors.Is(err, ErrIllegalParameter) { + t.Fatalf("NewConn: %v, want ErrIllegalParameter", err) + } + if got, want := c.Writer.(*bytes.Buffer).Bytes(), []byte{0x15, 0x03, 0x03, 0x00, 0x02, 2, 47}; !bytes.Equal(got, want) { + t.Errorf("Alert = %v, want %v", got, want) + } +} diff --git a/transport.go b/transport.go index 0f9f44a..57fbb66 100644 --- a/transport.go +++ b/transport.go @@ -18,11 +18,13 @@ var _ http.RoundTripper = (*Transport)(nil) // NewTransport returns a [Transport] that is ready to be used with // [http.Client]. // -// By default, the returned [Transport] uses Encrypted Client Hello opportunistically -// and refuses to execute plaintext HTTP transactions. This behavior can be changed -// by modifiying the appropriate parameters. +// By default, the returned [Transport] uses Encrypted Client Hello opportunistically. +// Plaintext HTTP requests are upgraded to HTTPS when the hostname has a HTTPS RR. +// Otherwise, they are sent in plaintext. This behavior can be changed by +// modifiying the appropriate parameters. // -// For example, to require ECH, set Dialer.RequireECH = true. +// For example, to require ECH, set Dialer.RequireECH = true. This also refuses +// to execute plaintext HTTP transactions. func NewTransport() *Transport { t := &Transport{ Resolver: DefaultResolver,