From 141e13373fffb631d055fe8197c6b90151f5d856 Mon Sep 17 00:00:00 2001 From: viveks Date: Tue, 6 Feb 2024 13:19:27 +0530 Subject: [PATCH 1/2] Change attstmt to handle x5c array/list; update APIs too --- src/cbor.c | 28 ++++++++++++++++++--- src/cred.c | 65 ++++++++++++++++++++++++++++++++++++++++++------ src/export.gnu | 3 +++ src/export.llvm | 3 +++ src/export.msvc | 3 +++ src/fido.h | 3 +++ src/fido/types.h | 12 ++++----- 7 files changed, 100 insertions(+), 17 deletions(-) diff --git a/src/cbor.c b/src/cbor.c index ab99b34d..cd809f8c 100644 --- a/src/cbor.c +++ b/src/cbor.c @@ -1386,12 +1386,32 @@ cbor_decode_assert_authdata(const cbor_item_t *item, fido_blob_t *authdata_cbor, static int decode_x5c(const cbor_item_t *item, void *arg) { - fido_blob_t *x5c = arg; + fido_blob_array_t *x5c = arg; + fido_blob_t *list_ptr = NULL; + fido_blob_t x5c_blob; - if (x5c->len) - return (0); /* ignore */ + memset(&x5c_blob, 0, sizeof(x5c_blob)); - return (fido_blob_decode(item, x5c)); + if (fido_blob_decode(item, &x5c_blob) < 0) { + fido_log_debug("%s: fido_blob_decode", __func__); + return (-1); + } + + if (x5c->len == SIZE_MAX) { + fido_blob_reset(&x5c_blob); + return (-1); + } + + if ((list_ptr = recallocarray(x5c->ptr, x5c->len, + x5c->len + 1, sizeof(x5c_blob))) == NULL) { + fido_blob_reset(&x5c_blob); + return (-1); + } + + list_ptr[x5c->len++] = x5c_blob; + x5c->ptr = list_ptr; + + return (0); } static int diff --git a/src/cred.c b/src/cred.c index 4a7a7257..c5d2be90 100644 --- a/src/cred.c +++ b/src/cred.c @@ -284,15 +284,21 @@ verify_attstmt(const fido_blob_t *dgst, const fido_attstmt_t *attstmt) EVP_PKEY *pkey = NULL; int ok = -1; - /* openssl needs ints */ - if (attstmt->x5c.len > INT_MAX) { + if (!attstmt->x5c.len) { fido_log_debug("%s: x5c.len=%zu", __func__, attstmt->x5c.len); return (-1); } + /* openssl needs ints */ + if (attstmt->x5c.ptr[0].len > INT_MAX) { + fido_log_debug("%s: x5c[0].len=%zu", __func__, + attstmt->x5c.ptr[0].len); + return (-1); + } + /* fetch key from x509 */ - if ((rawcert = BIO_new_mem_buf(attstmt->x5c.ptr, - (int)attstmt->x5c.len)) == NULL || + if ((rawcert = BIO_new_mem_buf(attstmt->x5c.ptr[0].ptr, + (int)attstmt->x5c.ptr[0].len)) == NULL || (cert = d2i_X509_bio(rawcert, NULL)) == NULL || (pkey = X509_get_pubkey(cert)) == NULL) { fido_log_debug("%s: x509 key", __func__); @@ -543,7 +549,7 @@ fido_cred_clean_attstmt(fido_attstmt_t *attstmt) fido_blob_reset(&attstmt->certinfo); fido_blob_reset(&attstmt->pubarea); fido_blob_reset(&attstmt->cbor); - fido_blob_reset(&attstmt->x5c); + fido_free_blob_array(&attstmt->x5c); fido_blob_reset(&attstmt->sig); memset(attstmt, 0, sizeof(*attstmt)); @@ -688,9 +694,30 @@ fido_cred_set_id(fido_cred_t *cred, const unsigned char *ptr, size_t len) int fido_cred_set_x509(fido_cred_t *cred, const unsigned char *ptr, size_t len) { - if (fido_blob_set(&cred->attstmt.x5c, ptr, len) < 0) + fido_blob_t x5c_blob; + fido_blob_t *list_ptr = NULL; + + memset(&x5c_blob, 0, sizeof(x5c_blob)); + fido_free_blob_array(&cred->attstmt.x5c); + + if (fido_blob_set(&x5c_blob, ptr, len) < 0) return (FIDO_ERR_INVALID_ARGUMENT); + if (cred->attstmt.x5c.len == SIZE_MAX) { + fido_blob_reset(&x5c_blob); + return (FIDO_ERR_INVALID_ARGUMENT); + } + + if ((list_ptr = recallocarray(cred->attstmt.x5c.ptr, + cred->attstmt.x5c.len, cred->attstmt.x5c.len + 1, + sizeof(x5c_blob))) == NULL) { + fido_blob_reset(&x5c_blob); + return (FIDO_ERR_INTERNAL); + } + + list_ptr[cred->attstmt.x5c.len++] = x5c_blob; + cred->attstmt.x5c.ptr = list_ptr; + return (FIDO_OK); } @@ -1030,15 +1057,39 @@ fido_cred_clientdata_hash_len(const fido_cred_t *cred) const unsigned char * fido_cred_x5c_ptr(const fido_cred_t *cred) { - return (cred->attstmt.x5c.ptr); + return (fido_cred_x5c_list_ptr(cred, 0)); } size_t fido_cred_x5c_len(const fido_cred_t *cred) +{ + return (fido_cred_x5c_list_len(cred, 0)); +} + +size_t +fido_cred_x5c_list_count(const fido_cred_t *cred) { return (cred->attstmt.x5c.len); } +const unsigned char * +fido_cred_x5c_list_ptr(const fido_cred_t *cred, size_t i) +{ + if (i >= cred->attstmt.x5c.len) + return (NULL); + + return (cred->attstmt.x5c.ptr[i].ptr); +} + +size_t +fido_cred_x5c_list_len(const fido_cred_t *cred, size_t i) +{ + if (i >= cred->attstmt.x5c.len) + return (0); + + return (cred->attstmt.x5c.ptr[i].len); +} + const unsigned char * fido_cred_sig_ptr(const fido_cred_t *cred) { diff --git a/src/export.gnu b/src/export.gnu index ea6ca7d3..8fcb6d4d 100644 --- a/src/export.gnu +++ b/src/export.gnu @@ -196,6 +196,9 @@ fido_cred_verify; fido_cred_verify_self; fido_cred_x5c_len; + fido_cred_x5c_list_count; + fido_cred_x5c_list_len; + fido_cred_x5c_list_ptr; fido_cred_x5c_ptr; fido_dev_build; fido_dev_cancel; diff --git a/src/export.llvm b/src/export.llvm index 2a923814..ddf05a48 100644 --- a/src/export.llvm +++ b/src/export.llvm @@ -194,6 +194,9 @@ _fido_cred_user_name _fido_cred_verify _fido_cred_verify_self _fido_cred_x5c_len +_fido_cred_x5c_list_count +_fido_cred_x5c_list_len +_fido_cred_x5c_list_ptr _fido_cred_x5c_ptr _fido_dev_build _fido_dev_cancel diff --git a/src/export.msvc b/src/export.msvc index c5b2edc0..7ad41713 100644 --- a/src/export.msvc +++ b/src/export.msvc @@ -195,6 +195,9 @@ fido_cred_user_name fido_cred_verify fido_cred_verify_self fido_cred_x5c_len +fido_cred_x5c_list_count +fido_cred_x5c_list_len +fido_cred_x5c_list_ptr fido_cred_x5c_ptr fido_dev_build fido_dev_cancel diff --git a/src/fido.h b/src/fido.h index 914e3776..79b31112 100644 --- a/src/fido.h +++ b/src/fido.h @@ -124,6 +124,7 @@ const unsigned char *fido_cred_pubkey_ptr(const fido_cred_t *); const unsigned char *fido_cred_sig_ptr(const fido_cred_t *); const unsigned char *fido_cred_user_id_ptr(const fido_cred_t *); const unsigned char *fido_cred_x5c_ptr(const fido_cred_t *); +const unsigned char *fido_cred_x5c_list_ptr(const fido_cred_t *, size_t); int fido_assert_allow_cred(fido_assert_t *, const unsigned char *, size_t); int fido_assert_empty_allow_list(fido_assert_t *); @@ -226,6 +227,8 @@ size_t fido_cred_pubkey_len(const fido_cred_t *); size_t fido_cred_sig_len(const fido_cred_t *); size_t fido_cred_user_id_len(const fido_cred_t *); size_t fido_cred_x5c_len(const fido_cred_t *); +size_t fido_cred_x5c_list_count(const fido_cred_t *); +size_t fido_cred_x5c_list_len(const fido_cred_t *, size_t); uint8_t fido_assert_flags(const fido_assert_t *, size_t); uint32_t fido_assert_sigcount(const fido_assert_t *, size_t); diff --git a/src/fido/types.h b/src/fido/types.h index 01d68200..0aaa8cb6 100644 --- a/src/fido/types.h +++ b/src/fido/types.h @@ -140,12 +140,12 @@ typedef struct fido_attcred { } fido_attcred_t; typedef struct fido_attstmt { - fido_blob_t certinfo; /* tpm attestation TPMS_ATTEST structure */ - fido_blob_t pubarea; /* tpm attestation TPMT_PUBLIC structure */ - fido_blob_t cbor; /* cbor-encoded attestation statement */ - fido_blob_t x5c; /* attestation certificate */ - fido_blob_t sig; /* attestation signature */ - int alg; /* attestation algorithm (cose) */ + fido_blob_t certinfo; /* tpm attestation TPMS_ATTEST structure */ + fido_blob_t pubarea; /* tpm attestation TPMT_PUBLIC structure */ + fido_blob_t cbor; /* cbor-encoded attestation statement */ + fido_blob_array_t x5c; /* attestation certificate chain */ + fido_blob_t sig; /* attestation signature */ + int alg; /* attestation algorithm (cose) */ } fido_attstmt_t; typedef struct fido_rp { From 2e2d5c84e840eb31acf9ef1db73ccf7eca220910 Mon Sep 17 00:00:00 2001 From: Ludvig Michaelsson Date: Tue, 6 Feb 2024 15:24:29 +0100 Subject: [PATCH 2/2] cbor: add decode_x5c_array() Adds explicit checks that the target array is empty when we parse the CBOR map. This triggers a failure for malformed responses with duplicate keys. --- src/cbor.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/cbor.c b/src/cbor.c index cd809f8c..52d13e4e 100644 --- a/src/cbor.c +++ b/src/cbor.c @@ -1414,6 +1414,21 @@ decode_x5c(const cbor_item_t *item, void *arg) return (0); } +static int +decode_x5c_array(const cbor_item_t *item, fido_blob_array_t *arr) +{ + if (arr->len) { + fido_log_debug("%s: dup", __func__); + return (-1); + } + if (cbor_isa_array(item) == false || + cbor_array_is_definite(item) == false) { + fido_log_debug("%s: cbor", __func__); + return (-1); + } + return (cbor_array_iter(item, arr, decode_x5c)); +} + static int decode_attstmt_entry(const cbor_item_t *key, const cbor_item_t *val, void *arg) { @@ -1447,9 +1462,7 @@ decode_attstmt_entry(const cbor_item_t *key, const cbor_item_t *val, void *arg) goto out; } } else if (!strcmp(name, "x5c")) { - if (cbor_isa_array(val) == false || - cbor_array_is_definite(val) == false || - cbor_array_iter(val, &attstmt->x5c, decode_x5c) < 0) { + if (decode_x5c_array(val, &attstmt->x5c)) { fido_log_debug("%s: x5c", __func__); goto out; }