From 91429c37b5821c322828d3195ccbaa186bd3056e Mon Sep 17 00:00:00 2001 From: stevenwdv Date: Thu, 14 Oct 2021 13:12:42 +0200 Subject: [PATCH 1/2] Fix parsing errors in ctap_parse_hmac_secret: saltEnc type was not checked causing assertion to fail; parameters could be omitted by duplicating known keys, causing parsed_count to increase anyway. --- fido2/ctap_parse.c | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/fido2/ctap_parse.c b/fido2/ctap_parse.c index 115bf4ea..c91d0270 100644 --- a/fido2/ctap_parse.c +++ b/fido2/ctap_parse.c @@ -561,7 +561,7 @@ uint8_t ctap_parse_hmac_secret(CborValue * val, CTAP_hmac_secret * hs) { size_t map_length; size_t salt_len; - uint8_t parsed_count = 0; + size_t cose_key_parsed = 0, salt_enc_parsed = 0, salt_auth_parsed = 0; int key; int ret; unsigned int i; @@ -595,26 +595,34 @@ uint8_t ctap_parse_hmac_secret(CborValue * val, CTAP_hmac_secret * hs) switch(key) { case EXT_HMAC_SECRET_COSE_KEY: + ++cose_key_parsed; ret = parse_cose_key(&map, &hs->keyAgreement); check_retr(ret); - parsed_count++; break; case EXT_HMAC_SECRET_SALT_ENC: + ++salt_enc_parsed; salt_len = 64; - ret = cbor_value_copy_byte_string(&map, hs->saltEnc, &salt_len, NULL); - if ((salt_len != 32 && salt_len != 64) || ret == CborErrorOutOfMemory) + if (cbor_value_get_type(&map) == CborByteStringType) { - return CTAP1_ERR_INVALID_LENGTH; + ret = cbor_value_copy_byte_string(&map, hs->saltEnc, &salt_len, NULL); + if ((salt_len != 32 && salt_len != 64) || ret == CborErrorOutOfMemory) + { + return CTAP1_ERR_INVALID_LENGTH; + } + check_ret(ret); + } + else + { + printf2(TAG_ERR, "error, CborByteStringType expected\r\n"); + return CTAP2_ERR_INVALID_CBOR_TYPE; } - check_ret(ret); hs->saltLen = salt_len; - parsed_count++; break; case EXT_HMAC_SECRET_SALT_AUTH: + ++salt_auth_parsed; salt_len = 32; ret = cbor_value_copy_byte_string(&map, hs->saltAuth, &salt_len, NULL); check_ret(ret); - parsed_count++; break; } @@ -622,9 +630,16 @@ uint8_t ctap_parse_hmac_secret(CborValue * val, CTAP_hmac_secret * hs) check_ret(ret); } - if (parsed_count != 3) + if (cose_key_parsed > 1 || salt_enc_parsed > 1 || salt_auth_parsed > 1) + { + printf2(TAG_ERR, "ctap_parse_hmac_secret duplicate key.\r\n"); + return CTAP2_ERR_INVALID_CBOR; + } + + if (!(cose_key_parsed && salt_enc_parsed && salt_auth_parsed)) { - printf2(TAG_ERR, "ctap_parse_hmac_secret missing parameter. Got %d.\r\n", parsed_count); + printf2(TAG_ERR, "ctap_parse_hmac_secret missing parameter. Got %d.\r\n", + cose_key_parsed + salt_enc_parsed + salt_auth_parsed); return CTAP2_ERR_MISSING_PARAMETER; } From 38e71821b55daba410c3ae836322c07c8d4d4c58 Mon Sep 17 00:00:00 2001 From: stevenwdv Date: Tue, 19 Oct 2021 21:55:43 +0200 Subject: [PATCH 2/2] Fixed other type checking bug in ctap_parse_hmac_secret --- fido2/ctap_parse.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/fido2/ctap_parse.c b/fido2/ctap_parse.c index c91d0270..e1d26e5d 100644 --- a/fido2/ctap_parse.c +++ b/fido2/ctap_parse.c @@ -621,8 +621,16 @@ uint8_t ctap_parse_hmac_secret(CborValue * val, CTAP_hmac_secret * hs) case EXT_HMAC_SECRET_SALT_AUTH: ++salt_auth_parsed; salt_len = 32; - ret = cbor_value_copy_byte_string(&map, hs->saltAuth, &salt_len, NULL); - check_ret(ret); + if (cbor_value_get_type(&map) == CborByteStringType) + { + ret = cbor_value_copy_byte_string(&map, hs->saltAuth, &salt_len, NULL); + check_ret(ret); + } + else + { + printf2(TAG_ERR, "error, CborByteStringType expected\r\n"); + return CTAP2_ERR_INVALID_CBOR_TYPE; + } break; }