Skip to content
This repository has been archived by the owner on Aug 6, 2024. It is now read-only.

client_pin_new_requirements_set_pin and client_pin_new_requirements_change_pin pin padding is to 64 bytes not 32 #89

Open
ve7jtb opened this issue Nov 19, 2020 · 5 comments
Assignees

Comments

@ve7jtb
Copy link

ve7jtb commented Nov 19, 2020

In CTAP2.1 the max pin length is 63 bytes and is padded out to 64 bytes.
In CTAP2.0 "The decrypted padded newPin should be of at least 64 bytes length"
So 64 bytes is what should be tested for.

Just for fun CTAP2.0 doesn't define errors.
For CTAP2.1 the error for too short is CTAP1_ERR_INVALID_PARAMETER someone needs to speak up if they want that changed.

For paddedNewPin being longer than 64 bytes nether spec mentions an error.

I will see about fixing that in CTAP2.1

@ve7jtb
Copy link
Author

ve7jtb commented Nov 19, 2020

Related CTAP2.1 issue https://github.com/fido-alliance/fido-2-specs/issues/1115

@kaczmarczyck
Copy link
Contributor

LGTM and thanks for the spec fix for longer padded PINs.

Here's my reasoning for the test. CTAP 2.0 says:

The decrypted padded newPin should be of at least 64 bytes length and authenticator determines actual PIN
length by looking for first 0x00 byte which terminates the PIN.

Therefore, CTAP 2.1 seemed like a clarification to me. The sentence above seems to imply at least one byte of padding, and 2.1 makes this explicit. Also, this sentence speaks about "padded" first, which is correctly stated to be 64 bytes at least, and then later talks about "actual" length. This test is checking for the "actual" PIN length.

For more anecdotal evidence, I think this is the "majority" interpretation on my small sample of tested devices.
To be fair, I always found this sentence strange. Because "looking for the first" byte implies that it's important from what direction you start looking.

@kaczmarczyck
Copy link
Contributor

@ve7jtb The title has a get_assertion_empty_user_id, but doesn't mention it. Is this by mistake?

@kaczmarczyck kaczmarczyck self-assigned this Nov 23, 2020
@ve7jtb ve7jtb changed the title client_pin_new_requirements_set_pin and get_assertion_empty_user_id pin padding is to 64 bytes not 32 client_pin_new_requirements_set_pin and client_pin_new_requirements_change_pin pin padding is to 64 bytes not 32 Nov 23, 2020
@ve7jtb
Copy link
Author

ve7jtb commented Nov 23, 2020

I fixed the title. Cut and paste error.

Perhaps it is the error description that confused me "Accepted a PIN with padding of length 32"

should that be "Accepted a PIN with padding of length less than 64" I guess practically the only length less than 64 is 32 that you can test.

@kaczmarczyck
Copy link
Contributor

Oh, I thought this is about actual PIN length vs padded PIN length. Your last error message sounds more like a padded PIN length problem. Are you saying the test is incorrect, or just the message misleading?

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

No branches or pull requests

2 participants