Merge pull request #7812 from dgarske/fix_tls12_secret_callback

Fix for TLS v1.2 secret callback, incorrectly detecting bad master secret
wolfSSL · Jul 30, 2024 · ad76038 · ad76038
2 parents 6a1139a + 1d9b86e
commit ad76038
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 34 deletions.
diff --git a/src/internal.c b/src/internal.c
@@ -344,7 +344,7 @@ void wolfssl_priv_der_unblind(DerBuffer* key, DerBuffer* mask)
     {
         wolfSSL_CTX_keylog_cb_func logCb = NULL;
         int msSz;
-        int hasVal;
+        int invalidCount;
         int i;
         const char* label = SSC_CR;
         int labelSz = sizeof(SSC_CR);
@@ -355,32 +355,34 @@ void wolfssl_priv_der_unblind(DerBuffer* key, DerBuffer* mask)
         int ret;
         (void)ctx;
 
-        if (ssl == NULL || secret == NULL || *secretSz == 0)
+        if (ssl == NULL || secret == NULL || secretSz == NULL || *secretSz == 0)
             return BAD_FUNC_ARG;
         if (ssl->arrays == NULL)
             return BAD_FUNC_ARG;
 
-        /* get the user-callback func from CTX*/
+        /* get the user-callback func from CTX */
         logCb = ssl->ctx->keyLogCb;
-        if (logCb == NULL)
-            return 0;
+        if (logCb == NULL) {
+            return 0; /* no logging callback */
+        }
 
-        /* need to make sure the given master-secret has a meaningful value */
+        /* make sure the given master-secret has a meaningful value */
         msSz   = *secretSz;
-        hasVal = 0;
+        invalidCount = 0;
         for (i = 0; i < msSz; i++) {
-            if (*((byte*)secret) != 0) {
-                hasVal = 1;
-                break;
+            if (((byte*)secret)[i] == 0) {
+                invalidCount++;
             }
         }
-        if (hasVal == 0)
-            return 0; /* master-secret looks invalid */
+        if (invalidCount == *secretSz) {
+            WOLFSSL_MSG("master-secret is not valid");
+            return 0; /* ignore error */
+        }
 
         /* build up a hex-decoded keylog string
-           "CLIENT_RANDOM <hex-encoded client random> <hex-encoded master-secret>"
-           note that each keylog string does not have CR/LF.
-        */
+         * "CLIENT_RANDOM <hex-encoded client rand> <hex-encoded master-secret>"
+         * note that each keylog string does not have CR/LF.
+         */
         buffSz  = labelSz + (RAN_LEN * 2) + 1 + ((*secretSz) * 2) + 1;
         log     = XMALLOC(buffSz, ssl->heap, DYNAMIC_TYPE_SECRET);
         if (log == NULL)
@@ -410,8 +412,9 @@ void wolfssl_priv_der_unblind(DerBuffer* key, DerBuffer* mask)
                     ret = 0;
                 }
             }
-            else
-                ret = MEMORY_E;
+            else {
+                ret = BUFFER_E;
+            }
         }
         /* Zero out Base16 encoded secret and other data. */
         ForceZero(log, buffSz);

diff --git a/src/ssl.c b/src/ssl.c
@@ -23353,7 +23353,7 @@ void wolfSSL_CTX_set_keylog_callback(WOLFSSL_CTX* ctx,
     wolfSSL_CTX_keylog_cb_func cb)
 {
     WOLFSSL_ENTER("wolfSSL_CTX_set_keylog_callback");
-  /* stores the callback into WOLFSSL_CTX */
+    /* stores the callback into WOLFSSL_CTX */
     if (ctx != NULL) {
         ctx->keyLogCb = cb;
     }
@@ -23364,8 +23364,7 @@ wolfSSL_CTX_keylog_cb_func wolfSSL_CTX_get_keylog_callback(
     WOLFSSL_ENTER("wolfSSL_CTX_get_keylog_callback");
     if (ctx != NULL)
         return ctx->keyLogCb;
-    else
-        return NULL;
+    return NULL;
 }
 #endif /* OPENSSL_EXTRA && HAVE_SECRET_CALLBACK */
 

diff --git a/tests/api.c b/tests/api.c
@@ -49570,20 +49570,19 @@ static THREAD_RETURN WOLFSSL_THREAD server_task_ech(void* args)
 #endif /* HAVE_ECH && WOLFSSL_TLS13 */
 
 #if defined(OPENSSL_EXTRA) && defined(HAVE_SECRET_CALLBACK)
-static void keyLog_callback(const WOLFSSL* ssl, const char* line )
+static void keyLog_callback(const WOLFSSL* ssl, const char* line)
 {
+    XFILE fp;
+    const byte lf = '\n';
 
     AssertNotNull(ssl);
     AssertNotNull(line);
 
-    XFILE fp;
-    const byte  lf = '\n';
     fp = XFOPEN("./MyKeyLog.txt", "a");
-    XFWRITE( line, 1, strlen(line),fp);
-    XFWRITE( (void*)&lf,1,1,fp);
+    XFWRITE(line, 1, XSTRLEN(line), fp);
+    XFWRITE((void*)&lf, 1, 1, fp);
     XFFLUSH(fp);
     XFCLOSE(fp);
-
 }
 #endif /* OPENSSL_EXTRA && HAVE_SECRET_CALLBACK */
 static int test_wolfSSL_CTX_set_keylog_callback(void)
@@ -49631,12 +49630,14 @@ static int test_wolfSSL_Tls12_Key_Logging_test(void)
 {
     EXPECT_DECLS;
 #if defined(OPENSSL_EXTRA) && defined(HAVE_SECRET_CALLBACK)
-/* This test is intended for checking whether keylog callback is called
- * in client during TLS handshake between the client and a server.
- */
+    /* This test is intended for checking whether keylog callback is called
+     * in client during TLS handshake between the client and a server.
+     */
     test_ssl_cbf server_cbf;
     test_ssl_cbf client_cbf;
     XFILE fp = XBADFILE;
+    char  buff[500];
+    int   found = 0;
 
     XMEMSET(&server_cbf, 0, sizeof(test_ssl_cbf));
     XMEMSET(&client_cbf, 0, sizeof(test_ssl_cbf));
@@ -49653,16 +49654,12 @@ static int test_wolfSSL_Tls12_Key_Logging_test(void)
 
     ExpectIntEQ(test_wolfSSL_client_server_nofail_memio(&client_cbf,
         &server_cbf, NULL), TEST_SUCCESS);
-    XSLEEP_MS(100);
 
     /* check if the keylog file exists */
-
-    char  buff[300] = {0};
-    int  found = 0;
-
     ExpectTrue((fp = XFOPEN("./MyKeyLog.txt", "r")) != XBADFILE);
     XFFLUSH(fp); /* Just to make sure any buffers get flushed */
 
+    XMEMSET(buff, 0, sizeof(buff));
     while (EXPECT_SUCCESS() && XFGETS(buff, (int)sizeof(buff), fp) != NULL) {
         if (0 == strncmp(buff,"CLIENT_RANDOM ", sizeof("CLIENT_RANDOM ")-1)) {
             found = 1;