diff --git a/src/wolfsftp.c b/src/wolfsftp.c index 921341772..c8638c77e 100644 --- a/src/wolfsftp.c +++ b/src/wolfsftp.c @@ -6668,7 +6668,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; } @@ -6830,6 +6855,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 diff --git a/tests/unit.c b/tests/unit.c index 85092fd6c..4d7d77561 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -3918,6 +3918,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; @@ -3988,6 +4092,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", diff --git a/wolfssh/wolfsftp.h b/wolfssh/wolfsftp.h index 0fe3bc38a..198f5bcf6 100644 --- a/wolfssh/wolfsftp.h +++ b/wolfssh/wolfsftp.h @@ -167,6 +167,14 @@ 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 @@ -174,6 +182,9 @@ struct WS_SFTPNAME { #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 /* functions for establishing a connection */ WOLFSSH_API int wolfSSH_SFTP_accept(WOLFSSH* ssh); @@ -287,6 +298,7 @@ WOLFSSH_LOCAL void wolfSSH_SFTP_ShowSizes(void); #ifdef WOLFSSH_TEST_INTERNAL WOLFSSH_API int wolfSSH_TestSftpBufferSend(WOLFSSH* ssh, byte* data, word32 sz, word32 idx); + WOLFSSH_API int wolfSSH_TestSftpDoName(WOLFSSH* ssh); #ifndef NO_WOLFSSH_SERVER WOLFSSH_API int wolfSSH_TestSftpValidateFileHandle(WOLFSSH* ssh, const byte* handle, word32 handleSz);