From 449faea7d3865aed6eb6311b7c00f0ea84dcd535 Mon Sep 17 00:00:00 2001 From: Szczepan Zalega Date: Fri, 1 Feb 2019 18:55:37 +0100 Subject: [PATCH] Fix buffer overread in ctap_encode_der_sig() 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 --- fido2/ctap.c | 52 +++++++++++++++++++++++++++++++--------------------- fido2/ctap.h | 2 +- 2 files changed, 32 insertions(+), 22 deletions(-) diff --git a/fido2/ctap.c b/fido2/ctap.c index 6b644f3..ce53695 100644 --- a/fido2/ctap.c +++ b/fido2/ctap.c @@ -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) { diff --git a/fido2/ctap.h b/fido2/ctap.h index 7448b44..52716db 100644 --- a/fido2/ctap.h +++ b/fido2/ctap.h @@ -279,7 +279,7 @@ uint8_t ctap_request(uint8_t * pkt_raw, int length, CTAP_RESPONSE * resp); // Encodes R,S signature to 2 der sequence of two integers. Sigder must be at least 72 bytes. // @return length of der signature -int ctap_encode_der_sig(uint8_t * sigbuf, uint8_t * sigder); +int ctap_encode_der_sig(uint8_t const * const in_sigbuf, uint8_t * const out_sigder); // Run ctap related power-up procedures (init pinToken, generate shared secret) void ctap_init();