sk-usbhid: preserve UV requirement for resident keys

[savely-krasovsky]

What changed

Derive SSH_SK_USER_VERIFICATION_REQD directly from a resident credential's
credProtect policy when loading it from an authenticator.

Set the flag only when the credential reports
FIDO_CRED_PROT_UV_REQUIRED, independently of whether the authenticator
currently advertises built-in user verification through the uv option.

The policy check is guarded by HAVE_FIDO_CRED_PROT for compatibility with
older libfido2 versions.

Why

Resident credentials created with verify-required store
FIDO_CRED_PROT_UV_REQUIRED as a per-credential policy on the authenticator.

During ssh-keygen -K and ssh-add -K, read_rks() currently propagates this
policy as SSH_SK_USER_VERIFICATION_REQD only when the authenticator does not
advertise the uv capability.

As a result, authenticators with built-in user verification download
verify-required resident keys with flags 0x21 instead of 0x25.

The authenticator's uv option describes a device capability, while
credProtect describes the policy of an individual credential. The credential
policy should therefore be preserved independently of the authenticator's
current UV capabilities.

Compatibility

Older FIDO 2.0 authenticators and credentials that do not expose a
credProtect policy are unaffected. In that case, libfido2 reports no matching
FIDO_CRED_PROT_UV_REQUIRED value, so the OpenSSH UV-required flag is not added.

When OpenSSH is built against a libfido2 version without
fido_cred_prot(), policy inference is skipped. This also avoids treating the
compatibility fallback value for an unavailable credProtect API as
UV_REQUIRED.

The change only affects reconstruction of flags while loading resident keys
through ssh-keygen -K and ssh-add -K. Credential enrollment, signing, and
non-resident credentials are unchanged.

Validation

Built the portable tree with --with-security-key-builtin and libfido2 1.16.

Tested on an authenticator with built-in user verification:

• A resident credential with UV_OPTIONAL_WITH_ID (0x02) downloaded with
  OpenSSH flags 0x21.
• A resident credential with UV_REQUIRED (0x03) downloaded with OpenSSH
  flags 0x25.
• Enrolled a new resident credential with verify-required and confirmed that
  it downloaded with flags 0x25.
• Confirmed that signing with the downloaded key still required user
  verification and user presence and completed successfully.

[My1]

okay I dug a little appanrently this is a case of nice idea, with not the best of executions.

f3c34df

the idea was to skip pin prompts on keys with UV but:

1. it doesnt seem to care whether UV is on or off
2. UV can be turned off later (or be blocked) so this shouldnt be an option stored in the keystub to begin with
3. UV can mean both user verification in General (so client-side PIN OR internal methods to the authenticator like a fingerprint, or an on-device pin) or speicifically user verification using internal methods.

in my opinion, the choice on whether to specifically ask for a client-side PIN should not be in the key stub to begin with, just the info on whether the credential needs User verification(generic) or not. with the method to be used being something that should solely be chosen at the point of authentication.

At this point one could check

1. does the device support internal UV and if yes, is it active?
2. is UV blocked
3. is there potentially an "I want to use my PIN" override (could be a flag to skip internal UV if you e.g. know that fingerprint wont work, e.g. when you have chalk on your fingers as a teacher).

and if the answers are specifically both yes, no and no, then to skip asking for PIN, even if the credential says I want UV.

obvious edge case: device with ONLY internal UV (mainly things like Cryptocoin wallets with FIDO): IF UV is blocked, just fail directly.

[savely-krasovsky]

Thank you for the feedback. I agree that this UV should not have been stored in the first place, but the decision was made and now I don't think we can just drop it, at least for this exact key stub file version.

People already have those files with the UV flag enforced based on the fact that the "uv" option was true at the moment they ran ssh-keygen -K. A sudden migration would break this behavior, and the authenticator would start to use default policy (which could be less secure).

[My1]

does it let you make credentials with UV required if the key doesnt have Cred Protect? at some point i tried using SSH with FIDO it blocked me for using a key that didnt have it.

because if cred protect is required anyway, that wouldnt be a big issue, also OBVIOUSLY the server should be the one to enforce UV on the authentication anyway.

also the fact to store UV requirements in the file isnt even the big "issue". the issue is that at least the change assumed "stored uv means ask for PIN", which obviously is not the point.

the file can keep the info whether UV as it helps against cred protect bonking when the client isnt aware, just the idea WHICH UV is actually used should not be decided upon a file flag.

so I agree with the pull request putting the UV info back and at the same time, the prompt for PIN should be revamped to check for internal UV availability.

[savely-krasovsky]

No, you cannot use ssh FIDO 2.0 keys without credProtect even without my patch. So yes, FIDO 2.1+ keys required, not sure if credProtect is a mandatory part of CTAP 2.1 though.

[My1]

well if credprotect is mandatory for SSH anyway, whether the option is set in the key stub is basically not relevant anyway especially as the keystub isnt exactly the most tamper proof thing so if someone actually wants to change the flag in the keystub they likely could easily which is why these things should be enforced by the server and maybe credprotect.

[savely-krasovsky]

My change from that perspective is pretty safe I think. Logic re-haul could be a part of dedicated patchset.

[My1]

absolutely. your change effectively just reverses a change to make authenticators with internal UV ask for PIN less, that had (in my opinion) a bunch of oversights anyway.