Rework the decrypt ticket callback

Don't call the decrypt ticket callback if we've already encountered a
fatal error. Do call it if we have an empty ticket present.

Change the return code to have 5 distinct returns codes and separate it
from the input status value.

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6198)

2 files changed, 126 insertions, 39 deletions

diff --git a/test/handshake_helper.c b/test/handshake_helper.c
index fc5fcd6f4f..b3d94bb1ee 100644
--- a/test/handshake_helper.c
+++ b/test/handshake_helper.c
@@ -469,12 +469,24 @@ static int generate_session_ticket_cb(SSL *s, void *arg)
     return SSL_SESSION_set1_ticket_appdata(ss, app_data, strlen(app_data));
 }
 
-static SSL_TICKET_RETURN decrypt_session_ticket_cb(SSL *s, SSL_SESSION *ss,
-                                                   const unsigned char *keyname,
-                                                   size_t keyname_len,
-                                                   SSL_TICKET_RETURN retv, void *arg)
+static int decrypt_session_ticket_cb(SSL *s, SSL_SESSION *ss,
+                                     const unsigned char *keyname,
+                                     size_t keyname_len,
+                                     SSL_TICKET_STATUS status,
+                                     void *arg)
 {
-    return retv;
+    switch (status) {
+    case SSL_TICKET_EMPTY:
+    case SSL_TICKET_NO_DECRYPT:
+        return SSL_TICKET_RETURN_IGNORE_RENEW;
+    case SSL_TICKET_SUCCESS:
+        return SSL_TICKET_RETURN_USE;
+    case SSL_TICKET_SUCCESS_RENEW:
+        return SSL_TICKET_RETURN_USE_RENEW;
+    default:
+        break;
+    }
+    return SSL_TICKET_RETURN_ABORT;
 }
 
 /*
diff --git a/test/sslapitest.c b/test/sslapitest.c
index efce84d2d9..fc7288d57d 100644
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -4596,7 +4596,9 @@ static int test_ssl_get_shared_ciphers(int tst)
 }
 
 static const char *appdata = "Hello World";
-static int gen_tick_called, dec_tick_called;
+static int gen_tick_called, dec_tick_called, tick_key_cb_called;
+static int tick_key_renew = 0;
+static SSL_TICKET_RETURN tick_dec_ret = SSL_TICKET_RETURN_ABORT;
 
 static int gen_tick_cb(SSL *s, void *arg)
 {
@@ -4609,26 +4611,46 @@ static int gen_tick_cb(SSL *s, void *arg)
 static SSL_TICKET_RETURN dec_tick_cb(SSL *s, SSL_SESSION *ss,
                                      const unsigned char *keyname,
                                      size_t keyname_length,
-                                     SSL_TICKET_RETURN retv, void *arg)
+                                     SSL_TICKET_STATUS status,
+                                     void *arg)
 {
     void *tickdata;
     size_t tickdlen;
 
     dec_tick_called = 1;
 
-    if (!TEST_true(retv == SSL_TICKET_SUCCESS
-                   || retv == SSL_TICKET_SUCCESS_RENEW))
-        return retv;
+    if (status == SSL_TICKET_EMPTY)
+        return SSL_TICKET_RETURN_IGNORE_RENEW;
 
-    if (!TEST_true(SSL_SESSION_get0_ticket_appdata(ss, &tickdata, &tickdlen))
+    if (!TEST_true(status == SSL_TICKET_SUCCESS
+                   || status == SSL_TICKET_SUCCESS_RENEW))
+        return SSL_TICKET_RETURN_ABORT;
+
+    if (!TEST_true(SSL_SESSION_get0_ticket_appdata(ss, &tickdata,
+                                                   &tickdlen))
             || !TEST_size_t_eq(tickdlen, strlen(appdata))
             || !TEST_int_eq(memcmp(tickdata, appdata, tickdlen), 0))
-        return SSL_TICKET_FATAL_ERR_OTHER;
+        return SSL_TICKET_RETURN_ABORT;
 
-    return retv;
-}
+    if (tick_key_cb_called)  {
+        /* Don't change what the ticket key callback wanted to do */
+        switch (status) {
+        case SSL_TICKET_NO_DECRYPT:
+            return SSL_TICKET_RETURN_IGNORE_RENEW;
+
+        case SSL_TICKET_SUCCESS:
+            return SSL_TICKET_RETURN_USE;
 
-static int tick_renew = 0;
+        case SSL_TICKET_SUCCESS_RENEW:
+            return SSL_TICKET_RETURN_USE_RENEW;
+
+        default:
+            return SSL_TICKET_RETURN_ABORT;
+        }
+    }
+    return tick_dec_ret;
+
+}
 
 static int tick_key_cb(SSL *s, unsigned char key_name[16],
                        unsigned char iv[EVP_MAX_IV_LENGTH], EVP_CIPHER_CTX *ctx,
@@ -4637,6 +4659,7 @@ static int tick_key_cb(SSL *s, unsigned char key_name[16],
     const unsigned char tick_aes_key[16] = "0123456789abcdef";
     const unsigned char tick_hmac_key[16] = "0123456789abcdef";
 
+    tick_key_cb_called = 1;
     memset(iv, 0, AES_BLOCK_SIZE);
     memset(key_name, 0, 16);
     if (!EVP_CipherInit_ex(ctx, EVP_aes_128_cbc(), NULL, tick_aes_key, iv, enc)
@@ -4644,17 +4667,23 @@ static int tick_key_cb(SSL *s, unsigned char key_name[16],
                              EVP_sha256(), NULL))
         return -1;
 
-    return tick_renew ? 2 : 1;
+    return tick_key_renew ? 2 : 1;
 }
 
 /*
  * Test the various ticket callbacks
- * Test 0: TLSv1.2, no ticket key callback (default no ticket renewal)
- * Test 1: TLSv1.3, no ticket key callback (default ticket renewal)
- * Test 2: TLSv1.2, ticket key callback, no ticket renewal
- * Test 3: TLSv1.3, ticket key callback, no ticket renewal
- * Test 4: TLSv1.2, ticket key callback, ticket renewal
- * Test 5: TLSv1.3, ticket key callback, ticket renewal
+ * Test 0: TLSv1.2, no ticket key callback, no ticket, no renewal
+ * Test 1: TLSv1.3, no ticket key callback, no ticket, no renewal
+ * Test 2: TLSv1.2, no ticket key callback, no ticket, renewal
+ * Test 3: TLSv1.3, no ticket key callback, no ticket, renewal
+ * Test 4: TLSv1.2, no ticket key callback, ticket, no renewal
+ * Test 5: TLSv1.3, no ticket key callback, ticket, no renewal
+ * Test 6: TLSv1.2, no ticket key callback, ticket, renewal
+ * Test 7: TLSv1.3, no ticket key callback, ticket, renewal
+ * Test 8: TLSv1.2, ticket key callback, ticket, no renewal
+ * Test 9: TLSv1.3, ticket key callback, ticket, no renewal
+ * Test 10: TLSv1.2, ticket key callback, ticket, renewal
+ * Test 11: TLSv1.3, ticket key callback, ticket, renewal
  */
 static int test_ticket_callbacks(int tst)
 {
@@ -4672,27 +4701,60 @@ static int test_ticket_callbacks(int tst)
         return 1;
 #endif
 
-    gen_tick_called = dec_tick_called = 0;
+    gen_tick_called = dec_tick_called = tick_key_cb_called = 0;
 
     /* Which tests the ticket key callback should request renewal for */
-    if (tst == 4 || tst == 5)
-        tick_renew = 1;
+    if (tst == 10 || tst == 11)
+        tick_key_renew = 1;
     else
-        tick_renew = 0;
+        tick_key_renew = 0;
+
+    /* Which tests the decrypt ticket callback should request renewal for */
+    switch (tst) {
+    case 0:
+    case 1:
+        tick_dec_ret = SSL_TICKET_RETURN_IGNORE;
+        break;
+
+    case 2:
+    case 3:
+        tick_dec_ret = SSL_TICKET_RETURN_IGNORE_RENEW;
+        break;
+
+    case 4:
+    case 5:
+        tick_dec_ret = SSL_TICKET_RETURN_USE;
+        break;
+
+    case 6:
+    case 7:
+        tick_dec_ret = SSL_TICKET_RETURN_USE_RENEW;
+        break;
+
+    default:
+        tick_dec_ret = SSL_TICKET_RETURN_ABORT;
+    }
 
     if (!TEST_true(create_ssl_ctx_pair(TLS_server_method(),
                                        TLS_client_method(),
                                        TLS1_VERSION,
-                                       tst == 0 ? TLS1_2_VERSION
-                                                : TLS1_3_VERSION,
+                                       ((tst % 2) == 0) ? TLS1_2_VERSION
+                                                        : TLS1_3_VERSION,
                                        &sctx, &cctx, cert, privkey)))
         goto end;
 
+    /*
+     * We only want sessions to resume from tickets - not the session cache. So
+     * switch the cache off.
+     */
+    if (!TEST_true(SSL_CTX_set_session_cache_mode(sctx, SSL_SESS_CACHE_OFF)))
+        goto end;
+
     if (!TEST_true(SSL_CTX_set_session_ticket_cb(sctx, gen_tick_cb, dec_tick_cb,
                                                  NULL)))
         goto end;
 
-    if (tst >= 2
+    if (tst >= 8
             && !TEST_true(SSL_CTX_set_tlsext_ticket_key_cb(sctx, tick_key_cb)))
         goto end;
 
@@ -4702,8 +4764,15 @@ static int test_ticket_callbacks(int tst)
                                                 SSL_ERROR_NONE)))
         goto end;
 
+    /*
+     * The decrypt ticket key callback in TLSv1.2 should be called even though
+     * we have no ticket yet, because it gets called with a status of
+     * SSL_TICKET_EMPTY (the client indicates support for tickets but does not
+     * actually send any ticket data). This does not happen in TLSv1.3 because
+     * it is not valid to send empty ticket data in TLSv1.3.
+     */
     if (!TEST_int_eq(gen_tick_called, 1)
-            || !TEST_int_eq(dec_tick_called, 0))
+            || !TEST_int_eq(dec_tick_called, ((tst % 2) == 0) ? 1 : 0))
         goto end;
 
     gen_tick_called = dec_tick_called = 0;
@@ -4720,17 +4789,23 @@ static int test_ticket_callbacks(int tst)
                                       NULL))
             || !TEST_true(SSL_set_session(clientssl, clntsess))
             || !TEST_true(create_ssl_connection(serverssl, clientssl,
-                                                SSL_ERROR_NONE))
-            || !TEST_true(SSL_session_reused(clientssl)))
+                                                SSL_ERROR_NONE)))
         goto end;
 
-    /*
-     * In TLSv1.2 we default to not renewing the ticket everytime. In TLSv1.3
-     * we default to renewing. The defaults are overridden if a ticket key
-     * callback is in place.
-     */
+    if (tick_dec_ret == SSL_TICKET_RETURN_IGNORE
+            || tick_dec_ret == SSL_TICKET_RETURN_IGNORE_RENEW) {
+        if (!TEST_false(SSL_session_reused(clientssl)))
+            goto end;
+    } else {
+        if (!TEST_true(SSL_session_reused(clientssl)))
+            goto end;
+    }
+
     if (!TEST_int_eq(gen_tick_called,
-                     (tst == 0 || tst == 2 || tst == 3) ? 0 : 1)
+                     (tick_key_renew
+                      || tick_dec_ret == SSL_TICKET_RETURN_IGNORE_RENEW
+                      || tick_dec_ret == SSL_TICKET_RETURN_USE_RENEW)
+                     ? 1 : 0)
             || !TEST_int_eq(dec_tick_called, 1))
         goto end;
 
@@ -4839,7 +4914,7 @@ int setup_tests(void)
     ADD_ALL_TESTS(test_info_callback, 6);
     ADD_ALL_TESTS(test_ssl_pending, 2);
     ADD_ALL_TESTS(test_ssl_get_shared_ciphers, OSSL_NELEM(shared_ciphers_data));
-    ADD_ALL_TESTS(test_ticket_callbacks, 6);
+    ADD_ALL_TESTS(test_ticket_callbacks, 12);
     return 1;
 }