diff --git a/src/wolfsftp.c b/src/wolfsftp.c index 57804eb8f..31408cd73 100644 --- a/src/wolfsftp.c +++ b/src/wolfsftp.c @@ -6437,73 +6437,89 @@ int wolfSSH_SFTP_TestInvalidateHeadFd(WOLFSSH* ssh) * returns WS_SUCCESS on success */ static int SFTP_ClientRecvInit(WOLFSSH* ssh) { - int len, ret; + enum { + RECV_INIT_SIZE = LENGTH_SZ + MSG_ID_SZ + UINT32_SZ + }; + + int ret; byte id; word32 sz = 0; word32 version = 0; - byte buf[LENGTH_SZ + MSG_ID_SZ + UINT32_SZ]; + WS_SFTP_RECV_INIT_STATE* state; + + /* The VERSION message can arrive split over several reads, so the bytes + * seen so far are kept here instead of on the stack. */ + state = ssh->recvInitState; + if (state == NULL) { + state = (WS_SFTP_RECV_INIT_STATE*)WMALLOC( + sizeof(WS_SFTP_RECV_INIT_STATE), + ssh->ctx->heap, DYNTYPE_SFTP_STATE); + if (state == NULL) { + ssh->error = WS_MEMORY_E; + return WS_FATAL_ERROR; + } + WMEMSET(state, 0, sizeof(WS_SFTP_RECV_INIT_STATE)); + ssh->recvInitState = state; + } switch (ssh->sftpState) { case SFTP_RECV: - ret = wolfSSH_worker(ssh,NULL); - if (ret != 0 && ret != WS_CHAN_RXD) { - return ret; + ret = wolfSSH_SFTP_buffer_read(ssh, + &state->buffer, RECV_INIT_SIZE); + if (ret < 0) { + return WS_FATAL_ERROR; } - if ((len = wolfSSH_stream_read(ssh, buf, sizeof(buf))) - != sizeof(buf)) { - /* @TODO partial read on small packet */ - return len; + if (ret < WOLFSSH_SFTP_HEADER) { + WLOG(WS_LOG_SFTP, "Unable to read SFTP VERSION message"); + return WS_FATAL_ERROR; } - if (SFTP_GetSz(buf, &sz, + if (SFTP_GetSz(state->buffer.data, &sz, MSG_ID_SZ + UINT32_SZ, WOLFSSH_MAX_SFTP_RECV) != WS_SUCCESS) { + wolfSSH_SFTP_ClearState(ssh, STATE_ID_ALL); return WS_BUFFER_E; } /* expecting */ - id = buf[LENGTH_SZ]; + id = state->buffer.data[LENGTH_SZ]; if (id != WOLFSSH_FTP_VERSION) { WLOG(WS_LOG_SFTP, "Unexpected SFTP type received"); + wolfSSH_SFTP_ClearState(ssh, STATE_ID_ALL); return WS_BUFFER_E; } - ato32(buf + LENGTH_SZ + MSG_ID_SZ, &version); - /* The server is supposed to reply with the lower of its own and - * our version, so a value above ours is a non-conforming server. In - * that case we continue on with the same v3 version. If the server - * replies with a version before v3 then return early here since - * there will be SSH_FXP_STATUS incompatibility issues. */ + ato32(state->buffer.data + LENGTH_SZ + MSG_ID_SZ, &version); + /* A version above ours is non-conforming; continue with v3. A + * version below v3 has SSH_FXP_STATUS incompatibilities, bail. */ if (version < WOLFSSH_SFTP_VERSION) { WLOG(WS_LOG_SFTP, "Unsupported SFTP version from server"); + wolfSSH_SFTP_ClearState(ssh, STATE_ID_ALL); return WS_VERSION_E; } - sz = sz - MSG_ID_SZ - UINT32_SZ; - ssh->sftpExtSz = sz; + wolfSSH_SFTP_buffer_free(ssh, &state->buffer); + + state->extSz = sz - MSG_ID_SZ - UINT32_SZ; ssh->sftpState = SFTP_EXT; FALL_THROUGH; case SFTP_EXT: /* silently ignore extensions if not supported */ - if (ssh->sftpExtSz > 0) { - byte* data = (byte*)WMALLOC(ssh->sftpExtSz, ssh->ctx->heap, - DYNTYPE_BUFFER); - if (data == NULL) return WS_MEMORY_E; - if ((len = wolfSSH_stream_read(ssh, data, ssh->sftpExtSz)) - <= 0) { - WFREE(data, ssh->ctx->heap, DYNTYPE_BUFFER); - return len; + if (state->extSz > 0) { + ret = wolfSSH_SFTP_buffer_read(ssh, + &state->buffer, (int)state->extSz); + if (ret < 0) { + return WS_FATAL_ERROR; } - WFREE(data, ssh->ctx->heap, DYNTYPE_BUFFER); - /* case where expecting more */ - if ((word32)len < ssh->sftpExtSz) { - ssh->sftpExtSz -= len; - ssh->error = WS_WANT_READ; + if (ret < (int)state->extSz) { + WLOG(WS_LOG_SFTP, "Unable to read SFTP VERSION extensions"); return WS_FATAL_ERROR; } + + wolfSSH_SFTP_buffer_free(ssh, &state->buffer); } break; @@ -6579,11 +6595,18 @@ int wolfSSH_SFTP_connect(WOLFSSH* ssh) case SFTP_RECV: case SFTP_EXT: - if (SFTP_ClientRecvInit(ssh) != WS_SUCCESS) { + ret = SFTP_ClientRecvInit(ssh); + if (ret != WS_SUCCESS) { + /* keep the buffered partial VERSION message when the read can + * be retried, free it otherwise */ + if (!NoticeError(ssh)) { + wolfSSH_SFTP_ClearState(ssh, STATE_ID_ALL); + } return WS_FATAL_ERROR; } ssh->sftpState = SFTP_DONE; WLOG(WS_LOG_SFTP, "SFTP connection established"); + wolfSSH_SFTP_ClearState(ssh, STATE_ID_RECV_INIT); break; default: diff --git a/tests/unit.c b/tests/unit.c index a043ac080..f4693ed57 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -12158,6 +12158,264 @@ static int test_SftpRecvSizeBoundAccept(void) return 0; } + +#ifndef NO_WOLFSSH_CLIENT +/* Builds an SFTP VERSION message in the outSz byte buffer "out", with the + * declared length "len", message type "type", "version", and extSz bytes of + * extension data. Returns the size written, or 0 if it does not fit. */ +static word32 SftpBuildVersion(byte* out, word32 outSz, word32 len, byte type, + word32 version, word32 extSz) +{ + word32 i; + + if (WOLFSSH_SFTP_HEADER + extSz > outSz) { + return 0; + } + + PutU32BE(out, len); + out[LENGTH_SZ] = type; + PutU32BE(out + LENGTH_SZ + MSG_ID_SZ, version); + for (i = 0; i < extSz; i++) { + out[WOLFSSH_SFTP_HEADER + i] = (byte)i; + } + + return WOLFSSH_SFTP_HEADER + extSz; +} + +/* Creates a client session sitting past channel setup and already waiting on + * the VERSION reply, so a connect call runs only the receive half of the + * negotiation. The caller frees ctx and ssh on every path. */ +static int SftpClientNewSession(WOLFSSH_CTX** ctxOut, WOLFSSH** sshOut) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + WOLFSSH_CHANNEL* ch; + + *ctxOut = NULL; + *sshOut = NULL; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + if (ctx == NULL) + return -1010; + wolfSSH_SetIOSend(ctx, DiscardIoSend); + wolfSSH_SetIORecv(ctx, RecvAlwaysWantRead); + *ctxOut = ctx; + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) + return -1011; + *sshOut = ssh; + + ssh->connectState = CONNECT_SERVER_CHANNEL_REQUEST_DONE; + ssh->sftpState = SFTP_RECV; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + if (ch == NULL) + return -1012; + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + return -1013; + } + + return 0; +} + +/* Drives wolfSSH_SFTP_connect() over a VERSION message delivered in two pieces, + * split after "split" bytes, with RecvAlwaysWantRead standing in for a + * non-blocking socket. Returns 0, or a negative sentinel on a setup failure. */ +static int SftpClientDriveVersion(word32 extSz, word32 split, + int* firstRet, int* firstErr, int* secondRet, int* endState) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + byte msg[WOLFSSH_SFTP_HEADER + 16]; + word32 msgSz; + int result; + + *firstRet = WS_SUCCESS; + *firstErr = WS_SUCCESS; + *secondRet = WS_SUCCESS; + *endState = 0; + + msgSz = SftpBuildVersion(msg, (word32)sizeof(msg), + MSG_ID_SZ + UINT32_SZ + extSz, WOLFSSH_FTP_VERSION, + (word32)WOLFSSH_SFTP_VERSION, extSz); + if (msgSz == 0) + return -1017; + + result = SftpClientNewSession(&ctx, &ssh); + if (result == 0) { + if (wolfSSH_TestChannelPutData(ssh->channelList, msg, split) + != WS_SUCCESS) { + result = -1014; + } + } + if (result == 0) { + *firstRet = wolfSSH_SFTP_connect(ssh); + *firstErr = wolfSSH_get_error(ssh); + + if (wolfSSH_TestChannelPutData(ssh->channelList, msg + split, + msgSz - split) != WS_SUCCESS) { + result = -1015; + } + } + if (result == 0) { + *secondRet = wolfSSH_SFTP_connect(ssh); + *endState = ssh->sftpState; + } + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} + +/* Delivers one complete VERSION message in a single read, reporting the connect + * result, whether the buffered init state was released, and the ending state. */ +static int SftpClientDriveOnce(word32 len, byte type, word32 version, + int* connectRet, int* stateFreed, int* endState) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + byte msg[WOLFSSH_SFTP_HEADER]; + word32 msgSz; + int result; + + *connectRet = WS_SUCCESS; + *stateFreed = 0; + *endState = 0; + + msgSz = SftpBuildVersion(msg, (word32)sizeof(msg), len, type, version, 0); + if (msgSz == 0) + return -1018; + + result = SftpClientNewSession(&ctx, &ssh); + if (result == 0) { + if (wolfSSH_TestChannelPutData(ssh->channelList, msg, msgSz) + != WS_SUCCESS) { + result = -1016; + } + } + if (result == 0) { + *connectRet = wolfSSH_SFTP_connect(ssh); + *stateFreed = (ssh->recvInitState == NULL); + *endState = ssh->sftpState; + } + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} + +/* Regression for the client SFTP negotiation losing bytes on a short read: a + * split VERSION message must report WS_WANT_READ, then complete on the retry. + * The splits cover the SFTP_RECV arm (header) and the SFTP_EXT arm. */ +static int test_SftpClientRecvInitSplit(void) +{ + int rc; + int firstRet; + int firstErr; + int secondRet; + int endState; + + /* split inside the header, no extensions */ + rc = SftpClientDriveVersion(0, LENGTH_SZ, + &firstRet, &firstErr, &secondRet, &endState); + if (rc != 0) + return rc; + if (firstRet != WS_FATAL_ERROR) + return -960; + if (firstErr != WS_WANT_READ) + return -961; + if (secondRet != WS_SUCCESS) + return -962; + if (endState != SFTP_DONE) + return -963; + + /* split inside the extension data following a complete header */ + rc = SftpClientDriveVersion(8, WOLFSSH_SFTP_HEADER + 3, + &firstRet, &firstErr, &secondRet, &endState); + if (rc != 0) + return rc; + if (firstRet != WS_FATAL_ERROR) + return -964; + if (firstErr != WS_WANT_READ) + return -965; + if (secondRet != WS_SUCCESS) + return -966; + if (endState != SFTP_DONE) + return -967; + + return 0; +} + +/* Covers the VERSION checks on a complete message: each rejection must fail the + * connect and release the buffered init state, while a version above ours must + * still negotiate, keeping the bound a "<" rather than a "<=". */ +static int test_SftpClientRecvInitVersion(void) +{ + int rc; + int connectRet; + int stateFreed; + int endState; + + /* declared size smaller than the type and version fields */ + rc = SftpClientDriveOnce(1, WOLFSSH_FTP_VERSION, + (word32)WOLFSSH_SFTP_VERSION, &connectRet, &stateFreed, &endState); + if (rc != 0) + return rc; + if (connectRet != WS_FATAL_ERROR) + return -970; + if (!stateFreed) + return -971; + + /* declared size past the largest inbound message accepted */ + rc = SftpClientDriveOnce((word32)WOLFSSH_MAX_SFTP_RECV + 1, + WOLFSSH_FTP_VERSION, (word32)WOLFSSH_SFTP_VERSION, + &connectRet, &stateFreed, &endState); + if (rc != 0) + return rc; + if (connectRet != WS_FATAL_ERROR) + return -979; + if (!stateFreed) + return -980; + + /* INIT echoed back where a VERSION reply is expected */ + rc = SftpClientDriveOnce(MSG_ID_SZ + UINT32_SZ, WOLFSSH_FTP_INIT, + (word32)WOLFSSH_SFTP_VERSION, &connectRet, &stateFreed, &endState); + if (rc != 0) + return rc; + if (connectRet != WS_FATAL_ERROR) + return -972; + if (!stateFreed) + return -973; + + /* server reporting a version wolfSSH cannot talk to */ + rc = SftpClientDriveOnce(MSG_ID_SZ + UINT32_SZ, WOLFSSH_FTP_VERSION, + (word32)WOLFSSH_SFTP_VERSION - 1, &connectRet, &stateFreed, + &endState); + if (rc != 0) + return rc; + if (connectRet != WS_FATAL_ERROR) + return -974; + if (!stateFreed) + return -975; + + /* server above our version: continue on with ours */ + rc = SftpClientDriveOnce(MSG_ID_SZ + UINT32_SZ, WOLFSSH_FTP_VERSION, + (word32)WOLFSSH_SFTP_VERSION + 1, &connectRet, &stateFreed, + &endState); + if (rc != 0) + return rc; + if (connectRet != WS_SUCCESS) + return -976; + if (!stateFreed) + return -977; + if (endState != SFTP_DONE) + return -978; + + return 0; +} +#endif /* NO_WOLFSSH_CLIENT */ #endif /* WOLFSSH_SFTP */ #if defined(WOLFSSH_TEST_INTERNAL) && defined(WOLFSSH_SCP) && \ @@ -12984,6 +13242,18 @@ int wolfSSH_UnitTest(int argc, char** argv) printf("SftpRecvSizeBoundAccept: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; + +#ifndef NO_WOLFSSH_CLIENT + unitResult = test_SftpClientRecvInitSplit(); + printf("SftpClientRecvInitSplit: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + + unitResult = test_SftpClientRecvInitVersion(); + printf("SftpClientRecvInitVersion: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif #endif #if defined(WOLFSSH_SCP) && !defined(WOLFSSH_SCP_USER_CALLBACKS) && \ diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 1836c816b..2967b05cc 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1120,7 +1120,6 @@ struct WOLFSSH { byte sftpState; byte realState; byte sftpInt; - word32 sftpExtSz; /* size of extension buffer (buffer not currently used) */ SFTP_OFST sftpOfst[WOLFSSH_MAX_SFTPOFST]; char* sftpDefaultPath; #ifndef NO_WOLFSSH_DIR