Skip to content

Hash the pad byte of K without a branch #1283

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ejohnstown wants to merge 1 commit into
base: master
Choose a base branch
from
+101 −40
Open

Hash the pad byte of K without a branch #1283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 38 additions & 40 deletions src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,22 @@ static int HashUpdate(wc_HashAlg* hash, enum wc_HashType type,
}


/* Hash an mpint's length and optional 0 pad byte as one update of
* LENGTH_SZ + pad bytes. For K the pad is secret: this avoids a branch on
* it and a zero-length update, which some hash ports reject. The hashed
* length still depends on the pad. */
static INLINE int HashMpintHeader(wc_HashAlg* hash, enum wc_HashType type,
word32 sz, byte pad)
{
byte hdr[LENGTH_SZ + 1];

pad &= 1;
c32toa(sz + pad, hdr);
hdr[LENGTH_SZ] = 0;
return HashUpdate(hash, type, hdr, LENGTH_SZ + pad);
}


/* returns WS_SUCCESS on success */
static INLINE int HighwaterCheck(WOLFSSH* ssh, byte side)
{
Expand Down Expand Up @@ -3511,8 +3527,6 @@ int GenerateKey(byte hashId, byte keyId,
wc_HashAlg hash;
enum wc_HashType enmhashId = (enum wc_HashType)hashId;
byte kPad = 0;
byte pad = 0;
byte kSzFlat[LENGTH_SZ];
int digestSz;
int ret;

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

blocks = keySz / digestSz;
remainder = keySz % digestSz;

ret = wc_HashInit(&hash, enmhashId);
if (ret == WS_SUCCESS)
ret = HashUpdate(&hash, enmhashId, kSzFlat, LENGTH_SZ);
if (ret == WS_SUCCESS && kPad)
ret = HashUpdate(&hash, enmhashId, &pad, 1);
ret = HashMpintHeader(&hash, enmhashId, kSz, kPad);
if (ret == WS_SUCCESS)
ret = HashUpdate(&hash, enmhashId, k, kSz);
if (ret == WS_SUCCESS)
Expand Down Expand Up @@ -3575,10 +3586,7 @@ int GenerateKey(byte hashId, byte keyId,
curBlock++) {
ret = wc_HashInit(&hash, enmhashId);
if (ret != WS_SUCCESS) break;
ret = HashUpdate(&hash, enmhashId, kSzFlat, LENGTH_SZ);
if (ret != WS_SUCCESS) break;
if (kPad)
ret = HashUpdate(&hash, enmhashId, &pad, 1);
ret = HashMpintHeader(&hash, enmhashId, kSz, kPad);
if (ret != WS_SUCCESS) break;
ret = HashUpdate(&hash, enmhashId, k, kSz);
if (ret != WS_SUCCESS) break;
Expand All @@ -3596,9 +3604,7 @@ int GenerateKey(byte hashId, byte keyId,
if (ret == WS_SUCCESS)
ret = wc_HashInit(&hash, enmhashId);
if (ret == WS_SUCCESS)
ret = HashUpdate(&hash, enmhashId, kSzFlat, LENGTH_SZ);
if (ret == WS_SUCCESS && kPad)
ret = HashUpdate(&hash, enmhashId, &pad, 1);
ret = HashMpintHeader(&hash, enmhashId, kSz, kPad);
if (ret == WS_SUCCESS)
ret = HashUpdate(&hash, enmhashId, k, kSz);
if (ret == WS_SUCCESS)
Expand Down Expand Up @@ -6961,36 +6967,33 @@ static int DoKexInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
*/
static int CreateMpint(byte* buf, word32* sz, byte* pad)
{
word32 i;
word32 i, top, back;

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

if (*sz == 0)
return WS_SUCCESS;

/* check for leading 0's */
for (i = 0; i < *sz; i++) {
if (buf[i] != 0x00)
break;
}
/* all-zero buffer encodes as empty mpint per RFC 4251 */
/* empty or all-zero buffer encodes as empty mpint per RFC 4251 */
if (i == *sz) {
*pad = 0;
*sz = 0;
return WS_SUCCESS;
}

*pad = (buf[i] & 0x80) ? 1 : 0;

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

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

if (ret == 0) {
c32toa(ssh->kSz + kPad, scratchLen);
ret = HashUpdate(hash, hashId, scratchLen, LENGTH_SZ);
}

if ((ret == 0) && (kPad)) {
scratchLen[0] = 0;
ret = HashUpdate(hash, hashId, scratchLen, 1);
ret = HashMpintHeader(hash, hashId, ssh->kSz, kPad);
}

if (ret == 0) {
Expand Down Expand Up @@ -18351,6 +18348,7 @@ int SendKexDhReply(WOLFSSH* ssh)
c32toa(fSz + fPad, scratchLen);
ret = HashUpdate(hash, hashId, scratchLen, LENGTH_SZ);
}
/* f is public, so branching on its pad is fine. */
if ((ret == 0) && (fPad)) {
scratchLen[0] = 0;
ret = HashUpdate(hash, hashId, scratchLen, 1);
Expand All @@ -18364,12 +18362,7 @@ int SendKexDhReply(WOLFSSH* ssh)
ret = CreateMpint(ssh->k, &ssh->kSz, &kPad);
}
if (ret == 0) {
c32toa(ssh->kSz + kPad, scratchLen);
ret = HashUpdate(hash, hashId, scratchLen, LENGTH_SZ);
}
if ((ret == 0) && (kPad)) {
scratchLen[0] = 0;
ret = HashUpdate(hash, hashId, scratchLen, 1);
ret = HashMpintHeader(hash, hashId, ssh->kSz, kPad);
}
if (ret == 0) {
ret = HashUpdate(hash, hashId, ssh->k, ssh->kSz);
Expand Down Expand Up @@ -25831,6 +25824,11 @@ int wolfSSH_TestChannelPutData(WOLFSSH_CHANNEL* channel, byte* data,
return ChannelPutData(channel, data, dataSz);
}

int wolfSSH_TestCreateMpint(byte* buf, word32* sz, byte* pad)
{
return CreateMpint(buf, sz, pad);
}

int wolfSSH_TestBuildNameList(char* buf, word32 bufSz,
const byte* src, word32 srcSz)
{
Expand Down
62 changes: 62 additions & 0 deletions tests/unit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10234,6 +10234,63 @@ static int test_ChannelIdRead_deferredWindowAdjust(void)

#endif /* NO_WOLFSSH_SERVER */

/* CreateMpint() strips leading zeros, keeping one 0 in front of a set MSB
* instead of asking for a pad byte. */
static int test_CreateMpint(void)
{
static const struct {
byte in[4];
word32 inSz;
byte out[4];
word32 outSz;
byte pad;
} cases[] = {
{ {0x00}, 1, {0}, 0, 0 },
{ {0x00, 0x00, 0x00}, 3, {0}, 0, 0 },
{ {0x7F}, 1, {0x7F}, 1, 0 },
{ {0x80}, 1, {0x80}, 1, 1 },
{ {0xFF, 0x01}, 2, {0xFF, 0x01}, 2, 1 },
{ {0x00, 0x7F}, 2, {0x7F}, 1, 0 },
{ {0x00, 0x80}, 2, {0x00, 0x80}, 2, 0 },
{ {0x00, 0x00, 0x7F, 0x01}, 4, {0x7F, 0x01}, 2, 0 },
{ {0x00, 0x00, 0x80, 0x01}, 4, {0x00, 0x80, 0x01}, 3, 0 },
{ {0x01, 0x80, 0x00, 0x00}, 4, {0x01, 0x80, 0x00, 0x00}, 4, 0 },
};
byte buf[4];
word32 i, sz;
byte pad;
int ret;
int result = 0;

for (i = 0; i < (word32)(sizeof(cases) / sizeof(cases[0])); i++) {
WMEMCPY(buf, cases[i].in, sizeof(buf));
sz = cases[i].inSz;
pad = 0xAA;
ret = wolfSSH_TestCreateMpint(buf, &sz, &pad);
if (ret != WS_SUCCESS || sz != cases[i].outSz
|| pad != cases[i].pad
|| WMEMCMP(buf, cases[i].out, sz) != 0) {
printf("CreateMpint: case %u ret=%d sz=%u pad=%u\n",
i, ret, sz, pad);
result = -1450;
}
}

/* An empty input is an empty mpint and still sets pad. */
sz = 0;
pad = 0xAA;
ret = wolfSSH_TestCreateMpint(buf, &sz, &pad);
if (ret != WS_SUCCESS || sz != 0 || pad != 0)
result = -1451;

if (wolfSSH_TestCreateMpint(NULL, &sz, &pad) != WS_BAD_ARGUMENT
|| wolfSSH_TestCreateMpint(buf, NULL, &pad) != WS_BAD_ARGUMENT
|| wolfSSH_TestCreateMpint(buf, &sz, NULL) != WS_BAD_ARGUMENT)
result = -1452;

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. */
Expand Down Expand Up @@ -23221,6 +23278,11 @@ int wolfSSH_UnitTest(int argc, char** argv)
testResult = testResult || unitResult;
#endif /* NO_WOLFSSH_SERVER */

unitResult = test_CreateMpint();
printf("CreateMpint: %s\n",
(unitResult == 0 ? "SUCCESS" : "FAILED"));
testResult = testResult || unitResult;

unitResult = test_BuildNameList_emptySrc();
printf("BuildNameList_emptySrc: %s\n",
(unitResult == 0 ? "SUCCESS" : "FAILED"));
Expand Down
1 change: 1 addition & 0 deletions wolfssh/internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2088,6 +2088,7 @@ enum WS_MessageIdLimits {
word32 len, word32* idx);
WOLFSSH_API int wolfSSH_TestChannelPutData(WOLFSSH_CHANNEL* channel,
byte* data, word32 dataSz);
WOLFSSH_API int wolfSSH_TestCreateMpint(byte* buf, word32* sz, byte* pad);
WOLFSSH_API int wolfSSH_TestBuildNameList(char* buf, word32 bufSz,
const byte* src, word32 srcSz);
WOLFSSH_API int wolfSSH_TestDoUserAuthRequest(WOLFSSH* ssh, byte* buf,
Expand Down
Loading