From: Daniel McCarney Date: Sat, 17 May 2025 15:17:21 +0000 (-0400) Subject: crypto/tls: use decode alert for handshake msg unmarshal err X-Git-Tag: go1.25rc1~101 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=aab8552088ae06ee7d6515d0dfc9efa7979feb5c;p=gostls13.git crypto/tls: use decode alert for handshake msg unmarshal err Previously if instances of the handshakeMessage interface returned false from unmarshal(), indicating an umarshalling error, the crypto/tls package would emit an unexpected_message alert. This commit changes to use a decode_error alert for this condition instead. The usage-pattern of the handshakeMessage interface is that we switch on the message type, invoke a specific concrete handshakeMessage type's unmarshal function, and then return it to the caller on success. At this point the caller looks at the message type and can determine if the message was unexpected or not. If it was unexpected, the call-sites emit the correct error for that case. Only the caller knows the current protocol state and allowed message types, not the generic handshake decoding logic. With the above in mind, if we find that within the unmarshal logic for a specific message type that the data we have in hand doesn't match the protocol syntax we should emit a decode_error. An unexpected_message error isn't appropriate because we don't yet know if the message is unexpected or not, only that the message can't be decoded based on the spec's syntax for the type the message claimed to be. Notably one unit test, TestQUICPostHandshakeKeyUpdate, had to have its test data adjusted because it was previously not testing the right thing: it was double-encoding the type & length prefix data for a key update message and expecting the QUIC logic to reject it as an inappropriate post-handshake message. In reality it was being rejected sooner as an invalid key update message from the double-encoding and this was masked by the previous alert for this condition matching the expected alert. Finally, changing our alert allows enabling a handful of BoGo tests related to duplicate extensions of the form "DuplicateExtension[Server|Client]-TLS-[TLS1|TLS11|TLS12|TLS13]". One test remains skipped (DuplicateExtensionClient-TLS-TLS13), as it requires additional follow-up. Updates #72006 Change-Id: I27a2cd231e4b8762b0d9e2dbd3d8ddd5b87fd5d1 Reviewed-on: https://go-review.googlesource.com/c/go/+/673738 Reviewed-by: Roland Shoemaker Reviewed-by: David Chase Auto-Submit: Daniel McCarney LUCI-TryBot-Result: Go LUCI Reviewed-by: Filippo Valsorda --- diff --git a/src/crypto/tls/bogo_config.json b/src/crypto/tls/bogo_config.json index b88201a457..8276d08d35 100644 --- a/src/crypto/tls/bogo_config.json +++ b/src/crypto/tls/bogo_config.json @@ -66,20 +66,13 @@ "SupportTicketsWithSessionID": "We don't support session ID resumption", "KeyUpdate-RequestACK": "TODO: first pass, this should be fixed", "SupportedVersionSelection-TLS12": "TODO: first pass, this should be fixed", - "DuplicateExtensionServer-TLS-TLS1": "TODO: first pass, this should be fixed", - "DuplicateExtensionClient-TLS-TLS1": "TODO: first pass, this should be fixed", "UnsolicitedServerNameAck-TLS-TLS1": "TODO: first pass, this should be fixed", "TicketSessionIDLength-33-TLS-TLS1": "TODO: first pass, this should be fixed", - "DuplicateExtensionServer-TLS-TLS11": "TODO: first pass, this should be fixed", - "DuplicateExtensionClient-TLS-TLS11": "TODO: first pass, this should be fixed", "UnsolicitedServerNameAck-TLS-TLS11": "TODO: first pass, this should be fixed", "TicketSessionIDLength-33-TLS-TLS11": "TODO: first pass, this should be fixed", - "DuplicateExtensionServer-TLS-TLS12": "TODO: first pass, this should be fixed", - "DuplicateExtensionClient-TLS-TLS12": "TODO: first pass, this should be fixed", "UnsolicitedServerNameAck-TLS-TLS12": "TODO: first pass, this should be fixed", "TicketSessionIDLength-33-TLS-TLS12": "TODO: first pass, this should be fixed", "DuplicateExtensionClient-TLS-TLS13": "TODO: first pass, this should be fixed", - "DuplicateExtensionServer-TLS-TLS13": "TODO: first pass, this should be fixed", "UnsolicitedServerNameAck-TLS-TLS13": "TODO: first pass, this should be fixed", "RenegotiationInfo-Forbidden-TLS13": "TODO: first pass, this should be fixed", "EMS-Forbidden-TLS13": "TODO: first pass, this should be fixed", diff --git a/src/crypto/tls/conn.go b/src/crypto/tls/conn.go index 141175c801..cd9b9778fd 100644 --- a/src/crypto/tls/conn.go +++ b/src/crypto/tls/conn.go @@ -1179,7 +1179,7 @@ func (c *Conn) unmarshalHandshakeMessage(data []byte, transcript transcriptHash) data = append([]byte(nil), data...) if !m.unmarshal(data) { - return nil, c.in.setErrorLocked(c.sendAlert(alertUnexpectedMessage)) + return nil, c.in.setErrorLocked(c.sendAlert(alertDecodeError)) } if transcript != nil { diff --git a/src/crypto/tls/handshake_server_test.go b/src/crypto/tls/handshake_server_test.go index c72974ef95..a6d64a506a 100644 --- a/src/crypto/tls/handshake_server_test.go +++ b/src/crypto/tls/handshake_server_test.go @@ -157,7 +157,7 @@ func TestRejectSNIWithTrailingDot(t *testing.T) { vers: VersionTLS12, random: make([]byte, 32), serverName: "foo.com.", - }, "unexpected message") + }, "decoding message") } func TestDontSelectECDSAWithRSAKey(t *testing.T) { diff --git a/src/crypto/tls/quic_test.go b/src/crypto/tls/quic_test.go index ba75101dd5..51cd4ef765 100644 --- a/src/crypto/tls/quic_test.go +++ b/src/crypto/tls/quic_test.go @@ -9,6 +9,7 @@ import ( "context" "errors" "reflect" + "strings" "testing" ) @@ -308,11 +309,11 @@ func TestQUICPostHandshakeKeyUpdate(t *testing.T) { if err != nil { t.Fatal(err) } - if err := cli.conn.HandleData(QUICEncryptionLevelApplication, append([]byte{ - byte(typeKeyUpdate), - byte(0), byte(0), byte(len(keyUpdateBytes)), - }, keyUpdateBytes...)); !errors.Is(err, alertUnexpectedMessage) { - t.Fatalf("key update request: got error %v, want alertUnexpectedMessage", err) + expectedErr := "unexpected key update message" + if err = cli.conn.HandleData(QUICEncryptionLevelApplication, keyUpdateBytes); err == nil { + t.Fatalf("key update request: expected error from post-handshake key update, got nil") + } else if !strings.Contains(err.Error(), expectedErr) { + t.Fatalf("key update request: got error %v, expected substring %q", err, expectedErr) } }