CTAP 2.3#54
Conversation
0x0ece
force-pushed
the
0x0ece/ctap
branch
4 times, most recently
from
8713128 to
0e5245f
Compare
…erableCredentials
robin-nitrokey
left a comment
robin-nitrokey
left a comment
There was a problem hiding this comment.
First batch of comments. I’ve already prepared the suggested changes so you don’t have to do any changes yourself, just take a look at the comments and questions and let me know if you agree.
| let pin_auth = request.pin_auth.ok_or(Error::MissingParameter)?; | ||
|
|
||
| // pinUvAuthData = 0xff * 32 || 0x0d || subCommand || subCommandParams (CBOR) | ||
| let mut data: Bytes<{ 32 + 2 + sizes::MAX_CREDENTIAL_ID_LENGTH }> = Bytes::new(); |
There was a problem hiding this comment.
Where does this size come from?
There was a problem hiding this comment.
Placeholder never properly cleaned up :(
Maybe we should add a const to ctap-types? This should be the precise size, but we could also add some extra just in case.
pub const MAX_AUTH_CONFIG_PARAMS_LENGTH: usize =
1 // header
+ 3 // newMinPINLength
+ 2 + MAX_MIN_PIN_LENGTH_RP_IDS
* (2 + MAX_RP_ID_LENGTH) // minPinLengthRPIDs
+ 2; // forceChangePIN
robin-nitrokey
left a comment
robin-nitrokey
left a comment
There was a problem hiding this comment.
I have some more questions regarding the minPinLength implementation. We should definitely have some unit tests for this feature before merging it to make sure that all edge cases are handled correctly.
| // floor, force the platform to change it. We can't measure the | ||
| // existing PIN length here (only its hash is stored), so we set the | ||
| // flag unconditionally on any tightening. | ||
| if self.pin_hash.is_some() { |
There was a problem hiding this comment.
Why don’t we store the PIN length together with the PIN hash?
There was a problem hiding this comment.
Personally, I would avoid it (it leaks info about the user pin).
There was a problem hiding this comment.
I see, this is a requirement from the spec though:
Authenticator remembers newPin length internally as PINCodePointLength.
Let’s think about it for a while and discuss next week how to deal with it.
| new_value: u8, | ||
| ) -> Result<()> { | ||
| // Spec: setMinPINLength may only raise the value, never lower it. | ||
| if new_value <= self.min_pin_length() { |
There was a problem hiding this comment.
Shouldn’t this be < instead of <=?
There was a problem hiding this comment.
lol - could the spec be more ambiguous? 😆
I guess if it's = you can either return err or success, the end result is the same.
I wanted to conservatively avoid any unnecessary state change.
Actually sorry, I think the spec says it should return err on <, which means it should succeed on =. So yes, we should fix.
|
|
||
| // impl<UP: UserPresence, T: TrussedRequirements> Authenticator for crate::Authenticator<UP, T> | ||
| impl<UP: UserPresence, T: TrussedRequirements> crate::Authenticator<UP, T> { | ||
| fn config_set_min_pin_length( |
There was a problem hiding this comment.
Isn’t this missing the handling of the forceChangePin parameter?
| // Successfully (re)setting the PIN clears any pending forcePINChange | ||
| // request — the platform has just complied (CTAP 2.1 §6.5.5.7). | ||
| self.force_pin_change = false; |
There was a problem hiding this comment.
I think this is missing the case where the PIN is set to the old value:
If the forcePINChange member of the authenticatorGetInfo response is true and LEFT(SHA-256(newPin), 16) is equal to its internal stored LEFT(SHA-256(curPin), 16) then authenticator returns CTAP2_ERR_PIN_POLICY_VIOLATION.
| let min_pin_length = self.state.persistent.min_pin_length() as usize; | ||
| if pin_length < min_pin_length || pin_length >= 64 { |
There was a problem hiding this comment.
When working on this, we should also try to test and fix this issue:
2 participants
See #53, deps need to be modified
commit overview: