Fido2 follow up

[Ollrogge]

Contribution description

This PR adjusts the FIDO2 CTAP flash handling to reserve flash memory at compile time using a static const buffer. This ensures that the FIDO2 CTAP implementation does not interfere with other modules wanting to use flash memory and doesn't corrupt firmware or e.g. bootloaders at the end of flash. Furthermore configuring CONFIG_FIDO2_CTAP_NUM_FLASHPAGES such that more flash than available is required will fail at build time rather than runtime.

Testing procedure

• tests/sys_fido2_ctap

I successfully ran the tests on the nrf52840dongle and nrf52840dk.

[PeterKietzmann]

Tested with a nrf52840dk:

• Compiled with default number of flashpages -> got 4.
• Compiled with 8 flashpages -> got 8.
• Compiled with CFLAGS=-DCONFIG_FIDO2_CTAP_NUM_FLASHPAGES=1 and 257 -> fails(expected).
• Unit tests succeed 126 passed, 7 skipped, 7 deselected in 149.62s (0:02:29).
• Registration and login with 4 accounts still works on webauthn.io.

[PeterKietzmann]

The changes look good in general, but I think the documentation can be improved. From a user perspective: I can now change the number of flashpages (jippie!) but what does that mean, how should I configure that value? For example, I know that I want to serve 12 accounts with my authenticator, how many flashpages do I need?

[benpicco]

A bit unrelated, but what made you chose TinyCBOR instead of NanoCBOR for CBOR parsing?
NanoCBOR is the default in RIOT (and used for SUIT update), so if you really enable firmware updates you'll end up with two CBOR libraries linked in.

[Ollrogge]

A bit unrelated, but what made you chose TinyCBOR instead of NanoCBOR for CBOR parsing? NanoCBOR is the default in RIOT (and used for SUIT update), so if you really enable firmware updates you'll end up with two CBOR libraries linked in.

Honestly I can't remember. This decision was made really early when I barely had any experience with RIOT. I think someone recommended TinyCBOR and I just went with it

[chrysn]

The way flashpage_{first,last}_free is used here begs clarification from their documentation.

If I came to these functions without the context of the present PR, I'd assume that these "free" pages are free for the application to use.

Here, something internal to RIOT uses them -- and my expectation of a modules system is that I can enable any additional modules and my application would still run unimpeded.

A possible way out here is for the flashpage_*_free mechanism would be to keep track of that some system component has "claimed" a chunk of flash (and because that happens in ordered start code, they get the same pages across reboots) -- but that would be doing a check at runtime that could also be done at built time.

(In own applications, I've always allocated flash like

static const uint8_t backing_memory[FLASHPAGE_SIZE] __attribute__((aligned(FLASHPAGE_SIZE))) = {0};

...
    int flashpage = flashpage_page((void*)backing_memory);

which cooperates nicely with the flashpage_*_free mechanism).

[Ollrogge]

@chrysn thanks for the suggestions ! I think your suggested aproach is the best solution. I updated the code accordingly.

[PeterKietzmann]

@chrysn thanks for the suggestions ! I think your suggested aproach is the best solution. I updated the code accordingly.

Does this still interfere with the firmware update case?

@Ollrogge can you update the documentation?

[Ollrogge]

@chrysn thanks for the suggestions ! I think your suggested aproach is the best solution. I updated the code accordingly.

Does this still interfere with the firmware update case?

To my understanding yes. This does still interfere with the firmware update case.

@Ollrogge can you update the documentation?

Which documentation do you think should be updated ?

[PeterKietzmann]

In sys/fido2/doc.txt a note on 1. how flash is allocated and 2. firmware updates are not possible with our fido2 implementation for thisandthat reason. @chrysn other things to document?

[chrysn]

@chrysn other things to document?

Not in here, now that it's not using flashpage_*_free any more. If the changes here removed the necessity to have flashpage_first_free and flashpage_last_free altogether (which AIU was prepared in a first stage of this PR), it may be worth considering deprecating (or removing, not having been in a release) them right away again, or documenting how they are supposed to be used, but there's nothing in this PR any more that makes that belong here.

[Ollrogge]

Is is executed when you send it :) It is the same kind of command like e.g. MakeCredential.

"Send it" from where? Is it a feature on webauthn.io, an other existing "client" implementation, our unit tests? Or do I have to write my own application to reset? In the latter case I would at least expect a little RIOT helper tool for that.

Therefore the PIN will still work.

I understand. My major concern is the absence, or missing documentation on how to reset it (reflashing "any other" application is not a solution).

I added a python script to the test application directory that allows for resetting the authenticator. Also documentated it in the README.
I also added full erasure of all flashpages containing CTAP data when the authenticator receives the reset command.

[PeterKietzmann]

• make fido2-up -> success
• register/login few accounts with residential keys -> success
• power off and login again -> success
• re-flash does not destroy the credentials (as agreed) -> success
• reset (by python script) removes the pin&credentials -> success
• old pin doesn't work anymore -> success
• lowering the number of flashpages to two throws an error after creating > 26 accounts -> success

Can you please verify one aspect (maybe I just did a mistake during monkey-creating several accounts): it seems that after hitting the maximum number of accounts (at 27 accounts with 2 flashpages) old accounts cannot login anymore.

[PeterKietzmann]

Otherwise, the PR looks good and the reset abilities really improve our implementation ;)

[PeterKietzmann]

Can you please verify one aspect

A bit more specifically:

• Registering 25 accounts (resident key required) works well and all seem to work for login.
• Registering account number 26 indicates success but after registering it, no login works anymore (like screenshot above).
• Registering account number 27 throws the error about out of memory.

[Ollrogge]

Can you please verify one aspect

A bit more specifically:

* Registering 25 accounts (resident key required) works well and all seem to work for login.

* Registering account number 26 indicates success **but** after registering it, no login works anymore (like screenshot above).

* Registering account number 27 throws the error about out of memory.

Found the issue, thanks for testing so rigorously. The problem was the wrong location of this check. _save_rk is also called when a rk is updated (e.g. during get_assertion). Therefore the check should only be executed if there is actually a new rk to be stored, not if one is just updated.

[Ollrogge]

Regarding the commit messages: should I just call the whole thing "fido2 follow up" or divide further?

[PeterKietzmann]

• make fido2-test -> success
• register/login few accounts with residential keys -> success
• power off and login again -> success
• reset (by python script) removes the pin&credentials -> success
• lowering the number of flashpages to two throws an error after creating > 26 accounts -> success
• old accounts still work -> success

[PeterKietzmann]

Think I would like three commits:

• flashpage stuff & recent fix
• reset command and script
• cleanup and documentation

What do you think?

[PeterKietzmann]

Murdock fails...

[PeterKietzmann]

What is actually the desired behavior? Silently disable user presence on boards that do not expose a pin? Or better throw a warning and exclude these boards from CI? Throw a warning here and disable user presence in the test by default?

[Ollrogge]

I think we had a discussion about this when I added these lines and we agreed on the first approach.
Murdock complained about a symbol being redefined. I hopefully fixed this issue now and it should work like the first approach you mentioned above with the recent changes.

[PeterKietzmann]

Doesn't seem to be fixed. Maybe run the compile test locally before.

[Ollrogge]

Sorry. Didn't think about just trying to compile locally with the failing boards. Did that now

[Ollrogge]

Success :)

[PeterKietzmann]

All good. Green lights, go!