diff --git a/src/internal.c b/src/internal.c index 99747dae..c284fb7e 100644 --- a/src/internal.c +++ b/src/internal.c @@ -12106,7 +12106,9 @@ static int DoChannelExtendedData(WOLFSSH* ssh, static int DoPacket(WOLFSSH* ssh, byte* bufferConsumed) { byte* buf = (byte*)ssh->inputBuffer.buffer; - word32 idx = ssh->inputBuffer.idx; + word32 pktStart = ssh->inputBuffer.idx; + word32 pktSz = ssh->curSz; + word32 idx = pktStart; word32 len = ssh->inputBuffer.length; word32 payloadSz; byte padSz; @@ -12129,11 +12131,11 @@ static int DoPacket(WOLFSSH* ssh, byte* bufferConsumed) } /* check for underflow */ - if ((word32)(PAD_LENGTH_SZ + padSz + MSG_ID_SZ) > ssh->curSz) { + if ((word32)(PAD_LENGTH_SZ + padSz + MSG_ID_SZ) > pktSz) { return WS_OVERFLOW_E; } - payloadSz = ssh->curSz - PAD_LENGTH_SZ - padSz - MSG_ID_SZ; + payloadSz = pktSz - PAD_LENGTH_SZ - padSz - MSG_ID_SZ; msg = buf[idx++]; /* At this point, payload starts at "buf + idx". */ @@ -12362,15 +12364,17 @@ static int DoPacket(WOLFSSH* ssh, byte* bufferConsumed) /* if the auth is still pending, don't discard the packet data */ if (ret != WS_AUTH_PENDING) { - if (payloadSz > 0) { - idx += payloadIdx; - if (idx + padSz > len) { - WLOG(WS_LOG_DEBUG, "Not enough data in buffer for pad."); - ret = WS_BUFFER_E; - } + /* Step over the packet using the length DoReceive already validated, + * not payloadIdx. A handler is free to read less than the payload it + * was handed -- the default case above reads none of it -- and that + * must not decide where the next packet begins. pktStart and pktSz + * are both from entry, so a handler cannot move the frame either. */ + idx = pktStart + UINT32_SZ + pktSz; + if (idx > len) { + WLOG(WS_LOG_DEBUG, "Not enough data in buffer for packet."); + ret = WS_BUFFER_E; + idx = len; } - - idx += padSz; ssh->inputBuffer.idx = idx; ssh->peerSeq++; ssh->rxMsgCount++; @@ -22921,6 +22925,11 @@ int wolfSSH_TestDoReceive(WOLFSSH* ssh) return DoReceive(ssh); } +int wolfSSH_TestDoPacket(WOLFSSH* ssh, byte* bufferConsumed) +{ + return DoPacket(ssh, bufferConsumed); +} + int wolfSSH_TestDoUserAuthBanner(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) { diff --git a/tests/unit.c b/tests/unit.c index 72e5ee42..2d77ecac 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -1279,6 +1279,140 @@ static word32 BuildMacTestPacketPrefix(byte msgId, byte padLen, #endif +/* The test below drives a server-side session. With NO_WOLFSSH_SERVER the + * message filter has no server branch, so every message on such a session is + * refused and the test cannot run. */ +#if defined(WOLFSSH_TEST_INTERNAL) && !defined(NO_WOLFSSH_SERVER) + +/* Swallow the SSH_MSG_UNIMPLEMENTED that DoPacket sends back, so its send + * does not decide the result of the test. */ +static int SinkIoSendUnimplemented(WOLFSSH* ssh, void* buf, word32 sz, + void* ctx) +{ + (void)ssh; (void)buf; (void)ctx; + return (int)sz; +} + +/* Two back-to-back packets in one buffer. DoPacket() is driven directly: + * DoReceive() force-frees the buffer after every packet, zeroing the cursor + * under test. */ +static const byte s_unimplStream[] = { + /* unimplemented message, 8 bytes of payload that no handler reads */ + 0x00, 0x00, 0x00, 0x10, /* packetSz = 16 */ + 0x06, /* padSz = 6 */ + 0x0A, /* msgId = 10 */ + 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, /* payload */ + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* padding */ + /* MSGID_IGNORE carrying a zero-length string */ + 0x00, 0x00, 0x00, 0x0C, /* packetSz = 12 */ + 0x06, /* padSz = 6 */ + 0x02, /* msgId = IGNORE */ + 0x00, 0x00, 0x00, 0x00, /* string, len 0 */ + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 /* padding */ +}; + +/* Message ID 10 is unassigned and always allowed, so it reaches DoPacket's + * default case, which reads none of its 8 payload bytes. DoPacket must step + * over them itself. Checked by the cursor it leaves, and by the length peeked + * there being the next packet's. Framing from payloadIdx lands 8 short. */ +static int test_DoPacket_UnimplementedConsumesPayload(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + int result = 0; + int ret; + int i; + /* packetSz of each packet in s_unimplStream, in order */ + static const word32 pktSizes[] = { 16, 12 }; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -220; + wolfSSH_SetIOSend(ctx, SinkIoSendUnimplemented); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -221; goto done; } + + /* Past user auth, so the connection-layer message IDs are allowed. */ + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + ssh->peerEncryptId = ID_NONE; + ssh->peerMacId = ID_NONE; + ssh->peerAeadMode = 0; + ssh->peerBlockSz = MIN_BLOCK_SZ; + ssh->peerMacSz = 0; + ssh->peerSeq = 0; + ssh->error = 0; + + /* Both packets in the buffer at once. */ + ShrinkBuffer(&ssh->inputBuffer, 1); + if (GrowBuffer(&ssh->inputBuffer, (word32)sizeof(s_unimplStream)) + != WS_SUCCESS) { + result = -222; + goto done; + } + WMEMCPY(ssh->inputBuffer.buffer, s_unimplStream, sizeof(s_unimplStream)); + ssh->inputBuffer.length = (word32)sizeof(s_unimplStream); + ssh->inputBuffer.idx = 0; + + for (i = 0; i < (int)(sizeof(pktSizes) / sizeof(pktSizes[0])); i++) { + word32 pktStart = ssh->inputBuffer.idx; + const byte* lenField = ssh->inputBuffer.buffer + pktStart; + word32 curSz; + byte bufferConsumed = 0; + + /* The peek DoReceive does; a short cursor reads leftover payload. */ + curSz = ((word32)lenField[0] << 24) | ((word32)lenField[1] << 16) + | ((word32)lenField[2] << 8) | (word32)lenField[3]; + if (curSz != pktSizes[i]) { + printf("DoPacket[%d]: packetSz=%u at cursor %u, expected %u\n", + i, curSz, pktStart, pktSizes[i]); + result = -223; + goto done; + } + ssh->curSz = curSz; + + ret = wolfSSH_TestDoPacket(ssh, &bufferConsumed); + if (ret != WS_SUCCESS) { + printf("DoPacket[%d]: ret=%d, error=%d\n", i, ret, ssh->error); + result = -224; + goto done; + } + if (!bufferConsumed) { + printf("DoPacket[%d]: packet not consumed\n", i); + result = -225; + goto done; + } + /* The whole packet, no more and no less. */ + if (ssh->inputBuffer.idx != pktStart + UINT32_SZ + curSz) { + printf("DoPacket[%d]: cursor at %u, expected %u\n", i, + ssh->inputBuffer.idx, pktStart + UINT32_SZ + curSz); + result = -226; + goto done; + } + if (ssh->peerSeq != (word32)(i + 1)) { + printf("DoPacket[%d]: peerSeq=%u, expected %d\n", i, + ssh->peerSeq, i + 1); + result = -227; + goto done; + } + } + + /* Buffer exactly spent. */ + if (ssh->inputBuffer.idx != (word32)sizeof(s_unimplStream)) { + printf("DoPacket: %u of %u bytes consumed\n", ssh->inputBuffer.idx, + (word32)sizeof(s_unimplStream)); + result = -228; + goto done; + } + +done: + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} +#endif /* WOLFSSH_TEST_INTERNAL && !NO_WOLFSSH_SERVER */ + + #if defined(WOLFSSH_TEST_INTERNAL) && \ (!defined(WOLFSSH_NO_HMAC_SHA1) || \ !defined(WOLFSSH_NO_HMAC_SHA1_96) || \ @@ -16544,6 +16678,13 @@ int wolfSSH_UnitTest(int argc, char** argv) testResult = testResult || unitResult; #endif +#if defined(WOLFSSH_TEST_INTERNAL) && !defined(NO_WOLFSSH_SERVER) + unitResult = test_DoPacket_UnimplementedConsumesPayload(); + printf("DoPacketUnimplemented: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif + #if defined(WOLFSSH_TEST_INTERNAL) && \ (!defined(WOLFSSH_NO_HMAC_SHA1) || \ !defined(WOLFSSH_NO_HMAC_SHA1_96) || \ diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 2fd7785c..6fa1f640 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1810,6 +1810,8 @@ enum WS_MessageIdLimits { WOLFSSH_API int wolfSSH_TestIsMessageAllowed(WOLFSSH* ssh, byte msg, byte state); WOLFSSH_API int wolfSSH_TestDoReceive(WOLFSSH* ssh); + WOLFSSH_API int wolfSSH_TestDoPacket(WOLFSSH* ssh, + byte* bufferConsumed); WOLFSSH_API int wolfSSH_TestDoUserAuthBanner(WOLFSSH* ssh, byte* buf, word32 len, word32* idx); WOLFSSH_API int wolfSSH_TestPrepareUserAuthRequestPassword(WOLFSSH* ssh,