Skip to content

Commit 2c47bd6

Browse files
yosuke-wolfsslyosuke-wolfssl
committed
internal, tests: start no highwater rekey after an inbound packet fails
- DoReceive() sets highwaterFlag and msgHighwaterFlag when an inbound packet fails decryption, its MAC, or its length or alignment check. - DoPacket() sets both flags when padding_length is below MIN_PAD_LENGTH or leaves no room for the message id. - The highwaterFlag and msgHighwaterFlag comments name that writer. - The wolfSSH_SendPacket() comment limits its ssh->error claim to the flush and says HighwaterCheck() writes nothing there but its callback may. - unit.c adds test_HighwaterQuietAfterBadPacket(): an idle worker pass fires the highwater callback, and a bad-MAC worker pass and the send after it do not. - unit.c adds test_DoReceive_RejectsPaddingUnderflow(), and the VerifyMacFailure, AeadTagFailure, RejectsShortPadding, RejectsMisalignedPacket and WorkerHardRecvErrorOutranksFlush tests check both flags are set. VerifyMacFailure clears them per case.
1 parent 6ed8e12 commit 2c47bd6

3 files changed

Lines changed: 223 additions & 5 deletions

File tree

‎src/internal.c‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5566,9 +5566,9 @@ static int SendPacketFlush(WOLFSSH* ssh)
55665566
}
55675567

55685568

5569-
/* returns WS_SUCCESS on success. Transport failures record their code in
5570-
* ssh->error, so a later write to that field on the same pass has to be
5571-
* conditional on this having succeeded, or it hides the dead transport. */
5569+
/* returns WS_SUCCESS on success. Flush failures record their code in
5570+
* ssh->error, so a later write there on the same pass must wait for this to
5571+
* succeed. HighwaterCheck() writes nothing there, but its callback may. */
55725572
int wolfSSH_SendPacket(WOLFSSH* ssh)
55735573
{
55745574
int ret;
@@ -13894,11 +13894,16 @@ static int DoPacket(WOLFSSH* ssh, byte* bufferConsumed)
1389413894
if (padSz < MIN_PAD_LENGTH) {
1389513895
WLOG(WS_LOG_DEBUG, "Packet padding length %u below minimum %u",
1389613896
(word32)padSz, (word32)MIN_PAD_LENGTH);
13897+
/* The input cannot be resynced, so no highwater rekey may start. */
13898+
ssh->highwaterFlag = 1;
13899+
ssh->msgHighwaterFlag = 1;
1389713900
return WS_BUFFER_E;
1389813901
}
1389913902

1390013903
/* check for underflow */
1390113904
if ((word32)(PAD_LENGTH_SZ + padSz + MSG_ID_SZ) > pktSz) {
13905+
ssh->highwaterFlag = 1;
13906+
ssh->msgHighwaterFlag = 1;
1390213907
return WS_OVERFLOW_E;
1390313908
}
1390413909

@@ -14631,6 +14636,10 @@ int DoReceive(WOLFSSH* ssh)
1463114636
if (ret != WS_SUCCESS) {
1463214637
WLOG(WS_LOG_DEBUG, "PR: First decrypt fail");
1463314638
ssh->error = ret;
14639+
/* The input cannot be resynced, so no highwater rekey
14640+
* may start. */
14641+
ssh->highwaterFlag = 1;
14642+
ssh->msgHighwaterFlag = 1;
1463414643
return WS_FATAL_ERROR;
1463514644
}
1463614645
}
@@ -14648,6 +14657,8 @@ int DoReceive(WOLFSSH* ssh)
1464814657
WLOG(WS_LOG_DEBUG, "Packet length overflow: size = %u",
1464914658
ssh->curSz);
1465014659
ssh->error = WS_OVERFLOW_E;
14660+
ssh->highwaterFlag = 1;
14661+
ssh->msgHighwaterFlag = 1;
1465114662
return WS_FATAL_ERROR;
1465214663
}
1465314664

@@ -14663,6 +14674,8 @@ int DoReceive(WOLFSSH* ssh)
1466314674
"block = %u, aead = %u",
1466414675
alignSz, (word32)alignBlockSz, (word32)aeadMode);
1466514676
ssh->error = WS_BUFFER_E;
14677+
ssh->highwaterFlag = 1;
14678+
ssh->msgHighwaterFlag = 1;
1466614679
return WS_FATAL_ERROR;
1466714680
}
1466814681
ssh->processReplyState = PROCESS_PACKET_FINISH;
@@ -14702,11 +14715,15 @@ int DoReceive(WOLFSSH* ssh)
1470214715
if (ret != WS_SUCCESS) {
1470314716
WLOG(WS_LOG_DEBUG, "PR: Decrypt fail");
1470414717
ssh->error = ret;
14718+
ssh->highwaterFlag = 1;
14719+
ssh->msgHighwaterFlag = 1;
1470514720
return WS_FATAL_ERROR;
1470614721
}
1470714722
if (verifyResult != WS_SUCCESS) {
1470814723
WLOG(WS_LOG_DEBUG, "PR: VerifyMac fail");
1470914724
ssh->error = verifyResult;
14725+
ssh->highwaterFlag = 1;
14726+
ssh->msgHighwaterFlag = 1;
1471014727
return WS_FATAL_ERROR;
1471114728
}
1471214729
}
@@ -14726,6 +14743,8 @@ int DoReceive(WOLFSSH* ssh)
1472614743
if (ret != WS_SUCCESS) {
1472714744
WLOG(WS_LOG_DEBUG, "PR: DecryptAead fail");
1472814745
ssh->error = ret;
14746+
ssh->highwaterFlag = 1;
14747+
ssh->msgHighwaterFlag = 1;
1472914748
return WS_FATAL_ERROR;
1473014749
}
1473114750
#endif

‎tests/unit.c‎

Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1707,6 +1707,8 @@ static int test_DoReceive_VerifyMacFailure(void)
17071707
ssh->curSz = 0;
17081708
ssh->processReplyState = PROCESS_INIT;
17091709
ssh->error = 0;
1710+
ssh->highwaterFlag = 0;
1711+
ssh->msgHighwaterFlag = 0;
17101712

17111713
flatSeq[0] = (byte)(ssh->peerSeq >> 24);
17121714
flatSeq[1] = (byte)(ssh->peerSeq >> 16);
@@ -1757,6 +1759,10 @@ static int test_DoReceive_VerifyMacFailure(void)
17571759
result = -207;
17581760
goto done;
17591761
}
1762+
if (!ssh->highwaterFlag || !ssh->msgHighwaterFlag) {
1763+
result = -209;
1764+
goto done;
1765+
}
17601766
}
17611767

17621768
done:
@@ -1862,6 +1868,10 @@ static int test_DoReceive_AeadTagFailure(void)
18621868
result = -229;
18631869
goto done;
18641870
}
1871+
if (!ssh->highwaterFlag || !ssh->msgHighwaterFlag) {
1872+
result = -230;
1873+
goto done;
1874+
}
18651875

18661876
done:
18671877
if (aesInited)
@@ -2106,6 +2116,10 @@ static int test_DoReceive_RejectsShortPadding(void)
21062116
result = -764;
21072117
goto done2;
21082118
}
2119+
if (!ssh->highwaterFlag || !ssh->msgHighwaterFlag) {
2120+
result = -765;
2121+
goto done2;
2122+
}
21092123

21102124
done2:
21112125
wolfSSH_free(ssh);
@@ -2114,6 +2128,58 @@ static int test_DoReceive_RejectsShortPadding(void)
21142128
}
21152129

21162130

2131+
/* Verify DoReceive rejects a cleartext packet whose padding_length leaves no
2132+
* room for the message id, returning WS_OVERFLOW_E, and stops the highwater
2133+
* callback. Layout: packet_length=4, padding_length=4 => 8 bytes total. */
2134+
static int test_DoReceive_RejectsPaddingUnderflow(void)
2135+
{
2136+
WOLFSSH_CTX* ctx = NULL;
2137+
WOLFSSH* ssh = NULL;
2138+
int ret;
2139+
int result = 0;
2140+
byte pkt[8];
2141+
word32 totalLen = (word32)sizeof(pkt);
2142+
2143+
ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL);
2144+
if (ctx == NULL)
2145+
return -766;
2146+
ssh = wolfSSH_new(ctx);
2147+
if (ssh == NULL) {
2148+
wolfSSH_CTX_free(ctx);
2149+
return -767;
2150+
}
2151+
2152+
WMEMSET(pkt, 0, sizeof(pkt));
2153+
pkt[3] = 4; /* packet_length */
2154+
pkt[4] = MIN_PAD_LENGTH;
2155+
2156+
ShrinkBuffer(&ssh->inputBuffer, 1);
2157+
ret = GrowBuffer(&ssh->inputBuffer, totalLen);
2158+
if (ret != WS_SUCCESS) {
2159+
result = -768;
2160+
goto done;
2161+
}
2162+
WMEMCPY(ssh->inputBuffer.buffer, pkt, totalLen);
2163+
ssh->inputBuffer.length = totalLen;
2164+
ssh->inputBuffer.idx = 0;
2165+
2166+
ret = wolfSSH_TestDoReceive(ssh);
2167+
if (ret != WS_FATAL_ERROR || ssh->error != WS_OVERFLOW_E) {
2168+
result = -769;
2169+
goto done;
2170+
}
2171+
if (!ssh->highwaterFlag || !ssh->msgHighwaterFlag) {
2172+
result = -793;
2173+
goto done;
2174+
}
2175+
2176+
done:
2177+
wolfSSH_free(ssh);
2178+
wolfSSH_CTX_free(ctx);
2179+
return result;
2180+
}
2181+
2182+
21172183
/* Verify DoReceive rejects a cleartext binary packet whose length field is
21182184
* not block aligned. The packet is valid in every other respect, so the
21192185
* RFC 4253 section 6 alignment check is the only thing that can reject it. */
@@ -2173,6 +2239,10 @@ static int test_DoReceive_RejectsMisalignedPacket(void)
21732239
result = -774;
21742240
goto done3;
21752241
}
2242+
if (!ssh->highwaterFlag || !ssh->msgHighwaterFlag) {
2243+
result = -794;
2244+
goto done3;
2245+
}
21762246

21772247
done3:
21782248
wolfSSH_free(ssh);
@@ -6576,6 +6646,117 @@ static int test_WorkerReportsExtDataChannelKeying(void)
65766646
return result;
65776647
}
65786648

6649+
#if defined(WOLFSSH_TEST_INTERNAL) && !defined(WOLFSSH_NO_HMAC_SHA2_256)
6650+
/* An inbound packet that fails its checks starts no highwater rekey, neither
6651+
* from the worker's flush on that pass nor from a later send. */
6652+
static int test_HighwaterQuietAfterBadPacket(void)
6653+
{
6654+
WOLFSSH_CTX* ctx = NULL;
6655+
WOLFSSH* ssh = NULL;
6656+
HwTestCtx hc;
6657+
Hmac hmac;
6658+
int result = 0;
6659+
int ret;
6660+
word32 prefixLen;
6661+
byte seq[LENGTH_SZ];
6662+
byte macKey[WC_SHA256_DIGEST_SIZE];
6663+
byte digest[WC_SHA256_DIGEST_SIZE];
6664+
byte pkt[UINT32_SZ + 12 + WC_SHA256_DIGEST_SIZE];
6665+
6666+
WMEMSET(&hc, 0, sizeof(hc));
6667+
s_extSendCount = 0;
6668+
6669+
ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL);
6670+
if (ctx == NULL)
6671+
return -1950;
6672+
wolfSSH_SetIOSend(ctx, CountIoSend);
6673+
wolfSSH_SetIORecv(ctx, PacketIoRecv);
6674+
wolfSSH_SetHighwaterCb(ctx, 1, HwTestCb);
6675+
6676+
ssh = wolfSSH_new(ctx);
6677+
if (ssh == NULL) { result = -1951; goto done; }
6678+
hc.ssh = ssh;
6679+
wolfSSH_SetHighwaterCtx(ssh, &hc);
6680+
wolfSSH_SetMsgHighwater(ssh, 1);
6681+
ssh->txCount = 1;
6682+
ssh->txMsgCount = 1;
6683+
6684+
/* Control: the flush after an idle receive fires the callback. */
6685+
ssh->outputBuffer.length = 1;
6686+
ssh->outputBuffer.idx = 0;
6687+
ssh->outputBuffer.buffer[0] = 0;
6688+
ret = wolfSSH_worker(ssh, NULL);
6689+
if (ret != WS_FATAL_ERROR || wolfSSH_get_error(ssh) != WS_WANT_READ) {
6690+
result = -1952;
6691+
goto done;
6692+
}
6693+
if (hc.count != 1) { result = -1953; goto done; }
6694+
6695+
/* A new key epoch clears both flags. */
6696+
ssh->highwaterFlag = 0;
6697+
ssh->msgHighwaterFlag = 0;
6698+
hc.count = 0;
6699+
6700+
/* An IGNORE packet under HMAC-SHA2-256 at peerSeq 0, one MAC bit off. */
6701+
prefixLen = BuildMacTestPacketPrefix(MSGID_IGNORE, 10, pkt, sizeof(pkt));
6702+
if (prefixLen == 0) { result = -1954; goto done; }
6703+
WMEMSET(macKey, 0xA5, sizeof(macKey));
6704+
WMEMSET(seq, 0, sizeof(seq));
6705+
ret = wc_HmacInit(&hmac, ssh->ctx->heap, INVALID_DEVID);
6706+
if (ret == 0) {
6707+
ret = wc_HmacSetKey(&hmac, WC_SHA256, macKey, sizeof(macKey));
6708+
if (ret == 0)
6709+
ret = wc_HmacUpdate(&hmac, seq, sizeof(seq));
6710+
if (ret == 0)
6711+
ret = wc_HmacUpdate(&hmac, pkt, prefixLen);
6712+
if (ret == 0)
6713+
ret = wc_HmacFinal(&hmac, digest);
6714+
wc_HmacFree(&hmac);
6715+
}
6716+
if (ret != 0) { result = -1955; goto done; }
6717+
WMEMCPY(pkt + prefixLen, digest, sizeof(digest));
6718+
pkt[prefixLen] ^= 0x01;
6719+
6720+
ssh->peerMacId = ID_HMAC_SHA2_256;
6721+
ssh->peerMacSz = WC_SHA256_DIGEST_SIZE;
6722+
WMEMCPY(ssh->peerKeys.macKey, macKey, sizeof(macKey));
6723+
ssh->peerKeys.macKeySz = sizeof(macKey);
6724+
s_recvPkt = pkt;
6725+
s_recvPktSz = prefixLen + WC_SHA256_DIGEST_SIZE;
6726+
s_recvPktOff = 0;
6727+
6728+
/* The worker still flushes what was queued, but fires nothing. */
6729+
ssh->outputBuffer.length = 1;
6730+
ssh->outputBuffer.idx = 0;
6731+
ssh->outputBuffer.buffer[0] = 0;
6732+
s_extSendCount = 0;
6733+
ret = wolfSSH_worker(ssh, NULL);
6734+
if (ret != WS_FATAL_ERROR || wolfSSH_get_error(ssh) != WS_VERIFY_MAC_E) {
6735+
result = -1957;
6736+
goto done;
6737+
}
6738+
if (wolfSSH_OutputPending(ssh) || s_extSendCount == 0) {
6739+
result = -1958;
6740+
goto done;
6741+
}
6742+
if (hc.count != 0) { result = -1959; goto done; }
6743+
6744+
/* Nor does the next send, as a teardown would make. */
6745+
ret = wolfSSH_SendIgnore(ssh, NULL, 0);
6746+
if (ret != WS_SUCCESS) { result = -1960; goto done; }
6747+
if (hc.count != 0) { result = -1961; goto done; }
6748+
6749+
done:
6750+
s_recvPkt = NULL;
6751+
s_recvPktSz = 0;
6752+
s_recvPktOff = 0;
6753+
s_extSendCount = 0;
6754+
wolfSSH_free(ssh);
6755+
wolfSSH_CTX_free(ctx);
6756+
return result;
6757+
}
6758+
#endif /* WOLFSSH_TEST_INTERNAL && !WOLFSSH_NO_HMAC_SHA2_256 */
6759+
65796760
/* channelId=0, type=1 (stderr), dataSz=10, payload all 0x44. */
65806761
static const byte s_workerExtBlob[] = {
65816762
0x00, 0x00, 0x00, 0x00,
@@ -6781,6 +6962,10 @@ static int test_WorkerHardRecvErrorOutranksFlush(void)
67816962
ret = wolfSSH_worker(ssh, NULL);
67826963
if (ret != WS_FATAL_ERROR) { result = -1735; goto done; }
67836964
if (wolfSSH_get_error(ssh) != WS_OVERFLOW_E) { result = -1736; goto done; }
6965+
if (!ssh->highwaterFlag || !ssh->msgHighwaterFlag) {
6966+
result = -1784;
6967+
goto done;
6968+
}
67846969

67856970
done:
67866971
s_recvPkt = NULL;
@@ -22776,6 +22961,11 @@ int wolfSSH_UnitTest(int argc, char** argv)
2277622961
printf("DoReceiveRejectsShortPadding: %s\n",
2277722962
(unitResult == 0 ? "SUCCESS" : "FAILED"));
2277822963
testResult = testResult || unitResult;
22964+
22965+
unitResult = test_DoReceive_RejectsPaddingUnderflow();
22966+
printf("DoReceiveRejectsPaddingUnderflow: %s\n",
22967+
(unitResult == 0 ? "SUCCESS" : "FAILED"));
22968+
testResult = testResult || unitResult;
2277922969
#endif
2278022970

2278122971
#ifdef WOLFSSH_TEST_INTERNAL
@@ -23004,6 +23194,13 @@ int wolfSSH_UnitTest(int argc, char** argv)
2300423194
(unitResult == 0 ? "SUCCESS" : "FAILED"));
2300523195
testResult = testResult || unitResult;
2300623196

23197+
#if defined(WOLFSSH_TEST_INTERNAL) && !defined(WOLFSSH_NO_HMAC_SHA2_256)
23198+
unitResult = test_HighwaterQuietAfterBadPacket();
23199+
printf("HighwaterQuietAfterBadPacket: %s\n",
23200+
(unitResult == 0 ? "SUCCESS" : "FAILED"));
23201+
testResult = testResult || unitResult;
23202+
#endif
23203+
2300723204
unitResult = test_WorkerFlushesOnIdleReceive();
2300823205
printf("WorkerFlushesOnIdleReceive: %s\n",
2300923206
(unitResult == 0 ? "SUCCESS" : "FAILED"));

‎wolfssh/internal.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1192,8 +1192,10 @@ struct WOLFSSH {
11921192
word32 txFlushCount; /* Output buffer drained, whatever came after */
11931193
word32 highwaterMark;
11941194
word32 msgHighwaterMark; /* Per-key packet limit (RFC 4344 Sec 3.1) */
1195-
byte highwaterFlag; /* Set when highwater CB called */
1196-
byte msgHighwaterFlag; /* Set when msg-count highwater CB called */
1195+
byte highwaterFlag; /* Set when highwater CB called, or when an
1196+
* inbound packet fails a framing or integrity
1197+
* check */
1198+
byte msgHighwaterFlag; /* Same, for the msg-count highwater CB */
11971199
void* highwaterCtx; /* Highwater CB context */
11981200
void* globalReqCtx; /* Global Request CB context */
11991201
void* reqSuccessCtx; /* Global Request Success CB context */

0 commit comments

Comments
 (0)