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 <roland@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Auto-Submit: Daniel McCarney <daniel@binaryparadox.net>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>

diff --git a/src/crypto/tls/bogo_config.json b/src/crypto/tls/bogo_config.json
index b88201a45757cda7aa249552bb0034154b605ca9..8276d08d35142c03f8398cf9a7e8dc703565fcbb 100644 (file)
--- 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 141175c801ea7f3a7dc1f7ec5de7bdaa62acd6a0..cd9b9778fd7053aa06ac03295b24cdc564694c24 100644 (file)
--- 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 c72974ef951791c180a8e13e46121900936c2c76..a6d64a506a0542f1498764cbf1da1b5962ba6c26 100644 (file)
--- 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 ba75101dd5cf54c5184e716e4495044daa7bdd8d..51cd4ef765dd6c189c6075d666333b497cb3e74a 100644 (file)
--- 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)
        }
 }