[riot-notifications] [RIOT-OS/RIOT] FIDO2 support in RIOT (#16489)

Peter Kietzmann notifications at github.com
Wed May 26 09:32:58 CEST 2021


@PeterKietzmann requested changes on this pull request.

second intermediate blob of comments and questions

> +}
+
+static inline bool locked(void)
+{
+    return g_state.rem_pin_att <= 0;
+}
+
+static inline bool boot_locked(void)
+{
+    return g_rem_pin_att_boot <= 0;
+}
+
+static int decrement_pin_attempts(void)
+{
+    g_state.rem_pin_att--;
+    g_rem_pin_att_boot--;

Why do you need to maintain both counters? Could it be relevant to protect against overflow?

> +
+    if (memcmp(auth, hmac, 16) != 0) {
+        return CTAP2_ERR_PIN_AUTH_INVALID;
+    }
+
+    return CTAP2_OK;
+}
+
+static inline uint16_t get_page_offset(uint16_t i)
+{
+    return i / (FLASHPAGE_SIZE / CTAP_PAD_RK_SZ);
+}
+
+static inline uint16_t get_offset_into_page(uint16_t i)
+{
+    return CTAP_PAD_RK_SZ * (i % (FLASHPAGE_SIZE / CTAP_PAD_RK_SZ));

What is `i`? What exactly is the "offset into page"? This function name next to `get_page_offset` is not very intuitive.

> +                    else {
+                        /* no match with stored key, try to decrypt */
+                        ret = ctap_decrypt_rk(&rks[index],
+                                              &allow_list[j].cred_id);
+                        if (ret == CTAP2_OK) {
+                            if (memcmp(rks[index].rp_id_hash, rk.rp_id_hash,
+                                       SHA256_DIGEST_LENGTH) == 0) {
+                                index++;
+                                break;
+                            }
+                        }
+                    }
+                }
+            }
+            else {
+                /* no allow list so rp_id_hash match is enough */

?

> +    g_state.sign_count = 0;
+
+    g_rem_pin_att_boot = CTAP_PIN_MAX_ATTS_BOOT;
+
+    fido2_ctap_crypto_prng(g_state.cred_key, sizeof(g_state.cred_key));
+
+    g_state.config.options |= CTAP_INFO_OPTIONS_FLAG_PLAT;
+    g_state.config.options |= CTAP_INFO_OPTIONS_FLAG_RK;
+    g_state.config.options |= CTAP_INFO_OPTIONS_FLAG_CLIENT_PIN;
+    g_state.config.options |= CTAP_INFO_OPTIONS_FLAG_UP;
+
+    uint8_t aaguid[CTAP_AAGUID_SIZE];
+
+    fmt_hex_bytes(aaguid, CTAP_AAGUID);
+
+    memmove(g_state.config.aaguid, aaguid, sizeof(g_state.config.aaguid));

Is there a partcular reason for using `memmove` and note`memcpy`?

> +    }
+
+    /* initialize pin_token */
+    fido2_ctap_crypto_prng(g_pin_token, sizeof(g_pin_token));
+
+    DEBUG("fido2_ctap: initialization successful \n");
+
+    return 0;
+}
+
+static void reset(void)
+{
+    g_state.initialized = CTAP_INITIALIZED_MARKER;
+    g_state.rem_pin_att = CTAP_PIN_MAX_ATTS;
+    g_state.pin_is_set = false;
+    g_state.rk_amount_stored = 0;

This is a pretty hard reset, right? It basically "invalidates" the key stored in flash, right? Is it intended? Please double check

> +
+/**
+ * @ingroup fido2_ctap
+ * @{
+ * @file
+ *
+ * @author      Nils Ollrogge <nils-ollrogge at outlook.de>
+ * @}
+ */
+
+#include "xtimer.h"
+
+#define ENABLE_DEBUG    (0)
+#include "debug.h"
+
+#include "fido2/ctap/ctap_cbor.h"

Place all `#inlcude` together.

> + * @ingroup fido2_ctap
+ * @{
+ * @file
+ *
+ * @author      Nils Ollrogge <nils-ollrogge at outlook.de>
+ * @}
+ */
+
+#include "xtimer.h"
+
+#define ENABLE_DEBUG    (0)
+#include "debug.h"
+
+#include "fido2/ctap/ctap_cbor.h"
+
+static int parse_rp(CborValue *it, ctap_rp_ent_t *rp);

I think a short line explaining what the function does wouldn't hurt.

> +static int parse_fixed_size_byte_array(CborValue *map, uint8_t *dst,
+                                       size_t *len);
+static int parse_byte_array(CborValue *it, uint8_t *dst, size_t *len);
+static int parse_text_string(CborValue *it, char *dst, size_t *len);
+static int parse_int(CborValue *it, int *num);
+
+int fido2_ctap_cbor_encode_info(CborEncoder *encoder, const ctap_info_t *info)
+{
+    int ret;
+    size_t sz = 0;
+    CborEncoder map;
+    CborEncoder map2;
+    CborEncoder array;
+    CborEncoder array2;
+
+    ret = cbor_encoder_create_map(encoder, &map, 5);

5?`Please use literals

> +    return CTAP2_OK;
+}
+
+int fido2_ctap_cbor_encode_attestation_object(CborEncoder *encoder,
+                                              const ctap_auth_data_t *auth_data,
+                                              const uint8_t *client_data_hash,
+                                              ctap_resident_key_t *rk)
+{
+    int ret;
+    uint16_t cred_id_sz;
+    uint16_t cred_header_sz;
+    size_t offset = 0;
+    uint8_t *cose_key_buf;
+    uint8_t sig_buf[CTAP_CRYPTO_ES256_DER_MAX_SIZE];
+    size_t sig_buf_len = sizeof(sig_buf);
+    uint8_t auth_data_buf[512] = { 0 };

Really 512? Better use literals.

> +
+    /* move attested credential data header into authenticator data buffer  */
+    memmove(auth_data_buf + offset, (void *)cred_header, cred_header_sz);
+    offset += cred_header_sz;
+
+    cose_key_buf = auth_data_buf + offset;
+
+    cbor_encoder_init(&cose_key, cose_key_buf, sizeof(auth_data_buf) - offset,
+                      0);
+
+    /* encode credential public key into COSE format */
+    ret = encode_cose_key(&cose_key, &auth_data->attested_cred_data.key);
+    if (ret != CborNoError) {
+        return CTAP2_ERR_CBOR_PARSING;
+    }
+    offset += cbor_encoder_get_buffer_size(&cose_key, cose_key_buf);

Better move the incrementation up to where the buffer gets initialized or down to before `offest` is used

> +    else {
+        ctap_cred_id_t *id = (ctap_cred_id_t *)cred_ptr;
+        ret = cbor_encode_byte_string(&desc, (uint8_t *)id,
+                                      sizeof(*id));
+    }
+
+    if (ret != CborNoError) {
+        return CTAP2_ERR_CBOR_PARSING;
+    }
+
+    ret = cbor_encode_text_string(&desc, "type", 4);
+    if (ret != CborNoError) {
+        return CTAP2_ERR_CBOR_PARSING;
+    }
+
+    ret = cbor_encode_text_string(&desc, "public-key", 10);

Why not use `strlen()`?

> +            return CTAP2_ERR_CBOR_UNEXPECTED_TYPE;
+        }
+
+        ret = cbor_value_get_int_checked(&map, &key);
+        if (ret != CborNoError) {
+            return CTAP2_ERR_CBOR_PARSING;
+        }
+
+        ret = cbor_value_advance(&map);
+        if (ret != CborNoError) {
+            return CTAP2_ERR_CBOR_PARSING;
+        }
+
+        switch (key) {
+        case CTAP_GA_REQ_RP_ID:
+            DEBUG("ctap_cbor: parse rp_id \n");

FYI: `DEBUG("%s\n", __FUNCTION__);` prints the function name that calls it

> +            DEBUG("ctap_cbor: unknown GetAssertion key: %d \n", key);
+            break;
+        }
+
+        if (ret != CTAP2_OK) {
+            return ret;
+        }
+
+        ret = cbor_value_advance(&map);
+        if (ret != CborNoError) {
+            return CTAP2_ERR_CBOR_PARSING;
+        }
+    }
+
+    /* rp_id and client_data_hash are required */
+    if (required_parsed != 2) {

I'm wondering how `required_parsed` ever increments to 2. It is only incremented in the cases that each hava a `break`. Hence, there is no fall through.

> +            break;
+        default:
+            DEBUG("parse_client_pin unknown key: %d \n", key);
+            break;
+        }
+        if (ret != CTAP2_OK) {
+            return ret;
+        }
+
+        ret = cbor_value_advance(&map);
+        if (ret != CborNoError) {
+            return CTAP2_ERR_CBOR_PARSING;
+        }
+    }
+
+    if (required_parsed != 2) {

same question as above

> +            req->pin_auth_present = true;
+            break;
+        case CTAP_CP_REQ_NEW_PIN_ENC:
+            DEBUG("ctap_cbor: parse newPinEnc \n");
+            len = sizeof(req->new_pin_enc);
+            ret = parse_byte_array(&map, req->new_pin_enc, &len);
+            req->new_pin_enc_size = len;
+            break;
+        case CTAP_CP_REQ_PIN_HASH_ENC:
+            DEBUG("ctap_cbor: parse pinHashEnc \n");
+            len = sizeof(req->pin_hash_enc);
+            ret = parse_fixed_size_byte_array(&map, req->pin_hash_enc, &len);
+            req->pin_hash_enc_present = true;
+            break;
+        default:
+            DEBUG("parse_client_pin unknown key: %d \n", key);

No specific return value in this case?

> +        default:
+            DEBUG("ctap_cbor: unknown MakeCredential key: %d \n", key);
+            break;
+        }
+
+        if (ret != CTAP2_OK) {
+            return ret;
+        }
+
+        ret = cbor_value_advance(&map);
+        if (ret != CborNoError) {
+            return CTAP2_ERR_CBOR_PARSING;
+        }
+    }
+
+    if (required_parsed != 4) {

same question as above

> +        return CTAP2_ERR_CBOR_PARSING;
+    }
+
+    for (size_t i = 0; i < map_len; i++) {
+        type = cbor_value_get_type(&map);
+        if (type != CborTextStringType) {
+            return CTAP2_ERR_INVALID_CBOR_TYPE;
+        }
+
+        key_len = sizeof(key);
+        ret = cbor_value_copy_text_string(&map, key, &key_len, NULL);
+        if (ret == CborErrorOutOfMemory) {
+            return CTAP2_ERR_LIMIT_EXCEEDED;
+        }
+
+        key[sizeof(key) - 1] = 0;

Is this a null termination? Using `\0` would be clearer

> +            if (ret != 0) {
+                return ret;
+            }
+        }
+        else {
+            DEBUG("CTAP_parse_rp: ignoring unknown key: %s \n", key);
+        }
+
+        ret = cbor_value_advance(&map);
+        if (ret != CborNoError) {
+            return CTAP2_ERR_CBOR_PARSING;
+        }
+    }
+
+    /* id is mandatory */
+    if (required_parsed != 1) {

You could possibly return much earlier in case the id is missing.

> +        ret = cbor_value_advance(&map);
+        if (ret != CborNoError) {
+            return CTAP2_ERR_CBOR_PARSING;
+        }
+    }
+
+    /* id is mandatory */
+    if (required_parsed != 1) {
+        return CTAP2_ERR_MISSING_PARAMETER;
+    }
+
+    return CTAP2_OK;
+}
+
+/* parse PublicKeyCredentialUserEntity dictionary */
+static int parse_user(CborValue *it, ctap_user_ent_t *user)

I didn't check the details, but these parser functions seem very similar. Is it possible to de-duplicate code, did you try already?

> +    ret = cbor_value_get_int_checked(&alg, (int *)alg_type);
+    if (ret != CborNoError) {
+        return CTAP2_ERR_CBOR_PARSING;
+    }
+
+    return 0;
+}
+
+/* parse options dictionary */
+static int parse_options(CborValue *it, ctap_options_t *options)
+{
+    int ret;
+    int type;
+    char key[8];
+    size_t key_len = sizeof(key), map_len;
+    bool b;

Please give more descriptive variable names.

> +            DEBUG("Ctap parse options, unknown uption: %s \n", key);
+        }
+
+        cbor_value_advance(&map);
+        if (ret != CborNoError) {
+            return CTAP2_ERR_CBOR_PARSING;
+        }
+    }
+
+    return CTAP2_OK;
+}
+
+static int parse_allow_list(CborValue *it, CborValue *allow_list,
+                            size_t *allow_list_len)
+{
+    return parse_exclude_list(it, allow_list, allow_list_len);

"allow list" list directly maps the "exclude list" ? That feels odd. Probably demands for a single parse function 

> +
+#include "periph/hwrng.h"
+
+#include "uECC.h"
+
+#include "fido2/ctap/ctap_crypto.h"
+#include "fido2/ctap/ctap_status.h"
+#include "fido2/ctap/ctap_utils.h"
+
+static ctap_crypto_key_agreement_key_t g_ag_key;
+static int sig_to_der_format(uint8_t *r, uint8_t *s, uint8_t *sig,
+                             size_t *sig_len);
+
+int fido2_ctap_crypto_init(void)
+{
+    hwrng_init();

Use the PRNG API instead and in your build dependencies, include `prng_sha256prng` as a generator. Alternatively, since all requirements for fido2 are yet only met by the nrf52 (?!) you could set the hwrng-prng as a generator in the test Makefile. 

 Furthermore, both HWRNG and PRNG should be initialized (/seeded) by auto_init already, no need to do it here.

> +int fido2_ctap_crypto_init(void)
+{
+    hwrng_init();
+
+    /**
+     * Generate authenticatorKeyAgreementKey.
+     * Configuration operations upon power up CTAP specification
+     * (version 20190130) section 5.5.2
+     */
+    return fido2_ctap_crypto_gen_keypair(&g_ag_key.pub,
+                                         (uint8_t *)&g_ag_key.priv);
+}
+
+void fido2_ctap_crypto_prng(uint8_t *buf, size_t size)
+{
+    hwrng_read(buf, size);

PRNG

> +    return fido2_ctap_crypto_gen_keypair(&g_ag_key.pub,
+                                         (uint8_t *)&g_ag_key.priv);
+}
+
+void fido2_ctap_crypto_prng(uint8_t *buf, size_t size)
+{
+    hwrng_read(buf, size);
+}
+
+int fido2_ctap_crypto_reset_key_agreement(void)
+{
+    return fido2_ctap_crypto_gen_keypair(&g_ag_key.pub,
+                                         (uint8_t *)&g_ag_key.priv);
+}
+
+void fido2_ctap_crypto_get_key_agreement(ctap_crypto_pub_key_t *key)

is "key agreement" a common term for EC point coordinates?

> +                             in, in_size, out);
+
+    if (len < 0) {
+        return CTAP1_ERR_OTHER;
+    }
+
+    return CTAP2_OK;
+}
+
+int fido2_ctap_crypto_gen_keypair(ctap_crypto_pub_key_t *pub_key,
+                                  uint8_t *priv_key)
+{
+    int ret;
+    const struct uECC_Curve_t *curve = uECC_secp256r1();
+
+    ret = uECC_make_key((uint8_t *)pub_key, priv_key, curve);

Just wondering: To create a key pair, uECC needs access to the RNG. AFAIR there was an init function  in uECC to pass the pointer to a random function. Is it somewhere or did you sole this differently?

> +    }
+
+    memmove(r_raw, sig, sizeof(r_raw));
+    memmove(s_raw, sig + sizeof(r_raw), sizeof(s_raw));
+
+    ret = sig_to_der_format(r_raw, s_raw, sig, sig_size);
+
+    if (ret != CTAP2_OK) {
+        return ret;
+    }
+
+    return CTAP2_OK;
+}
+
+/* Encode signature in ASN.1 DER format */
+static int sig_to_der_format(uint8_t *r, uint8_t *s, uint8_t *sig,

Is this conversion specific to fido2 or your implementation? Otherwise, it might be useful to extract such functionally (separate PR) and have dedicated tests.

> +
+    if (memcmp(addr, data, len) == 0) {
+        return FLASHPAGE_OK;
+    }
+    else {
+        return FLASHPAGE_NOMATCH;
+    }
+}
+
+static void flash_write(int page, const void *data, size_t len)
+{
+    assert(page < (int)FLASHPAGE_NUMOF);
+    uint32_t *page_addr = (uint32_t *)flashpage_addr(page);
+
+    /* erase given page */
+    NRF_NVMC->CONFIG = NVMC_CONFIG_WEN_Een;

Hardware specific code is misplaced here. Are there vulnerabilities with the flagpage API?

> @@ -0,0 +1,90 @@
+#include <string.h>

Missing copyright notice and stuff

> +        default:
+            DEBUG("ctap_cbor: unknown MakeCredential key: %d \n", key);
+            break;
+        }
+
+        if (ret != CTAP2_OK) {
+            return ret;
+        }
+
+        ret = cbor_value_advance(&map);
+        if (ret != CborNoError) {
+            return CTAP2_ERR_CBOR_PARSING;
+        }
+    }
+
+    if (required_parsed != 4) {

Maybe I'm blind. This pattern seems to repeat multiple times. Please check your code again and let me know the where is the misunderstanding.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/16489#pullrequestreview-667829134
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210526/4149d984/attachment-0001.htm>


More information about the notifications mailing list