Merge pull request #19756 from poettering/fido2-enroll-tweaks

further tweaks to fido2 code

man/sd_id128_get_machine.xml

@@ -106,7 +106,7 @@
     but might not on older. It is possible to convert the machine ID into a UUID v4-compatible one. For more
     information, see
     <citerefentry><refentrytitle>machine-id</refentrytitle><manvolnum>5</manvolnum></citerefentry>. It is
-    hence guaranteed that thes functions will never return the ID consisting of all zero or all one bits
+    hence guaranteed that these functions will never return the ID consisting of all zero or all one bits
     (<constant>SD_ID128_NULL</constant>, <constant>SD_ID128_ALLF</constant>) — with the possible exception of
     <function>sd_id128_get_machine()</function>, as mentioned.</para>
 

man/systemd-cryptenroll.xml

@@ -128,8 +128,11 @@
       <varlistentry>
         <term><option>--fido2-with-client-pin=</option><replaceable>BOOL</replaceable></term>
 
-        <listitem><para>When enrolling a FIDO2 security token, controls whether to require the user to
-        enter a PIN when unlocking the volume. Defaults to <literal>yes</literal>.</para></listitem>
+        <listitem><para>When enrolling a FIDO2 security token, controls whether to require the user to enter
+        a PIN when unlocking the volume (the FIDO2 <literal>clientPin</literal> feature). Defaults to
+        <literal>yes</literal>. (Note: this setting is without effect if the security token does not support
+        the <literal>clientPin</literal> feature at all, or does not allow enabling or disabling
+        it.)</para></listitem>
       </varlistentry>
 
       <varlistentry>
@@ -137,7 +140,8 @@
 
         <listitem><para>When enrolling a FIDO2 security token, controls whether to require the user to
         verify presence (tap the token, the FIDO2 <literal>up</literal> feature) when unlocking the volume.
-        Defaults to <literal>yes</literal>.
+        Defaults to <literal>yes</literal>. (Note: this setting is without effect if the security token does not support
+        the <literal>up</literal> feature at all, or does not allow enabling or disabling it.)
         </para></listitem>
       </varlistentry>
 
@@ -145,8 +149,9 @@
         <term><option>--fido2-with-user-verification=</option><replaceable>BOOL</replaceable></term>
 
         <listitem><para>When enrolling a FIDO2 security token, controls whether to require user verification
-        when unlocking the volume (the FIDO2 <literal>uv</literal> feature)). Defaults to <literal>no</literal>.
-        </para></listitem>
+        when unlocking the volume (the FIDO2 <literal>uv</literal> feature). Defaults to
+        <literal>no</literal>. (Note: this setting is without effect if the security token does not support
+        the <literal>uv</literal> feature at all, or does not allow enabling or disabling it.)</para></listitem>
       </varlistentry>
 
       <varlistentry>

src/basic/path-util.c

@@ -577,12 +577,13 @@ char* path_extend_internal(char **x, ...) {
                         continue;
 
                 add = 1 + strlen(p);
-                if (sz > SIZE_MAX - add) /* overflow check */
+                if (sz > SIZE_MAX - add) { /* overflow check */
+                        va_end(ap);
                         return NULL;
+                }
 
                 sz += add;
         }
-
         va_end(ap);
 
         nx = realloc(x ? *x : NULL, GREEDY_ALLOC_ROUND_UP(sz+1));

src/basic/unit-file.c

@@ -303,11 +303,11 @@ int unit_file_build_name_map(
                                 return log_oom();
 
                         if (paths) {
-                                r = set_consume(paths, filename);
+                                r = set_put(paths, filename);
                                 if (r < 0)
                                         return log_oom();
-                                /* We will still use filename below. This is safe because we know the set
-                                 * holds a reference. */
+                                if (r == 0)
+                                        _filename_free = filename; /* Make sure we free the filename. */
                         } else
                                 _filename_free = filename; /* Make sure we free the filename. */
 

src/core/unit.c

@@ -2708,7 +2708,7 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag
         }
 
         /* And now, add the unit or depending units to various queues that will act on the new situation if
-         * needed. These queues generally check for continous state changes rather than events (like most of
+         * needed. These queues generally check for continuous state changes rather than events (like most of
          * the state propagation above), and do work deferred instead of instantly, since they typically
          * don't want to run during reloading, and usually involve checking combined state of multiple units
          * at once. */
@@ -5831,7 +5831,7 @@ int unit_get_dependency_array(const Unit *u, UnitDependencyAtom atom, Unit ***re
 
         /* Gets a list of units matching a specific atom as array. This is useful when iterating through
          * dependencies while modifying them: the array is an "atomic snapshot" of sorts, that can be read
-         * while the dependency table is continously updated. */
+         * while the dependency table is continuously updated. */
 
         UNIT_FOREACH_DEPENDENCY(other, u, atom) {
                 if (!GREEDY_REALLOC(array, n + 1))

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;
 

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)

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,

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;
 

src/libsystemd/sd-device/sd-device.c

@@ -2141,7 +2141,7 @@ _public_ int sd_device_trigger_with_uuid(
 
         assert_return(device, -EINVAL);
 
-        /* If noone wants to know the UUID, use the simple interface from pre-4.13 times */
+        /* If no one wants to know the UUID, use the simple interface from pre-4.13 times */
         if (!ret_uuid)
                 return sd_device_trigger(device, action);
 

src/libsystemd/sd-hwdb/hwdb-util.c

@@ -377,7 +377,7 @@ static int trie_store(struct trie *trie, const char *filename, bool compat) {
         r = fopen_temporary(filename, &t.f, &filename_tmp);
         if (r < 0)
                 return r;
-        fchmod(fileno(t.f), 0444);
+        (void) fchmod(fileno(t.f), 0444);
 
         /* write nodes */
         if (fseeko(t.f, sizeof(struct trie_header_f), SEEK_SET) < 0)

src/network/networkd-lldp-rx.c

@@ -163,7 +163,7 @@ int link_lldp_save(Link *link) {
         if (r < 0)
                 goto finish;
 
-        fchmod(fileno(f), 0644);
+        (void) fchmod(fileno(f), 0644);
 
         for (i = 0; i < n; i++) {
                 const void *p;

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;
 
-                if (!has_client_pin)
-                        log_warning("Weird, device asked for client PIN, but does not advertise it as feature. Ignoring.");
+                if (FLAGS_SET(required, FIDO2ENROLL_PIN)) {
+                        char **i;
 
-                /* 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);
+
+                /* 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;
+                        }
+
+                        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;
                 }
-        } else
-                r = sym_fido_dev_get_assert(d, a, NULL);
+
+                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:
@@ -468,7 +546,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 +582,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 +609,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 +673,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.)",
@@ -600,13 +684,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");
@@ -715,11 +801,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 +901,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

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;
@@ -106,7 +109,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
 

src/test/test-alloc-util.c

@@ -175,7 +175,7 @@ static void test_malloc_size_safe(void) {
         assert_se(malloc_usable_size(NULL) == 0); /* as per man page, this is safe and defined */
         assert_se(__builtin_object_size(NULL, 0) == SIZE_MAX); /* as per docs SIZE_MAX is returned for pointers where the size isn't known */
 
-        /* Then, let's try these macros once with contant size values, so that __builtin_object_size()
+        /* Then, let's try these macros once with constant size values, so that __builtin_object_size()
          * definitely can work (as long as -O2 is used when compiling) */
         assert_se(f = new(uint32_t, n));
         TEST_SIZES(f, n);