Skip to content

Commit 08d12b3

Browse files
ejohnstownejohnstown
committed
internal: hash the pad byte of K without a branch
The MSB of the shared secret K decides whether its mpint encoding takes a 0 pad byte. CreateMpint now derives the pad with masks, and HashMpintHeader hashes the length and the optional pad as one update of LENGTH_SZ + kPad bytes: no branch on the pad in wolfSSH, and no zero-length update, which the SE050 and TI hash ports reject. - apply to DoKexDhReply, SendKexDhReply, and the in-tree GenerateKey - add wolfSSH_TestCreateMpint and a unit test of the pad, leading-zero, empty-input and NULL-argument cases Issue: F-8808
1 parent 8221e4e commit 08d12b3

3 files changed

Lines changed: 101 additions & 40 deletions

File tree

‎src/internal.c‎

Lines changed: 38 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -650,6 +650,22 @@ static int HashUpdate(wc_HashAlg* hash, enum wc_HashType type,
650650
}
651651

652652

653+
/* Hash an mpint's length and optional 0 pad byte as one update of
654+
* LENGTH_SZ + pad bytes. For K the pad is secret: this avoids a branch on
655+
* it and a zero-length update, which some hash ports reject. The hashed
656+
* length still depends on the pad. */
657+
static INLINE int HashMpintHeader(wc_HashAlg* hash, enum wc_HashType type,
658+
word32 sz, byte pad)
659+
{
660+
byte hdr[LENGTH_SZ + 1];
661+
662+
pad &= 1;
663+
c32toa(sz + pad, hdr);
664+
hdr[LENGTH_SZ] = 0;
665+
return HashUpdate(hash, type, hdr, LENGTH_SZ + pad);
666+
}
667+
668+
653669
/* returns WS_SUCCESS on success */
654670
static INLINE int HighwaterCheck(WOLFSSH* ssh, byte side)
655671
{
@@ -3535,8 +3551,6 @@ int GenerateKey(byte hashId, byte keyId,
35353551
wc_HashAlg hash;
35363552
enum wc_HashType enmhashId = (enum wc_HashType)hashId;
35373553
byte kPad = 0;
3538-
byte pad = 0;
3539-
byte kSzFlat[LENGTH_SZ];
35403554
int digestSz;
35413555
int ret;
35423556

@@ -3559,17 +3573,14 @@ int GenerateKey(byte hashId, byte keyId,
35593573
/* Data can be define as string and mpint. (see Section 5 of RFC4251).
35603574
* This padding is required in the case of an mpint, but not in the case of
35613575
* a string. */
3562-
if (doKeyPad && (k[0] & 0x80)) kPad = 1;
3563-
c32toa(kSz + kPad, kSzFlat);
3576+
kPad = (byte)((doKeyPad != 0) & (k[0] >> 7));
35643577

35653578
blocks = keySz / digestSz;
35663579
remainder = keySz % digestSz;
35673580

35683581
ret = wc_HashInit(&hash, enmhashId);
35693582
if (ret == WS_SUCCESS)
3570-
ret = HashUpdate(&hash, enmhashId, kSzFlat, LENGTH_SZ);
3571-
if (ret == WS_SUCCESS && kPad)
3572-
ret = HashUpdate(&hash, enmhashId, &pad, 1);
3583+
ret = HashMpintHeader(&hash, enmhashId, kSz, kPad);
35733584
if (ret == WS_SUCCESS)
35743585
ret = HashUpdate(&hash, enmhashId, k, kSz);
35753586
if (ret == WS_SUCCESS)
@@ -3599,10 +3610,7 @@ int GenerateKey(byte hashId, byte keyId,
35993610
curBlock++) {
36003611
ret = wc_HashInit(&hash, enmhashId);
36013612
if (ret != WS_SUCCESS) break;
3602-
ret = HashUpdate(&hash, enmhashId, kSzFlat, LENGTH_SZ);
3603-
if (ret != WS_SUCCESS) break;
3604-
if (kPad)
3605-
ret = HashUpdate(&hash, enmhashId, &pad, 1);
3613+
ret = HashMpintHeader(&hash, enmhashId, kSz, kPad);
36063614
if (ret != WS_SUCCESS) break;
36073615
ret = HashUpdate(&hash, enmhashId, k, kSz);
36083616
if (ret != WS_SUCCESS) break;
@@ -3620,9 +3628,7 @@ int GenerateKey(byte hashId, byte keyId,
36203628
if (ret == WS_SUCCESS)
36213629
ret = wc_HashInit(&hash, enmhashId);
36223630
if (ret == WS_SUCCESS)
3623-
ret = HashUpdate(&hash, enmhashId, kSzFlat, LENGTH_SZ);
3624-
if (ret == WS_SUCCESS && kPad)
3625-
ret = HashUpdate(&hash, enmhashId, &pad, 1);
3631+
ret = HashMpintHeader(&hash, enmhashId, kSz, kPad);
36263632
if (ret == WS_SUCCESS)
36273633
ret = HashUpdate(&hash, enmhashId, k, kSz);
36283634
if (ret == WS_SUCCESS)
@@ -7058,36 +7064,33 @@ static int DoKexInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
70587064
*/
70597065
static int CreateMpint(byte* buf, word32* sz, byte* pad)
70607066
{
7061-
word32 i;
7067+
word32 i, top, back;
70627068

70637069
if (buf == NULL || sz == NULL || pad == NULL) {
70647070
WLOG(WS_LOG_ERROR, "Internal argument error with CreateMpint");
70657071
return WS_BAD_ARGUMENT;
70667072
}
70677073

7068-
if (*sz == 0)
7069-
return WS_SUCCESS;
7070-
70717074
/* check for leading 0's */
70727075
for (i = 0; i < *sz; i++) {
70737076
if (buf[i] != 0x00)
70747077
break;
70757078
}
7076-
/* all-zero buffer encodes as empty mpint per RFC 4251 */
7079+
/* empty or all-zero buffer encodes as empty mpint per RFC 4251 */
70777080
if (i == *sz) {
70787081
*pad = 0;
70797082
*sz = 0;
70807083
return WS_SUCCESS;
70817084
}
70827085

7083-
*pad = (buf[i] & 0x80) ? 1 : 0;
7084-
7085-
/* if padding would be needed and have leading 0's already then do not add
7086-
* extra 0's */
7087-
if (i > 0 && *pad == 1) {
7088-
i = i - 1;
7089-
*pad = 0;
7090-
}
7086+
/* If padding is needed and there is a leading 0 already, keep one 0
7087+
* instead of adding a pad byte. No branch on the MSB, which is secret
7088+
* for K. The leading-zero count still shows in the loop above and the
7089+
* move below; the mpint length depends on it by definition. */
7090+
top = (word32)buf[i] >> 7;
7091+
back = top & (word32)(i > 0);
7092+
i -= back;
7093+
*pad = (byte)(top ^ back);
70917094

70927095
/* if i is still greater than 0 then the buffer needs shifted to remove
70937096
* leading 0's */
@@ -8834,13 +8837,7 @@ static int DoKexDhReply(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
88348837
}
88358838

88368839
if (ret == 0) {
8837-
c32toa(ssh->kSz + kPad, scratchLen);
8838-
ret = HashUpdate(hash, hashId, scratchLen, LENGTH_SZ);
8839-
}
8840-
8841-
if ((ret == 0) && (kPad)) {
8842-
scratchLen[0] = 0;
8843-
ret = HashUpdate(hash, hashId, scratchLen, 1);
8840+
ret = HashMpintHeader(hash, hashId, ssh->kSz, kPad);
88448841
}
88458842

88468843
if (ret == 0) {
@@ -18558,6 +18555,7 @@ int SendKexDhReply(WOLFSSH* ssh)
1855818555
c32toa(fSz + fPad, scratchLen);
1855918556
ret = HashUpdate(hash, hashId, scratchLen, LENGTH_SZ);
1856018557
}
18558+
/* f is public, so branching on its pad is fine. */
1856118559
if ((ret == 0) && (fPad)) {
1856218560
scratchLen[0] = 0;
1856318561
ret = HashUpdate(hash, hashId, scratchLen, 1);
@@ -18571,12 +18569,7 @@ int SendKexDhReply(WOLFSSH* ssh)
1857118569
ret = CreateMpint(ssh->k, &ssh->kSz, &kPad);
1857218570
}
1857318571
if (ret == 0) {
18574-
c32toa(ssh->kSz + kPad, scratchLen);
18575-
ret = HashUpdate(hash, hashId, scratchLen, LENGTH_SZ);
18576-
}
18577-
if ((ret == 0) && (kPad)) {
18578-
scratchLen[0] = 0;
18579-
ret = HashUpdate(hash, hashId, scratchLen, 1);
18572+
ret = HashMpintHeader(hash, hashId, ssh->kSz, kPad);
1858018573
}
1858118574
if (ret == 0) {
1858218575
ret = HashUpdate(hash, hashId, ssh->k, ssh->kSz);
@@ -26051,6 +26044,11 @@ int wolfSSH_TestChannelPutData(WOLFSSH_CHANNEL* channel, byte* data,
2605126044
return ChannelPutData(channel, data, dataSz);
2605226045
}
2605326046

26047+
int wolfSSH_TestCreateMpint(byte* buf, word32* sz, byte* pad)
26048+
{
26049+
return CreateMpint(buf, sz, pad);
26050+
}
26051+
2605426052
int wolfSSH_TestBuildNameList(char* buf, word32 bufSz,
2605526053
const byte* src, word32 srcSz)
2605626054
{

‎tests/unit.c‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10332,6 +10332,63 @@ static int test_ChannelIdRead_deferredWindowAdjust(void)
1033210332

1033310333
#endif /* NO_WOLFSSH_SERVER */
1033410334

10335+
/* CreateMpint() strips leading zeros, keeping one 0 in front of a set MSB
10336+
* instead of asking for a pad byte. */
10337+
static int test_CreateMpint(void)
10338+
{
10339+
static const struct {
10340+
byte in[4];
10341+
word32 inSz;
10342+
byte out[4];
10343+
word32 outSz;
10344+
byte pad;
10345+
} cases[] = {
10346+
{ {0x00}, 1, {0}, 0, 0 },
10347+
{ {0x00, 0x00, 0x00}, 3, {0}, 0, 0 },
10348+
{ {0x7F}, 1, {0x7F}, 1, 0 },
10349+
{ {0x80}, 1, {0x80}, 1, 1 },
10350+
{ {0xFF, 0x01}, 2, {0xFF, 0x01}, 2, 1 },
10351+
{ {0x00, 0x7F}, 2, {0x7F}, 1, 0 },
10352+
{ {0x00, 0x80}, 2, {0x00, 0x80}, 2, 0 },
10353+
{ {0x00, 0x00, 0x7F, 0x01}, 4, {0x7F, 0x01}, 2, 0 },
10354+
{ {0x00, 0x00, 0x80, 0x01}, 4, {0x00, 0x80, 0x01}, 3, 0 },
10355+
{ {0x01, 0x80, 0x00, 0x00}, 4, {0x01, 0x80, 0x00, 0x00}, 4, 0 },
10356+
};
10357+
byte buf[4];
10358+
word32 i, sz;
10359+
byte pad;
10360+
int ret;
10361+
int result = 0;
10362+
10363+
for (i = 0; i < (word32)(sizeof(cases) / sizeof(cases[0])); i++) {
10364+
WMEMCPY(buf, cases[i].in, sizeof(buf));
10365+
sz = cases[i].inSz;
10366+
pad = 0xAA;
10367+
ret = wolfSSH_TestCreateMpint(buf, &sz, &pad);
10368+
if (ret != WS_SUCCESS || sz != cases[i].outSz
10369+
|| pad != cases[i].pad
10370+
|| WMEMCMP(buf, cases[i].out, sz) != 0) {
10371+
printf("CreateMpint: case %u ret=%d sz=%u pad=%u\n",
10372+
i, ret, sz, pad);
10373+
result = -1450;
10374+
}
10375+
}
10376+
10377+
/* An empty input is an empty mpint and still sets pad. */
10378+
sz = 0;
10379+
pad = 0xAA;
10380+
ret = wolfSSH_TestCreateMpint(buf, &sz, &pad);
10381+
if (ret != WS_SUCCESS || sz != 0 || pad != 0)
10382+
result = -1451;
10383+
10384+
if (wolfSSH_TestCreateMpint(NULL, &sz, &pad) != WS_BAD_ARGUMENT
10385+
|| wolfSSH_TestCreateMpint(buf, NULL, &pad) != WS_BAD_ARGUMENT
10386+
|| wolfSSH_TestCreateMpint(buf, &sz, NULL) != WS_BAD_ARGUMENT)
10387+
result = -1452;
10388+
10389+
return result;
10390+
}
10391+
1033510392
/* BuildNameList() returns a C string. On an empty id list it must still
1033610393
* terminate the buffer: SendKexInit() measures the result with WSTRLEN
1033710394
* through AlgoListSz() and copies that many bytes into the KEXINIT. */
@@ -23324,6 +23381,11 @@ int wolfSSH_UnitTest(int argc, char** argv)
2332423381
testResult = testResult || unitResult;
2332523382
#endif /* NO_WOLFSSH_SERVER */
2332623383

23384+
unitResult = test_CreateMpint();
23385+
printf("CreateMpint: %s\n",
23386+
(unitResult == 0 ? "SUCCESS" : "FAILED"));
23387+
testResult = testResult || unitResult;
23388+
2332723389
unitResult = test_BuildNameList_emptySrc();
2332823390
printf("BuildNameList_emptySrc: %s\n",
2332923391
(unitResult == 0 ? "SUCCESS" : "FAILED"));

‎wolfssh/internal.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2096,6 +2096,7 @@ enum WS_MessageIdLimits {
20962096
word32 len, word32* idx);
20972097
WOLFSSH_API int wolfSSH_TestChannelPutData(WOLFSSH_CHANNEL* channel,
20982098
byte* data, word32 dataSz);
2099+
WOLFSSH_API int wolfSSH_TestCreateMpint(byte* buf, word32* sz, byte* pad);
20992100
WOLFSSH_API int wolfSSH_TestBuildNameList(char* buf, word32 bufSz,
21002101
const byte* src, word32 srcSz);
21012102
WOLFSSH_API int wolfSSH_TestDoUserAuthRequest(WOLFSSH* ssh, byte* buf,

0 commit comments

Comments
 (0)