From 0735ed950a8a8c476dc7760eb294e3048a253369 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 27 May 2021 18:47:48 +0200 Subject: [PATCH 1/5] cryptenroll: handle FIDO2 tokens gracefully that lack requested features Let's try to handle keys gracefully that do not implement all features we ask for: simply turn the feature off, and continue. This is in particular relevant since we enroll with PIN and UP by default, and on devices that don't support that we should just work. Replaces: #18509 --- src/cryptenroll/cryptenroll-fido2.c | 3 +- src/home/homectl-fido2.c | 3 +- src/shared/libfido2-util.c | 109 +++++++++++++++++++++++----- src/shared/libfido2-util.h | 3 +- 4 files changed, 97 insertions(+), 21 deletions(-) diff --git a/src/cryptenroll/cryptenroll-fido2.c b/src/cryptenroll/cryptenroll-fido2.c index 3ba7866738..fbf76ee586 100644 --- a/src/cryptenroll/cryptenroll-fido2.c +++ b/src/cryptenroll/cryptenroll-fido2.c @@ -45,7 +45,8 @@ int enroll_fido2( &cid, &cid_size, &salt, &salt_size, &secret, &secret_size, - NULL); + NULL, + &lock_with); if (r < 0) return r; diff --git a/src/home/homectl-fido2.c b/src/home/homectl-fido2.c index a2054fcf73..e8e9826bb9 100644 --- a/src/home/homectl-fido2.c +++ b/src/home/homectl-fido2.c @@ -162,7 +162,8 @@ int identity_add_fido2_parameters( &cid, &cid_size, &salt, &salt_size, &secret, &secret_size, - &used_pin); + &used_pin, + NULL); if (r < 0) return r; diff --git a/src/shared/libfido2-util.c b/src/shared/libfido2-util.c index 24c9b76b7a..4a901cd38c 100644 --- a/src/shared/libfido2-util.c +++ b/src/shared/libfido2-util.c @@ -468,7 +468,8 @@ int fido2_generate_hmac_hash( void **ret_cid, size_t *ret_cid_size, void **ret_salt, size_t *ret_salt_size, void **ret_secret, size_t *ret_secret_size, - char **ret_usedpin) { + char **ret_usedpin, + Fido2EnrollFlags *ret_locked_with) { _cleanup_(erase_and_freep) void *salt = NULL, *secret_copy = NULL; _cleanup_(fido_assert_free_wrapper) fido_assert_t *a = NULL; @@ -503,6 +504,7 @@ int fido2_generate_hmac_hash( */ assert(device); + assert((lock_with & ~(FIDO2ENROLL_PIN|FIDO2ENROLL_UP|FIDO2ENROLL_UV)) == 0); r = dlopen_libfido2(); if (r < 0) @@ -529,20 +531,21 @@ int fido2_generate_hmac_hash( if (r < 0) return r; - if (!has_client_pin && FLAGS_SET(lock_with, FIDO2ENROLL_PIN)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Requested to lock with PIN, but FIDO2 device %s does not support it.", - device); + /* While enrolling degrade gracefully if the requested feature set isn't available, but let the user know */ + if (!has_client_pin && FLAGS_SET(lock_with, FIDO2ENROLL_PIN)) { + log_notice("Requested to lock with PIN, but FIDO2 device %s does not support it, disabling.", device); + lock_with &= ~FIDO2ENROLL_PIN; + } - if (!has_up && FLAGS_SET(lock_with, FIDO2ENROLL_UP)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Locking with user presence test requested, but FIDO2 device %s does not support it.", - device); + if (!has_up && FLAGS_SET(lock_with, FIDO2ENROLL_UP)) { + log_notice("Locking with user presence test requested, but FIDO2 device %s does not support it, disabling.", device); + lock_with &= ~FIDO2ENROLL_UP; + } - if (!has_uv && FLAGS_SET(lock_with, FIDO2ENROLL_UV)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Locking with user verification requested, but FIDO2 device %s does not support it.", - device); + if (!has_uv && FLAGS_SET(lock_with, FIDO2ENROLL_UV)) { + log_notice("Locking with user verification requested, but FIDO2 device %s does not support it, disabling.", device); + lock_with &= ~FIDO2ENROLL_UV; + } c = sym_fido_cred_new(); if (!c) @@ -592,6 +595,9 @@ int fido2_generate_hmac_hash( "Failed to turn off FIDO2 user verification option of credential: %s", sym_fido_strerr(r)); } + /* As per specification "up" is assumed to be implicit when making credentials, hence we don't + * explicitly enable/disable it here */ + log_info("Initializing FIDO2 credential on security token."); log_notice("%s%s(Hint: This might require verification of user presence on security token.)", @@ -715,11 +721,75 @@ int fido2_generate_hmac_hash( emoji_enabled() ? " " : ""); } - r = sym_fido_dev_get_assert(d, a, FLAGS_SET(lock_with, FIDO2ENROLL_PIN) ? used_pin : NULL); - if (r == FIDO_ERR_UP_REQUIRED && !FLAGS_SET(lock_with, FIDO2ENROLL_UP)) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Locking without user presence test requested, but FIDO2 device %s requires it.", - device); + for (;;) { + bool retry_with_up = false, retry_with_pin = false; + + r = sym_fido_dev_get_assert(d, a, FLAGS_SET(lock_with, FIDO2ENROLL_PIN) ? used_pin : NULL); + + switch (r) { + + case FIDO_ERR_UP_REQUIRED: + /* If the token asks for "up" when we turn off, then this might be a feature that + * isn't optional. Let's enable it */ + + if (!has_up) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Token asks for user presence check but doesn't advertise 'up' feature."); + + if (FLAGS_SET(lock_with, FIDO2ENROLL_UP)) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Token asks for user presence check but was already enabled."); + + log_notice("Locking without user presence test requested, but FIDO2 device %s requires it, enabling.", device); + retry_with_up = true; + break; + + case FIDO_ERR_UNSUPPORTED_OPTION: + /* AuthenTrend ATKey.Pro says it supports "up", but if we disable it it will fail + * with FIDO_ERR_UNSUPPORTED_OPTION, probably because it isn't actually + * optional. Let's see if turning it on works. This is very similar to the + * FIDO_ERR_UP_REQUIRED case, but since the error is so vague we implement it + * slightly more defensively. */ + + if (has_up && !FLAGS_SET(lock_with, FIDO2ENROLL_UP)) { + log_notice("Got unsupported option error when when user presence test is turned off. Trying with user presence test turned on."); + retry_with_up = true; + } + + break; + + case FIDO_ERR_PIN_REQUIRED: + if (!has_client_pin) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Token asks for client PIN check but doesn't advertise 'clientPin' feature."); + + if (FLAGS_SET(lock_with, FIDO2ENROLL_PIN)) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Token asks for user client PIN check but was already enabled."); + + log_debug("Token requires PIN for assertion, enabling."); + retry_with_pin = true; + break; + + default: + break; + } + + if (!retry_with_up && !retry_with_pin) + break; + + if (retry_with_up) { + r = sym_fido_assert_set_up(a, FIDO_OPT_TRUE); + if (r != FIDO_OK) + return log_error_errno(SYNTHETIC_ERRNO(EIO), "Failed to enable FIDO2 user presence test: %s", sym_fido_strerr(r)); + + lock_with |= FIDO2ENROLL_UP; + } + + if (retry_with_pin) + lock_with |= FIDO2ENROLL_PIN; + } + if (r == FIDO_ERR_ACTION_TIMEOUT) return log_error_errno(SYNTHETIC_ERRNO(ENOSTR), "Token action timeout. (User didn't interact with token quickly enough.)"); @@ -751,6 +821,9 @@ int fido2_generate_hmac_hash( if (ret_usedpin) *ret_usedpin = TAKE_PTR(used_pin); + if (ret_locked_with) + *ret_locked_with = lock_with; + return 0; } #endif diff --git a/src/shared/libfido2-util.h b/src/shared/libfido2-util.h index 1b31577e06..bcf3c73706 100644 --- a/src/shared/libfido2-util.h +++ b/src/shared/libfido2-util.h @@ -106,7 +106,8 @@ int fido2_generate_hmac_hash( void **ret_cid, size_t *ret_cid_size, void **ret_salt, size_t *ret_salt_size, void **ret_secret, size_t *ret_secret_size, - char **ret_usedpin); + char **ret_usedpin, + Fido2EnrollFlags *ret_locked_with); #endif From ec543d18d459ad39cd34923eaeafb233e031b196 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 27 May 2021 22:59:18 +0200 Subject: [PATCH 2/5] fido2: make misadvertised clientPin feature fatal We need really need to trust the feature set, since we are about to set it in stone storing the result in JSON, hence react a bit more allergic about token that misadvertise the feature. Note that I added this to be defensive, I am not aware any token that actually misadvertises this. hence it should be safe to make this fatal, and should this not work we can always revisit things. --- src/shared/libfido2-util.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/shared/libfido2-util.c b/src/shared/libfido2-util.c index 4a901cd38c..573aef238c 100644 --- a/src/shared/libfido2-util.c +++ b/src/shared/libfido2-util.c @@ -606,13 +606,15 @@ int fido2_generate_hmac_hash( r = sym_fido_dev_make_cred(d, c, NULL); if (r == FIDO_ERR_PIN_REQUIRED) { + + if (!has_client_pin) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Token asks for PIN but doesn't advertise 'clientPin' feature."); + for (;;) { _cleanup_(strv_free_erasep) char **pin = NULL; char **i; - if (!has_client_pin) - log_warning("Weird, device asked for client PIN, but does not advertise it as feature. Ignoring."); - r = ask_password_auto("Please enter security token PIN:", askpw_icon_name, NULL, "fido2-pin", "fido2-pin", USEC_INFINITY, 0, &pin); if (r < 0) return log_error_errno(r, "Failed to acquire user PIN: %m"); From 1ce8f69dbd9b6dc931defb32cb7ee0229935d176 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 27 May 2021 22:06:25 +0200 Subject: [PATCH 3/5] fido2: properly handle case when no PINs are specified during auth Also, drop redundant check for has_client_pin, which can never happen, since we already filtered this case a bit further up. --- src/shared/libfido2-util.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/shared/libfido2-util.c b/src/shared/libfido2-util.c index 573aef238c..3da64b549a 100644 --- a/src/shared/libfido2-util.c +++ b/src/shared/libfido2-util.c @@ -325,15 +325,15 @@ static int fido2_use_hmac_hash_specific_token( if (FLAGS_SET(required, FIDO2ENROLL_PIN)) { char **i; - if (!has_client_pin) - log_warning("Weird, device asked for client PIN, but does not advertise it as feature. Ignoring."); - - /* OK, we needed a pin, try with all pins in turn */ - STRV_FOREACH(i, pins) { - r = sym_fido_dev_get_assert(d, a, *i); - if (r != FIDO_ERR_PIN_INVALID) - break; - } + /* OK, we need a pin, try with all pins in turn */ + if (strv_isempty(pins)) + r = FIDO_ERR_PIN_REQUIRED; + else + STRV_FOREACH(i, pins) { + r = sym_fido_dev_get_assert(d, a, *i); + if (r != FIDO_ERR_PIN_INVALID) + break; + } } else r = sym_fido_dev_get_assert(d, a, NULL); From 3cc00ba60594cbce0aa416e54b846988376685f8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 27 May 2021 22:55:39 +0200 Subject: [PATCH 4/5] cryptsetup: revert to systemd 248 up/pin/uv FIDO2 settings when we don't have LUKS2 JSON data telling us the precise configuration Let's improve compatibility with systemd 248 enrollments of FIDO2 keys: if we have no information about the up/uv/pin settings, let's try to determine them automatically, i.e. use up and pin if needed. This only has an effect on LUKS2 volumes where a FIDO2 key was enrolled with systemd 248 and thus the JSON data lacks the up/uv/pin fields. It also matters if the user configured FIDO2 parameters explicitly via crypttab options, so that the JSON data is not used. For newer enrollments we'll stick to the explicit settings, as that's generally much safer and robust. --- src/cryptsetup/cryptsetup-fido2.c | 12 ++-- src/cryptsetup/cryptsetup.c | 6 +- src/shared/libfido2-util.c | 104 ++++++++++++++++++++++++++---- src/shared/libfido2-util.h | 3 + 4 files changed, 106 insertions(+), 19 deletions(-) diff --git a/src/cryptsetup/cryptsetup-fido2.c b/src/cryptsetup/cryptsetup-fido2.c index b21f970db7..6e400e44e3 100644 --- a/src/cryptsetup/cryptsetup-fido2.c +++ b/src/cryptsetup/cryptsetup-fido2.c @@ -117,8 +117,7 @@ int find_fido2_auto_data( size_t cid_size = 0, salt_size = 0; _cleanup_free_ char *rp = NULL; int r, keyslot = -1; - /* For backward compatibility, require pin and presence by default */ - Fido2EnrollFlags required = FIDO2ENROLL_PIN | FIDO2ENROLL_UP; + Fido2EnrollFlags required = 0; assert(cd); assert(ret_salt); @@ -193,7 +192,8 @@ int find_fido2_auto_data( "FIDO2 token data's 'fido2-clientPin-required' field is not a boolean."); SET_FLAG(required, FIDO2ENROLL_PIN, json_variant_boolean(w)); - } + } else + required |= FIDO2ENROLL_PIN_IF_NEEDED; /* compat with 248, where the field was unset */ w = json_variant_by_key(v, "fido2-up-required"); if (w) { @@ -204,7 +204,8 @@ int find_fido2_auto_data( "FIDO2 token data's 'fido2-up-required' field is not a boolean."); SET_FLAG(required, FIDO2ENROLL_UP, json_variant_boolean(w)); - } + } else + required |= FIDO2ENROLL_UP_IF_NEEDED; /* compat with 248 */ w = json_variant_by_key(v, "fido2-uv-required"); if (w) { @@ -215,7 +216,8 @@ int find_fido2_auto_data( "FIDO2 token data's 'fido2-uv-required' field is not a boolean."); SET_FLAG(required, FIDO2ENROLL_UV, json_variant_boolean(w)); - } + } else + required |= FIDO2ENROLL_UV_OMIT; /* compat with 248 */ } if (!cid) diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c index 74bd071c10..f0826c47e5 100644 --- a/src/cryptsetup/cryptsetup.c +++ b/src/cryptsetup/cryptsetup.c @@ -758,7 +758,11 @@ static int attach_luks_or_plain_or_bitlk_by_fido2( cid = arg_fido2_cid; cid_size = arg_fido2_cid_size; - required = FIDO2ENROLL_PIN | FIDO2ENROLL_UP; /* For backwards compatibility, PIN+presence is required by default. */ + /* For now and for compatibility, if the user explicitly configured FIDO2 support and we do + * not read FIDO2 metadata off the LUKS2 header, default to the systemd 248 logic, where we + * use PIN + UP when needed, and do not configure UV at all. Eventually, we should make this + * explicitly configurable. */ + required = FIDO2ENROLL_PIN_IF_NEEDED | FIDO2ENROLL_UP_IF_NEEDED | FIDO2ENROLL_UV_OMIT; } else { r = find_fido2_auto_data( cd, diff --git a/src/shared/libfido2-util.c b/src/shared/libfido2-util.c index 3da64b549a..0272d0bde7 100644 --- a/src/shared/libfido2-util.c +++ b/src/shared/libfido2-util.c @@ -310,7 +310,7 @@ static int fido2_use_hmac_hash_specific_token( log_info("User presence required to unlock."); } - if (has_uv) { + if (has_uv && !FLAGS_SET(required, FIDO2ENROLL_UV_OMIT)) { r = sym_fido_assert_set_uv(a, FLAGS_SET(required, FIDO2ENROLL_UV) ? FIDO_OPT_TRUE : FIDO_OPT_FALSE); if (r != FIDO_OK) return log_error_errno(SYNTHETIC_ERRNO(EIO), @@ -322,20 +322,98 @@ static int fido2_use_hmac_hash_specific_token( log_info("User verification required to unlock."); } - if (FLAGS_SET(required, FIDO2ENROLL_PIN)) { - char **i; + for (;;) { + bool retry_with_up = false, retry_with_pin = false; - /* OK, we need a pin, try with all pins in turn */ - if (strv_isempty(pins)) - r = FIDO_ERR_PIN_REQUIRED; - else - STRV_FOREACH(i, pins) { - r = sym_fido_dev_get_assert(d, a, *i); - if (r != FIDO_ERR_PIN_INVALID) - break; + if (FLAGS_SET(required, FIDO2ENROLL_PIN)) { + char **i; + + /* OK, we need a pin, try with all pins in turn */ + if (strv_isempty(pins)) + r = FIDO_ERR_PIN_REQUIRED; + else + STRV_FOREACH(i, pins) { + r = sym_fido_dev_get_assert(d, a, *i); + if (r != FIDO_ERR_PIN_INVALID) + break; + } + + } else + r = sym_fido_dev_get_assert(d, a, NULL); + + /* In some conditions, where a PIN or UP is required we might accept that. Let's check the + * conditions and if so try immediately again. */ + + switch (r) { + + case FIDO_ERR_UP_REQUIRED: + /* So the token asked for "up". Try to turn it on, for compat with systemd 248 and try again. */ + + if (!has_up) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Token asks for user presence check but doesn't advertise 'up' feature."); + + if (FLAGS_SET(required, FIDO2ENROLL_UP)) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Token asks for user presence check but was already enabled."); + + if (FLAGS_SET(required, FIDO2ENROLL_UP_IF_NEEDED)) { + log_info("User presence required to unlock."); + retry_with_up = true; } - } else - r = sym_fido_dev_get_assert(d, a, NULL); + + break; + + case FIDO_ERR_UNSUPPORTED_OPTION: + /* AuthenTrend ATKey.Pro returns this instead of FIDO_ERR_UP_REQUIRED, let's handle + * it gracefully (also see below.) */ + + if (has_up && (required & (FIDO2ENROLL_UP|FIDO2ENROLL_UP_IF_NEEDED)) == FIDO2ENROLL_UP_IF_NEEDED) { + log_notice("Got unsupported option error when when user presence test is turned off. Trying with user presence test turned on."); + retry_with_up = true; + } + + break; + + case FIDO_ERR_PIN_REQUIRED: + /* A pin was requested. Maybe supply one, if we are configured to do so on request */ + + if (!has_client_pin) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Token asks for PIN but doesn't advertise 'clientPin' feature."); + + if (FLAGS_SET(required, FIDO2ENROLL_PIN) && !strv_isempty(pins)) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Token asks for PIN but one was already supplied."); + + if ((required & (FIDO2ENROLL_PIN|FIDO2ENROLL_PIN_IF_NEEDED)) == FIDO2ENROLL_PIN_IF_NEEDED) { + /* If a PIN so far wasn't specified but is requested by the device, and + * FIDO2ENROLL_PIN_IF_NEEDED is set, then provide it */ + log_debug("Retrying to create credential with PIN."); + retry_with_pin = true; + } + + break; + + default: + break; + } + + if (!retry_with_up && !retry_with_pin) + break; + + if (retry_with_up) { + r = sym_fido_assert_set_up(a, FIDO_OPT_TRUE); + if (r != FIDO_OK) + return log_error_errno(SYNTHETIC_ERRNO(EIO), + "Failed to enable FIDO2 user presence test: %s", sym_fido_strerr(r)); + + required |= FIDO2ENROLL_UP; + } + + if (retry_with_pin) + required |= FIDO2ENROLL_PIN; + } switch (r) { case FIDO_OK: diff --git a/src/shared/libfido2-util.h b/src/shared/libfido2-util.h index bcf3c73706..5640cca5e3 100644 --- a/src/shared/libfido2-util.h +++ b/src/shared/libfido2-util.h @@ -7,6 +7,9 @@ typedef enum Fido2EnrollFlags { FIDO2ENROLL_PIN = 1 << 0, FIDO2ENROLL_UP = 1 << 1, /* User presence (ie: touching token) */ FIDO2ENROLL_UV = 1 << 2, /* User verification (ie: fingerprint) */ + FIDO2ENROLL_PIN_IF_NEEDED = 1 << 3, /* If auth doesn't work without PIN ask for one, as in systemd 248 */ + FIDO2ENROLL_UP_IF_NEEDED = 1 << 4, /* If auth doesn't work without UP, enable it, as in systemd 248 */ + FIDO2ENROLL_UV_OMIT = 1 << 5, /* Leave "uv" untouched, as in systemd 248 */ _FIDO2ENROLL_TYPE_MAX, _FIDO2ENROLL_TYPE_INVALID = -EINVAL, } Fido2EnrollFlags; From 72c154226753cb9b0cfad1552c1b958d3889118c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 28 May 2021 16:45:12 +0200 Subject: [PATCH 5/5] man: document that FIDO2 uv/up/clientPin feature support is now handled gracefully --- man/systemd-cryptenroll.xml | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/man/systemd-cryptenroll.xml b/man/systemd-cryptenroll.xml index 097cf7518b..d5719bc463 100644 --- a/man/systemd-cryptenroll.xml +++ b/man/systemd-cryptenroll.xml @@ -128,8 +128,11 @@ BOOL - When enrolling a FIDO2 security token, controls whether to require the user to - enter a PIN when unlocking the volume. Defaults to yes. + When enrolling a FIDO2 security token, controls whether to require the user to enter + a PIN when unlocking the volume (the FIDO2 clientPin feature). Defaults to + yes. (Note: this setting is without effect if the security token does not support + the clientPin feature at all, or does not allow enabling or disabling + it.) @@ -137,7 +140,8 @@ When enrolling a FIDO2 security token, controls whether to require the user to verify presence (tap the token, the FIDO2 up feature) when unlocking the volume. - Defaults to yes. + Defaults to yes. (Note: this setting is without effect if the security token does not support + the up feature at all, or does not allow enabling or disabling it.) @@ -145,8 +149,9 @@ BOOL When enrolling a FIDO2 security token, controls whether to require user verification - when unlocking the volume (the FIDO2 uv feature)). Defaults to no. - + when unlocking the volume (the FIDO2 uv feature). Defaults to + no. (Note: this setting is without effect if the security token does not support + the uv feature at all, or does not allow enabling or disabling it.)