Implement commands for management of resident keys

[rgerganov]

Implement command 0x41 which is used by OpenSSH for reading RKs. It has
the following subcommands:

• CMD_CRED_METADATA - get number of saved/remaining RKs
• CMD_RP_BEGIN/CMD_RP_NEXT - iterate over the saved RPs
• CMD_RK_BEGIN/CMD_RK_NEXT - iterate over the RKs for a given RP

This fixes issue #374

[rgerganov]

There are still some loose ends to deal with but it is pretty close

[jolo1581]

Think this is also related to my issue #314

[nickray]

Nice reverse engineering! Some comments:

• the planned CM command is 0x0A (although ssh using vendor command 0x41 means that one will be forever "burned"...), maybe respond to both?
• there is also a subcommand 0x06 to delete credentials (credentialID as PublicKeyCredentialDescriptor passed as subcommand parameter 0x02 after rpIDHash at 0x01; error NO_CREDENTIALS (0x2E) if non-existent).
• the subcommands 0x01, 0x02, 0x04, 0x06 should expect pinAuth as command parameter 0x04, and check that it's LEFT(HMAC-SHA-256(pinToken, <subcommand> || subCommandParams), 16), where <subcommand> is e.g. the byte 0x01 and subCommandParams is empty for 0x1, 0x2. Else error with PIN_AUTH_INVALID (0x33) or, on 3 consecutive mismatches, PIN_AUTH_BLOCKED (0x34).
• if there are no RKs for the RP in subcommand 0x03, return NO_CREDENTIALS (0x2E).
• technically, command parameter 0x03 (pinProtocol) should be checked to be 0x01.
• your implementations of 0x04/0x05 may never terminate, need to bound on STATE.rk_stored.
• subcommand 0x04 should return number of RKs for that RP, not overall, for privacy, so it needs a counting loop.

Does ssh send pinAuth? For privacy reasons I think it's mandatory to check it. Looking at https://github.com/openssh/openssh-portable/blob/9b47bd7b09d191991ad9e0506bb66b74bbc93d34/ssh-sk.c#L61-L70 I assume ssh does send it, if not, then I think we should only respond to command 0x41 (not 0x0A), and only for the ssh: RP to contain the privacy leak.

Implementing credential deletion needs some changes to assumptions e.g. on STATE.rk_stored, so probably better (as you are doing) not to implement it in this PR.

[rgerganov]

Thanks for the review, comments inline:

• the planned CM command is 0x0A (although ssh using vendor command 0x41 means that one will be forever "burned"...), maybe respond to both?

The command id 0x41 is in libfido2 which is used by OpenSSH. I guess they will change the number in libfido2 when the time comes. It won't hurt to support both I guess.

• there is also a subcommand 0x06 to delete credentials (credentialID as PublicKeyCredentialDescriptor passed as subcommand parameter 0x02 after rpIDHash at 0x01; error NO_CREDENTIALS (0x2E) if non-existent).

Yes, let's implement this in another patch.

• the subcommands 0x01, 0x02, 0x04, 0x06 should expect pinAuth as command parameter 0x04, and check that it's LEFT(HMAC-SHA-256(pinToken, <subcommand> || subCommandParams), 16), where <subcommand> is e.g. the byte 0x01 and subCommandParams is empty for 0x1, 0x2. Else error with PIN_AUTH_INVALID (0x33) or, on 3 consecutive mismatches, PIN_AUTH_BLOCKED (0x34).

I was about to debug how pinAuth is computed and put the relevant checks in the code. Thanks for the hint!

• if there are no RKs for the RP in subcommand 0x03, return NO_CREDENTIALS (0x2E).
• technically, command parameter 0x03 (pinProtocol) should be checked to be 0x01.

Thanks, I will address this.

• your implementations of 0x04/0x05 may never terminate, need to bound on STATE.rk_stored.

Yeah, I have been sloppy here, will fix this.

• subcommand 0x04 should return number of RKs for that RP, not overall, for privacy, so it needs a counting loop.

Correct.

Does ssh send pinAuth? For privacy reasons I think it's mandatory to check it. Looking at https://github.com/openssh/openssh-portable/blob/9b47bd7b09d191991ad9e0506bb66b74bbc93d34/ssh-sk.c#L61-L70 I assume ssh does send it, if not, then I think we should only respond to command 0x41 (not 0x0A), and only for the ssh: RP to contain the privacy leak.

Yes, it does send pinAuth, will add the checks.

Implementing credential deletion needs some changes to assumptions e.g. on STATE.rk_stored, so probably better (as you are doing) not to implement it in this PR.

We should also discuss changes in how RKs are stored that may break backward compatibility. For example right now we don't store rpId which is retrieved with CMD_RP_BEGIN/CMD_RP_NEXT

[rgerganov]

Addressed some of the comments from @nickray

• added bound checks when iterating over the RKs
• return the number of RKs for the specified RP when handing CM_cmdRKBegin
• return CTAP2_ERR_NO_CREDENTIALS when appropriate

Next thing will be parsing and checking pinAuth. We have this for CM_cmdMetadata, CM_cmdRPBegin and CM_cmdRKBegin. This is how the commands look like:

{1: 1, 3: 1, 4: h'8F7AEAC7A06790A086D97279841F7DF6'}
{1: 2, 3: 1, 4: h'79667456537537D652B306A69855D640'}
{1: 3}
{1: 4, 2: {1: h'E30610E8A162115960FE1EC223E6529C9F4B6E80200DCB5E5C321C8AF1E2B1BF'}, 3: 1, 4: h'BF126409013EC15D52412943F49853F4'}
{1: 5}

[rgerganov]

Checking pinAuth is implemented. I have tested with both HIDG and real hardware and everything seems to work with OpenSSH :)

@nickray We already have unsuccessful attempts count and device locking in CTAP_CLIENT_PIN which is sent before credMgmt commands. Do we need this for credMgmt as well?

[nickray]

From the point of view of the spec, there should be a persistent retries counter, with the properties:

• reset to 8 each time pinAuth is successfully verified
• after 3 consecutive failed attempts, PIN_AUTH_BLOCKED and require power cycle for any operations that require a PIN
• after 8 consecutive failed attempts, full key reset

So this behaviour is needed also for subcommands 0x01, 0x02, 0x04, 0x06 here.

--

Not necessarily a todo for you, but I think we need more tests in https://github.com/solokeys/fido2-tests before we can merge this.

[rgerganov]

From the point of view of the spec, there should be a persistent retries counter, with the properties:

• reset to 8 each time pinAuth is successfully verified
• after 3 consecutive failed attempts, PIN_AUTH_BLOCKED and require power cycle for any operations that require a PIN
• after 8 consecutive failed attempts, full key reset

So this behaviour is needed also for subcommands 0x01, 0x02, 0x04, 0x06 here.

OK, I will try to address this

--

Not necessarily a todo for you, but I think we need more tests in https://github.com/solokeys/fido2-tests before we can merge this.

Done: trussed-dev/fido2-tests#33

[rgerganov]

I updated the PR to address last comment from @nickray

[conorpp]

If an unsolicited CM_cmdRPNext or CM_cmdRKNext was sent, this would trigger the retries counter to get reset? Then there would be a way for malware to have unlimited PIN attempts. Ideally this should only done for commands that need pinAuth, and there should be a check that the "next" commands come after a valid command.

[rgerganov]

Good catch. I will rework the pinAuth handling and add tests for these scenarios.

[rgerganov]

I have extracted the pinAuth handling into a separate function and introduced some flags to guard against calling Next commands without corresponding Begin. Tests are also updated.

[conorpp]

I think this is still vulnerable, but now if an invalid sub command is sent, it will reset the pin attempts. Maybe all of the attempt decrement + reset logic should be placed in the verify_pin_auth_ex function to be safe?

[rgerganov]

verify_pin_auth_ex is called by verify_pin_auth so if I move the checks there it will have implications to the rest of the code.
I will change line 1299 to check if the command should have pinAuth and return if this is not the case.

[conorpp]

Ok, looks good!

[conorpp]

This is awesome, thank you! I'm going to work on running against Microsoft tests, and adding the credProtect extensions. Will make an official update soon.