openssh bugs - Nov 2021 - [Bug 3366] New: SSH should skip sk-* keys that don't match the connected security key

https://bugzilla.mindrot.org/show_bug.cgi?id=3366

            Bug ID: 3366
           Summary: SSH should skip sk-* keys that don't match the
                    connected security key
           Product: Portable OpenSSH
           Version: 8.8p1
          Hardware: amd64
                OS: Linux
            Status: NEW
          Severity: minor
          Priority: P5
         Component: ssh
          Assignee: unassigned-bugs at mindrot.org
          Reporter: businesscorrespondence+openssh at rkjnsn.net

I have two U2F tokens and would like to be able to use both of them for
SSH access, and hence have created an ecdsa-sk key for each of them,
and authorized both on the server.

If I don't have either token plugged in, ssh will print out a "signing
failed" message for each of the signing keys and skip them, which makes
sense. If I have both tokens plugged in, ssh will look at the first
signing key, correctly determine to which token it belongs, and then
request a touch from that token.

The issue is that if I only have the second token plugged in (which is
often), ssh will unconditionally wait for a touch from it when trying
the first signing key even though the two don't match. Only after
receiving the touch will ssh realize that the key doesn't belong to the
attached token and move on to the correct signing key, which requires a
second touch. As a result, I'll have to touch my token twice to connect
(e.g., for every remote git operation), which is annoying.

It would be nice if ssh checked if first signing key actually belonged
to the connected token *before* waiting for a touch so it could
immediately skip it and I'd only have to touch my token once to
connect.

Looking at the code, it appears fixing this issue would be as simple as
reverting this change:
https://anongit.mindrot.org/openssh.git/commit/?id=b969072cc3d62d05cb41bc6d6f3c22c764ed932f

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3366

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org

--- Comment #1 from Damien Miller <djm at mindrot.org> ---
There is no way to test whether a given FIDO key handle belongs to a
particular token without trying to make a signature using one, so that
is what we do. If they key doesn't match the token, then the token
should not require a touch to return an error.

What tokens are you using? Can you capture a debug log (ssh -vvv ...)

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3366

--- Comment #2 from Erik Jensen <businesscorrespondence+openssh at
rkjnsn.net> ---
Created attachment 3553
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3553&action=edit
-vvv connection log requiring two touches

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3366

--- Comment #3 from Erik Jensen <businesscorrespondence+openssh at
rkjnsn.net> ---
Created attachment 3554
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3554&action=edit
-vvv connection log with patch requiring only one touch

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3366

--- Comment #4 from Erik Jensen <businesscorrespondence+openssh at
rkjnsn.net> ---
They are both standard YubiKey 5 NFC. I've attached two logs. The first
is with plain OpenSSH_8.6p1, which requires two touches. The second is
with OpenSSH patched to revert
https://anongit.mindrot.org/openssh.git/commit/?id=b969072cc3d62d05cb41bc6d6f3c22c764ed932f,
which only requires one touch. Because it's not otherwise obvious from
the log output, I added "*** WAITS FOR SK TOUCH HERE ***" lines to the
logs at the points where ssh stopped and waited for a touch.

As you can see, the patched version still prints out "Confirm user
presence for key ECDSA-SK SHA256:?" for both keys, but only *actually*
waits for the key associated with the connected token. The unpatched
version waits for a touch for both keys.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3366

--- Comment #5 from Damien Miller <djm at mindrot.org>
---
>From Pedro, who knows way more about this than me:
> Reverting b969072c would break the "uv handled by token" logic
implemented in
> f3c34df8. (I am including links to both commits at the bottom of this
message).
> 
> With f3c34df8, -O verify-required credentials without a corresponding
PIN/UV
> gesture and a credential unknown to the token (#3366) are indistinguishable
to
> the middleware: an attempt to sign results in ERR_NO_CREDENTIALS and
> SSH_SK_USER_VERIFICATION_REQD is clear.
> 
> Reverting both commits would address #3366 but break builtin UV (f3c34df8).
A
> best-effort compromise would be to attempt to sign if a) there's only
one token;
> b) the token supports builtin UV (so we can't tell whether the
credential exists
> on the token without asking the user to complete UV, i.e. supply a PIN or
> perform UV gesture):
> 
> -       if (skvcnt == 1) {
> +       if (skvcnt == 1 && fido_dev_has_uv(skv[0]->dev)) {
> 
> b969072c: https://github.com/openssh/openssh-portable/commit/b969072c
> f3c34df8: https://github.com/openssh/openssh-portable/commit/f3c34df8
I've committed a version of this as
https://github.com/openssh/openssh-portable/commit/b560120214 but it
will only improve matters for biometric keys. I don't think this can be
avoidable in the general case.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3366

--- Comment #6 from Erik Jensen <businesscorrespondence+openssh at
rkjnsn.net> ---
If I'm understanding correctly, shouldn't that be "it will only
improve
matters for *non-biometric* keys"? If I'm reading Pedro's
explanation
right, it sounds like the problem for biometric/uv tokens is that
there's no way to test whether a credential belongs to the token
without requiring a gesture, as attempting to sign an unknown
credential and attempting to sign a known credential without a
user-verification gesture both fail with the same error, so the only
option is to assume the credential might belong to the token and try it
with a user-verification gesture.

The change in b5601202, then, keeps the logic introduced in b969072c
intact for biometric/uv keys: if there is a single token connected, and
that token supports on-token user verification, that key will be
selected for signing with a uv gesture unconditionally, since there's
no way to test whether the credential belongs to the token ahead of
time. However, with b5601202, ssh will no longer try to use a single
non-biometric, non-uv token unconditionally, as such a token *can* be
tested before requiring a user gesture. Instead, sk_select_by_cred will
now invoke sk_try for the key, the same as it would if multiple keys
were attached, and only return it if that succeeds.

(Given this understanding, I think the commit message for b5601202 is
incorrect, though the change itself is correct, since it is actually
tokens that do *not* support on-token user-verification that are now
asked if the credential belongs to them.)

In any event, with b5601202 applied to my ssh-agent, I now see my
desired behavior with my YubiKey 5s: when only one token is attached,
attempting to use the key that doesn't match that token immediately
fails, so I only have to touch my token once, even when the matching
key is tried second. Thanks!

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=3366

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #7 from Damien Miller <djm at mindrot.org> ---
AFAIK this is fixed

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.