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

Peter Kietzmann notifications at github.com
Wed Jun 30 12:41:43 CEST 2021


@PeterKietzmann commented on this pull request.

@Ollrogge thanks for addressing my comments! Here are some responses to the open discussions. Next, I will run the tests again, and start a fresh review round

> + */
+
+/**
+ * @defgroup    fido2 FIDO2 - Fast Identity Online 2
+ * @ingroup     sys
+ *
+ * FIDO2 is an authentication standard that seeks to solve the password problem
+ * by enabling passwordless authentication. Instead of using passwords to
+ * authenticate to web services, FIDO2 enables users to use common devices
+ * (authenticators) to create cryptographic credentials which are then used
+ * for authentication. FIDO2 consists of the W3C Web Authentication
+ * specification (WebAuthn) and the Client to Authenticator Protocol (CTAP).
+ *
+ * This code implements the FIDO2 CTAP protocol.
+ *
+ * This is an experimental implementation not yet supporting all features of

This information should get a doxygen `@note` tag or similar which you could add below `@defgroup` or close by. Please check other documentation files

> + * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @defgroup    fido2 FIDO2 - Fast Identity Online 2
+ * @ingroup     sys
+ *
+ * FIDO2 is an authentication standard that seeks to solve the password problem
+ * by enabling passwordless authentication. Instead of using passwords to
+ * authenticate to web services, FIDO2 enables users to use common devices
+ * (authenticators) to create cryptographic credentials which are then used
+ * for authentication. FIDO2 consists of the W3C Web Authentication
+ * specification (WebAuthn) and the Client to Authenticator Protocol (CTAP).
+ *
+ * This code implements the FIDO2 CTAP protocol.

Do you think it is feasible to draw an ascii overview of the system blocks? Thinking about the core functions (make credential, ...) on top of CTAP, CTAP_HID, HID. Additionally, you could add the helper modules of your implementation, would have to see an example to judge if it makes sense.

Then, add two sections, one to introduce the *implemented* features, including the figure proposed above, and a second section to list the limitations, so basically *unimplemented* features aka limitations

> +    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;
+}
+
+static int RNG(uint8_t *dest, size_t size)
+{
+    fido2_ctap_crypto_prng(dest, size);
+    return 1;

The idea was to add status codes to ´fido2_ctap_crypto_prng´. Once done, these should be processed here I guess

> +extern "C" {
+#endif
+
+/**
+ * @brief CTAP supported transport prototocols
+ */
+typedef enum {
+    USB,
+    NFC,
+    BLE
+} ctap_transport_type_t;
+
+/**
+ * @brief CTAP_TRANSPORT thread stack size
+ */
+#define CTAP_TRANSPORT_STACKSIZE 15000

It needs to be configurable because the implementation should be compatible with different platforms (in future) that might compile a bit differently and eventually require more or less stack. As a user, you want to be able to configure this (via Kconfig) and not go to the header file to change a value.

> @@ -718,4 +718,35 @@ ifneq (,$(filter dbgpin,$(USEMODULE)))
   FEATURES_REQUIRED += dbgpin
 endif
 
+ifneq (,$(filter fido2_ctap_%,$(USEMODULE)))
+  USEMODULE += fido2_ctap_transport
+  USEMODULE += fido2_ctap
+  ifneq (,$(filter fido2_ctap_hid,$(USEMODULE)))
+      ifneq (native, $(BOARD))
+        USEMODULE += usbus_hid
+        DISABLE_MODULE += auto_init_usbus

Still there

> +
+#include "fido2/ctap/transport/ctap_transport.h"
+#include "fido2/ctap.h"
+#include "fido2/ctap/ctap_utils.h"
+#include "fido2/ctap/ctap_cbor.h"
+#include "fido2/ctap/ctap_mem.h"
+
+#define ENABLE_DEBUG    (0)
+#include "debug.h"
+
+/*** main CTAP2 functions ***/
+
+/**
+ * @brief MakeCredential method
+ *
+ * CTAP specification (version 20190130) section 5.1

There are (still) multiple occurrences of "CTAP2" in your code (for example in line 35 of this file). Please correct them. I wanted to say that you should also check new files&folders for usage of "CTAP2" instead of "CTAP"

> +static ctap_get_assertion_state_t g_assert_state;
+static uint8_t g_pin_token[CTAP_PIN_TOKEN_SIZE];
+/* remaining PIN attempts until authenticator is boot locked */
+static int g_rem_pin_att_boot = CTAP_PIN_MAX_ATTS_BOOT;
+
+int fido2_ctap_init(void)
+{
+    int ret;
+
+    load_state(&g_state);
+
+    ret = fido2_ctap_crypto_init();
+
+    if (ret != CTAP2_OK) {
+        DEBUG("fido2_ctap: initialization failed \n");
+        return -1;

I meant RIOT internal status codes returnain an enum value. 

> +    load_state(&g_state);
+
+    ret = fido2_ctap_crypto_init();
+
+    if (ret != CTAP2_OK) {
+        DEBUG("fido2_ctap: initialization failed \n");
+        return -1;
+    }
+
+    /* first startup up of the device */
+    if (g_state.initialized != CTAP_INITIALIZED_MARKER) {
+        reset();
+    }
+
+    /* initialize pin_token */
+    fido2_ctap_crypto_prng(g_pin_token, sizeof(g_pin_token));

No change on this?

> +    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;
+    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));

I don't quire understand that. According to your comment above, the reset function intends to:

> reset an authenticator back to a factory default state, invalidating all generated credentials

So what credentials need to be encrypted here and why?

> +
+    /* sha256 of shared secret ((abG).x) to obtain shared key */
+    sha256(shared_secret, sizeof(shared_secret), shared_key);
+
+    hmac_sha256_init(&ctx, shared_key, sizeof(shared_key));
+    hmac_sha256_update(&ctx, req->new_pin_enc, req->new_pin_enc_size);
+    hmac_sha256_update(&ctx, req->pin_hash_enc, sizeof(req->pin_hash_enc));
+    hmac_sha256_final(&ctx, hmac);
+
+    if (memcmp(hmac, req->pin_auth, 16) != 0) {
+        DEBUG("fido2_ctap: pin hmac and pin_auth differ \n");
+        return CTAP2_ERR_PIN_AUTH_INVALID;
+    }
+
+    sz = sizeof(pin_hash_dec);
+    ret = fido2_ctap_crypto_aes_dec(pin_hash_dec, &sz, req->pin_hash_enc,

Although the abstractions don't seem necessary to me, it is not bad practice. So for consistency I would argue to add abstractions for all crypto operations then.

> +
+    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));

Still not so clear to me. What is `i`? An offset of what? It is not clear from the doc of that function. Maybe it helps if you give `i` a more speaking variable name.

> +    }
+
+    /* 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;

Please add this information to the documentation of that function.

> +            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) {

You could add a comment similarly to the above discussion point.

> +            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);

Just to double check: it is intended that `ret = cbor_value_advance(&map);` 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) {

You could add a comment similarly to the above discussion point.

> +        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)

Please try. The are very similar so it is worth it. The if/else paths are implemented a little differently but I think many cases can be harmonized (e.g.,  `if (ret != CborNoError)` vs `if (ret != CTAP2_OK)`, `else if (strncmp(key, CTAP_CBOR_DISPLAY_NAME, strlen(CTAP_CBOR_DISPLAY_NAME)) == 0)` vs `else if (strcmp(key, CTAP_CBOR_STR_NAME) == 0)` ...)

> +            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);

Sorry, you are just parsing here right? In the initial comment I had in mind the creation of these lists...

> +    }
+
+    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,

So :)?

> + * @param[in] size              size of buffer
+ * @param[in] timeout           timeout
+ *
+ * @return CTAP status code
+ */
+int fido2_ctap_transport_read_timeout(uint8_t type, void *buffer, size_t size,
+                                      uint32_t timeout);
+
+/**
+ * @brief Write to specified transport layer
+ *
+ * @param[in] type              transport layer to read from
+ * @param[in] buffer            data buffer
+ * @param[in] size              size of buffer
+ *
+ * @return CTAP status code

Can you point to the status codes via doxygen?

> + */
+void fido2_ctap_transport_hid_handle_packet(void *pkt_raw, int size);
+
+/**
+ * @brief Send keepalive packet
+ *
+ * @param[in] status    CTAP_HID status code
+ *
+ */
+void fido2_ctap_transport_hid_send_keepalive(uint8_t status);
+
+/**
+ * @brief Check logical channels for timeouts
+ *
+ */
+void fido2_ctap_transport_hid_check_timeouts(void);

1. So should this rather be an internal function?
2. How about extending the documentation with your explanation above?

-- 
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-695794397
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210630/e92311f4/attachment-0001.htm>


More information about the notifications mailing list