Reduce credential ID size

[robin-nitrokey]

It is still possible to overflow the credential ID when creating a non-discoverable credential, especially by providing a long RP ID. (The allowed maximum is 256, and the maximum credential ID length is 255.) Stripping some of the metadata was not enough to solve the problem. To improve this, I suggest the following changes:

1. Introduce a separate type for the stripped credential. Currently, we just set the stripped fields to None. This provides the potential for subtle bugs if we assume some field to be set that has been stripped. Therefore I suggest to introduce a new StrippedCredential type that only includes the relevant fields.

2. Flatten the serialized data structure. Currently, we have three nested levels: Credential → CredentialData → PublicKeyCredentialRpEntity. This adds unnecessary overhead.

3. Remove unused fields. Having a separate type for stripped credentials makes it possible to identify the fields that are never used. Currently, these are:

   • rp_id: String<256>
   • creation_time: u32
   • use_counter: Option<bool>
   • hmac_secret: Option<bool>

   rp_id is obviously the most problematic one. It remains to be investigated if we really don’t need it or if this is a problem with the current implementation.

[robin-nitrokey]

If an allowList is present and is non-empty, locate all denoted credentials present on this authenticator and bound to the specified rpId.

https://fidoalliance.org/specs/fido-v2.0-ps-20190130/fido-client-to-authenticator-protocol-v2.0-ps-20190130.html#authenticatorGetAssertion

I think this means that we should make sure that the credential provided in allowList actually belongs to the RP. We already use the RP ID hash as associated data when encrypting the credential data. So I think it is safe to just remove the full RP ID from the stripped credential data.

[szszszsz]

Good spot! With AEAD we do not need RPID encoded, and the rest is not needed.

Edit: this could improve working on the older Nextcloud instances - here a case, where we overflow exactly 1 byte on the derived key:

• Passwordless WebAuthn with Nitrokey 3 does not work nextcloud/server#34476 (comment)

[robin-nitrokey]

I’ve drafted a PR against the Nitrokey fork because it has fixed some formatting issues and uses a newer Trussed version: Nitrokey#32. Please review there. I’ll backport it for the upstream repository once it is finalized, reviewed and approved.

[spectras]

I have been tracking an issue with Nitrokey 3 and it seems this is the root cause.

• My key works on gitlab.com
• But it does not on my local instance.

After trial and error on a browser's console:

• I realized that passing a shorter user handle to navigator.credentials.create made it work.
• By experimenting with user handle lengths, I realized it would fail when the returned rawId size would exceed 255 bytes.
• By comparing the results when running on my local instance and on gitlab.com I realized the that the returned rawId is longer on my local instance.

⇒ It appears it is built from the hostname. Which is problematic as hostnames can be very long.

I checked with my Yubikey, and the rawId size is fixed. It looks like it computes a hash the fields instead.

We are looking forward to this change, as this issue makes Nitrokeys unusable on most sites on our intranet (on-premise gitlab and nextcloud). Thanks for your work on it!

[robin-nitrokey]

@spectras This issue should be fixed for the Nitrokey 3 with v1.5.0-test.20231030.

[spectras]

Thanks! I will get people to upgrade their firmware!

[robin-nitrokey]

Fixed in Nitrokey#32 and merged upstream in #41.