Wipe channel strings that can carry secrets

src/internal.c

@@ -1967,11 +1967,7 @@ void SshResourceFree(WOLFSSH* ssh, void* heap)
     if (ssh->userName) {
         WFREE(ssh->userName, heap, DYNTYPE_STRING);
     }
-    if (ssh->channelName != NULL) {
-        WFREE(ssh->channelName, heap, DYNTYPE_STRING);
-        ssh->channelName = NULL;
-        ssh->channelNameSz = 0;
-    }
+    FreeChannelName(ssh, heap);
     if (ssh->peerProtoId) {
         WFREE(ssh->peerProtoId, heap, DYNTYPE_STRING);
     }
@@ -4470,6 +4466,20 @@ static void FreeChannelCommand(void* heap, char* command, word32 commandSz)
 }
 
 
+/* Wipe and free the client's stored exec/subsystem name. */
+void FreeChannelName(WOLFSSH* ssh, void* heap)
+{
+    WOLFSSH_UNUSED(heap);
+
+    if (ssh->channelName != NULL) {
+        WS_FORCEZERO(ssh->channelName, ssh->channelNameSz);
+        WFREE(ssh->channelName, heap, DYNTYPE_STRING);
+        ssh->channelName = NULL;
+    }
+    ssh->channelNameSz = 0;
+}
+
+
 void ChannelDelete(WOLFSSH_CHANNEL* channel, void* heap)
 {
     WOLFSSH_UNUSED(heap);
@@ -13866,6 +13876,7 @@ static int DoChannelRequest(WOLFSSH* ssh,
                 else
                     WLOG(WS_LOG_DEBUG, "  %s = %s", name, value);
             }
+            WS_FORCEZERO(value, sizeof(value));
         }
         else if (ChannelRequestIs(type, typeSz, "shell")) {
             ret = DoChannelRequestSession(ssh, channelId, channel,

src/ssh.c

@@ -2059,11 +2059,7 @@ int wolfSSH_SetChannelType(WOLFSSH* ssh, byte type, byte* name, word32 nameSz)
     switch (type) {
         case WOLFSSH_SESSION_SHELL:
             /* shell has no name; drop any name left by a prior subsystem/exec */
-            if (ssh->channelName != NULL) {
-                WFREE(ssh->channelName, ssh->ctx->heap, DYNTYPE_STRING);
-                ssh->channelName = NULL;
-            }
-            ssh->channelNameSz = 0;
+            FreeChannelName(ssh, ssh->ctx->heap);
             ssh->connectChannelId = type;
             break;
 
@@ -2100,9 +2096,7 @@ int wolfSSH_SetChannelType(WOLFSSH* ssh, byte type, byte* name, word32 nameSz)
                     }
                     WMEMCPY(newName, name, nameSz);
                     newName[nameSz] = 0;
-                    if (ssh->channelName != NULL) {
-                        WFREE(ssh->channelName, ssh->ctx->heap, DYNTYPE_STRING);
-                    }
+                    FreeChannelName(ssh, ssh->ctx->heap);
                     ssh->channelName = newName;
                     ssh->channelNameSz = nameSz;
                 }
@@ -2128,11 +2122,7 @@ int wolfSSH_SetChannelType(WOLFSSH* ssh, byte type, byte* name, word32 nameSz)
         case WOLFSSH_SESSION_TERMINAL:
             /* send a pseudo-terminal request and shell channel */
             ssh->sendTerminalRequest = 1;
-            if (ssh->channelName != NULL) {
-                WFREE(ssh->channelName, ssh->ctx->heap, DYNTYPE_STRING);
-                ssh->channelName = NULL;
-            }
-            ssh->channelNameSz = 0;
+            FreeChannelName(ssh, ssh->ctx->heap);
             ssh->connectChannelId = WOLFSSH_SESSION_SHELL;
             break;
 #endif

tests/unit.c

@@ -20355,6 +20355,140 @@ static int test_ChannelDelete_zeroesCommand(void)
     return result;
 }
 
+
+/* Store a command line as the client's exec channel name and return the
+ * buffer holding it, or NULL. */
+static const byte* SetExecName(WOLFSSH* ssh, const char* command)
+{
+    if (wolfSSH_SetChannelType(ssh, WOLFSSH_SESSION_EXEC, (byte*)command,
+                (word32)WSTRLEN(command)) != WS_SUCCESS)
+        return NULL;
+    return ssh->channelName;
+}
+
+static int IsWiped(const byte* p, word32 sz)
+{
+    word32 i;
+
+    if (p == NULL || !IsRetained((void*)p))
+        return 0;
+    for (i = 0; i < sz; i++) {
+        if (p[i] != 0)
+            return 0;
+    }
+    return 1;
+}
+
+/* Verify every release of the client's exec command line in
+ * ssh->channelName wipes it first: switching to shell or terminal,
+ * replacing it with another command, and wolfSSH_free(). The
+ * retain-on-free allocator is installed just around each release so the
+ * freed bytes can be read back. */
+static int test_ChannelName_zeroedOnFree(void)
+{
+    static const char command[] = "sh -c 'login --password hunter2'";
+    WOLFSSH_CTX* ctx = NULL;
+    WOLFSSH* ssh = NULL;
+    const byte* name;
+    word32 commandSz = (word32)WSTRLEN(command);
+    int result = 0;
+    wolfSSL_Malloc_cb prevMf = NULL;
+    wolfSSL_Free_cb prevFf = NULL;
+    wolfSSL_Realloc_cb prevRf = NULL;
+
+    ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL);
+    if (ctx == NULL)
+        return -740;
+    ssh = wolfSSH_new(ctx);
+    if (ssh == NULL) {
+        result = -741;
+        goto out;
+    }
+    wolfSSL_GetAllocators(&prevMf, &prevFf, &prevRf);
+
+    /* exec to shell */
+    if ((name = SetExecName(ssh, command)) == NULL) {
+        result = -742;
+        goto out;
+    }
+    if (wolfSSL_SetAllocators(RetainMalloc, RetainFree,
+                              RetainRealloc) != 0) {
+        result = -743;
+        goto out;
+    }
+    wolfSSH_SetChannelType(ssh, WOLFSSH_SESSION_SHELL, NULL, 0);
+    wolfSSL_SetAllocators(prevMf, prevFf, prevRf);
+    if (!IsWiped(name, commandSz)) {
+        result = -744;
+        goto out;
+    }
+    DrainRetained();
+
+    /* exec replaced by another exec */
+    if ((name = SetExecName(ssh, command)) == NULL) {
+        result = -745;
+        goto out;
+    }
+    if (wolfSSL_SetAllocators(RetainMalloc, RetainFree,
+                              RetainRealloc) != 0) {
+        result = -746;
+        goto out;
+    }
+    wolfSSH_SetChannelType(ssh, WOLFSSH_SESSION_EXEC, (byte*)"ls", 2);
+    wolfSSL_SetAllocators(prevMf, prevFf, prevRf);
+    if (!IsWiped(name, commandSz)) {
+        result = -747;
+        goto out;
+    }
+    DrainRetained();
+
+#ifdef WOLFSSH_TERM
+    /* exec to terminal */
+    if ((name = SetExecName(ssh, command)) == NULL) {
+        result = -748;
+        goto out;
+    }
+    if (wolfSSL_SetAllocators(RetainMalloc, RetainFree,
+                              RetainRealloc) != 0) {
+        result = -749;
+        goto out;
+    }
+    wolfSSH_SetChannelType(ssh, WOLFSSH_SESSION_TERMINAL, NULL, 0);
+    wolfSSL_SetAllocators(prevMf, prevFf, prevRf);
+    if (!IsWiped(name, commandSz)) {
+        result = -750;
+        goto out;
+    }
+    DrainRetained();
+#endif
+
+    /* wolfSSH_free() */
+    if ((name = SetExecName(ssh, command)) == NULL) {
+        result = -751;
+        goto out;
+    }
+    if (wolfSSL_SetAllocators(RetainMalloc, RetainFree,
+                              RetainRealloc) != 0) {
+        result = -752;
+        goto out;
+    }
+    wolfSSH_free(ssh);
+    wolfSSL_SetAllocators(prevMf, prevFf, prevRf);
+    ssh = NULL;
+    if (!IsWiped(name, commandSz)) {
+        result = -753;
+        goto out;
+    }
+
+out:
+    DrainRetained();
+    if (ssh != NULL)
+        wolfSSH_free(ssh);
+    if (ctx != NULL)
+        wolfSSH_CTX_free(ctx);
+    return result;
+}
+
 #ifdef WOLFSSH_TEST_INTERNAL
 
 /* [uint32 channelId][string "exec"][byte wantReply][string command] */
@@ -26945,6 +27079,11 @@ int wolfSSH_UnitTest(int argc, char** argv)
             (unitResult == 0 ? "SUCCESS" : "FAILED"));
     testResult = testResult || unitResult;
 
+    unitResult = test_ChannelName_zeroedOnFree();
+    printf("ChannelName_zeroedOnFree: %s\n",
+            (unitResult == 0 ? "SUCCESS" : "FAILED"));
+    testResult = testResult || unitResult;
+
 #ifdef WOLFSSH_TEST_INTERNAL
     unitResult = test_DoChannelRequest_zeroesRefusedCommand();
     printf("DoChannelRequest_zeroesRefusedCommand: %s\n",

wolfssh/internal.h

@@ -1571,6 +1571,7 @@ WOLFSSH_LOCAL WOLFSSH_CTX* CtxInit(WOLFSSH_CTX* ctx, byte side, void* heap);
 WOLFSSH_LOCAL void CtxResourceFree(WOLFSSH_CTX* ctx);
 WOLFSSH_LOCAL WOLFSSH* SshInit(WOLFSSH* ssh, WOLFSSH_CTX* ctx);
 WOLFSSH_LOCAL void SshResourceFree(WOLFSSH* ssh, void* heap);
+WOLFSSH_LOCAL void FreeChannelName(WOLFSSH* ssh, void* heap);
 
 WOLFSSH_LOCAL WOLFSSH_CHANNEL* ChannelNew(WOLFSSH* ssh, byte channelType,
         word32 initialWindowSz, word32 maxPacketSz);