Created attachment 1559 [details] 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.
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).
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?
Yes, that sounds quite reasonable.
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?
Created attachment 1561 [details] 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.
Comment on attachment 1561 [details] patch to retry smartcard once on detached reader, and purge PIN and all H/W keys from agent on failure >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?
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.
Created attachment 1600 [details] 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.
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?
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+
Created attachment 1630 [details] 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.
only changes to portable OpenSSH are being considered for 5.3 at this stage.
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?
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.
Move resolved bugs to CLOSED after 5.7 release