Skip to content

Commit 4107baa

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 96fdbe5 commit 4107baa

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
{
@@ -3511,8 +3527,6 @@ int GenerateKey(byte hashId, byte keyId,
35113527
wc_HashAlg hash;
35123528
enum wc_HashType enmhashId = (enum wc_HashType)hashId;
35133529
byte kPad = 0;
3514-
byte pad = 0;
3515-
byte kSzFlat[LENGTH_SZ];
35163530
int digestSz;
35173531
int ret;
35183532

@@ -3535,17 +3549,14 @@ int GenerateKey(byte hashId, byte keyId,
35353549
/* Data can be define as string and mpint. (see Section 5 of RFC4251).
35363550
* This padding is required in the case of an mpint, but not in the case of
35373551
* a string. */
3538-
if (doKeyPad && (k[0] & 0x80)) kPad = 1;
3539-
c32toa(kSz + kPad, kSzFlat);
3552+
kPad = (byte)((doKeyPad != 0) & (k[0] >> 7));
35403553

35413554
blocks = keySz / digestSz;
35423555
remainder = keySz % digestSz;
35433556

35443557
ret = wc_HashInit(&hash, enmhashId);
35453558
if (ret == WS_SUCCESS)
3546-
ret = HashUpdate(&hash, enmhashId, kSzFlat, LENGTH_SZ);
3547-
if (ret == WS_SUCCESS && kPad)
3548-
ret = HashUpdate(&hash, enmhashId, &pad, 1);
3559+
ret = HashMpintHeader(&hash, enmhashId, kSz, kPad);
35493560
if (ret == WS_SUCCESS)
35503561
ret = HashUpdate(&hash, enmhashId, k, kSz);
35513562
if (ret == WS_SUCCESS)
@@ -3575,10 +3586,7 @@ int GenerateKey(byte hashId, byte keyId,
35753586
curBlock++) {
35763587
ret = wc_HashInit(&hash, enmhashId);
35773588
if (ret != WS_SUCCESS) break;
3578-
ret = HashUpdate(&hash, enmhashId, kSzFlat, LENGTH_SZ);
3579-
if (ret != WS_SUCCESS) break;
3580-
if (kPad)
3581-
ret = HashUpdate(&hash, enmhashId, &pad, 1);
3589+
ret = HashMpintHeader(&hash, enmhashId, kSz, kPad);
35823590
if (ret != WS_SUCCESS) break;
35833591
ret = HashUpdate(&hash, enmhashId, k, kSz);
35843592
if (ret != WS_SUCCESS) break;
@@ -3596,9 +3604,7 @@ int GenerateKey(byte hashId, byte keyId,
35963604
if (ret == WS_SUCCESS)
35973605
ret = wc_HashInit(&hash, enmhashId);
35983606
if (ret == WS_SUCCESS)
3599-
ret = HashUpdate(&hash, enmhashId, kSzFlat, LENGTH_SZ);
3600-
if (ret == WS_SUCCESS && kPad)
3601-
ret = HashUpdate(&hash, enmhashId, &pad, 1);
3607+
ret = HashMpintHeader(&hash, enmhashId, kSz, kPad);
36023608
if (ret == WS_SUCCESS)
36033609
ret = HashUpdate(&hash, enmhashId, k, kSz);
36043610
if (ret == WS_SUCCESS)
@@ -6961,36 +6967,33 @@ static int DoKexInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
69616967
*/
69626968
static int CreateMpint(byte* buf, word32* sz, byte* pad)
69636969
{
6964-
word32 i;
6970+
word32 i, top, back;
69656971

69666972
if (buf == NULL || sz == NULL || pad == NULL) {
69676973
WLOG(WS_LOG_ERROR, "Internal argument error with CreateMpint");
69686974
return WS_BAD_ARGUMENT;
69696975
}
69706976

6971-
if (*sz == 0)
6972-
return WS_SUCCESS;
6973-
69746977
/* check for leading 0's */
69756978
for (i = 0; i < *sz; i++) {
69766979
if (buf[i] != 0x00)
69776980
break;
69786981
}
6979-
/* all-zero buffer encodes as empty mpint per RFC 4251 */
6982+
/* empty or all-zero buffer encodes as empty mpint per RFC 4251 */
69806983
if (i == *sz) {
69816984
*pad = 0;
69826985
*sz = 0;
69836986
return WS_SUCCESS;
69846987
}
69856988

6986-
*pad = (buf[i] & 0x80) ? 1 : 0;
6987-
6988-
/* if padding would be needed and have leading 0's already then do not add
6989-
* extra 0's */
6990-
if (i > 0 && *pad == 1) {
6991-
i = i - 1;
6992-
*pad = 0;
6993-
}
6989+
/* If padding is needed and there is a leading 0 already, keep one 0
6990+
* instead of adding a pad byte. No branch on the MSB, which is secret
6991+
* for K. The leading-zero count still shows in the loop above and the
6992+
* move below; the mpint length depends on it by definition. */
6993+
top = (word32)buf[i] >> 7;
6994+
back = top & (word32)(i > 0);
6995+
i -= back;
6996+
*pad = (byte)(top ^ back);
69946997

69956998
/* if i is still greater than 0 then the buffer needs shifted to remove
69966999
* leading 0's */
@@ -8733,13 +8736,7 @@ static int DoKexDhReply(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
87338736
}
87348737

87358738
if (ret == 0) {
8736-
c32toa(ssh->kSz + kPad, scratchLen);
8737-
ret = HashUpdate(hash, hashId, scratchLen, LENGTH_SZ);
8738-
}
8739-
8740-
if ((ret == 0) && (kPad)) {
8741-
scratchLen[0] = 0;
8742-
ret = HashUpdate(hash, hashId, scratchLen, 1);
8739+
ret = HashMpintHeader(hash, hashId, ssh->kSz, kPad);
87438740
}
87448741

87458742
if (ret == 0) {
@@ -18351,6 +18348,7 @@ int SendKexDhReply(WOLFSSH* ssh)
1835118348
c32toa(fSz + fPad, scratchLen);
1835218349
ret = HashUpdate(hash, hashId, scratchLen, LENGTH_SZ);
1835318350
}
18351+
/* f is public, so branching on its pad is fine. */
1835418352
if ((ret == 0) && (fPad)) {
1835518353
scratchLen[0] = 0;
1835618354
ret = HashUpdate(hash, hashId, scratchLen, 1);
@@ -18364,12 +18362,7 @@ int SendKexDhReply(WOLFSSH* ssh)
1836418362
ret = CreateMpint(ssh->k, &ssh->kSz, &kPad);
1836518363
}
1836618364
if (ret == 0) {
18367-
c32toa(ssh->kSz + kPad, scratchLen);
18368-
ret = HashUpdate(hash, hashId, scratchLen, LENGTH_SZ);
18369-
}
18370-
if ((ret == 0) && (kPad)) {
18371-
scratchLen[0] = 0;
18372-
ret = HashUpdate(hash, hashId, scratchLen, 1);
18365+
ret = HashMpintHeader(hash, hashId, ssh->kSz, kPad);
1837318366
}
1837418367
if (ret == 0) {
1837518368
ret = HashUpdate(hash, hashId, ssh->k, ssh->kSz);
@@ -25831,6 +25824,11 @@ int wolfSSH_TestChannelPutData(WOLFSSH_CHANNEL* channel, byte* data,
2583125824
return ChannelPutData(channel, data, dataSz);
2583225825
}
2583325826

25827+
int wolfSSH_TestCreateMpint(byte* buf, word32* sz, byte* pad)
25828+
{
25829+
return CreateMpint(buf, sz, pad);
25830+
}
25831+
2583425832
int wolfSSH_TestBuildNameList(char* buf, word32 bufSz,
2583525833
const byte* src, word32 srcSz)
2583625834
{

‎tests/unit.c‎

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

1023510235
#endif /* NO_WOLFSSH_SERVER */
1023610236

10237+
/* CreateMpint() strips leading zeros, keeping one 0 in front of a set MSB
10238+
* instead of asking for a pad byte. */
10239+
static int test_CreateMpint(void)
10240+
{
10241+
static const struct {
10242+
byte in[4];
10243+
word32 inSz;
10244+
byte out[4];
10245+
word32 outSz;
10246+
byte pad;
10247+
} cases[] = {
10248+
{ {0x00}, 1, {0}, 0, 0 },
10249+
{ {0x00, 0x00, 0x00}, 3, {0}, 0, 0 },
10250+
{ {0x7F}, 1, {0x7F}, 1, 0 },
10251+
{ {0x80}, 1, {0x80}, 1, 1 },
10252+
{ {0xFF, 0x01}, 2, {0xFF, 0x01}, 2, 1 },
10253+
{ {0x00, 0x7F}, 2, {0x7F}, 1, 0 },
10254+
{ {0x00, 0x80}, 2, {0x00, 0x80}, 2, 0 },
10255+
{ {0x00, 0x00, 0x7F, 0x01}, 4, {0x7F, 0x01}, 2, 0 },
10256+
{ {0x00, 0x00, 0x80, 0x01}, 4, {0x00, 0x80, 0x01}, 3, 0 },
10257+
{ {0x01, 0x80, 0x00, 0x00}, 4, {0x01, 0x80, 0x00, 0x00}, 4, 0 },
10258+
};
10259+
byte buf[4];
10260+
word32 i, sz;
10261+
byte pad;
10262+
int ret;
10263+
int result = 0;
10264+
10265+
for (i = 0; i < (word32)(sizeof(cases) / sizeof(cases[0])); i++) {
10266+
WMEMCPY(buf, cases[i].in, sizeof(buf));
10267+
sz = cases[i].inSz;
10268+
pad = 0xAA;
10269+
ret = wolfSSH_TestCreateMpint(buf, &sz, &pad);
10270+
if (ret != WS_SUCCESS || sz != cases[i].outSz
10271+
|| pad != cases[i].pad
10272+
|| WMEMCMP(buf, cases[i].out, sz) != 0) {
10273+
printf("CreateMpint: case %u ret=%d sz=%u pad=%u\n",
10274+
i, ret, sz, pad);
10275+
result = -1450;
10276+
}
10277+
}
10278+
10279+
/* An empty input is an empty mpint and still sets pad. */
10280+
sz = 0;
10281+
pad = 0xAA;
10282+
ret = wolfSSH_TestCreateMpint(buf, &sz, &pad);
10283+
if (ret != WS_SUCCESS || sz != 0 || pad != 0)
10284+
result = -1451;
10285+
10286+
if (wolfSSH_TestCreateMpint(NULL, &sz, &pad) != WS_BAD_ARGUMENT
10287+
|| wolfSSH_TestCreateMpint(buf, NULL, &pad) != WS_BAD_ARGUMENT
10288+
|| wolfSSH_TestCreateMpint(buf, &sz, NULL) != WS_BAD_ARGUMENT)
10289+
result = -1452;
10290+
10291+
return result;
10292+
}
10293+
1023710294
/* BuildNameList() returns a C string. On an empty id list it must still
1023810295
* terminate the buffer: SendKexInit() measures the result with WSTRLEN
1023910296
* through AlgoListSz() and copies that many bytes into the KEXINIT. */
@@ -23221,6 +23278,11 @@ int wolfSSH_UnitTest(int argc, char** argv)
2322123278
testResult = testResult || unitResult;
2322223279
#endif /* NO_WOLFSSH_SERVER */
2322323280

23281+
unitResult = test_CreateMpint();
23282+
printf("CreateMpint: %s\n",
23283+
(unitResult == 0 ? "SUCCESS" : "FAILED"));
23284+
testResult = testResult || unitResult;
23285+
2322423286
unitResult = test_BuildNameList_emptySrc();
2322523287
printf("BuildNameList_emptySrc: %s\n",
2322623288
(unitResult == 0 ? "SUCCESS" : "FAILED"));

‎wolfssh/internal.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2088,6 +2088,7 @@ enum WS_MessageIdLimits {
20882088
word32 len, word32* idx);
20892089
WOLFSSH_API int wolfSSH_TestChannelPutData(WOLFSSH_CHANNEL* channel,
20902090
byte* data, word32 dataSz);
2091+
WOLFSSH_API int wolfSSH_TestCreateMpint(byte* buf, word32* sz, byte* pad);
20912092
WOLFSSH_API int wolfSSH_TestBuildNameList(char* buf, word32 bufSz,
20922093
const byte* src, word32 srcSz);
20932094
WOLFSSH_API int wolfSSH_TestDoUserAuthRequest(WOLFSSH* ssh, byte* buf,

0 commit comments

Comments
 (0)