Bound SFTP NAME response size on the client

Author: yosuke-wolfssl

src/wolfsftp.c

@@ -6750,7 +6750,32 @@ static WS_SFTPNAME* wolfSSH_SFTP_DoName(WOLFSSH* ssh)
 
         case SFTP_NAME_GETHEADER_PACKET:
             maxSz = SFTP_GetHeader(ssh, &reqId, &type, &state->buffer);
-            if (maxSz <= 0 || ssh->error == WS_WANT_READ) {
+            if (maxSz <= 0) {
+                /* A non-positive size is either a retryable notice (want
+                 * read/write, rekey, etc.), where state is kept so the read
+                 * can resume, or a real failure. SFTP_GetHeader returns the
+                 * wire length in an int, so a length above INT_MAX also lands
+                 * here. On a non-notice case, record a size error if none was
+                 * set and clear NAME state so the caller sees the failure. */
+                if (!NoticeError(ssh)) {
+                    if (ssh->error == WS_SUCCESS) {
+                        ssh->error = WS_BUFFER_E;
+                    }
+                    wolfSSH_SFTP_ClearState(ssh, STATE_ID_NAME);
+                }
+                return NULL;
+            }
+
+            /* Bound the NAME message size. The buffer is allocated at this
+             * size and one WS_SFTPNAME node is created per entry, so an
+             * over-large packet from a malicious or MITM server would amplify
+             * into several times that much live heap. */
+            if (maxSz > WOLFSSH_MAX_SFTP_NAME) {
+                WLOG(WS_LOG_SFTP,
+                        "SFTP NAME packet size %d exceeds limit %d",
+                        maxSz, (int)WOLFSSH_MAX_SFTP_NAME);
+                ssh->error = WS_BUFFER_E;
+                wolfSSH_SFTP_ClearState(ssh, STATE_ID_NAME);
                 return NULL;
             }
 
@@ -6912,6 +6937,26 @@ static WS_SFTPNAME* wolfSSH_SFTP_DoName(WOLFSSH* ssh)
 }
 
 
+#ifdef WOLFSSH_TEST_INTERNAL
+/* Drive wolfSSH_SFTP_DoName for unit testing. Frees any returned name list
+ * and reports ssh->error so the caller can check the NAME size bound. */
+int wolfSSH_TestSftpDoName(WOLFSSH* ssh)
+{
+    WS_SFTPNAME* name;
+
+    if (ssh == NULL) {
+        return WS_BAD_ARGUMENT;
+    }
+
+    name = wolfSSH_SFTP_DoName(ssh);
+    if (name != NULL) {
+        wolfSSH_SFTPNAME_list_free(name);
+    }
+    return ssh->error;
+}
+#endif
+
+
 /* get the file handle from SSH stream
  *
  * handle   buffer to hold result

tests/unit.c

@@ -4983,6 +4983,110 @@ static int test_Errors(void)
     return result;
 }
 
+#if defined(WOLFSSH_SFTP) && defined(WOLFSSH_TEST_INTERNAL)
+/* Inject a crafted SFTP NAME header declaring an on-wire payload length of
+ * 'wireLen' into a channel, drive wolfSSH_SFTP_DoName, and report ssh->error
+ * via outErr. Only the 9-byte header is needed: the NAME size bound is
+ * checked before the message body is read. Returns 0 on setup success. */
+static int sftpDoNameInjectErr(word32 wireLen, int* outErr)
+{
+    WOLFSSH_CTX*     ctx = NULL;
+    WOLFSSH*         ssh = NULL;
+    WOLFSSH_CHANNEL* ch  = NULL;
+    byte   hdr[LENGTH_SZ + MSG_ID_SZ + UINT32_SZ];
+    int    result = 0;
+
+    ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL);
+    if (ctx == NULL)
+        return -560;
+    ssh = wolfSSH_new(ctx);
+    if (ssh == NULL) {
+        wolfSSH_CTX_free(ctx);
+        return -561;
+    }
+
+    ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 128, 128);
+    if (ch == NULL) {
+        result = -562;
+        goto done;
+    }
+    if (ChannelAppend(ssh, ch) != WS_SUCCESS) {
+        ChannelDelete(ch, ssh->ctx->heap);
+        result = -563;
+        goto done;
+    }
+
+    /* SFTP header: [uint32 length][byte type][uint32 reqId]. */
+    hdr[0] = (byte)(wireLen >> 24);
+    hdr[1] = (byte)(wireLen >> 16);
+    hdr[2] = (byte)(wireLen >> 8);
+    hdr[3] = (byte)(wireLen);
+    hdr[LENGTH_SZ] = WOLFSSH_FTP_NAME;
+    hdr[LENGTH_SZ + MSG_ID_SZ + 0] = 0;
+    hdr[LENGTH_SZ + MSG_ID_SZ + 1] = 0;
+    hdr[LENGTH_SZ + MSG_ID_SZ + 2] = 0;
+    hdr[LENGTH_SZ + MSG_ID_SZ + 3] = 0;
+
+    /* Leave reqId non-matching so an in-bound header exits at the request-id
+     * check without setting WS_BUFFER_E. */
+    ssh->reqId = 0xFFFFFFFF;
+    ssh->error = WS_SUCCESS;
+
+    if (wolfSSH_TestChannelPutData(ch, hdr, (word32)sizeof(hdr))
+            != WS_SUCCESS) {
+        result = -564;
+        goto done;
+    }
+
+    *outErr = wolfSSH_TestSftpDoName(ssh);
+
+done:
+    wolfSSH_free(ssh);
+    wolfSSH_CTX_free(ctx);
+    return result;
+}
+
+/* Verify wolfSSH_SFTP_DoName rejects a NAME message larger than
+ * WOLFSSH_MAX_SFTP_NAME and accepts one at the limit. SFTP_GetHeader
+ * returns the wire length minus the type and request-id fields, so the
+ * reported maxSz is wireLen - (UINT32_SZ + MSG_ID_SZ). */
+static int test_SftpDoName_sizeBound(void)
+{
+    word32 overhead = UINT32_SZ + MSG_ID_SZ;
+    int    err = 0;
+    int    result;
+
+    /* maxSz = WOLFSSH_MAX_SFTP_NAME + 1 -> over the bound, rejected. */
+    err = WS_SUCCESS;
+    result = sftpDoNameInjectErr(WOLFSSH_MAX_SFTP_NAME + overhead + 1, &err);
+    if (result != 0)
+        return result;
+    if (err != WS_BUFFER_E)
+        return -570;
+
+    /* maxSz = WOLFSSH_MAX_SFTP_NAME -> at the bound, not rejected (exits at
+     * the request-id check instead). */
+    err = WS_SUCCESS;
+    result = sftpDoNameInjectErr(WOLFSSH_MAX_SFTP_NAME + overhead, &err);
+    if (result != 0)
+        return result;
+    if (err == WS_BUFFER_E)
+        return -571;
+
+    /* A wire length above INT_MAX makes SFTP_GetHeader's int result wrap
+     * non-positive; this must be reported as a size error, not a silent
+     * NULL with WS_SUCCESS. */
+    err = WS_SUCCESS;
+    result = sftpDoNameInjectErr(0x80000000U + overhead, &err);
+    if (result != 0)
+        return result;
+    if (err != WS_BUFFER_E)
+        return -572;
+
+    return 0;
+}
+#endif /* WOLFSSH_SFTP && WOLFSSH_TEST_INTERNAL */
+
 int wolfSSH_UnitTest(int argc, char** argv)
 {
     int testResult = 0, unitResult = 0;
@@ -5057,6 +5161,12 @@ int wolfSSH_UnitTest(int argc, char** argv)
     unitResult = test_SendChannelData_eofTxd();
     printf("SendChannelData_eofTxd: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED"));
     testResult = testResult || unitResult;
+
+#ifdef WOLFSSH_SFTP
+    unitResult = test_SftpDoName_sizeBound();
+    printf("SftpDoName_sizeBound: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED"));
+    testResult = testResult || unitResult;
+#endif
 #if !defined(WOLFSSH_NO_RSA)
     unitResult = test_RsaVerify_BadDigest();
     printf("RsaVerify_BadDigest: %s\n",

wolfssh/wolfsftp.h

@@ -167,13 +167,24 @@ struct WS_SFTPNAME {
  *     expects the amount requested to be sent, and that's 32kiB.
  * WOLFSSH_MAX_SFTP_RECV: Used as a bounds check on a SFTP message's size.
  *     Is not used to allocate any buffers directly.
+ * WOLFSSH_MAX_SFTP_NAME: Upper bound on the size of a single SFTP NAME
+ *     response (READDIR, REALPATH, LS). The client allocates one buffer of
+ *     this size for the message and then one WS_SFTPNAME node per entry, so
+ *     an unbounded NAME packet lets a malicious or MITM server amplify a
+ *     modest packet into several times that much live heap. The default of
+ *     1 MiB leaves room for very large single-packet directory listings (the
+ *     server sends a whole directory in one NAME packet) while bounding peak
+ *     heap to a few MiB.
  */
 #ifndef WOLFSSH_MAX_SFTP_RW
     #define WOLFSSH_MAX_SFTP_RW 32768
 #endif
 #ifndef WOLFSSH_MAX_SFTP_RECV
     #define WOLFSSH_MAX_SFTP_RECV 32768
 #endif
+#ifndef WOLFSSH_MAX_SFTP_NAME
+    #define WOLFSSH_MAX_SFTP_NAME (1024 * 1024)
+#endif
 /*
  * WOLFSSH_MAX_SFTP_PACKET: Upper bound on the body size of an inbound SFTP
  *     request the server accepts in its steady-state receive loop
@@ -302,6 +313,7 @@ WOLFSSH_LOCAL void wolfSSH_SFTP_ShowSizes(void);
     WOLFSSH_API int wolfSSH_TestSftpBufferSend(WOLFSSH* ssh,
             byte* data, word32 sz, word32 idx);
     WOLFSSH_API int wolfSSH_TestSftpRecvSizeCheck(int sz);
+    WOLFSSH_API int wolfSSH_TestSftpDoName(WOLFSSH* ssh);
     #ifndef NO_WOLFSSH_SERVER
     WOLFSSH_API int wolfSSH_TestSftpValidateFileHandle(WOLFSSH* ssh,
             const byte* handle, word32 handleSz);