openssh bugs - Aug 2008 - [Bug 1506] New: rationalize agent behavior on smartcard removal/reattachment

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

           Summary: rationalize agent behavior on smartcard
                    removal/reattachment
           Product: Portable OpenSSH
           Version: 5.1p1
          Platform: Other
        OS/Version: Linux
            Status: NEW
          Severity: normal
          Priority: P2
         Component: Smartcard
        AssignedTo: unassigned-bugs at mindrot.org
        ReportedBy: dkg at fifthhorseman.net


Created an attachment (id=1559)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1559)
patch to retry smartcard if detached reader/card is detected.

Currently, if you use an OpenSC-supported smartcard with your
ssh-agent, the passphrase is cached while the smartcard is in use (up
until the expiry indicated by the user during ssh-add).

In this situation, if the user removes and re-inserts the
smartcard/reader, the next authentication attempt using the token on
the card will fail because the card had been detached.

However, the *subsequent* attempt to use the card will succeed again,
because the passphrase is still cached, and the agent simply needs to
re-initialize the reader.

This seems like misbehavior to me.  Either one of the following
scenarios would make more sense:

0) If the agent notices that the card or reader is missing or had been
detached, it could invalidate the cached information and remove it from
the list of keys, requiring the user to re-add the device to the agent.

or

1) If the agent notices that the card or reader is missing or had been
detached, it could simply scan for the card again, re-initialize it,
and use it again.

Simply put, i can see no reason for the first attempt to use the
detached/reattached device to fail while previous and subsequent
attempts succeed.

I'm attaching a patch that implements resolution (1) above (the agent
notices detachment, and tries a single extra time to re-initialize the
device), though i could see the argument for (0) as well.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

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


Damien Miller <djm at mindrot.org> changed:

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




--- Comment #1 from Damien Miller <djm at mindrot.org>  2008-08-17
00:32:51 ---
Quite a few smartcards will permanently erase their contents if too
many incorrect PINs are entered, right? If so, I think behaviour (0)
would be the most friendly - we wouldn't want to put users in a
situation where they could erase their cards by thinking they had one
more PIN retry left before they had to dig out the scrap of paper they
wrote it down on when they really had none (because ssh-agent
implicitly retried).

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1506





--- Comment #2 from Daniel Kahn Gillmor <dkg at fifthhorseman.net> 
2008-08-18 14:28:20 ---
Yeah, we definitely don't want to lock people out of their cards with
the agent.

Given that the attached patch only retries if the error message is
SC_ERROR_READER_DETACHED, i don't think it will actually register two
strikes against the card's count of excessive retries.  It would
register one if the stored PIN was bad, but it should do that anyway,
whether or not the card was detached or re-attached.

However, if the user inserts a different card with a different PIN, the
proposed patch would trigger one count against that card.

OTOH, the most common use case is the one where the user gets up to go
to the bathroom and takes card and/or reader with him or her. 
Requiring re-authentication at each step (when the user probably
already had to re-authenticate somehow to unlock hir screensaver) seems
like a pain.

Of course, the current behavior is worse than either alternative,
particularly if the device wasn't added to the agent with -c, and the
authentication steps are happening without user approval.  You could
try to connect to a half-dozen hosts (using cssh, for example), and
immediately disable your card if you have a cached-but-wrong PIN.

What if the stored PIN (and the reference to the public key) were
entirely removed from the agent at any failure *other* than the initial
SC_ERROR_READER_DETACHED?  That way, we can protect the card against
destruction by the agent, and we clear the PIN if the agent is used
when the card is actually absent.  But we also doesn't cause annoyance
in the most common case (where people have to use the bathroom).

Does that seem reasonable?

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1506





--- Comment #3 from Damien Miller <djm at mindrot.org>  2008-08-18
17:24:11 ---
Yes, that sounds quite reasonable.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1506





--- Comment #4 from Daniel Kahn Gillmor <dkg at fifthhorseman.net> 
2008-08-22 01:46:24 ---
Hrm.  It looks like when the keys are stored in the agent, they're not
associated with any particular smartcard or reader, i think they're
just marked by Identity.key.flags |= KEY_FLAG_EXT.  Is that right?

It also looks like only a single smartcard PIN can be cached by the
agent at once.  So a user alternating between two smartcards (or using
two different keys with different PINs on a single smartcard, which is
possible at least on the cryptoFlex eGate) won't be able to use them
properly with a single agent.  This is probably a different bug that i
should file separately.

Also, the code for removing identities from the agent is all statically
declared within ssh-agent.c, so it won't be accessible from within
scard-opensc.c.

In light of all this, the best solution to me seems to be to invalidate
*all* hardware-stored keys as soon as any one of them reports a
failure.  This should be able to work in conjunction with the above
patch, because the above patch will avoid an error in the common case.

Does this sound right?

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1506


Daniel Kahn Gillmor <dkg at fifthhorseman.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1559|0                           |1
        is obsolete|                            |




--- Comment #5 from Daniel Kahn Gillmor <dkg at fifthhorseman.net> 
2008-08-22 02:14:33 ---
Created an attachment (id=1561)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1561)
patch to retry smartcard once on detached reader, and purge PIN and all
H/W keys from agent on failure

Here's a patch which implements the strategy i outlined above.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1506


Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |1481




-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1506





--- Comment #6 from Damien Miller <djm at mindrot.org>  2009-02-02
10:01:59 ---
(From update of attachment 1561)
>diff -ruN openssh-5.1p1/scard-opensc.c openssh-5.1p1.dkg/scard-opensc.c
>--- openssh-5.1p1/scard-opensc.c	2007-03-12 16:35:39.000000000 -0400
>+++ openssh-5.1p1.dkg/scard-opensc.c	2008-08-19 21:35:31.000000000 -0400
>@@ -124,7 +121,9 @@
> 	struct sc_pkcs15_prkey_info *key;
> 	struct sc_pkcs15_object *pin_obj;
> 	struct sc_pkcs15_pin_info *pin;
>+	int detach_retry = 1;
> 
>+ detachretry:
> 	priv = (struct sc_priv_data *) RSA_get_app_data(rsa);
> 	if (priv == NULL)
> 		return -1;
>@@ -162,6 +161,13 @@
> 	}
> 	pin = pin_obj->data;
> 	r = sc_lock(card);
>+	if (r == SC_ERROR_READER_DETACHED) {
Shouldn't this be (r == SC_ERROR_READER_DETACHED && detach_retry)
>diff -ruN openssh-5.1p1/ssh-agent.c openssh-5.1p1.dkg/ssh-agent.c
>--- openssh-5.1p1/ssh-agent.c	2008-07-04 09:10:49.000000000 -0400
>+++ openssh-5.1p1.dkg/ssh-agent.c	2008-08-21 11:49:47.000000000 -0400
>@@ -136,6 +136,11 @@
> /* Default lifetime (0 == forever) */
> static int lifetime = 0;
> 
>+#ifdef SMARTCARD
>+/* forward declaration needed */
>+static void remove_all_smartcard_keys();
>+#endif /* SMARTCARD */
>+
> static void
> close_socket(SocketEntry *e)
> {
>@@ -330,8 +335,14 @@
> 	key = key_from_blob(blob, blen);
> 	if (key != NULL) {
> 		Identity *id = lookup_identity(key, 2);
>-		if (id != NULL && (!id->confirm || confirm_key(id) == 0))
>+		if (id != NULL && (!id->confirm || confirm_key(id) == 0)) {
> 			ok = key_sign(id->key, &signature, &slen, data, dlen);
>+#ifdef SMARTCARD
>+			if ((ok != 0) && (id->key->flags &= KEY_FLAG_EXT)) {
>+				remove_all_smartcard_keys();
>+			}
>+#endif /* SMARTCARD */
Could this be moved into scard-opensc.c somehow?

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1506





--- Comment #7 from Daniel Kahn Gillmor <dkg at fifthhorseman.net> 
2009-02-02 11:14:17 ---
Damien wrote:
> Shouldn't this be (r == SC_ERROR_READER_DETACHED &&
detach_retry)
How embarrassing!  You are right, of course.
> [...]
> Could this be moved into scard-opensc.c somehow?
It's been a while since i wrote this.  i'll dig back into it and see if
there's a way to pull this off.  It does seem like an agent-specific
action, though, since none of the other tools actively cache
connections to smartcards.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1506


Daniel Kahn Gillmor <dkg at fifthhorseman.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1561|0                           |1
        is obsolete|                            |




--- Comment #8 from Daniel Kahn Gillmor <dkg at fifthhorseman.net> 
2009-02-04 05:23:13 ---
Created an attachment (id=1600)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1600)
retry smartcard at most once on detached reader, and purge PIN and all
H/W keys from agent on failure

I've updated the attached patch with damien's fix.

I just spent a bit of time looking at the agent and the scard-opensc
code, and it's not clear to me how to reasonably move those later hunks
out of ssh-agent.c and into scard-opensc.c.   Please point out if i'm
missing something or misunderstanding something.

Basically, we'd need to do something like having one possible side
effect of the sc_sign() call be to disable the RSA* object (which
ssh-agent sees as identity.key.rsa).  How would such a disabling work? 
RSA_free() doesn't make sense, because it looks like we wouldn't be
able to propagate that information back to the Key structure, as it
holds the pointer to the RSA object which would be invalid after an
RSA_free().

Even if we can figure out how to invalidate a key in a safe way, we'd
then need to update not only the agent, but also all the other code
that ever calls key_sign() to be aware of the possibility that a side
effect of key_sign() could be the disabling of the passed Key object.

So my current preference is to leave the code in ssh-agent, though i
could probably be convinced otherwise if a good technique was
suggested.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1506


Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|1481                        |1560




-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1506





--- Comment #9 from Daniel Kahn Gillmor <dkg at fifthhorseman.net> 
2009-02-24 02:36:40 ---
I see that this has been moved out from 5.2 to 5.3 -- thanks for
keeping the ticket up-to-date on its status.  Do you need more from me
on this, or should i just be content to wait?

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1506





--- Comment #10 from Daniel Kahn Gillmor <dkg at fifthhorseman.net> 
2009-05-01 06:50:41 ---
In 0.11.5, opensc looks like it added "Basic reader hotplug support.",
which included a new error code SC_ERROR_READER_REATTACHED in addition
to SC_ERROR_READER_DETACHED.  I'm re-working the patch to make it work
properly with opensc 0.11.5+

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1506


Daniel Kahn Gillmor <dkg at fifthhorseman.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #1600|0                           |1
        is obsolete|                            |




--- Comment #11 from Daniel Kahn Gillmor <dkg at fifthhorseman.net> 
2009-05-01 07:24:37 ---
Created an attachment (id=1630)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=1630)
retry smartcard at most once on detached reader, and purge PIN and all
H/W keys from agent on failure

This version of the patch should build against newer versions of
libopensc as well, and handle the new SC_ERROR_READER_REATTACHED return
code.  I'm using it live with libopensc 0.11.7-2+b1 on debian testing
and it works fine.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1506


Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|1560                        |1626


--- Comment #12 from Damien Miller <djm at mindrot.org> 2009-08-18
06:30:16 EST ---
only changes to portable OpenSSH are being considered for 5.3 at this
stage.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1506


Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|1626                        |1560


-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1506


Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|1560                        |1626


-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1506

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|1626                        |

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1506

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |1708

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1506

--- Comment #13 from Damien Miller <djm at mindrot.org> 2010-04-23
11:34:19 EST ---
This is probably obsolete given the replacement of the old smartcard
support with PKCS#11 in OpenSSH 5.4p1. Would you like to close this or
repurpose it for the new code?

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1506

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|1708                        |
             Status|NEW                         |RESOLVED
         Resolution|                            |WORKSFORME

--- Comment #14 from Damien Miller <djm at mindrot.org>  ---
I'll close this since the new PKCS#11 code has rototilled all of the
smartcard-related SSH code. If you still need something like this with
the new PKCS#11 code then feel free to reopen.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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=1506

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #15 from Damien Miller <djm at mindrot.org> 2011-01-24
12:33:33 EST ---
Move resolved bugs to CLOSED after 5.7 release

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- 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.