libfido2-util: fix a regression in the pre-flight mechanism

[kszczek]

The recently merged PR #32295 introduced support for the credProtect extension, but in doing so, it broke the discoverability of credentials by setting the policy to FIDO_CRED_PROT_UV_REQUIRED. This policy would require us to pass the PIN to the token in the pre-flight request to be able to discover it, which defeats the purpose of pre-flight requests as they're supposed to be non-interactive.

Instead of completely removing credProtect support, we could just relax the policy to FIDO_CRED_PROT_UV_OPTIONAL_WITH_ID, but I decided against that for two reasons:

1. This issue would still occur for an edge case described in the CTAP 2.1 specification.

   Note: Some authenticators for high-security environments may be configured to always set credProtect 3 for all created credentials regardless of what the platform requests...

2. FIDO_CRED_PROT_UV_OPTIONAL_WITH_ID policy seems to be useful only for resident keys, because you always have to pass the credential ID anyway for non-resident keys, so this policy would always be fulfilled, as we use non-resident keys.

I stumbled upon this issue while trying to create a user's home directory with a FIDO2 token using homectl, systemd-homed and systemd-homework from the tip of the main branch.

# ./homectl create --fido2-device=auto test
Initializing FIDO2 credential on security token.
👆 (Hint: This might require confirmation of user presence on security token.)
🔐 Please enter security token PIN: ••••••                  
Generating secret key on FIDO2 security token.
👆 In order to allow secret key generation, please confirm presence on security token.
Password suggestions: ERXuJyl6ocub7i Eg@KUKINAt3fsEx arinUd0JiBx@rUh MyzhohwiMzeLS ax0BAPOBPoKwaK oNarDes3JYq=EK
🔐 Please enter new password for user test: ••••••••                
🔐 Please enter new password for user test (repeat): ••••••••                
👆 Please confirm presence on security token.
Operation on home test failed: Failed to execute operation: Resource temporarily unavailable

Snippet from systemd-homed's journal (running with SYSTEMD_LOG_LEVEL=debug)

The credential is not in the token /dev/hidraw2, skipping.
Got notify message lacking both ERRNO= and SYSTEMD_LUKS_LOCK_FD= field, ignoring.
Worker reported error code EAGAIN.
Operation on test failed: Resource temporarily unavailable

[github-actions]

Important

An -rc1 tag has been created and a release is being prepared, so please note that PRs introducing new features and APIs will be held back until the new version has been released.

[YHNdnzj]

cc @BryanJacobs

[BryanJacobs]

If you want to discover tokens without requiring a PIN (knowing that to use that token later the PIN will be required), then using credProtect level 2 is appropriate.

Doing this would make possible (again) the problem from the other ticket though: if the token is created without a PIN then the disk will be encrypted using the "wrong" secret, and decryption will fail.

Is systemd-homed using the hmac-secret extension the same as systemd-cryptenroll? If so, it might be better to change the path to prompt the user for their PIN if the home directory was set up with a credential requiring a PIN...

[BryanJacobs]

My understanding of the state of things if this PR is merged:

• systemd-homed will be able to discover credentials UNLESS the authenticator is configured with alwaysUv
• systemd-cryptsetup will function properly (thanks to the other un-reverted half of cryptenroll: Use CTAP2.1 credProtect extension #32295 )
• if a change is made to either to codepath to allow using a credential without UV when it was set up using UV, systemd-cryptenroll not requesting a PIN when enrolling a token using fido2-with-client-pin #31443 will be reintroduced

Seems generally okay, but I think the ideal solution would be to track whether a certain credential requires UV for usage, and if so, also require UV for discovery. That's one user interaction in the case the correct credential is present, and unlike the other proposed solutions, it works in every case (including alwaysUv authenticators).

[kszczek]

Then I guess we could use the credProtect extension only when using UV, but not when using a PIN without UV. This wouldn't break the pre-flight, as it explicitly doesn't support UV:

systemd/src/shared/libfido2-util.c

Lines 321 to 330 in eb1ec9c

	/* FIDO2 devices may not support pre-flight requests with UV, at least not
	* without user interaction [1]. As a result, let's just return true
	* here and go ahead with trying the unlock directly.
	* Reference:
	* 1: https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-20210615.html#sctn-getAssert-authnr-alg
	* See section 7.4 */
	if (has_uv && FLAGS_SET(flags, FIDO2ENROLL_UV)) {
	log_debug("Pre-flight requests with UV are unsupported, device: %s", path);
	return true;
	}

So then instead of completely removing credProtect support, we could just do something like this:

int extensions = FIDO_EXT_HMAC_SECRET;
if (FLAGS_SET(lock_with, FIDO2ENROLL_UV)) {
        /* Attempt to use the "cred protect" extension, requiring user verification (UV) for this
         * credential. If the authenticator doesn't support the extension, it will be ignored. */
        extensions |= FIDO_EXT_CRED_PROTECT;

        r = sym_fido_cred_set_prot(c, FIDO_CRED_PROT_UV_REQUIRED);
        if (r != FIDO_OK)
                log_warning("Failed to set protection level on FIDO2 credential, ignoring: %s", sym_fido_strerr(r));
}

What do you think?

[BryanJacobs]

Then I guess we could use the credProtect extension only when using UV, but not when using a PIN without UV. This wouldn't break the pre-flight, as it explicitly doesn't support UV:

systemd/src/shared/libfido2-util.c

Lines 321 to 330 in eb1ec9c

	/* FIDO2 devices may not support pre-flight requests with UV, at least not
	* without user interaction [1]. As a result, let's just return true
	* here and go ahead with trying the unlock directly.
	* Reference:
	* 1: https://fidoalliance.org/specs/fido-v2.1-ps-20210615/fido-client-to-authenticator-protocol-v2.1-ps-20210615.html#sctn-getAssert-authnr-alg
	* See section 7.4 */
	if (has_uv && FLAGS_SET(flags, FIDO2ENROLL_UV)) {
	log_debug("Pre-flight requests with UV are unsupported, device: %s", path);
	return true;
	}

So then instead of completely removing credProtect support, we could just do something like this:

int extensions = FIDO_EXT_HMAC_SECRET;
if (FLAGS_SET(lock_with, FIDO2ENROLL_UV)) {
        /* Attempt to use the "cred protect" extension, requiring user verification (UV) for this
         * credential. If the authenticator doesn't support the extension, it will be ignored. */
        extensions |= FIDO_EXT_CRED_PROTECT;

        r = sym_fido_cred_set_prot(c, FIDO_CRED_PROT_UV_REQUIRED);
        if (r != FIDO_OK)
                log_warning("Failed to set protection level on FIDO2 credential, ignoring: %s", sym_fido_strerr(r));
}

What do you think?

I think it might be worth testing how systemd-homed behaves when attempting to use it with a token that has alwaysUv set. I think it might be broken in that scenario, because the preflight would cause the token to reply with PIN_REQUIRED, breaking discovery.

What you've proposed above is better in my opinion than entirely removing the use of credProtect, but I think there's still an underlying issue - the current code assumes that credentials requiring UV to use can always be discovered without user interaction. That assumption isn't true, by design. First, some authenticators have alwaysUv=true. Second, some authenticators have makeCredUvNotRequired=false.

I still think it's better to check if any of the credentials to be used will require PIN/UV, and if they will, prompt for the PIN (or touch, or whatever) - and thus authenticator selection - prior to trying "discovery".

But that's more work, and at least it's not broken for everybody if you only use credProtect for non-PIN UV, so still an improvement.

[kszczek]

I think it might be worth testing how systemd-homed behaves when attempting to use it with a token that has alwaysUv set. I think it might be broken in that scenario, because the preflight would cause the token to reply with PIN_REQUIRED, breaking discovery.

I unfortunately don't own any FIDO2 tokens with proper UV, so I can't test this properly, but I guess the best solution for this problem would be to parse the make credential response and if uv is enabled but not requested by user, then throw an error? This would inform the user that credentials without UV are not supported by their token and they either have to disable alwaysUv or enroll with --fido2-with-user-verification=true explicitly.

What you've proposed above is better in my opinion than entirely removing the use of credProtect, but I think there's still an underlying issue - the current code assumes that credentials requiring UV to use can always be discovered without user interaction. That assumption isn't true, by design. First, some authenticators have alwaysUv=true. Second, some authenticators have makeCredUvNotRequired=false.

The pre-flight exits early, returning true if the credential was created with UV without getting an assertion, so if you have multiple tokens with UV support, systemd-homed/systemd-cryptsetup will try each one, prompting for interaction.

I still think it's better to check if any of the credentials to be used will require PIN/UV, and if they will, prompt for the PIN (or touch, or whatever) - and thus authenticator selection - prior to trying "discovery".

I guess that's the problem that pre-flights solved. If the user has multiple FIDO2 tokens plugged in, they don't have to enter the PIN for each one of them to find the right one.

[BryanJacobs]

I unfortunately don't own any FIDO2 tokens with proper UV, so I can't test this properly, but I guess the best solution for this problem would be to parse the make credential response and if uv is enabled but not requested by user, then throw an error? This would inform the user that credentials without UV are not supported by their token and they either have to disable alwaysUv or enroll with --fido2-with-user-verification=true explicitly.

No, alwaysUv is a mutable flag. fido2-token -S -u <device> enables it, and it can be disabled again later. It could be disabled when enrolling and then enabled when trying to use the credential.

What you've proposed above is better in my opinion than entirely removing the use of credProtect, but I think there's still an underlying issue - the current code assumes that credentials requiring UV to use can always be discovered without user interaction. That assumption isn't true, by design. First, some authenticators have alwaysUv=true. Second, some authenticators have makeCredUvNotRequired=false.

The pre-flight exits early, returning true if the credential was created with UV without getting an assertion, so if you have multiple tokens with UV support, systemd-homed/systemd-cryptsetup will try each one, prompting for interaction.

That's not great.

I still think it's better to check if any of the credentials to be used will require PIN/UV, and if they will, prompt for the PIN (or touch, or whatever) - and thus authenticator selection - prior to trying "discovery".

I guess that's the problem that pre-flights solved. If the user has multiple FIDO2 tokens plugged in, they don't have to enter the PIN for each one of them to find the right one.

You can check all the ones that definitely don't have a PIN at once :-P. But yes, sorry, there is no way to get that convenience reliably that I see.

[BryanJacobs]

Here's a crazy idea - what about using the CTAP2.1 authenticatorSelection command when available to find the right authenticator?

As users get rid of older non-2.1 tokens, the hassle would go away.

[kszczek]

No, alwaysUv is a mutable flag. fido2-token -S -u enables it, and it can be disabled again later. It could be disabled when enrolling and then enabled when trying to use the credential.

I don't think we should be modifying the alwaysUv flag when enrolling. If user sets alwaysUv to a truthy value, they do it for a reason, and we shouldn't try to be smarter than the user, just notify them that what they tried to do is illegal.

Here's a crazy idea - what about using the CTAP2.1 authenticatorSelection command when available to find the right authenticator?

Would this work for non-resident keys that we use? Sorry, but I'm still learning about FIDO2 and CTAP.

systemd/src/shared/libfido2-util.c

Lines 822 to 827 in eb1ec9c

	if (has_rk) {
	r = sym_fido_cred_set_rk(c, FIDO_OPT_FALSE);
	if (r != FIDO_OK)
	return log_error_errno(SYNTHETIC_ERRNO(EIO),
	"Failed to turn off FIDO2 resident key option of credential: %s", sym_fido_strerr(r));
	}

[BryanJacobs]

I don't think we should be modifying the alwaysUv flag when enrolling. If user sets alwaysUv to a truthy value, they do it for a reason, and we shouldn't try to be smarter than the user, just notify them that what they tried to is illegal.

Not what I was suggesting. I'm pointing out that when creating the home directory it can be not-set, and when trying to unlock the home directory it can be set. The user could have turned it on in the intervening time.

If at any point systemd relies on a user-verification-less discovery mechanism, then the user will be locked out.

Also, authenticators can support turning on alwaysUv and not-support disabling it: it's valid to have the transition be one-way unless a reset is performed.

Here's a crazy idea - what about using the CTAP2.1 authenticatorSelection command when available to find the right authenticator?

Would this work for non-resident keys that we use? Sorry, but I'm still learning about FIDO2 and CTAP.

systemd/src/shared/libfido2-util.c

Lines 822 to 827 in eb1ec9c

	if (has_rk) {
	r = sym_fido_cred_set_rk(c, FIDO_OPT_FALSE);
	if (r != FIDO_OK)
	return log_error_errno(SYNTHETIC_ERRNO(EIO),
	"Failed to turn off FIDO2 resident key option of credential: %s", sym_fido_strerr(r));
	}

authenticatorSelection doesn't have anything to do with the credentials. It makes all the tokens flash/whatever, and the user explicitly chooses the one they want to use.

[kszczek]

Good points, but I think we should create separate RFEs for those issues and resolve the issue at hand in this PR.

/please-review

[BryanJacobs]

Good points, but I think we should create separate RFEs for those issues and resolve the issue at hand in this PR.

/please-review

Agreed!