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

Peter Kietzmann notifications at github.com
Thu Aug 19 15:15:02 CEST 2021


@PeterKietzmann requested changes on this pull request.

Please fix: 
```
sys/usb/usbus/hid/hid.c:69: warning (nullPointer): Possible null pointer dereference: hid_dev
```
```
sys/fido2/ctap/ctap.c:59: warning: line is longer than 100 characters
sys/fido2/ctap/ctap.c:913: warning: too many consecutive empty lines
sys/include/fido2/ctap/ctap.h:206: warning: too many consecutive empty lines
sys/include/fido2/ctap/ctap.h:443: warning: line is longer than 100 characters
sys/include/fido2/ctap/ctap.h:446: warning: line is longer than 100 characters
sys/include/fido2/ctap/ctap.h:448: warning: line is longer than 100 characters
sys/include/fido2/ctap/ctap.h:454: warning: line is longer than 100 characters
sys/include/fido2/ctap/ctap.h:462: warning: line is longer than 100 characters
sys/include/fido2/ctap/ctap.h:463: warning: line is longer than 100 characters
sys/include/fido2/ctap/ctap.h:464: warning: line is longer than 100 characters
sys/include/fido2/ctap/ctap.h:466: warning: line is longer than 100 characters
sys/include/fido2/ctap/ctap.h:469: warning: line is longer than 100 characters
sys/include/fido2/ctap/ctap.h:477: warning: line is longer than 100 characters
sys/include/fido2/ctap/ctap.h:478: warning: line is longer than 100 characters
sys/include/fido2/ctap/ctap.h:479: warning: line is longer than 100 characters
sys/include/fido2/ctap/ctap.h:480: warning: line is longer than 100 characters
sys/include/fido2/ctap/ctap.h:481: warning: line is longer than 100 characters
sys/include/fido2/ctap/ctap.h:483: warning: line is longer than 100 characters
sys/include/fido2/ctap/ctap.h:485: warning: line is longer than 100 characters
sys/include/fido2/ctap/ctap.h:486: warning: line is longer than 100 characters
sys/include/fido2/ctap/ctap.h:496: warning: line is longer than 100 characters
sys/include/fido2/ctap/ctap.h:499: warning: line is longer than 100 characters
sys/include/fido2/ctap/transport/hid/ctap_hid.h:38: warning: line is longer than 100 characters
sys/include/fido2/ctap/transport/hid/ctap_hid.h:39: warning: line is longer than 100 characters
sys/include/fido2/ctap/transport/hid/ctap_hid.h:99: warning: line is longer than 100 characters
sys/include/fido2/ctap/transport/hid/ctap_hid.h:100: warning: line is longer than 100 characters
sys/include/fido2/ctap/transport/hid/ctap_hid.h:101: warning: line is longer than 100 characters
sys/include/fido2/ctap/transport/hid/ctap_hid.h:112: warning: line is longer than 100 characters
sys/include/fido2/ctap/transport/hid/ctap_hid.h:115: warning: line is longer than 100 characters

```
You can check your fixes locally by running  `make static-test` (from RIOT root directory).

@Ollrogge besides the remaining view comments, I am overall fine with this PR. Please address the latest comments. Squash your current commits and add a last one, including the new fixes. Besides the static tests, please make sure that the doc is fine, Kconfig (`menuconfig`)  works, and all tests (functional and unit tests) pass. I will then – hopefully tomorrow– run all tests again myself and would like to move forward.

> +* 15000 bytes FIDO2 CTAP
+
+## Usage
+The FIDO2 authenticator can be tested in two ways:
+
+### Functional testing
+1. Flash the device with `make flash`.
+2. Test the authenticator on a website like [Webauthn.io](https://webauthn.io/).
+
+Note:
+* Due to limited support of FIDO2 CTAP in browsers as of now, make sure to use the
+  Chromium or Google Chrome browser when testing on [Webauthn.io](https://webauthn.io/).
+* When registering and authenticating on [Webauthn.io](https://webauthn.io/) you
+will need to push button 1 on your device in order to show user presence.
+
+### Unit testing

How about adding a note about the python dependencies and link to the solokeys README?

> @@ -0,0 +1,46 @@
+# Test Application for FIDO2 CTAP
+

Should be note that, so far, this has only been tested with nrf52840?

> +        break;
+    default:
+        DEBUG("fido2_ctap: unknown req: %u \n", req->method);
+        resp->status = CTAP1_ERR_INVALID_COMMAND;
+        break;
+    }
+
+    return 0;
+}
+
+size_t fido2_ctap_get_info(ctap_resp_t *resp)
+{
+    assert(resp);
+
+    memset(&_encoder, 0, sizeof(_encoder));
+    cbor_encoder_init(&_encoder, resp->data, CTAP_MAX_MSG_SIZE, 0);

Here and in the following: Isn't this a function specific to tinyCBOR? This makes the CBOR abstraction seem useless.

> +#include <stdint.h>
+
+#include "mutex.h"
+#include "cbor.h"
+#include "assert.h"
+#include "crypto/modes/ccm.h"
+
+#include "fido2/ctap.h"
+#include "fido2/ctap/ctap_crypto.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @brief Size of pin auth

Meaning? IIRC this is the first 16 bytes of a MAC or something?

> + */
+typedef struct {
+    uint8_t options;                    /**< options */
+    uint8_t aaguid[CTAP_AAGUID_SIZE];   /**< AAGUID of device */
+} ctap_config_t;
+
+/**
+ * @brief CTAP state struct
+ *
+ * state of authenticator. Stored in flash memory
+ */
+typedef struct {
+    uint8_t initialized_marker;                 /**< CTAP initialized marker */
+    bool pin_is_set;                            /**< PIN is set or not */
+    int rem_pin_att;                            /**< remaining PIN tries */
+    uint8_t pin_hash[SHA256_DIGEST_LENGTH / 2]; /**< LEFT(SHA-256(pin), 16) */

Here and in other occasions: Didn't you introduce a `define` for these sizes? If yes, please use it. If no, leave it as is.

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


More information about the notifications mailing list