Fido2 - conte91 #437

conte91 · 2020-03-30T09:15:37Z

Anything else to say?

Move the functions to generate/verify U2F keyhandles to a separate module.

Rename U2F_KEY_SIZE to U2F_COORD_SIZE for better clarity. The "32" refers to the size of one of the coordinates of the (x, y) point on the EC. The actual "key" is referred to as "point" in the same header and has size U2F_POINT_SIZE.

Create a new function in u2f_keyhandle that creates new keys. This makes the code cleaner as a side effect.

USB sends can and will be sent only through a straight path starting when a USB packet is received. This is inappropriate as it forces processing the whole USB packet at once (hence blocking workflows etc). Start moving the responsibility for packet sending out of the main USB rx function - although the coupling is still there for now.

benma · 2020-04-05T10:11:54Z

src/CMakeLists.txt

@@ -211,8 +213,14 @@ set(FIRMWARE-DRIVER-SOURCES
 set(FIRMWARE-DRIVER-SOURCES ${FIRMWARE-DRIVER-SOURCES} PARENT_SCOPE)

 set(FIRMWARE-U2F-SOURCES
+  ${CMAKE_SOURCE_DIR}/src/fido2/ctap.c


did you modify those files in any way?

would be great to move them to external/ instead and compile them as a lib, like ctaes for example.

+1 for moving to external/

Actually, quite a few changes diffing against their latest at solokeys/solo1@c248b5d which is 4.0.0+. Here's the diff: https://gist.github.com/x1ddos/19cce770b2929bd6c0a4f933f527ecbe

But diffing against their earlier 3.1.3 release seems to diverge even more: https://gist.github.com/x1ddos/8f93bc4d91455460225c0300fea57603.

I'm guessing it might be somewhere between 3.1.3 and 4.0.0. @conte91 which upstream commit did you start with?

x1ddos

Of course, needs some clean up but nice overall. Left just a couple comments for now but before continuing, would love this to be rebased - we did quite a few changes meanwhile - then move solokeys' fido2 to external/ and, most importantly, which solokeys commit was that? It's hard to see what's upstream and what isn't otherwise, and how to update in the future.

@conte91 should I take it over or you're still on this?

x1ddos · 2020-04-29T16:42:09Z

.gitmodules

@@ -19,3 +19,6 @@
 [submodule "tools/ttf2ugui"]
 	path = tools/ttf2ugui
 	url = https://github.com/digitalbitbox/ttf2ugui
+[submodule "external/tinycbor"]
+	path = external/tinycbor
+	url = https://github.com/intel/tinycbor


It would make sense to fork and point here to a digitalbitbox/tinycbor imho, like all the other submodule deps.

Also, quite a few annoying warnings about things like __cplusplus not being defined.
I wonder if tinycbor needs different or additional flags. For example, looks like they're building with -std=gnu99 but we default to -std=c99.

x1ddos · 2020-04-29T17:46:58Z

scripts/create_fido2_attestation_cert

+    run_ssl_command(openssl_command)
+
+def parse_privkey(privkey_filename):
+    p = subprocess.Popen(['openssl', 'ec', '-in', privkey_filename, '-text', '-noout'], stdin=None, stdout=subprocess.PIPE, stderr=sys.stderr)


A slightly too long line.
stdin can be dropped, it's None by default: https://docs.python.org/3/library/subprocess.html#subprocess.Popen

x1ddos · 2020-04-29T18:01:04Z

scripts/create_fido2_attestation_cert

+    cert_filename = get_final_cert(base_dir)
+    csr_filename = os.path.join(base_dir, 'final_csr.csr')
+    run_ssl_command(['openssl', 'req', '-new', '-key', get_final_privkey(base_dir), '-out', csr_filename,
+                       '-subj', '/C=CH/ST=Zurich/L=Adliswil/O=Shift Cryptosecurity AG/OU=Authenticator Attestation/CN=Shift Cryptosecurity/[email protected]'


Also, a bit too long lines here and in other places, harder to review.

x1ddos · 2020-04-29T18:01:51Z

scripts/create_fido2_attestation_cert

+                       '-subj', '/C=CH/ST=Zurich/L=Adliswil/O=Shift Cryptosecurity AG/OU=Authenticator Attestation/CN=Shift Cryptosecurity/[email protected]'
+    ])
+    print("**** Signing final certificate ****")
+    openssl_command = ['openssl', 'x509', '-req', '-in', csr_filename, '-extfile', ext_file, '-CA', get_root_cert(base_dir), '-CAkey', get_root_privkey(base_dir), '-CAcreateserial', '-outform', 'DER', '-out', cert_filename, '-days', '10000'


note to self: is 10000 days a good number?

x1ddos · 2020-04-29T18:02:49Z

scripts/create_fido2_attestation_cert

+    cert_filename = get_root_cert(base_dir)
+    privkey_filename = get_root_privkey(base_dir)
+    openssl_command = ['openssl', 'req', '-new', '-x509', '-key', privkey_filename, '-outform', 'PEM', '-out', cert_filename, '-days', '10000',
+                       '-subj', '/C=CH/ST=Zurich/L=Adliswil/O=Shift Cryptosecurity AG/CN=Shift Cryptosecurity/[email protected]'


Is this for testing? Actually, I think we'll need to change this anyway, and I'd drop ST, L and emailAddress.

x1ddos · 2020-04-29T22:01:29Z

src/fido2/fido2_keys.c

+        0xd9, 0x6e, 0x5e, 0x56, 0x34, 0x15, 0x98, 0xa6, 0x98, 0xd6,
+        0xd4, 0x9e, 0x5d, 0x15, 0x4f, 0xef, 0x04, 0xe3, 0x45, 0x5c,
+        0x19, 0x51, 0xd6, 0x31, 0x79, 0x00, 0x83, 0x20, 0x52, 0xae,
+        0x03, 0x71


Is this supposed to actually be checked in?

x1ddos · 2020-04-29T22:05:41Z

src/memory/memory.c

+    fido2_resident_key_chunk_t chunk = {0};
+    CLEANUP_CHUNK(chunk);
+    _read_chunk(_ctap_resident_key_chunk(key_idx), chunk.bytes);
+    /** TODO: simo: implement */


implement what? :)

x1ddos · 2020-04-29T22:19:06Z

src/memory/memory.c

+
+static int _ctap_resident_key_chunk(int key_idx)
+{
+    return CHUNK_3 + key_idx / MEMORY_CTAP_RESIDENT_KEYS_PER_CHUNK;


What if key_idx is 46? Then I suppose _read_chunk below tries reading from an invalid chunk 5. Haven't checked but can _read_chunk handle that?

x1ddos · 2020-04-29T22:23:25Z

src/u2f.c

+     * for blocking U2F requests, but false for CTAP
+     * requests (as CTAP requests are not sent multiple times).
+     */
+    bool allow_cmd_retries;


In other words, true == U2F mode and false is CTAP?

x1ddos · 2020-04-29T22:33:37Z

src/usb/usb.c

 #endif

-#define TIMEOUT_TICK_PERIOD_MS 100
+#define TIMEOUT_TICK_PERIOD_MS 50


I wonder how this can impact other non-U2F operations.

Looks like the value might reused here but without using a common definition?

https://github.com/digitalbitbox/bitbox02-firmware/blob/master/src/usb/usb_processing.c#L43-L44

So that might need to be updated too.

And this comment: https://github.com/digitalbitbox/bitbox02-firmware/blob/master/src/usb/usb_processing.c#L119

x1ddos · 2020-05-19T10:41:04Z

@conte91 should I take it over or you're still on this?

Going to take this over, cherry-picking with updates and rebasing, and sending PRs to fido2 branch. When all is done, will merge fido2 with master.

Let's keep this PR open for now, as a reference.

benma · 2020-08-03T23:11:18Z

In Chromium on Linux, cancelling the u2f (not fido2) registration process during the password unlock does not cancel the unlock workflow. It still works on Firefox on Linux. Haven't tested other combinations.

This works on master on both Chromium and Firefox on Linux, so it is a regression of this PR.

edit: enabling passwordless login during reg at https://demo.yubico.com/playground does not work at all for me actually. On Chromium, I can pass unlock but after that nothing happens. ~~On FF, the unlock does not even appear~~ (edit: FF does not support fido2, so that part is okay).

Possibly memory issues?

benma · 2020-08-03T23:32:14Z

I rebased this PR on current master (2a8c435) here:

https://github.com/benma/bitbox02-firmware/tree/fido2

All comments of this PR still apply, I haven't changed any semantics (not deliberately, anyway).

conte91 force-pushed the fido2 branch from 3e0af6f to b5f622b Compare March 30, 2020 15:16

conte91 and others added 15 commits March 30, 2020 17:16

u2f: Split U2F keyhandle generation/verification.

4ebd905

Move the functions to generate/verify U2F keyhandles to a separate module.

u2f: Improve #define names.

dd3dc43

Rename U2F_KEY_SIZE to U2F_COORD_SIZE for better clarity. The "32" refers to the size of one of the coordinates of the (x, y) point on the EC. The actual "key" is referred to as "point" in the same header and has size U2F_POINT_SIZE.

Squash me: Revert the crappy change to U2F_EC_KEY_SIZE.

d84e8fa

u2f: Extract a function to create new keys.

2a9a2cd

Create a new function in u2f_keyhandle that creates new keys. This makes the code cleaner as a side effect.

Bring the timer for U2F timeouts down to 50ms

d9319b2

FIDO2: Add script to generate FIDO2 certificate.

cdea7e5

Squash me: allocate packet memory on the stack.

6ea40fc

Add FIDO2 support to the BB02-multi firmware.

ecfa287

Add timer on the U2F stack for CTAP keepalives.

b03bf65

Add proper memory backing to FIDO2

72fd7db

scroll_through_all_variants: Accept a custom callback parameter.

5ab351f

FIDO2: Select credential to be returned when logging in.

1aa154f

Fix user menu for fido2

134903f

FIDO2: Don't select user if there's only one to choose from.

d41647d

conte91 force-pushed the fido2 branch from b5f622b to d41647d Compare March 31, 2020 15:27

benma requested changes Apr 5, 2020

View reviewed changes

benma mentioned this pull request Apr 14, 2020

u2f: use Rust unlock/confirm workflows #467

Merged

x1ddos mentioned this pull request Apr 22, 2020

More known U2F App IDs #472

Merged

x1ddos reviewed Apr 29, 2020

View reviewed changes

x1ddos changed the title ~~Fido2~~ Fido2 - conte91 May 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fido2 - conte91 #437

Fido2 - conte91 #437

conte91 commented Mar 30, 2020

benma Apr 5, 2020

x1ddos Apr 29, 2020

x1ddos Apr 29, 2020

x1ddos left a comment

x1ddos Apr 29, 2020

x1ddos Apr 29, 2020

x1ddos Apr 29, 2020

x1ddos Apr 29, 2020

x1ddos Apr 29, 2020

x1ddos Apr 29, 2020

x1ddos Apr 29, 2020

x1ddos Apr 29, 2020

x1ddos Apr 29, 2020 •

edited

Loading

x1ddos Apr 29, 2020

x1ddos Apr 29, 2020

benma May 18, 2020 •

edited

Loading

benma May 18, 2020

x1ddos commented May 19, 2020

benma commented Aug 3, 2020 •

edited

Loading

benma commented Aug 3, 2020

Fido2 - conte91 #437

Are you sure you want to change the base?

Fido2 - conte91 #437

Conversation

conte91 commented Mar 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

x1ddos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

x1ddos Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benma May 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

x1ddos commented May 19, 2020

benma commented Aug 3, 2020 • edited Loading

benma commented Aug 3, 2020

x1ddos Apr 29, 2020 •

edited

Loading

benma May 18, 2020 •

edited

Loading

benma commented Aug 3, 2020 •

edited

Loading