Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fido2 - conte91 #437

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Fido2 - conte91 #437

wants to merge 15 commits into from

Conversation

conte91
Copy link
Contributor

@conte91 conte91 commented Mar 30, 2020

Anything else to say?

conte91 and others added 15 commits March 30, 2020 17:16
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.
@@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for moving to external/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@x1ddos x1ddos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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]'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'-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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note to self: is 10000 days a good number?

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]'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to actually be checked in?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implement what? :)


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

@x1ddos x1ddos Apr 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#endif

#define TIMEOUT_TICK_PERIOD_MS 100
#define TIMEOUT_TICK_PERIOD_MS 50
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@benma benma May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@x1ddos x1ddos changed the title Fido2 Fido2 - conte91 May 4, 2020
@x1ddos
Copy link
Contributor

x1ddos commented May 19, 2020

@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
Copy link
Collaborator

benma commented Aug 3, 2020

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
Copy link
Collaborator

benma commented Aug 3, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants