Fix security review findings

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

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)
+	}
+}

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)
 		}

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)
+	}
+}

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,