WolfSSL
/
wolfssh
mirror of https://github.com/wolfSSL/wolfssh.git
1
0
Fork 0
Code Issues Releases Wiki Activity

sftp: bound reply lengths before client allocs 

SFTP_GetHeader now takes a limit from its caller and refuses a declared
body over it, or one too short to hold the header, with WS_BUFFER_E.
Every client reply handler allocated a buffer of the peer-declared size
first, so one 9-byte header could make the client allocate 2 GiB.

- STATUS, ATTRS and HANDLE replies are held to WOLFSSH_MAX_SFTP_PACKET
- NAME keeps WOLFSSH_MAX_SFTP_NAME; DoName's own check moves into
  the call
- a READ reply may also be as large as the caller's buffer plus its
  length prefix
- SendWritePacket keeps an error already set instead of
  WS_SFTP_BAD_HEADER
- regress test drives all eleven client calls at and past the bound

Issue: F-10598, F-13430, F-14433
pull/1340/merge
John Safranek 2026-10-20 14:51:19 -07:00 committed by philljj
parent dce29c4369
commit eb3d3cedb8
4 changed files with 255 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

97
src/wolfsftp.c
View File

@ -1052,10 +1052,12 @@ int wolfSSH_SFTP_PendingSend(WOLFSSH* ssh)
/* Gets packet header information
* request Id, type, and size of type specific data
* return value is length of type specific data still on the wire to be read
* maxSz bounds the type specific data, not the packet length field
* return value is length of type specific data still on the wire to be read,
* or WS_BUFFER_E if the peer declared more than maxSz or more than an int holds
*/
static int SFTP_GetHeader(WOLFSSH* ssh, word32* reqId, byte* type,
WS_SFTP_BUFFER* buffer)
WS_SFTP_BUFFER* buffer, word32 maxSz)
{
int ret;
word32 len;
@ -1081,9 +1083,23 @@ static int SFTP_GetHeader(WOLFSSH* ssh, word32* reqId, byte* type,
ato32(buffer->data + UINT32_SZ + MSG_ID_SZ, reqId);
wolfSSH_SFTP_buffer_free(ssh, buffer);
/* The caller allocates the body at this size, so bound it first. */
if (len < UINT32_SZ + MSG_ID_SZ) {
WLOG(WS_LOG_SFTP, "SFTP packet length %u too short", len);
ssh->error = WS_BUFFER_E;
return WS_BUFFER_E;
}
len -= UINT32_SZ + MSG_ID_SZ;
if (len > maxSz || len > (word32)0x7FFFFFFF) {
WLOG(WS_LOG_SFTP, "SFTP packet length %u over limit %u", len, maxSz);
ssh->error = WS_BUFFER_E;
return WS_BUFFER_E;
}
WLOG(WS_LOG_SFTP, "Leaving SFTP_GetHeader(), packet length = %d id = %d"
" type = %d", len - UINT32_SZ - MSG_ID_SZ, *reqId, *type);
return len - UINT32_SZ - MSG_ID_SZ;
" type = %d", len, *reqId, *type);
return (int)len;
}
@ -1709,17 +1725,11 @@ int wolfSSH_SFTP_read(WOLFSSH* ssh)
/* Wait for packet to come in then handle it, maxSz is the size of
* packet expected to come. */
ret = SFTP_GetHeader(ssh, (word32*)&state->reqId,
&state->type, &state->buffer);
&state->type, &state->buffer, WOLFSSH_MAX_SFTP_PACKET);
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.
* parsed but the declared body length is zero, 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
@ -7335,11 +7345,15 @@ static WS_SFTPNAME* wolfSSH_SFTP_DoName(WOLFSSH* ssh)
switch (state->state) {
case SFTP_NAME_GETHEADER_PACKET:
maxSz = SFTP_GetHeader(ssh, &reqId, &type, &state->buffer);
/* Bound the NAME size: the buffer is allocated at this size and a
* WS_SFTPNAME node is made per entry, so an over-large packet
* would amplify into several times that much live heap. */
maxSz = SFTP_GetHeader(ssh, &reqId, &type, &state->buffer,
WOLFSSH_MAX_SFTP_NAME);
if (maxSz <= 0) {
/* Non-positive size: a retryable notice keeps NAME state for
* resume; otherwise (including an int wrap above INT_MAX)
* record a size error if unset and clear state. */
* resume; otherwise record a size error if unset and clear
* state. */
if (!NoticeError(ssh)) {
if (ssh->error == WS_SUCCESS) {
ssh->error = WS_BUFFER_E;
@ -7349,18 +7363,6 @@ static WS_SFTPNAME* wolfSSH_SFTP_DoName(WOLFSSH* ssh)
return NULL;
}
/* Bound the NAME size: the buffer is allocated at this size and a
* WS_SFTPNAME node is made per entry, so an over-large packet
* 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;
}
if (reqId != ssh->reqId) {
WLOG(WS_LOG_SFTP, "unexpected ID");
return NULL;
@ -7585,7 +7587,8 @@ static int wolfSSH_SFTP_GetHandle(WOLFSSH* ssh, byte* handle, word32* handleSz)
case STATE_GET_HANDLE_GET_HEADER:
WLOG(WS_LOG_SFTP, "SFTP GET HANDLE STATE: GET_HEADER");
ret = SFTP_GetHeader(ssh, &handleState->reqId, &type, &handleState->buffer);
ret = SFTP_GetHeader(ssh, &handleState->reqId, &type,
&handleState->buffer, WOLFSSH_MAX_SFTP_PACKET);
if (ret <= 0) {
if (NoticeError(ssh)) {
return WS_FATAL_ERROR;
@ -7987,7 +7990,7 @@ static int SFTP_STAT(WOLFSSH* ssh, char* dir, WS_SFTP_FILEATRB* atr, byte type)
WLOG(WS_LOG_SFTP, "SFTP LSTAT STATE: GET_HEADER");
/* get attributes response */
ret = SFTP_GetHeader(ssh, &state->reqId, &state->type,
&state->buffer);
&state->buffer, WOLFSSH_MAX_SFTP_PACKET);
if (ret <= 0) {
if (NoticeError(ssh)) {
return WS_FATAL_ERROR;
@ -8212,7 +8215,8 @@ int wolfSSH_SFTP_SetSTAT(WOLFSSH* ssh, char* dir, WS_SFTP_FILEATRB* atr)
case STATE_SET_ATR_GET:
maxSz = SFTP_GetHeader(ssh, &state->reqId, &type, &state->buffer);
maxSz = SFTP_GetHeader(ssh, &state->reqId, &type, &state->buffer,
WOLFSSH_MAX_SFTP_PACKET);
if (maxSz <= 0) {
if (!NoticeError(ssh)) {
ret = WS_FATAL_ERROR;
@ -8556,12 +8560,14 @@ int wolfSSH_SFTP_SendWritePacket(WOLFSSH* ssh, byte* handle, word32 handleSz,
WLOG(WS_LOG_SFTP, "SFTP SEND_WRITE STATE: GET_HEADER");
/* Get response */
state->maxSz = SFTP_GetHeader(ssh, &state->reqId, &type,
&state->buffer);
&state->buffer, WOLFSSH_MAX_SFTP_PACKET);
if (state->maxSz <= 0) {
if (NoticeError(ssh)) {
return WS_FATAL_ERROR;
}
ssh->error = WS_SFTP_BAD_HEADER;
if (ssh->error == WS_SUCCESS) {
ssh->error = WS_SFTP_BAD_HEADER;
}
ret = WS_FATAL_ERROR;
state->state = STATE_SEND_WRITE_CLEANUP;
continue;
@ -8665,6 +8671,7 @@ int wolfSSH_SFTP_SendReadPacket(WOLFSSH* ssh, byte* handle, word32 handleSz,
WS_SFTP_SEND_READ_STATE* state = NULL;
int ret = WS_SUCCESS;
word32 sz;
word32 maxSz;
WLOG(WS_LOG_SFTP, "Entering wolfSSH_SFTP_SendReadPacket()");
if (ssh == NULL || handle == NULL || out == NULL)
@ -8754,9 +8761,18 @@ int wolfSSH_SFTP_SendReadPacket(WOLFSSH* ssh, byte* handle, word32 handleSz,
case STATE_SEND_READ_GET_HEADER:
WLOG(WS_LOG_SFTP, "SFTP SEND_READ STATE: GET_HEADER");
/* A DATA reply holds up to outSz bytes as a string, which can
* need more room than WOLFSSH_MAX_SFTP_PACKET. The type is not
* known yet, so the bound covers any reply. */
maxSz = WOLFSSH_MAX_SFTP_PACKET;
if (outSz > maxSz - UINT32_SZ) {
maxSz = outSz + UINT32_SZ;
if (maxSz < outSz)
maxSz = outSz;
}
/* Get response */
if ((ret = SFTP_GetHeader(ssh, &state->reqId, &state->type,
&state->buffer)) <= 0) {
&state->buffer, maxSz)) <= 0) {
if (NoticeError(ssh)) {
return WS_FATAL_ERROR;
}
@ -9021,7 +9037,7 @@ int wolfSSH_SFTP_MKDIR(WOLFSSH* ssh, char* dir, WS_SFTP_FILEATRB* atr)
case STATE_MKDIR_GET:
/* Get response */
if ((ret = SFTP_GetHeader(ssh, &state->reqId, &type,
&state->buffer)) <= 0) {
&state->buffer, WOLFSSH_MAX_SFTP_PACKET)) <= 0) {
if (!NoticeError(ssh)) {
wolfSSH_SFTP_ClearState(ssh, STATE_ID_MKDIR);
}
@ -9190,7 +9206,8 @@ int wolfSSH_SFTP_Close(WOLFSSH* ssh, byte* handle, word32 handleSz)
case STATE_CLOSE_GET_HEADER:
WLOG(WS_LOG_SFTP, "SFTP CLOSE STATE: GET_HEADER");
ret = SFTP_GetHeader(ssh, &state->reqId, &type, &state->buffer);
ret = SFTP_GetHeader(ssh, &state->reqId, &type, &state->buffer,
WOLFSSH_MAX_SFTP_PACKET);
if (ret <= 0 && NoticeError(ssh)) {
return WS_FATAL_ERROR;
}
@ -9580,7 +9597,7 @@ int wolfSSH_SFTP_Rename(WOLFSSH* ssh, const char* old, const char* nw)
WLOG(WS_LOG_SFTP, "SFTP RENAME STATE: GET_HEADER");
/* Get response */
ret = SFTP_GetHeader(ssh, &state->reqId,
&type, &state->buffer);
&type, &state->buffer, WOLFSSH_MAX_SFTP_PACKET);
if (ret <= 0) {
if (ssh->error == WS_WANT_READ ||
ssh->error == WS_WANT_WRITE) {
@ -9730,7 +9747,8 @@ int wolfSSH_SFTP_Remove(WOLFSSH* ssh, char* f)
FALL_THROUGH;
case STATE_RM_GET:
ret = SFTP_GetHeader(ssh, &state->reqId, &type, &state->buffer);
ret = SFTP_GetHeader(ssh, &state->reqId, &type, &state->buffer,
WOLFSSH_MAX_SFTP_PACKET);
if (ret <= 0 || type != WOLFSSH_FTP_STATUS) {
if (!NoticeError(ssh)) {
WLOG(WS_LOG_SFTP, "Unexpected packet type");
@ -9820,7 +9838,8 @@ int wolfSSH_SFTP_RMDIR(WOLFSSH* ssh, char* dir)
FALL_THROUGH;
case STATE_RMDIR_GET:
ret = SFTP_GetHeader(ssh, &state->reqId, &type, &state->buffer);
ret = SFTP_GetHeader(ssh, &state->reqId, &type, &state->buffer,
WOLFSSH_MAX_SFTP_PACKET);
if (ret <= 0 || type != WOLFSSH_FTP_STATUS) {
if (!NoticeError(ssh)) {
wolfSSH_SFTP_ClearState(ssh, STATE_ID_RMDIR);

190
tests/regress.c
View File

@ -15423,6 +15423,195 @@ static void TestOct2DecRejectsInvalidNonLeadingDigit(void)
wolfSSH_free(ssh);
wolfSSH_CTX_free(ctx);
}
/* SSH_MSG_CHANNEL_DATA carrying the start of an SFTP reply: the header with
* the given length field, then bodySz bytes of body. */
static word32 BuildSftpReplyPacket(word32 recipientChannelId, word32 wireLen,
byte type, word32 reqId, const byte* body, word32 bodySz,
byte* out, word32 outSz)
{
byte payload[64];
word32 idx = 0;
idx = AppendUint32(payload, sizeof(payload), idx, recipientChannelId);
idx = AppendUint32(payload, sizeof(payload), idx,
WOLFSSH_SFTP_HEADER + bodySz);
idx = AppendUint32(payload, sizeof(payload), idx, wireLen);
idx = AppendByte(payload, sizeof(payload), idx, type);
idx = AppendUint32(payload, sizeof(payload), idx, reqId);
idx = AppendData(payload, sizeof(payload), idx, body, bodySz);
return WrapPacket(MSGID_CHANNEL_DATA, payload, idx, out, outSz);
}
enum {
SFTP_OP_OPEN,
SFTP_OP_CLOSE,
SFTP_OP_READ,
SFTP_OP_WRITE,
SFTP_OP_STAT,
SFTP_OP_LSTAT,
SFTP_OP_SETSTAT,
SFTP_OP_MKDIR,
SFTP_OP_RMDIR,
SFTP_OP_RENAME,
SFTP_OP_REMOVE,
SFTP_OP_COUNT
};
/* Run one client request against a reply of the given type whose header
* carries wireLen in its length field and no body. Remove asks for an LSTAT
* first, so it gets a well-formed ATTRS reply ahead of the one under test.
* readSz is the out buffer size given to the READ. Returns the call's result;
* the session error goes to outErr. */
static int SftpClientReplyLen(int op, byte replyType, word32 wireLen,
word32 readSz, int* outErr)
{
static const byte noAttrs[] = { 0x00, 0x00, 0x00, 0x00 };
WOLFSSH_CTX* ctx;
WOLFSSH* ssh;
MemIo io;
WS_SFTP_FILEATRB atrb;
byte in[256];
byte out[1024];
byte handle[4] = { 0, 0, 0, 1 };
word32 handleSz = sizeof(handle);
word32 ofst[2] = { 0, 0 };
word32 chId;
word32 reqId;
word32 inSz = 0;
byte* data;
char path[] = "/x";
int ret = WS_FATAL_ERROR;
ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL);
AssertNotNull(ctx);
wolfSSH_SetIORecv(ctx, MemRecv);
wolfSSH_SetIOSend(ctx, MemSend);
ssh = wolfSSH_new(ctx);
AssertNotNull(ssh);
ssh->connectState = CONNECT_DONE;
ssh->sftpState = SFTP_DONE;
AddSessionChannel(ssh);
chId = ssh->channelList->channel;
reqId = ssh->reqId;
if (op == SFTP_OP_REMOVE) {
inSz = BuildSftpReplyPacket(chId,
MSG_ID_SZ + UINT32_SZ + (word32)sizeof(noAttrs),
WOLFSSH_FTP_ATTRS, reqId++, noAttrs, (word32)sizeof(noAttrs),
in, sizeof(in));
}
inSz += BuildSftpReplyPacket(chId, wireLen, replyType, reqId,
NULL, 0, in + inSz, sizeof(in) - inSz);
MemIoInit(&io, in, inSz, out, sizeof(out));
wolfSSH_SetIOReadCtx(ssh, &io);
wolfSSH_SetIOWriteCtx(ssh, &io);
data = (byte*)WMALLOC(readSz, NULL, DYNTYPE_BUFFER);
AssertNotNull(data);
WMEMSET(data, 0, readSz);
WMEMSET(&atrb, 0, sizeof(atrb));
switch (op) {
case SFTP_OP_OPEN:
ret = wolfSSH_SFTP_Open(ssh, path, WOLFSSH_FXF_READ, &atrb,
handle, &handleSz);
break;
case SFTP_OP_CLOSE:
ret = wolfSSH_SFTP_Close(ssh, handle, handleSz);
break;
case SFTP_OP_READ:
ret = wolfSSH_SFTP_SendReadPacket(ssh, handle, handleSz, ofst,
data, readSz);
break;
case SFTP_OP_WRITE:
ret = wolfSSH_SFTP_SendWritePacket(ssh, handle, handleSz, ofst,
data, 16);
break;
case SFTP_OP_STAT:
ret = wolfSSH_SFTP_STAT(ssh, path, &atrb);
break;
case SFTP_OP_LSTAT:
ret = wolfSSH_SFTP_LSTAT(ssh, path, &atrb);
break;
case SFTP_OP_SETSTAT:
ret = wolfSSH_SFTP_SetSTAT(ssh, path, &atrb);
break;
case SFTP_OP_MKDIR:
ret = wolfSSH_SFTP_MKDIR(ssh, path, &atrb);
break;
case SFTP_OP_RMDIR:
ret = wolfSSH_SFTP_RMDIR(ssh, path);
break;
case SFTP_OP_RENAME:
ret = wolfSSH_SFTP_Rename(ssh, path, "/y");
break;
case SFTP_OP_REMOVE:
ret = wolfSSH_SFTP_Remove(ssh, path);
break;
default:
AssertTrue(op < SFTP_OP_COUNT);
}
*outErr = wolfSSH_get_error(ssh);
/* every reply byte reached the SFTP layer */
AssertIntEQ(io.inOff, io.inSz);
WFREE(data, NULL, DYNTYPE_BUFFER);
wolfSSH_free(ssh);
wolfSSH_CTX_free(ctx);
return ret;
}
/* fenrir 10598, 13430, 14433: a client reply's declared length is bounded
* before the client allocates a buffer of that size. A reply at the bound is
* taken and the call waits for its body; one past the bound fails at once. */
static void TestSftpClientReplyLenBound(void)
{
const word32 hdrSz = MSG_ID_SZ + UINT32_SZ;
const word32 bigRead = 2 * WOLFSSH_MAX_SFTP_PACKET;
int op;
int ret;
int err;
for (op = 0; op < SFTP_OP_COUNT; op++) {
ret = SftpClientReplyLen(op, WOLFSSH_FTP_STATUS,
hdrSz + WOLFSSH_MAX_SFTP_PACKET, 16, &err);
AssertTrue(ret < 0);
AssertIntEQ(err, WS_WANT_READ);
ret = SftpClientReplyLen(op, WOLFSSH_FTP_STATUS,
hdrSz + WOLFSSH_MAX_SFTP_PACKET + 1, 16, &err);
AssertTrue(ret < 0);
AssertIntEQ(err, WS_BUFFER_E);
/* a length field the header itself does not fit in */
ret = SftpClientReplyLen(op, WOLFSSH_FTP_STATUS, hdrSz - 1, 16, &err);
AssertTrue(ret < 0);
AssertIntEQ(err, WS_BUFFER_E);
ret = SftpClientReplyLen(op, WOLFSSH_FTP_STATUS, 0xFFFFFFFF, 16, &err);
AssertTrue(ret < 0);
AssertIntEQ(err, WS_BUFFER_E);
}
/* A READ may ask for more than WOLFSSH_MAX_SFTP_PACKET; the DATA reply
* holding all of it, a string of readSz bytes, is still taken. */
ret = SftpClientReplyLen(SFTP_OP_READ, WOLFSSH_FTP_DATA,
hdrSz + UINT32_SZ + bigRead, bigRead, &err);
AssertTrue(ret < 0);
AssertIntEQ(err, WS_WANT_READ);
ret = SftpClientReplyLen(SFTP_OP_READ, WOLFSSH_FTP_DATA,
hdrSz + UINT32_SZ + bigRead + 1, bigRead, &err);
AssertTrue(ret < 0);
AssertIntEQ(err, WS_BUFFER_E);
}
#endif /* WOLFSSH_SFTP && !NO_WOLFSSH_CLIENT */
@ -22046,6 +22235,7 @@ int main(int argc, char** argv)
#ifdef WOLFSSH_SFTP
#ifndef NO_WOLFSSH_CLIENT
TestOct2DecRejectsInvalidNonLeadingDigit();
TestSftpClientReplyLenBound();
#endif
TestSftpBufferSendPendingOutput();
/* the attribute encoder and decoder agree, extensions included */

6
tests/unit.c
View File

@ -25069,9 +25069,9 @@ static int test_SftpDoName_sizeBound(void)
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. */
/* A wire length above INT_MAX is far past WOLFSSH_MAX_SFTP_NAME, so
* SFTP_GetHeader refuses it; 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)

7
wolfssh/wolfsftp.h
View File

@ -201,9 +201,10 @@ struct WS_SFTPNAME {
* 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.
* peer-declared length before any buffer is allocated. The client applies
* it to its replies too, except VERSION (WOLFSSH_MAX_SFTP_RECV), NAME
* (WOLFSSH_MAX_SFTP_NAME) and any reply to a READ, which may be as large
* as the caller's buffer plus the data string's length prefix.
*/
#ifndef WOLFSSH_MAX_SFTP_PACKET
#define WOLFSSH_MAX_SFTP_PACKET (WOLFSSH_MAX_SFTP_RW + WOLFSSH_MAX_SFTP_RECV)