Merge pull request #10534 from SparkiDev/tls13_psk_id_fix

TLSv1.3 PSK binders: always use id protection

Author: douzzer

src/tls13.c

@@ -57,7 +57,6 @@
  * WOLFSSL_PSK_ONE_ID:       Single PSK identity per connect       default: off
  * WOLFSSL_PSK_MULTI_ID_PER_CS: Multiple PSK IDs per cipher suite default: off
  * WOLFSSL_PRIORITIZE_PSK:   Prioritize PSK over ciphersuite order default: off
- * WOLFSSL_PSK_ID_PROTECTION: Enable PSK identity protection       default: off
  *
  * TLS 1.3 Session Tickets:
  * WOLFSSL_TICKET_HAVE_ID:   Session tickets include ID            default: off
@@ -6544,20 +6543,8 @@ static int DoPreSharedKeys(WOLFSSL* ssl, const byte* input, word32 inputSz,
     }
 
     if (current == NULL) {
-#ifdef WOLFSSL_PSK_ID_PROTECTION
-    #ifndef NO_CERTS
-        if (ssl->buffers.certChainCnt != 0) {
-            ret = 0;
-            goto cleanup;
-        }
-    #endif
-        WOLFSSL_ERROR_VERBOSE(BAD_BINDER);
-        ret = BAD_BINDER;
-        goto cleanup;
-#else
         ret = 0;
         goto cleanup;
-#endif
     }
 
     *first = (current == ext->data);
@@ -6664,6 +6651,20 @@ static int CheckPreSharedKeys(WOLFSSL* ssl, const byte* input, word32 helloSz,
     }
 #endif
 
+    if (!*usingPSK) {
+    #ifndef NO_CERTS
+        if (ssl->buffers.certificate == NULL
+        #ifdef WOLFSSL_CERT_SETUP_CB
+                && ssl->ctx->certSetupCb == NULL
+        #endif
+                )
+    #endif
+        {
+            WOLFSSL_ERROR_VERBOSE(BAD_BINDER);
+            return BAD_BINDER;
+        }
+    }
+
     if (*usingPSK) {
         /* While verifying the selected PSK, we updated the
          * handshake hash up to the binder bytes in the PSK extensions.
@@ -6834,14 +6835,16 @@ static int CheckPreSharedKeys(WOLFSSL* ssl, const byte* input, word32 helloSz,
         TLSX_Remove(&ssl->extensions, TLSX_CERT_WITH_EXTERN_PSK, ssl->heap);
         ssl->options.certWithExternPsk = 0;
 #endif
-#ifdef WOLFSSL_PSK_ID_PROTECTION
     #ifndef NO_CERTS
-        if (ssl->buffers.certChainCnt != 0)
+        if (ssl->buffers.certificate != NULL
+        #ifdef WOLFSSL_CERT_SETUP_CB
+                || ssl->ctx->certSetupCb != NULL
+        #endif
+                )
             return 0;
     #endif
         WOLFSSL_ERROR_VERBOSE(BAD_BINDER);
         return BAD_BINDER;
-#endif
     }
 
     WOLFSSL_LEAVE("CheckPreSharedKeys", ret);

tests/api/test_tls13.c

@@ -1775,6 +1775,162 @@ int test_tls13_bad_psk_binder(void)
 }
 
 
+#if defined(WOLFSSL_TLS13) && \
+    defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES_BUILD) && \
+    !defined(NO_PSK)
+static unsigned int test_tls13_psk_no_cert_client_cb(WOLFSSL* ssl,
+        const char* hint, char* identity, unsigned int id_max_len,
+        unsigned char* key, unsigned int key_max_len)
+{
+    (void)ssl;
+    (void)hint;
+    (void)key_max_len;
+
+    /* Offer a PSK so the client sends a pre_shared_key extension. */
+    XSTRNCPY(identity, "Client_identity", id_max_len);
+    key[0] = 0x20;
+    return 1;
+}
+
+static unsigned int test_tls13_psk_no_cert_server_cb(WOLFSSL* ssl,
+        const char* id, unsigned char* key, unsigned int key_max_len)
+{
+    (void)ssl;
+    (void)id;
+    (void)key;
+    (void)key_max_len;
+
+    /* Reject every identity so the server finds no matching PSK. */
+    return 0;
+}
+#endif
+
+/* When no offered PSK matches and the server has no certificate to fall back
+ * to, the server must abort the handshake with BAD_BINDER rather than silently
+ * continuing. This covers both configurations:
+ *   - NO_CERTS defined: the certificate fall-back branch is compiled out.
+ *   - certificates compiled in but none loaded: ssl->buffers.certificate is
+ *     NULL, so the runtime check takes the same abort path.
+ * The contexts are built by hand (no certificate loaded) so the test exercises
+ * whichever branch the build provides.
+ * When certificates are compiled in, a second connection sets a certificate
+ * and key against the server context and verifies the opposite branch: the
+ * non-matching PSK is ignored and the handshake falls back to a full
+ * certificate handshake instead of aborting. */
+int test_tls13_psk_no_cert_bad_binder(void)
+{
+    EXPECT_DECLS;
+#if defined(WOLFSSL_TLS13) && \
+    defined(HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES_BUILD) && \
+    !defined(NO_PSK)
+    WOLFSSL_CTX *ctx_c = NULL;
+    WOLFSSL_CTX *ctx_s = NULL;
+    WOLFSSL *ssl_c = NULL;
+    WOLFSSL *ssl_s = NULL;
+    struct test_memio_ctx test_ctx;
+    WOLFSSL_ALERT_HISTORY h;
+
+    XMEMSET(&test_ctx, 0, sizeof(test_ctx));
+
+    /* Don't use test_memio_setup(): it loads a default server certificate,
+     * which would let the server fall back to a certificate handshake. Build
+     * the contexts by hand so the server has no certificate loaded. */
+    ExpectNotNull(ctx_c = wolfSSL_CTX_new(wolfTLSv1_3_client_method()));
+    ExpectNotNull(ctx_s = wolfSSL_CTX_new(wolfTLSv1_3_server_method()));
+    if (ctx_c != NULL) {
+        wolfSSL_SetIORecv(ctx_c, test_memio_read_cb);
+        wolfSSL_SetIOSend(ctx_c, test_memio_write_cb);
+    }
+    if (ctx_s != NULL) {
+        wolfSSL_SetIORecv(ctx_s, test_memio_read_cb);
+        wolfSSL_SetIOSend(ctx_s, test_memio_write_cb);
+    }
+
+    /* Set the PSK callbacks on the contexts, not the SSL objects: with
+     * certificates compiled in, creating a server-side SSL object without a
+     * certificate and key fails (NO_PRIVATE_KEY) unless ctx->havePSK is
+     * already set when wolfSSL_new() is called. */
+    wolfSSL_CTX_set_psk_client_callback(ctx_c,
+        test_tls13_psk_no_cert_client_cb);
+    wolfSSL_CTX_set_psk_server_callback(ctx_s,
+        test_tls13_psk_no_cert_server_cb);
+
+    ExpectNotNull(ssl_c = wolfSSL_new(ctx_c));
+    ExpectNotNull(ssl_s = wolfSSL_new(ctx_s));
+    if (ssl_c != NULL) {
+        wolfSSL_SetIOWriteCtx(ssl_c, &test_ctx);
+        wolfSSL_SetIOReadCtx(ssl_c, &test_ctx);
+    }
+    if (ssl_s != NULL) {
+        wolfSSL_SetIOWriteCtx(ssl_s, &test_ctx);
+        wolfSSL_SetIOReadCtx(ssl_s, &test_ctx);
+    }
+
+    /* Confirm the precondition: the server really has no certificate. */
+#ifndef NO_CERTS
+    if (ssl_s != NULL) {
+        ExpectNull(ssl_s->buffers.certificate);
+    }
+#endif
+
+    /* Client sends ClientHello (with PSK) and waits for the response. */
+    ExpectIntNE(wolfSSL_connect(ssl_c), WOLFSSL_SUCCESS);
+    ExpectIntEQ(wolfSSL_get_error(ssl_c, WC_NO_ERR_TRACE(WOLFSSL_FATAL_ERROR)),
+        WOLFSSL_ERROR_WANT_READ);
+
+    /* Server processes ClientHello: no PSK matches and no certificate is
+     * available, so it must abort with BAD_BINDER. */
+    ExpectIntNE(wolfSSL_accept(ssl_s), WOLFSSL_SUCCESS);
+    ExpectIntEQ(wolfSSL_get_error(ssl_s, WC_NO_ERR_TRACE(WOLFSSL_FATAL_ERROR)),
+        WC_NO_ERR_TRACE(BAD_BINDER));
+
+    /* Client reads the server's alert: BAD_BINDER maps to a fatal
+     * illegal_parameter alert (see TranslateErrorToAlert). */
+    ExpectIntNE(wolfSSL_connect(ssl_c), WOLFSSL_SUCCESS);
+    ExpectIntEQ(wolfSSL_get_error(ssl_c, WC_NO_ERR_TRACE(WOLFSSL_FATAL_ERROR)),
+        WC_NO_ERR_TRACE(FATAL_ERROR));
+    ExpectIntEQ(wolfSSL_get_alert_history(ssl_c, &h), WOLFSSL_SUCCESS);
+    ExpectIntEQ(h.last_rx.code, illegal_parameter);
+    ExpectIntEQ(h.last_rx.level, alert_fatal);
+
+    wolfSSL_free(ssl_c);
+    ssl_c = NULL;
+    wolfSSL_CTX_free(ctx_c);
+    ctx_c = NULL;
+    wolfSSL_free(ssl_s);
+    ssl_s = NULL;
+    wolfSSL_CTX_free(ctx_s);
+    ctx_s = NULL;
+
+#ifndef NO_CERTS
+    /* Conversely, with a certificate and key set against the server context,
+     * a non-matching PSK must not leak the mismatch: the server ignores the
+     * PSK and falls back to a full certificate handshake. test_memio_setup()
+     * loads the default CA, server certificate and key. */
+    XMEMSET(&test_ctx, 0, sizeof(test_ctx));
+    ExpectIntEQ(test_memio_setup(&test_ctx, &ctx_c, &ctx_s, &ssl_c, &ssl_s,
+        wolfTLSv1_3_client_method, wolfTLSv1_3_server_method), 0);
+
+    wolfSSL_set_psk_client_callback(ssl_c, test_tls13_psk_no_cert_client_cb);
+    wolfSSL_set_psk_server_callback(ssl_s, test_tls13_psk_no_cert_server_cb);
+
+    /* Confirm the precondition: the server has a certificate this time. */
+    if (ssl_s != NULL) {
+        ExpectNotNull(ssl_s->buffers.certificate);
+    }
+
+    ExpectIntEQ(test_memio_do_handshake(ssl_c, ssl_s, 10, NULL), 0);
+
+    wolfSSL_free(ssl_c);
+    wolfSSL_CTX_free(ctx_c);
+    wolfSSL_free(ssl_s);
+    wolfSSL_CTX_free(ctx_s);
+#endif /* !NO_CERTS */
+#endif
+    return EXPECT_RESULT();
+}
+
+
 #if defined(HAVE_RPK) && !defined(NO_TLS) && !defined(NO_WOLFSSL_CLIENT) && \
     !defined(NO_WOLFSSL_SERVER)
 

tests/api/test_tls13.h

@@ -63,6 +63,7 @@ int test_tls13_post_handshake_auth_no_ext(void);
 int test_tls13_post_handshake_auth_late_allow(void);
 int test_tls13_downgrade_sentinel(void);
 int test_tls13_serverhello_bad_cipher_suites(void);
+int test_tls13_psk_no_cert_bad_binder(void);
 int test_tls13_cert_with_extern_psk_apis(void);
 int test_tls13_cert_with_extern_psk_handshake(void);
 int test_tls13_cert_with_extern_psk_requires_key_share(void);
@@ -122,6 +123,7 @@ int test_tls13_AEAD_limit_KU_aes128_ccm_8_sha256(void);
     TEST_DECL_GROUP("tls13", test_tls13_post_handshake_auth_late_allow), \
     TEST_DECL_GROUP("tls13", test_tls13_downgrade_sentinel), \
     TEST_DECL_GROUP("tls13", test_tls13_serverhello_bad_cipher_suites), \
+    TEST_DECL_GROUP("tls13", test_tls13_psk_no_cert_bad_binder), \
     TEST_DECL_GROUP("tls13", test_tls13_cert_with_extern_psk_apis), \
     TEST_DECL_GROUP("tls13", test_tls13_cert_with_extern_psk_handshake), \
     TEST_DECL_GROUP("tls13", test_tls13_cert_with_extern_psk_requires_key_share), \

tests/utils.c

@@ -23,7 +23,7 @@
 #include <tests/utils.h>
 #include <wolfssl/wolfcrypt/error-crypt.h>
 
-#ifdef HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES
+#ifdef HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES_BUILD
 
 /* This set of memio functions allows for more fine tuned control of the TLS
  * connection operations. For new tests, try to use ssl_memio first. */
@@ -784,7 +784,7 @@ int test_memio_setup(struct test_memio_ctx *ctx,
                                method_s, NULL, 0, NULL, 0, NULL, 0);
 }
 
-#endif /* HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES */
+#endif /* HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES_BUILD */
 
 #if !defined(NO_FILESYSTEM) && defined(OPENSSL_EXTRA) && \
     defined(DEBUG_UNIT_TEST_CERTS)

tests/utils.h

@@ -32,11 +32,26 @@ extern char tmpDirName[16];
 extern const char* currentTestName;
 #endif
 
-#if !defined(NO_FILESYSTEM) && !defined(NO_CERTS) && \
-    (!defined(NO_RSA) || defined(HAVE_RPK)) && \
-    !defined(NO_WOLFSSL_SERVER) && !defined(NO_WOLFSSL_CLIENT) && \
-    (!defined(WOLFSSL_NO_TLS12) || defined(WOLFSSL_TLS13))
+/* Base dependencies for the manual memio test harness. The harness itself does
+ * not require certificate support, so cert-less tests (e.g. PSK-only) can use
+ * it through this narrower macro. */
+#if !defined(NO_WOLFSSL_SERVER) && !defined(NO_WOLFSSL_CLIENT) && \
+    (!defined(WOLFSSL_NO_TLS12) || defined(WOLFSSL_TLS13)) && defined(NO_CERTS)
+#define HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES_NO_CERTS
+#define HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES_BUILD
+#endif
+
+/* Full dependencies: the base harness plus certificate support. Most memio
+ * tests set up a certificate-based handshake and must use this macro. */
+#if !defined(NO_WOLFSSL_SERVER) && !defined(NO_WOLFSSL_CLIENT) && \
+    (!defined(WOLFSSL_NO_TLS12) || defined(WOLFSSL_TLS13)) && \
+    !defined(NO_FILESYSTEM) && !defined(NO_CERTS) && \
+    (!defined(NO_RSA) || defined(HAVE_RPK))
 #define HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES
+#define HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES_BUILD
+#endif
+
+#ifdef HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES_BUILD
 #define TEST_MEMIO_BUF_SZ (64 * 1024)
 #define TEST_MEMIO_MAX_MSGS 32
 
@@ -85,7 +100,7 @@ int test_memio_move_message(struct test_memio_ctx *ctx, int client,
 int test_memio_drop_message(struct test_memio_ctx *ctx, int client, int msg_pos);
 int test_memio_modify_message_len(struct test_memio_ctx *ctx, int client, int msg_pos, int new_len);
 int test_memio_remove_from_buffer(struct test_memio_ctx *ctx, int client, int off, int sz);
-#endif
+#endif /* HAVE_MANUAL_MEMIO_TESTS_DEPENDENCIES_BUILD */
 
 /* Shared TLS server/client thread bodies, defined in tests/api.c. The
  * definitions are gated on ENABLE_TLS_CALLBACK_TEST (a composite condition