From cdf0e51c863aff7cbbf5ab9bc1b9e021e731e859 Mon Sep 17 00:00:00 2001 From: Yosuke Shimizu Date: Fri, 12 Jun 2026 17:52:09 +0900 Subject: [PATCH] Bound server-side inbound SFTP request size in wolfSSH_SFTP_read --- src/wolfsftp.c | 48 +++++++++- tests/unit.c | 217 +++++++++++++++++++++++++++++++++++++++++++++ wolfssh/wolfsftp.h | 15 ++++ 3 files changed, 279 insertions(+), 1 deletion(-) diff --git a/src/wolfsftp.c b/src/wolfsftp.c index 921341772..99cfdcbb7 100644 --- a/src/wolfsftp.c +++ b/src/wolfsftp.c @@ -408,6 +408,25 @@ static int SFTP_ParseAttributes_buffer(WOLFSSH* ssh, WS_SFTP_FILEATRB* atr, static WS_SFTPNAME* wolfSSH_SFTPNAME_new(void* heap); +/* Returns WS_SUCCESS if a server-side inbound SFTP message body of the given + * size is acceptable, WS_BUFFER_E otherwise. The largest legitimate request a + * server receives is a WRITE carrying WOLFSSH_MAX_SFTP_RW bytes of file data + * plus the handle, offset, and length framing, so the body must be positive + * and no larger than WOLFSSH_MAX_SFTP_PACKET. Bounding this before allocation + * stops an authenticated client from declaring an arbitrarily large length. + * Only defined when it has a caller: the server receive loop or the test hook; + * a client-only build without WOLFSSH_TEST_INTERNAL leaves it unused. */ +#if !defined(NO_WOLFSSH_SERVER) || defined(WOLFSSH_TEST_INTERNAL) +static INLINE int SFTP_CheckRecvSz(int sz) +{ + if (sz <= 0 || sz > WOLFSSH_MAX_SFTP_PACKET) { + return WS_BUFFER_E; + } + return WS_SUCCESS; +} +#endif + + /* A few errors are OK to get. They are a notice rather that a fault. * return TRUE if ssh->error is one of the following: */ static INLINE int NoticeError(WOLFSSH* ssh) @@ -588,6 +607,16 @@ int wolfSSH_TestSftpBufferSend(WOLFSSH* ssh, buffer.idx = idx; return wolfSSH_SFTP_buffer_send(ssh, &buffer); } + + +/* Exposes the server-side received-packet size bound applied in + * wolfSSH_SFTP_read() so the unit tests can exercise the boundary between the + * largest legal inbound SFTP message and an over-large peer-declared length. + * Returns WS_SUCCESS when the size is accepted, WS_BUFFER_E otherwise. */ +int wolfSSH_TestSftpRecvSizeCheck(int sz) +{ + return SFTP_CheckRecvSz(sz); +} #endif @@ -1487,7 +1516,24 @@ int wolfSSH_SFTP_read(WOLFSSH* ssh) * packet expected to come. */ ret = SFTP_GetHeader(ssh, (word32*)&state->reqId, &state->type, &state->buffer); - if (ret <= 0) { + if (SFTP_CheckRecvSz(ret) != WS_SUCCESS) { + /* A positive ret is a genuine over-large declared length; log + * it as such. */ + if (ret > 0) { + WLOG(WS_LOG_SFTP, + "Received SFTP packet size out of bounds"); + } + /* If no lower layer left a reason (ssh->error == 0), the header + * parsed but the declared body length is invalid - over the + * cap, zero, or underflowed to negative - so record WS_BUFFER_E. + * This keeps the caller from seeing ret == WS_FATAL_ERROR with + * ssh->error == 0 and misclassifying it as a clean close via the + * channel-EOF fallback. Genuine header-read failures set + * ssh->error themselves (WS_WANT_READ to retry, WS_EOF on close, + * or a transport error) and are left untouched. */ + if (ssh->error == WS_SUCCESS) { + ssh->error = WS_BUFFER_E; + } return WS_FATAL_ERROR; } if (wolfSSH_SFTP_buffer_create(ssh, &state->buffer, ret) != diff --git a/tests/unit.c b/tests/unit.c index 85092fd6c..a9ad56d69 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -63,6 +63,10 @@ #include #endif +#ifdef WOLFSSH_SFTP +#include +#endif + #if defined(WOLFSSH_SCP) && !defined(WOLFSSH_SCP_USER_CALLBACKS) && \ !defined(NO_FILESYSTEM) && !defined(WOLFSSL_NUCLEUS) && \ !defined(_WIN32) && !defined(WOLFSSH_ZEPHYR) @@ -3863,6 +3867,203 @@ static int test_ScpRecvCallback_NewDirChdirFail(void) #endif /* WOLFSSH_SCP recv callback depth guard test */ +#ifdef WOLFSSH_SFTP +/* Property test for the server-side received-packet size bound applied in + * wolfSSH_SFTP_read(). A non-positive size or one above the largest legal + * inbound SFTP message must be rejected before any buffer is allocated; + * in-range sizes are accepted. */ +static int test_SftpRecvSizeBound(void) +{ + int testVals[7]; + int expectOk; + int ret; + int i; + int maxWriteBody; + + testVals[0] = -1; /* underflow / error sentinel */ + testVals[1] = 0; /* empty body */ + testVals[2] = 1; /* smallest accepted body */ + testVals[3] = WOLFSSH_MAX_SFTP_PACKET; /* upper bound, accepted */ + testVals[4] = WOLFSSH_MAX_SFTP_PACKET + 1; /* just over the bound */ + testVals[5] = 0x40000000; /* the ~1 GB attack value */ + testVals[6] = 0x7FFFFFFF; /* INT32_MAX */ + + for (i = 0; i < (int)(sizeof(testVals) / sizeof(testVals[0])); i++) { + ret = wolfSSH_TestSftpRecvSizeCheck(testVals[i]); + + expectOk = (testVals[i] > 0 && + testVals[i] <= WOLFSSH_MAX_SFTP_PACKET); + + if (expectOk) { + if (ret != WS_SUCCESS) + return -900 - i; + } + else { + if (ret == WS_SUCCESS) + return -920 - i; + } + } + + /* The bound value must be large enough to admit a real maximum-size WRITE, + * otherwise legitimate large writes would be silently rejected. A WRITE + * body is a handle string (length prefix + up to WOLFSSH_MAX_HANDLE), an + * 8-byte file offset, then a data string (length prefix + up to + * WOLFSSH_MAX_SFTP_RW). Guards against a future shrink of the bound (e.g. + * to WOLFSSH_MAX_SFTP_RECV alone). */ + maxWriteBody = UINT32_SZ + WOLFSSH_MAX_HANDLE /* handle string */ + + (2 * UINT32_SZ) /* 64-bit offset */ + + UINT32_SZ + WOLFSSH_MAX_SFTP_RW; /* data string */ + if (wolfSSH_TestSftpRecvSizeCheck(maxWriteBody) != WS_SUCCESS) + return -930; + + return 0; +} + +/* IORecv mock that reports no data is available yet. ReceiveData maps this to + * WS_WANT_READ, letting the receive loop reach its body-read state without a + * live socket. */ +static int RecvAlwaysWantRead(WOLFSSH* ssh, void* data, word32 sz, void* ctx) +{ + WOLFSSH_UNUSED(ssh); + WOLFSSH_UNUSED(data); + WOLFSSH_UNUSED(sz); + WOLFSSH_UNUSED(ctx); + return WS_CBIO_ERR_WANT_READ; +} + +/* Drives a single crafted SFTP request header through wolfSSH_SFTP_read() on a + * fresh server session. The on-wire length field is set to len and the message + * type to WRITE; only the 9-byte header is staged in the channel input buffer, + * never a body. ioRecv, when non-NULL, stands in for the socket if the loop is + * admitted and reaches its body-read state. On success fills *outRet with the + * wolfSSH_SFTP_read() return and *outErr with ssh->error, and returns 0; on a + * setup failure returns a negative sentinel. DiscardIoSend absorbs the + * window-adjust emitted after the header is consumed, and acceptState is + * advanced so that adjust is allowed on the bare session. */ +static int SftpRecvDriveHeader(word32 len, WS_CallbackIORecv ioRecv, + int* outRet, int* outErr) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + byte header[WOLFSSH_SFTP_HEADER]; + + *outRet = WS_SUCCESS; + *outErr = WS_SUCCESS; + + header[0] = (byte)((len >> 24) & 0xFF); + header[1] = (byte)((len >> 16) & 0xFF); + header[2] = (byte)((len >> 8) & 0xFF); + header[3] = (byte)( len & 0xFF); + header[LENGTH_SZ] = WOLFSSH_FTP_WRITE; /* message type */ + header[LENGTH_SZ + MSG_ID_SZ + 0] = 0x00; /* request id = 1 */ + header[LENGTH_SZ + MSG_ID_SZ + 1] = 0x00; + header[LENGTH_SZ + MSG_ID_SZ + 2] = 0x00; + header[LENGTH_SZ + MSG_ID_SZ + 3] = 0x01; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -1000; + wolfSSH_SetIOSend(ctx, DiscardIoSend); + if (ioRecv != NULL) + wolfSSH_SetIORecv(ctx, ioRecv); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1001; goto done; } + /* Allow MSGID_CHANNEL_WINDOW_ADJUST on this bare session. */ + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch == NULL) { result = -1002; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1003; + goto done; + } + + if (wolfSSH_TestChannelPutData(ssh->channelList, header, + (word32)sizeof(header)) != WS_SUCCESS) { + result = -1004; + goto done; + } + + *outRet = wolfSSH_SFTP_read(ssh); + *outErr = wolfSSH_get_error(ssh); + +done: + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} + +/* End-to-end check that the receive loop itself rejects an invalid declared + * inbound length, not just the SFTP_CheckRecvSz helper in isolation. Drives two + * crafted headers whose decoded body length the bound must refuse: one well + * past WOLFSSH_MAX_SFTP_PACKET and one of zero. Each must return WS_FATAL_ERROR + * with ssh->error == WS_BUFFER_E and allocate no body buffer. The zero case + * also guards against ssh->error being left at 0, which a caller could misread + * as a clean channel close. */ +static int test_SftpRecvSizeBoundIntegration(void) +{ + word32 lens[2]; + int rc; + int ret; + int err; + int i; + + /* On-wire length field counts the type byte, request id, and body. */ + lens[0] = (word32)WOLFSSH_MAX_SFTP_PACKET + 100 + MSG_ID_SZ + UINT32_SZ; + lens[1] = (word32)(MSG_ID_SZ + UINT32_SZ); /* decoded body length 0 */ + + for (i = 0; i < (int)(sizeof(lens) / sizeof(lens[0])); i++) { + rc = SftpRecvDriveHeader(lens[i], NULL, &ret, &err); + if (rc != 0) + return rc; + if (ret != WS_FATAL_ERROR) + return -945 - (i * 10); + if (err != WS_BUFFER_E) + return -946 - (i * 10); + } + + return 0; +} + +/* End-to-end check that a legitimate maximum-size WRITE is NOT rejected by the + * server bound. The decoded body length equals the largest real WRITE body + * (handle + 8-byte offset + max data). The bound must admit it: the loop + * allocates the body buffer and then asks for the body that has not arrived, so + * the call returns WS_FATAL_ERROR with ssh->error == WS_WANT_READ (a benign + * retry) rather than WS_BUFFER_E (a bound rejection). RecvAlwaysWantRead stands + * in for a non-blocking socket with no data yet. */ +static int test_SftpRecvSizeBoundAccept(void) +{ + word32 len; + int rc; + int ret; + int err; + + /* Largest legitimate WRITE body, matching test_SftpRecvSizeBound, plus the + * type byte and request id the on-wire length field carries. */ + len = (word32)(UINT32_SZ + WOLFSSH_MAX_HANDLE + + (2 * UINT32_SZ) + + UINT32_SZ + WOLFSSH_MAX_SFTP_RW) + + MSG_ID_SZ + UINT32_SZ; + + rc = SftpRecvDriveHeader(len, RecvAlwaysWantRead, &ret, &err); + if (rc != 0) + return rc; + if (ret != WS_FATAL_ERROR) + return -955; + /* WS_WANT_READ confirms the bound admitted the body and the loop is waiting + * on it; WS_BUFFER_E here would mean a legitimate max WRITE was rejected. */ + if (err != WS_WANT_READ) + return -956; + + return 0; +} +#endif /* WOLFSSH_SFTP */ + #endif /* WOLFSSH_TEST_INTERNAL */ /* Error Code And Message Test */ @@ -4019,6 +4220,22 @@ int wolfSSH_UnitTest(int argc, char** argv) printf("IdentifyAsn1Key: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; +#ifdef WOLFSSH_SFTP + unitResult = test_SftpRecvSizeBound(); + printf("SftpRecvSizeBound: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + + unitResult = test_SftpRecvSizeBoundIntegration(); + printf("SftpRecvSizeBoundIntegration: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + + unitResult = test_SftpRecvSizeBoundAccept(); + printf("SftpRecvSizeBoundAccept: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif + #if defined(WOLFSSH_SCP) && !defined(WOLFSSH_SCP_USER_CALLBACKS) && \ !defined(NO_FILESYSTEM) && !defined(WOLFSSL_NUCLEUS) && \ !defined(_WIN32) && !defined(WOLFSSH_ZEPHYR) diff --git a/wolfssh/wolfsftp.h b/wolfssh/wolfsftp.h index 0fe3bc38a..e7ac41495 100644 --- a/wolfssh/wolfsftp.h +++ b/wolfssh/wolfsftp.h @@ -174,6 +174,20 @@ struct WS_SFTPNAME { #ifndef WOLFSSH_MAX_SFTP_RECV #define WOLFSSH_MAX_SFTP_RECV 32768 #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 + * (wolfSSH_SFTP_read). The largest legitimate request is a WRITE carrying + * WOLFSSH_MAX_SFTP_RW bytes of file data plus the handle, offset, and + * length framing, so the bound is WOLFSSH_MAX_SFTP_RW plus the message + * overhead allowance of WOLFSSH_MAX_SFTP_RECV. Used to reject an over-large + * client-declared length before any buffer is allocated. This bound does + * not apply to client-side response handling (e.g. NAME directory listings + * or READ data), whose sizes are not derived from WOLFSSH_MAX_SFTP_RW. + */ +#ifndef WOLFSSH_MAX_SFTP_PACKET + #define WOLFSSH_MAX_SFTP_PACKET (WOLFSSH_MAX_SFTP_RW + WOLFSSH_MAX_SFTP_RECV) +#endif /* functions for establishing a connection */ WOLFSSH_API int wolfSSH_SFTP_accept(WOLFSSH* ssh); @@ -287,6 +301,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_TestSftpRecvSizeCheck(int sz); #ifndef NO_WOLFSSH_SERVER WOLFSSH_API int wolfSSH_TestSftpValidateFileHandle(WOLFSSH* ssh, const byte* handle, word32 handleSz);