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

Peter Kietzmann notifications at github.com
Wed May 26 16:16:27 CEST 2021


@PeterKietzmann requested changes on this pull request.

third blob of comments and questions

> +static void create_usb(const void *report_desc, size_t len);
+static void work_usb(void);
+static void *pkt_loop(void *arg);
+
+static char g_stack[CTAP_TRANSPORT_STACKSIZE];
+static kernel_pid_t g_pid;
+
+static void *pkt_loop(void *arg)
+{
+    (void)arg;
+    int ret;
+
+    ret = fido2_ctap_init();
+
+    if (ret < 0) {
+        return (void *)0;

that looks fun?

> +#endif
+
+/**
+ * @name CTAP supported transport prototocols
+ *
+ * @{
+ */
+#define CTAP_TRANSPORT_USB 0x1
+#define CTAP_TRANSPORT_NFC 0x2
+#define CTAP_TRANSPORT_BLE 0x3
+/** @} */
+
+/**
+ * @brief CTAP_TRANSPORT thread stack size
+ */
+#define CTAP_TRANSPORT_STACKSIZE 16000

This absolutely needs to be configurable!

> +
+/**
+ * @brief CTAP_TRANSPORT thread stack size
+ */
+#define CTAP_TRANSPORT_STACKSIZE 16000
+
+/**
+ * @brief Initialize ctap_transport layer and fido2_ctap
+ */
+void fido2_ctap_transport_init(void);
+
+/**
+ * @brief Initialize needed RIOT transport layers
+ *
+ * @param[in] type              transport layer to initialize
+ * @param[in] arg               optional arg

What is an optional arg?

> +
+/**
+ * @brief Initialize ctap_transport layer and fido2_ctap
+ */
+void fido2_ctap_transport_init(void);
+
+/**
+ * @brief Initialize needed RIOT transport layers
+ *
+ * @param[in] type              transport layer to initialize
+ * @param[in] arg               optional arg
+ * @param[in] size              size of optional arg
+ *
+ * @return CTAP status code
+ */
+int fido2_ctap_transport_create(uint8_t type, void *arg, size_t size);

Use enum for type?

> + *
+ * @param[in] type              transport layer to initialize
+ * @param[in] arg               optional arg
+ * @param[in] size              size of optional arg
+ *
+ * @return CTAP status code
+ */
+int fido2_ctap_transport_create(uint8_t type, void *arg, size_t size);
+
+/**
+ * @brief Try timeout long to read data from specified transport layer
+ *
+ * @param[in] type              transport layer to read from
+ * @param[in] buffer            buffer for data
+ * @param[in] size              size of buffer
+ * @param[in] timeout           timeout

unit of timeout?

> + * @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

If error codes had a typedef, you could point to it from here.

> + * @ingroup     fido2_ctap_transport
+ * @brief       CTAP_HID defines and functions
+ *
+ * @{
+ *
+ * @file
+ * @brief       Definition for CTAPHID helper functions
+ *
+ * @author      Nils Ollrogge <nils-ollrogge at outlook.de>
+ */
+
+#ifndef FIDO2_CTAP_TRANSPORT_HID_CTAP_TRANSPORT_HID_H
+#define FIDO2_CTAP_TRANSPORT_HID_CTAP_TRANSPORT_HID_H
+
+#include <stdint.h>
+#include "timex.h"

Where do you depend on timex.h?

> +#include <stdint.h>
+#include "timex.h"
+
+#include "usb/usbus/hid.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @name CTAP_HID packet type payload sizes
+ *
+ * @{
+ */
+#define CTAP_HID_INIT_PAYLOAD_SIZE  (CONFIG_USBUS_HID_INTERRUPT_EP_SIZE - 7)    /**< endpoint size - init packet metadata */
+#define CTAP_HID_CONT_PAYLOAD_SIZE  (CONFIG_USBUS_HID_INTERRUPT_EP_SIZE - 5)    /**< endpoint size - cont packet metadata */

Guess the line width is too long.

> + * @{
+ */
+#define CTAP_HID_INIT_PACKET 0x80
+#define CTAP_HID_CONT_PACKET 0x00
+/** @} */
+
+/**
+ * @brief CTAP_HID size of nonce for init request
+ */
+#define CTAP_HID_INIT_NONCE_SIZE 8
+
+/**
+ * @brief CTAP_HID transaction timeout
+ *
+ */
+#define CTAP_HID_TRANSACTION_TIMEOUT    (0.75 * US_PER_SEC)

Is this a configuration parameter (then it should be configurable) or is it defined by CTAP_HID? If so, please add also include the reference to the spec here.

> +
+#include "fido2/ctap.h"
+#include "fido2/ctap/transport/ctap_transport.h"
+#include "fido2/ctap/transport/hid/ctap_transport_hid.h"
+#include "fido2/ctap/ctap_utils.h"
+
+#define ENABLE_DEBUG    (0)
+#include "debug.h"
+
+/**
+ * @brief HID report descriptor
+ *
+ * CTAP specification (version 20190130) section 8.1.8.2
+ */
+static const uint8_t report_desc_ctap[] = {
+    0x06, 0xD0, 0xF1,   // HID_UsagePage ( FIDO_USAGE_PAGE ),

doc style

> + * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @ingroup fido2_ctap_transport_hid
+ * @{
+ * @file
+ *
+ * @author      Nils Ollrogge <nils-ollrogge at outlook.de>
+ * @}
+ */
+
+#include <string.h>
+
+#ifdef CONFIG_CTAP_NATIVE

What is that?

> +static int8_t delete_cid(uint32_t cid);
+static bool cid_exists(uint32_t cid);
+static inline uint32_t get_new_cid(void);
+static inline uint16_t get_packet_len(const ctap_hid_pkt_t *pkt);
+static void pkt_worker(void);
+static inline bool is_init_type_pkt(const ctap_hid_pkt_t *pkt);
+static inline bool should_cancel(void);
+
+static bool g_is_busy = false;
+static ctap_hid_buffer_t g_ctap_buffer;
+/* logical channels */
+static ctap_hid_cid_t g_cids[CTAP_HID_CIDS_MAX];
+/* channel id 0 is reserved */
+static uint32_t g_cid = 1;
+
+void fido2_ctap_transport_hid_create(void)

What do we need this wrapper for

> + */
+static void send_init_response(uint32_t cid_old, uint32_t cid_new,
+                               const uint8_t *nonce);
+
+static void reset_ctap_buffer(void);
+static uint8_t buffer_pkt(const ctap_hid_pkt_t *pkt);
+static void send_error_response(uint32_t cid, uint8_t err);
+static int8_t refresh_cid(uint32_t cid);
+static int8_t add_cid(uint32_t cid);
+static int8_t delete_cid(uint32_t cid);
+static bool cid_exists(uint32_t cid);
+static inline uint32_t get_new_cid(void);
+static inline uint16_t get_packet_len(const ctap_hid_pkt_t *pkt);
+static void pkt_worker(void);
+static inline bool is_init_type_pkt(const ctap_hid_pkt_t *pkt);
+static inline bool should_cancel(void);

Looks bit messy. No doc?

> +#define CTAP_VERSION_FLAG_FIDO      0x02
+#define CTAP_VERSION_FLAG_U2F_V2    0x04
+/** @} */
+
+/**
+ * @name CTAP get info response options map CBOR key values
+ *
+ * All options are in the form key-value pairs with string IDs and
+ * boolean values
+ * @{
+ */
+#define CTAP_GET_INFO_RESP_OPTIONS_ID_PLAT       "plat"
+#define CTAP_GET_INFO_RESP_OPTIONS_ID_RK         "rk"
+#define CTAP_GET_INFO_RESP_OPTIONS_ID_CLIENT_PIN "clientPin"
+#define CTAP_GET_INFO_RESP_OPTIONS_ID_UP         "up"
+#define CTAP_GET_INFO_RESP_OPTIONS_ID_UV         "uv"

I remember many occasions where you do use "rk", "up", "uv" (possibly others) directly and not the define.

> +    uint16_t max_msg_size;              /**< max message size */
+    uint8_t pin_protocol;               /**< supported PIN protocol versions */
+    bool pin_is_set;                    /**< PIN is set or not */
+} ctap_info_t;
+
+/**
+ * @brief Initialize ctap
+ *
+ * @return 0 for success, -1 for failure
+ */
+int fido2_ctap_init(void);
+
+/**
+ * @brief Handle CBOR encoded ctap request.
+ *
+ * @param[in] req               request

So what is a "request"?

> +
+/**
+ * @brief Encrypt resident key with AES CCM
+ *
+ * @param[in] rk                type of credential
+ * @param[in] nonce             CCM nonce
+ * @param[in] nonce_size        size of nonce buffer
+ * @param[in] id                credential id struct storing encrypted resident key
+ *
+ * @return CTAP status code
+ */
+int fido2_ctap_encrypt_rk(ctap_resident_key_t *rk, uint8_t *nonce,
+                          size_t nonce_size, ctap_cred_id_t *id);
+
+/**
+ * @brief Return if PIN has been set

The function also returns if PIN is *not* set. 

> +typedef struct {
+    ctap_crypto_pub_key_t pub;          /**< public key */
+    uint8_t priv[CTAP_CRYPTO_KEY_SIZE]; /**< private key */
+} ctap_crypto_key_agreement_key_t;
+
+/**
+ * @brief Initialize crypto helper
+ *
+ * Initializes crypto libs and creates key_agreement key pair
+ */
+int fido2_ctap_crypto_init(void);
+
+/**
+ * @brief Pseudorandom number generator
+ *
+ * Wrapper for hwrng_read

PRNG...

> + * @param[in] out              encrypted data
+ * @param[in] out_size         size of encrypted data
+ * @param[in] in               data to be encrypted
+ * @param[in] in_size          size of data to be encrypted buffer
+ * @param[in] auth_data        additional data to authenticate in MAC
+ * @param[in] auth_data_size   size of auth_data
+ * @param[in] mac_size         size of appended MAC
+ * @param[in] length_encoding  max supported length of plaintext
+ * @param[in] nonce            nonce for ctr mode encryption
+ * @param[in] nonce_size       size of nonce
+ * @param[in] key              symmetric key to use for encryption
+ * @param[in] key_size         size of key
+ *
+ * @return CTAP status code
+ */
+int fido2_ctap_crypto_aes_ccm_dec(uint8_t *out, size_t out_size,

As commented above, I did not quite see the reason for an other abstraction layer on top of the ciphers abstraction. The amount of parameters of this function indicates that something is odd.

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

It is unclear to me why and how this function should be used. Should it be an internal function?

> @@ -0,0 +1,17 @@
+#include <stdio.h>

Copyright header

> +#endif
+
+/**
+ * @name CTAP supported transport prototocols
+ *
+ * @{
+ */
+#define CTAP_TRANSPORT_USB 0x1
+#define CTAP_TRANSPORT_NFC 0x2
+#define CTAP_TRANSPORT_BLE 0x3
+/** @} */
+
+/**
+ * @brief CTAP_TRANSPORT thread stack size
+ */
+#define CTAP_TRANSPORT_STACKSIZE 16000

Btw, did you check again if 16k is really needed? It would be good to mention what takes so much memory (remember, default thread stacksize is 1024 bytes for Cortex-M devices).

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


More information about the notifications mailing list