diff --git a/src/agent.c b/src/agent.c index 1174f35b6..f3817275e 100644 --- a/src/agent.c +++ b/src/agent.c @@ -1392,24 +1392,18 @@ static int DoMessage(WOLFSSH_AGENT_CTX* agent, if (ret == WS_SUCCESS) { WLOG(WS_LOG_AGENT, "len = %u, idx = %u", len, *idx); begin = *idx; - if (begin > len) { - ret = WS_OVERFLOW_E; - } - } - if (ret == WS_SUCCESS) { - if (LENGTH_SZ + MSG_ID_SZ + begin > len) { + /* GetSize bounds the prefix and the payload; a nonzero payloadSz + * then covers the msg id read below. Reject 0: payloadSz - 1 is + * used as a length. */ + if (GetSize(&payloadSz, buf, len, &begin) != WS_SUCCESS) { ret = WS_OVERFLOW_E; } - } - - if (ret == WS_SUCCESS) { - ato32(buf + begin, &payloadSz); - WLOG(WS_LOG_AGENT, "payloadSz = %u", payloadSz); - begin += LENGTH_SZ; - /* reject 0: payloadSz - 1 is used as a length below */ - if (payloadSz == 0 || payloadSz > len - begin) { - ret = WS_OVERFLOW_E; + else { + WLOG(WS_LOG_AGENT, "payloadSz = %u", payloadSz); + if (payloadSz == 0) { + ret = WS_OVERFLOW_E; + } } } diff --git a/src/internal.c b/src/internal.c index 35b5e73df..e96ade89a 100644 --- a/src/internal.c +++ b/src/internal.c @@ -3606,6 +3606,12 @@ WOLFSSH_CHANNEL* ChannelNew(WOLFSSH* ssh, byte channelType, WMEMSET(newChannel, 0, sizeof(WOLFSSH_CHANNEL)); newChannel->ssh = ssh; newChannel->channelType = channelType; + /* Skip channel ids already in use to avoid collisions when + * nextChannel (word32) wraps around. */ + while (ChannelFind(ssh, ssh->nextChannel, + WS_CHANNEL_ID_SELF) != NULL) { + ssh->nextChannel++; + } newChannel->channel = ssh->nextChannel++; WLOG(WS_LOG_DEBUG, "New channel id = %u", newChannel->channel); newChannel->windowSz = initialWindowSz; @@ -4552,11 +4558,7 @@ static int GetNameList(byte* idList, word32* idListSz, */ if (ret == WS_SUCCESS) { - if (*idx >= len || *idx + 4 >= len) - ret = WS_BUFFER_E; - } - - if (ret == WS_SUCCESS) { + /* GetStringRef bounds the length prefix and the list. */ ret = GetStringRef(&nameListSz, &nameList, buf, len, idx); } @@ -6934,7 +6936,6 @@ static int DoKexDhReply(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) word32 pubKeySz; word32 fSz; word32 sigSz; - word32 scratch; word32 begin; int ret = WS_SUCCESS; enum wc_HashType hashId; @@ -7176,47 +7177,36 @@ static int DoKexDhReply(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) /* Verify h with the server's public key. */ if (ret == WS_SUCCESS) { -#ifndef WOLFSSH_NO_RSA - int tmpIdx = begin - sigSz; -#endif const char* expectedSigName = IdToName(SigTypeForId(ssh->handshake->pubKeyId)); word32 expectedSigNameSz = (word32)WSTRLEN(expectedSigName); + const byte* sigName = NULL; + word32 sigNameSz = 0; + word32 sigBlobSz = 0; begin = 0; - ret = GetUint32(&scratch, sig, sigSz, &begin); - if (ret == WS_SUCCESS) { - /* Check that scratch isn't larger than the remainder of the - * sig buffer and leaves enough room for another length. */ - if (scratch > sigSz - begin - LENGTH_SZ) { - WLOG(WS_LOG_DEBUG, "sig name size is too large"); - ret = WS_PARSE_E; - } - } + ret = GetStringRef(&sigNameSz, &sigName, sig, sigSz, &begin); if (ret == WS_SUCCESS) { - if (scratch != expectedSigNameSz || - WMEMCMP(sig + begin, expectedSigName, scratch) != 0) { + /* expectedSigName is never empty, so a null sigName fails + * on size first. */ + if (sigNameSz != expectedSigNameSz || + WMEMCMP(sigName, expectedSigName, sigNameSz) != 0) { WLOG(WS_LOG_DEBUG, "signature name %.*s did not match negotiated %s", - (int)scratch, (const char*)(sig + begin), + (int)sigNameSz, + (sigName != NULL) ? (const char*)sigName : "", expectedSigName); ret = WS_PARSE_E; } } if (ret == WS_SUCCESS) { - begin += scratch; - ret = GetUint32(&scratch, sig, sigSz, &begin); - } - if (ret == WS_SUCCESS) { - if (scratch > sigSz - begin) { - WLOG(WS_LOG_DEBUG, "sig name size is too large"); - ret = WS_PARSE_E; - } + /* GetSize leaves begin at the blob, and sig non-null when + * the blob is empty. */ + ret = GetSize(&sigBlobSz, sig, sigSz, &begin); } if (ret == WS_SUCCESS) { sig = sig + begin; - /* In the fuzz, sigSz ends up 1 and it has issues. */ - sigSz = scratch; + sigSz = sigBlobSz; if (sigKeyBlock_ptr->useRsa) { #ifndef WOLFSSH_NO_RSA @@ -7225,12 +7215,6 @@ static int DoKexDhReply(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) ret = WS_RSA_E; } - if (sigSz + begin + tmpIdx > len) { - WLOG(WS_LOG_DEBUG, - "Signature size found would result in error 2"); - ret = WS_BUFFER_E; - } - if (ret == WS_SUCCESS) { ret = wc_SignatureVerify( HashForId(ssh->handshake->pubKeyId), @@ -12771,6 +12755,14 @@ static int BuildNameList(char* buf, word32 bufSz, idx = 0; + if (srcSz == 0) { + /* Terminate: callers measure buf with WSTRLEN. */ + if (buf != NULL && bufSz > 0) { + buf[0] = '\0'; + } + return 0; + } + do { name = IdToName(*src); nameSz = (int)WSTRLEN(name); @@ -19431,15 +19423,22 @@ int SendChannelData(WOLFSSH* ssh, word32 channelId, word32 bound = min(channel->peerWindowSz, channel->peerMaxPacketSz); bound = min(bound, channel->maxPacketSz); - if (dataSz > bound) { - WLOG(WS_LOG_DEBUG, - "Trying to send %u, client will only accept %u, limiting", - dataSz, bound); - dataSz = bound; + if (bound == 0 && dataSz != 0) { + WLOG(WS_LOG_DEBUG, "peer max packet size is zero"); + ssh->error = WS_WINDOW_FULL; + ret = WS_WINDOW_FULL; } + else { + if (dataSz > bound) { + WLOG(WS_LOG_DEBUG, + "Trying to send %u, client will only accept %u, limiting", + dataSz, bound); + dataSz = bound; + } - ret = PreparePacket(ssh, - MSG_ID_SZ + UINT32_SZ + LENGTH_SZ + dataSz); + ret = PreparePacket(ssh, + MSG_ID_SZ + UINT32_SZ + LENGTH_SZ + dataSz); + } } if (ret == WS_SUCCESS) { @@ -19544,15 +19543,22 @@ int SendChannelExtendedData(WOLFSSH* ssh, word32 channelId, word32 bound = min(channel->peerWindowSz, channel->peerMaxPacketSz); bound = min(bound, channel->maxPacketSz); - if (dataSz > bound) { - WLOG(WS_LOG_DEBUG, - "Trying to send %u, client will only accept %u, limiting", - dataSz, bound); - dataSz = bound; + if (bound == 0 && dataSz != 0) { + WLOG(WS_LOG_DEBUG, "peer max packet size is zero"); + ssh->error = WS_WINDOW_FULL; + ret = WS_WINDOW_FULL; } + else { + if (dataSz > bound) { + WLOG(WS_LOG_DEBUG, + "Trying to send %u, client will only accept %u, limiting", + dataSz, bound); + dataSz = bound; + } - ret = PreparePacket(ssh, - MSG_ID_SZ + UINT32_SZ + UINT32_SZ + LENGTH_SZ + dataSz); + ret = PreparePacket(ssh, + MSG_ID_SZ + UINT32_SZ + UINT32_SZ + LENGTH_SZ + dataSz); + } } if (ret == WS_SUCCESS) { @@ -20625,6 +20631,12 @@ int wolfSSH_TestChannelPutData(WOLFSSH_CHANNEL* channel, byte* data, return ChannelPutData(channel, data, dataSz); } +int wolfSSH_TestBuildNameList(char* buf, word32 bufSz, + const byte* src, word32 srcSz) +{ + return BuildNameList(buf, bufSz, src, srcSz); +} + int wolfSSH_TestDoChannelSuccess(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) { diff --git a/src/wolfsftp.c b/src/wolfsftp.c index fc15f9fd2..1783ffa43 100644 --- a/src/wolfsftp.c +++ b/src/wolfsftp.c @@ -6636,6 +6636,14 @@ static int wolfSSH_SFTP_DoStatus(WOLFSSH* ssh, word32 reqId, return WS_FATAL_ERROR; } + /* status is a small enumerated value (0..WOLFSSH_FTP_UNSUPPORTED). An + * out-of-range value from a malicious or broken server must not be + * returned as a negative int, where it would alias an internal WS_* + * error code and bypass the WOLFSSH_FTP_* classification in callers. */ + if (status > (word32)WOLFSSH_FTP_UNSUPPORTED) { + status = WOLFSSH_FTP_FAILURE; + } + /* read error message */ if (GetStringRef(&sz, &str, buf, maxIdx, &localIdx) != WS_SUCCESS) { return WS_FATAL_ERROR; diff --git a/tests/regress.c b/tests/regress.c index 27a89f7d8..2750dec53 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -425,6 +425,7 @@ static word32 LoadFileBuffer(const char* path, byte* buf, word32 bufSz) /* KEXDH_REPLY mutation modes for the duplex mutator. */ #define REGRESS_MUTATE_SIG_NAME 0 #define REGRESS_MUTATE_SIG_DATA 1 +#define REGRESS_MUTATE_SIG_NAME_OVERRUN 2 typedef struct { byte data[REGRESS_DUPLEX_QUEUE_SZ]; @@ -676,6 +677,12 @@ static int RewriteSingleKexDhReplyPacket(const byte* packet, word32 packetSz, } innerSig[flipIdx] ^= 0xFF; } + else if (mode == REGRESS_MUTATE_SIG_NAME_OVERRUN) { + /* Nothing but a name length prefix, so the name it claims runs off + * the end of the blob. */ + innerSigSz = AppendUint32(innerSig, sizeof(innerSig), innerSigSz, + sigNameSz); + } else { innerSigSz = AppendString(innerSig, sizeof(innerSig), innerSigSz, replacement); @@ -1149,6 +1156,29 @@ static void TestKexDhReplyRejectsEd25519CorruptSig(void) } #endif +/* A signature blob holding only a name length prefix. The bounded read + * rejects it before the name is compared, whatever the host key type. */ +static void TestKexDhReplyRejectsSigNameOverrun(void) +{ + KexReplyHarness harness; + KexReplyRunResult result; + + InitKexReplyHarnessEx(&harness, REGRESS_DEFAULT_KEY_ALGO, + REGRESS_DEFAULT_KEY_PATH, 1, + REGRESS_MUTATE_SIG_NAME_OVERRUN, NULL, 0); + RunKexReplyHandshake(&harness, &result); + + AssertIntEQ(harness.mutator.parseError, 0); + AssertIntEQ(harness.mutator.matchedPackets, 1); + AssertIntEQ(harness.mutator.mutatedPackets, 1); + AssertFalse(result.clientSuccess); + AssertFalse(harness.client->connectState >= CONNECT_KEYED); + AssertTrue(result.clientRet == WS_FATAL_ERROR); + AssertIntEQ(result.clientErr, WS_BUFFER_E); + + FreeKexReplyHarness(&harness); +} + #endif /* KEXDH_REPLY_REGRESS_KEX_ALGO */ static word32 ParseChannelOpenFailRecipient(const byte* pkt, word32 sz) @@ -5175,6 +5205,7 @@ int main(int argc, char** argv) #ifndef WOLFSSH_NO_ED25519 TestKexDhReplyRejectsEd25519CorruptSig(); #endif + TestKexDhReplyRejectsSigNameOverrun(); #endif #ifdef WOLFSSH_SFTP diff --git a/tests/unit.c b/tests/unit.c index 633a50f4e..cf7ab9fa4 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -5277,6 +5277,102 @@ static int test_SendChannelData_eofTxd(void) return result; } +/* A peer may advertise a maximum packet size of 0 in its CHANNEL_OPEN or + * CHANNEL_OPEN_CONFIRMATION. Sending must report WS_WINDOW_FULL, not push + * dataSz past the bound and charge it against the window. */ +static int test_SendChannelData_zeroPeerMaxPacket(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + WOLFSSH_CHANNEL* ch = NULL; + int result = 0; + int ret; + byte buf[8] = { 0, 1, 2, 3, 4, 5, 6, 7 }; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + if (ctx == NULL) + return -1400; + wolfSSH_SetIOSend(ctx, DiscardIoSend); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -1401; goto done; } + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, + DEFAULT_WINDOW_SZ, DEFAULT_MAX_PACKET_SZ); + if (ch == NULL) { result = -1402; goto done; } + if (ChannelAppend(ssh, ch) != WS_SUCCESS) { + ChannelDelete(ch, ssh->ctx->heap); + result = -1403; + goto done; + } + ch->openConfirmed = 1; + + /* Window has room, but the peer will not accept a packet of any size. */ + ch->peerWindowSz = 100; + ch->peerMaxPacketSz = 0; + + ret = SendChannelData(ssh, ch->channel, buf, (word32)sizeof(buf)); + if (ret != WS_WINDOW_FULL) { result = -1410; goto done; } + if (ssh->error != WS_WINDOW_FULL) { result = -1411; goto done; } + /* Nothing queued, and the window was not charged. */ + if (ssh->outputBuffer.length != 0) { result = -1412; goto done; } + if (ch->peerWindowSz != 100) { result = -1413; goto done; } + + ssh->error = WS_SUCCESS; + ret = SendChannelExtendedData(ssh, ch->channel, buf, (word32)sizeof(buf)); + if (ret != WS_WINDOW_FULL) { result = -1420; goto done; } + if (ssh->error != WS_WINDOW_FULL) { result = -1421; goto done; } + if (ssh->outputBuffer.length != 0) { result = -1422; goto done; } + if (ch->peerWindowSz != 100) { result = -1423; goto done; } + + /* A zero-length send is still allowed through: it charges nothing. */ + ssh->error = WS_SUCCESS; + ret = SendChannelData(ssh, ch->channel, buf, 0); + if (ret != 0) { result = -1430; goto done; } + if (ch->peerWindowSz != 100) { result = -1431; goto done; } + +done: + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} + +/* BuildNameList() returns a C string. On an empty id list it must still + * terminate the buffer: SendKexInit() measures the result with WSTRLEN + * through AlgoListSz() and copies that many bytes into the KEXINIT. */ +static int test_BuildNameList_emptySrc(void) +{ + char buf[64]; + byte src[1] = { ID_AES128_GCM }; + int ret; + + /* Poison, so a missing terminator is a wrong length rather than luck. */ + WMEMSET(buf, 'A', sizeof(buf)); + + ret = wolfSSH_TestBuildNameList(buf, (word32)sizeof(buf), src, 0); + if (ret != 0) + return -1440; + if (buf[0] != '\0') + return -1441; + if (WSTRLEN(buf) != 0) + return -1442; + + /* The sizing call takes buf == NULL and must not touch anything. */ + ret = wolfSSH_TestBuildNameList(NULL, 0, src, 0); + if (ret != 0) + return -1443; + + /* One name still behaves: length excluding the terminator. */ + WMEMSET(buf, 'A', sizeof(buf)); + ret = wolfSSH_TestBuildNameList(buf, (word32)sizeof(buf), src, 1); + if (ret != (int)WSTRLEN(IdToName(ID_AES128_GCM))) + return -1444; + if (WSTRLEN(buf) != (size_t)ret) + return -1445; + + return 0; +} + /* Plaintext SSH packet from IoSend (before encryption/MAC): LENGTH_SZ, * PAD_LENGTH_SZ, then payload starting with the message ID (RFC 4253; * wolfSSH PreparePacket/BundlePacket). Not for encrypted payloads or @@ -7490,7 +7586,7 @@ static int test_DoUserAuthRequestMlDsa_CertPath(const char* keyTypeName) const word32 keyTypeNameSz = (word32)WSTRLEN(keyTypeName); /* NOTE: pubKeyBlob is an RFC 6187 wire blob, not leaf-cert DER. The real * server path calls ParseLeafCert() first to extract DER. This test - * exercises ASN.1-invalid rejection rather than cryptographic rejection — + * exercises ASN.1-invalid rejection rather than cryptographic rejection, * valid for a negative path test, but does not cover the DER-valid case. */ static const byte junkCert[] = { 0x30, 0x05, 0x00, 0x00, 0x00, 0x00 }; const word32 junkCertSz = (word32)sizeof(junkCert); @@ -11761,6 +11857,16 @@ int wolfSSH_UnitTest(int argc, char** argv) printf("SendChannelData_eofTxd: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; + unitResult = test_SendChannelData_zeroPeerMaxPacket(); + printf("SendChannelData_zeroPeerMaxPacket: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + + unitResult = test_BuildNameList_emptySrc(); + printf("BuildNameList_emptySrc: %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")); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 41996031e..7483b650c 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1611,6 +1611,8 @@ enum WS_MessageIdLimits { word32 len, word32* idx); WOLFSSH_API int wolfSSH_TestChannelPutData(WOLFSSH_CHANNEL* channel, byte* data, word32 dataSz); + WOLFSSH_API int wolfSSH_TestBuildNameList(char* buf, word32 bufSz, + const byte* src, word32 srcSz); WOLFSSH_API int wolfSSH_TestDoUserAuthRequest(WOLFSSH* ssh, byte* buf, word32 len, word32* idx); WOLFSSH_API int wolfSSH_TestSendUserAuthFailure(WOLFSSH* ssh,