Fix TLS 1.3 NewSessionTicket processing.
08b65f4e31f7eb63bad0e903e9f191bcc699eaca introduced a memory leak and
also got enums confused. Also fix a codepath that was missing an error
code.
Thanks to OSS-Fuzz which appears to have found it in a matter of hours.
Change-Id: Ia9e926c28a01daab3e6154d363d0acda91209a22
Reviewed-on: https://boringssl-review.googlesource.com/13104
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index 029fd4a..96bc39a 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -960,6 +960,10 @@
// extension of the NewSessionTicket message.
SendTicketEarlyDataInfo uint32
+ // DuplicateTicketEarlyDataInfo causes an extra empty extension of
+ // ticket_early_data_info to be sent in NewSessionTicket.
+ DuplicateTicketEarlyDataInfo bool
+
// ExpectTicketEarlyDataInfo, if true, means that the client will fail upon
// absence of the ticket_early_data_info extension.
ExpectTicketEarlyDataInfo bool
diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go
index 62a46bf..ee342fa 100644
--- a/ssl/test/runner/conn.go
+++ b/ssl/test/runner/conn.go
@@ -1767,11 +1767,12 @@
// TODO(davidben): Allow configuring these values.
m := &newSessionTicketMsg{
- version: c.vers,
- ticketLifetime: uint32(24 * time.Hour / time.Second),
- earlyDataInfo: c.config.Bugs.SendTicketEarlyDataInfo,
- customExtension: c.config.Bugs.CustomTicketExtension,
- ticketAgeAdd: ticketAgeAdd,
+ version: c.vers,
+ ticketLifetime: uint32(24 * time.Hour / time.Second),
+ earlyDataInfo: c.config.Bugs.SendTicketEarlyDataInfo,
+ duplicateEarlyDataInfo: c.config.Bugs.DuplicateTicketEarlyDataInfo,
+ customExtension: c.config.Bugs.CustomTicketExtension,
+ ticketAgeAdd: ticketAgeAdd,
}
state := sessionState{
diff --git a/ssl/test/runner/handshake_messages.go b/ssl/test/runner/handshake_messages.go
index a431cb5..62309b8 100644
--- a/ssl/test/runner/handshake_messages.go
+++ b/ssl/test/runner/handshake_messages.go
@@ -2003,14 +2003,15 @@
}
type newSessionTicketMsg struct {
- raw []byte
- version uint16
- ticketLifetime uint32
- ticketAgeAdd uint32
- ticket []byte
- earlyDataInfo uint32
- customExtension string
- hasGREASEExtension bool
+ raw []byte
+ version uint16
+ ticketLifetime uint32
+ ticketAgeAdd uint32
+ ticket []byte
+ earlyDataInfo uint32
+ customExtension string
+ duplicateEarlyDataInfo bool
+ hasGREASEExtension bool
}
func (m *newSessionTicketMsg) marshal() []byte {
@@ -2035,6 +2036,10 @@
if m.earlyDataInfo > 0 {
extensions.addU16(extensionTicketEarlyDataInfo)
extensions.addU16LengthPrefixed().addU32(m.earlyDataInfo)
+ if m.duplicateEarlyDataInfo {
+ extensions.addU16(extensionTicketEarlyDataInfo)
+ extensions.addU16LengthPrefixed().addU32(m.earlyDataInfo)
+ }
}
if len(m.customExtension) > 0 {
extensions.addU16(extensionCustom)
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 5c3d914..ba78fce 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -8394,6 +8394,21 @@
})
testCases = append(testCases, testCase{
+ testType: clientTest,
+ name: "TLS13-DuplicateTicketEarlyDataInfo",
+ config: Config{
+ MaxVersion: VersionTLS13,
+ Bugs: ProtocolBugs{
+ SendTicketEarlyDataInfo: 16384,
+ DuplicateTicketEarlyDataInfo: true,
+ },
+ },
+ shouldFail: true,
+ expectedError: ":DUPLICATE_EXTENSION:",
+ expectedLocalError: "remote error: illegal parameter",
+ })
+
+ testCases = append(testCases, testCase{
testType: serverTest,
name: "TLS13-ExpectTicketEarlyDataInfo",
config: Config{
diff --git a/ssl/tls13_client.c b/ssl/tls13_client.c
index 1d8bc54..6f2bb21 100644
--- a/ssl/tls13_client.c
+++ b/ssl/tls13_client.c
@@ -638,6 +638,7 @@
}
int tls13_process_new_session_ticket(SSL *ssl) {
+ int ret = 0;
SSL_SESSION *session =
SSL_SESSION_dup(ssl->s3->established_session,
SSL_SESSION_INCLUDE_NONAUTH);
@@ -655,10 +656,9 @@
!CBS_stow(&ticket, &session->tlsext_tick, &session->tlsext_ticklen) ||
!CBS_get_u16_length_prefixed(&cbs, &extensions) ||
CBS_len(&cbs) != 0) {
- SSL_SESSION_free(session);
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
- return 0;
+ goto err;
}
/* Parse out the extensions. */
@@ -674,14 +674,15 @@
OPENSSL_ARRAY_SIZE(ext_types),
1 /* ignore unknown */)) {
ssl3_send_alert(ssl, SSL3_AL_FATAL, alert);
- return ssl_hs_error;
+ goto err;
}
if (have_early_data_info) {
if (!CBS_get_u32(&early_data_info, &session->ticket_max_early_data) ||
CBS_len(&early_data_info) != 0) {
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
- return ssl_hs_error;
+ OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
+ goto err;
}
}
@@ -691,11 +692,14 @@
if (ssl->ctx->new_session_cb != NULL &&
ssl->ctx->new_session_cb(ssl, session)) {
/* |new_session_cb|'s return value signals that it took ownership. */
- return 1;
+ session = NULL;
}
+ ret = 1;
+
+err:
SSL_SESSION_free(session);
- return 1;
+ return ret;
}
void ssl_clear_tls13_state(SSL_HANDSHAKE *hs) {
