shimun/solo
1
0
Fork 0
Code Issues Pull Requests Releases 1 Wiki Activity

Fix buffer overread in ctap_encode_der_sig()

Browse Source 
Take into account leading zeroes in the size to copy, for both R and S
ingredients of the signature.
Issue was occuring only in cases, when there was a leading zero for the
S part.

Refactor ctap_encode_der_sig():
- add in_ and out_ prefixes to the function arguments
- mark pointers const
- clear out buffer

Tested via simulated device on:
- Fedora 29
- gcc (GCC) 8.2.1 20181215 (Red Hat 8.2.1-6)
- libasan 8.2.1 / 6.fc29
(same machine, as in the related issue description)
by running ctap_test() Python test in a loop for 20 minutes (dev's
counter 400k+). Earlier issue was occuring in first minutes.

Tested on Nucleo32 board, by running the ctap_test() 20 times.

Fixes https://github.com/solokeys/solo/issues/94

Signed-off-by: Szczepan Zalega <szczepan@nitrokey.com>
This commit is contained in:
Szczepan Zalega
2019-02-01 18:55:37 +01:00
parent 37f1790028
commit 449faea7d3
2 changed files with 32 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

52
fido2/ctap.c
View File

@@ -444,36 +444,46 @@ done_rk:
}
int ctap_encode_der_sig(uint8_t * sigbuf, uint8_t * sigder)
/**
*
* @param in_sigbuf IN location to deposit signature (must be 64 bytes)
* @param out_sigder OUT location to deposit der signature (must be 72 bytes)
* @return length of der signature
* // FIXME add tests for maximum and minimum length of the input and output
*/
int ctap_encode_der_sig(const uint8_t * const in_sigbuf, uint8_t * const out_sigder)
{
// Need to caress into dumb der format ..
int i;
int8_t lead_s = 0; // leading zeros
int8_t lead_r = 0;
uint8_t i;
uint8_t lead_s = 0; // leading zeros
uint8_t lead_r = 0;
for (i=0; i < 32; i++)
if (sigbuf[i] == 0) lead_r++;
if (in_sigbuf[i] == 0) lead_r++;
else break;
for (i=0; i < 32; i++)
if (sigbuf[i+32] == 0) lead_s++;
if (in_sigbuf[i+32] == 0) lead_s++;
else break;
int8_t pad_s = ((sigbuf[32 + lead_s] & 0x80) == 0x80);
int8_t pad_r = ((sigbuf[0 + lead_r] & 0x80) == 0x80);
int8_t pad_s = ((in_sigbuf[32 + lead_s] & 0x80) == 0x80);
int8_t pad_r = ((in_sigbuf[0 + lead_r] & 0x80) == 0x80);
sigder[0] = 0x30;
sigder[1] = 0x44 + pad_s + pad_r - lead_s - lead_r;
memset(out_sigder, 0, 72);
out_sigder[0] = 0x30;
out_sigder[1] = 0x44 + pad_s + pad_r - lead_s - lead_r;
sigder[2] = 0x02;
sigder[3 + pad_r] = 0;
sigder[3] = 0x20 + pad_r - lead_r;
memmove(sigder + 4 + pad_r, sigbuf + lead_r, 32);
// R ingredient
out_sigder[2] = 0x02;
out_sigder[3 + pad_r] = 0;
out_sigder[3] = 0x20 + pad_r - lead_r;
memmove(out_sigder + 4 + pad_r, in_sigbuf + lead_r, 32u - lead_r);
// S ingredient
out_sigder[4 + 32 + pad_r - lead_r] = 0x02;
out_sigder[5 + 32 + pad_r + pad_s - lead_r] = 0;
out_sigder[5 + 32 + pad_r - lead_r] = 0x20 + pad_s - lead_s;
memmove(out_sigder + 6 + 32 + pad_r + pad_s - lead_r, in_sigbuf + 32u + lead_s, 32u - lead_s);
sigder[4 + 32 + pad_r - lead_r] = 0x02;
sigder[5 + 32 + pad_r + pad_s - lead_r] = 0;
sigder[5 + 32 + pad_r - lead_r] = 0x20 + pad_s - lead_s;
memmove(sigder + 6 + 32 + pad_r + pad_s - lead_r, sigbuf + 32 + lead_s, 32);
//
return 0x46 + pad_s + pad_r - lead_r - lead_s;
}
@@ -481,8 +491,8 @@ int ctap_encode_der_sig(uint8_t * sigbuf, uint8_t * sigder)
// @data data to hash before signature
// @clientDataHash for signature
// @tmp buffer for hash. (can be same as data if data >= 32 bytes)
// @sigbuf location to deposit signature (must be 64 bytes)
// @sigder location to deposit der signature (must be 72 bytes)
// @sigbuf OUT location to deposit signature (must be 64 bytes)
// @sigder OUT location to deposit der signature (must be 72 bytes)
// @return length of der signature
int ctap_calculate_signature(uint8_t * data, int datalen, uint8_t * clientDataHash, uint8_t * hashbuf, uint8_t * sigbuf, uint8_t * sigder)
{