Consider session if the client supports tickets but offered a session ID.
This is a minor regression from
https://boringssl-review.googlesource.com/5235.
If the client, for whatever reason, had an ID-based session but also
supports tickets, it will send non-empty ID + empty ticket extension.
If the ticket extension is non-empty, then the ID is not an ID but a
dummy signaling value, so 5235 avoided looking it up. But if it is
present and empty, the ID is still an ID and should be looked up.
This shouldn't have any practical consequences, except if a server
switched from not supporting tickets and then started supporting it,
while keeping the session cache fixed.
Add a test for this case, and tighten up existing ID vs ticket tests so
they fail if we resume with the wrong type.
Change-Id: Id4d08cd809af00af30a2b67fe3a971078e404c75
Reviewed-on: https://boringssl-review.googlesource.com/6554
Reviewed-by: Adam Langley <alangley@gmail.com>
diff --git a/ssl/internal.h b/ssl/internal.h
index 5acb598..99f083d 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1219,13 +1219,13 @@
#define tlsext_tick_md EVP_sha256
-/* tls_process_ticket processes the session ticket extension. On success, it
- * sets |*out_session| to the decrypted session or NULL if the ticket was
- * rejected. It sets |*out_send_ticket| to whether a new ticket should be sent
- * at the end of the handshake. It returns one on success and zero on fatal
+/* tls_process_ticket processes a session ticket from the client. On success,
+ * it sets |*out_session| to the decrypted session or NULL if the ticket was
+ * rejected. If the ticket was valid, it sets |*out_renew_ticket| to whether
+ * the ticket should be renewed. It returns one on success and zero on fatal
* error. */
int tls_process_ticket(SSL *ssl, SSL_SESSION **out_session,
- int *out_send_ticket, const uint8_t *ticket,
+ int *out_renew_ticket, const uint8_t *ticket,
size_t ticket_len, const uint8_t *session_id,
size_t session_id_len);
diff --git a/ssl/ssl_session.c b/ssl/ssl_session.c
index 3d59bc3..24de4ec 100644
--- a/ssl/ssl_session.c
+++ b/ssl/ssl_session.c
@@ -430,7 +430,7 @@
/* This is used only by servers. */
assert(ssl->server);
SSL_SESSION *session = NULL;
- int send_ticket = 0;
+ int renew_ticket = 0;
/* If tickets are disabled, always behave as if no tickets are present. */
const uint8_t *ticket = NULL;
@@ -440,24 +440,27 @@
ssl->version > SSL3_VERSION &&
SSL_early_callback_ctx_extension_get(ctx, TLSEXT_TYPE_session_ticket,
&ticket, &ticket_len);
- if (tickets_supported) {
- if (!tls_process_ticket(ssl, &session, &send_ticket, ticket, ticket_len,
+ int from_cache = 0;
+ if (tickets_supported && ticket_len > 0) {
+ if (!tls_process_ticket(ssl, &session, &renew_ticket, ticket, ticket_len,
ctx->session_id, ctx->session_id_len)) {
return ssl_session_error;
}
} else {
- /* The client does not support session tickets, so the session ID should be
- * used instead. */
+ /* The client didn't send a ticket, so the session ID is a real ID. */
enum ssl_session_result_t lookup_ret = ssl_lookup_session(
ssl, &session, ctx->session_id, ctx->session_id_len);
if (lookup_ret != ssl_session_success) {
return lookup_ret;
}
+ from_cache = 1;
}
if (session == NULL ||
session->sid_ctx_length != ssl->sid_ctx_length ||
memcmp(session->sid_ctx, ssl->sid_ctx, ssl->sid_ctx_length) != 0) {
+ /* The client did not offer a suitable ticket or session ID. If supported,
+ * the new session should use a ticket. */
goto no_session;
}
@@ -471,11 +474,12 @@
* effectively disable the session cache by accident without anyone
* noticing). */
OPENSSL_PUT_ERROR(SSL, SSL_R_SESSION_ID_CONTEXT_UNINITIALIZED);
- goto fatal_error;
+ SSL_SESSION_free(session);
+ return ssl_session_error;
}
if (session->timeout < (long)(time(NULL) - session->time)) {
- if (!tickets_supported) {
+ if (from_cache) {
/* The session was from the cache, so remove it. */
SSL_CTX_remove_session(ssl->initial_ctx, session);
}
@@ -483,13 +487,9 @@
}
*out_session = session;
- *out_send_ticket = send_ticket;
+ *out_send_ticket = renew_ticket;
return ssl_session_success;
-fatal_error:
- SSL_SESSION_free(session);
- return ssl_session_error;
-
no_session:
*out_session = NULL;
*out_send_ticket = tickets_supported;
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 5aea08b..d12ec5b 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -2484,7 +2484,7 @@
}
int tls_process_ticket(SSL *ssl, SSL_SESSION **out_session,
- int *out_send_ticket, const uint8_t *ticket,
+ int *out_renew_ticket, const uint8_t *ticket,
size_t ticket_len, const uint8_t *session_id,
size_t session_id_len) {
int ret = 1; /* Most errors are non-fatal. */
@@ -2496,19 +2496,13 @@
EVP_CIPHER_CTX cipher_ctx;
EVP_CIPHER_CTX_init(&cipher_ctx);
- *out_send_ticket = 0;
+ *out_renew_ticket = 0;
*out_session = NULL;
if (session_id_len > SSL_MAX_SSL_SESSION_ID_LENGTH) {
goto done;
}
- if (ticket_len == 0) {
- /* The client will accept a ticket but doesn't currently have one. */
- *out_send_ticket = 1;
- goto done;
- }
-
/* Ensure there is room for the key name and the largest IV
* |tlsext_ticket_key_cb| may try to consume. The real limit may be lower, but
* the maximum IV length should be well under the minimum size for the
@@ -2530,7 +2524,7 @@
goto done;
}
if (cb_ret == 2) {
- *out_send_ticket = 1;
+ *out_renew_ticket = 1;
}
} else {
/* Check the key name matches. */
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index db3c675..f2ef360 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -814,6 +814,10 @@
// BadHelloRequest, if not nil, is what to send instead of a
// HelloRequest.
BadHelloRequest []byte
+
+ // RequireSessionTickets, if true, causes the client to require new
+ // sessions use session tickets instead of session IDs.
+ RequireSessionTickets bool
}
func (c *Config) serverInit() {
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go
index 64630ba..1f52dce 100644
--- a/ssl/test/runner/handshake_client.go
+++ b/ssl/test/runner/handshake_client.go
@@ -363,6 +363,9 @@
}
if sessionCache != nil && hs.session != nil && session != hs.session {
+ if c.config.Bugs.RequireSessionTickets && len(hs.session.sessionTicket) == 0 {
+ return errors.New("tls: new session used session IDs instead of tickets")
+ }
sessionCache.Put(cacheKey, hs.session)
}
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 45bb0b7..b9d3f51 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2088,6 +2088,15 @@
shouldFail: true,
expectedError: ":BAD_HELLO_REQUEST:",
},
+ {
+ testType: serverTest,
+ name: "SupportTicketsWithSessionID",
+ config: Config{
+ SessionTicketsDisabled: true,
+ },
+ resumeConfig: &Config{},
+ resumeSession: true,
+ },
}
testCases = append(testCases, basicTests...)
}
@@ -2667,6 +2676,8 @@
tests = append(tests, testCase{
name: "Basic-Client",
resumeSession: true,
+ // Ensure session tickets are used, not session IDs.
+ noSessionCache: true,
})
tests = append(tests, testCase{
name: "Basic-Client-RenewTicket",
@@ -2691,8 +2702,13 @@
resumeSession: true,
})
tests = append(tests, testCase{
- testType: serverTest,
- name: "Basic-Server",
+ testType: serverTest,
+ name: "Basic-Server",
+ config: Config{
+ Bugs: ProtocolBugs{
+ RequireSessionTickets: true,
+ },
+ },
resumeSession: true,
})
tests = append(tests, testCase{
