On Wed, 2019-04-24 at 14:09 +0200, Jakub Jelen wrote:
> On Sat, 2019-04-06 at 03:20 +1100, Damien Miller wrote:
> > On Fri, 5 Apr 2019, Jakub Jelen wrote:
> >
> > > There is also changed semantics of the ssh-keygen when listing
> > > keys
> > > from PKCS#11 modules. In the past, it was not needed to enter a
> > > PIN
> > > for
> > > this, but now.
> > >
> > > At least, it is not consistent with a comment in the function
> > > pkcs11_open_session(), which says
> > >
> > > 727 * if pin == NULL we delay login until key use
> > >
> > > Being logged in before listing keys prevents bug #2430, but as a
> > > side
> > > effect, even the ssh can not list keys before login and if the
> > > configuration contains a PKCS#11 module, the user is always
> > > prompted
> > > for a PIN, which is not very user friendly.
> > >
> > > I see this is a regression and the bug #2430 should get solved as
> > > proposed in the patches (will need some tweaks after the big
> > > refactoring).
> >
> > We'll take a look at this (and the other things you just reported)
> > after the release is done.
>
> Release is out with this regression. Is there any progress on this?
> The
> simplest thing how to reproduce is by extending the agent-pkcs11
> regress testsuite with the following line, which previously worked
> fine, but now asks for a pin:
>
> ${SSHKEYGEN} -D ${TEST_SSH_PKCS11}
>
> Is this on a radar or should I create a new bug? I am using keys from
> PKCS#11 all the time and this prevents me from updating to the newer
> version.
Hello there,
digging a bit in the git history, it looks like the regression was
introduced by the commit 7a7fdca [1] authored by markus@, which is
trying to fix a crash introduced by 41923ce [2]. That looks like also
my fault that I preliminary approved this change probably without
proper testing. Certainly the [2] is wrong -- there needs to be a way
to process session_open function without calling to the C_Login and
CKF_LOGIN_REQUIRED should not stay in the way (see the comments in the
bug #2652).
Actually I think both of the commits should get reverted since they are
not addressing any problem, but just breaking the default use cases.
The underlying problem of the bug #2652 is bug #2430 (still not
addressed even though several patches were proposed).
The attached patch is basically the revert that I am going to carry
downstream to have the PKCS#11 working and I recommend to fix this also
in openssh upstream before other people will start using this and
complaining. I would be also happy to help with solving the underlying
problem since there are indeed other users interested in that per the
bug reports.
[1] https://github.com/openssh/openssh-portable/commit/7a7fdca
[2] https://github.com/openssh/openssh-portable/commit/41923ce
[3]
http://docs.oasis-open.org/pkcs11/pkcs11-base/v2.40/os/pkcs11-base-v2.40-os.html
Regards,
--
Jakub Jelen
Senior Software Engineer
Security Technologies
Red Hat, Inc.
commit 14433141d35ccb2e9e1772a13e39e9f750832d68
Author: Jakub Jelen <jjelen@xxxxxxxxxx>
Date: Fri Apr 26 17:19:03 2019 +0200
Revert 7a7fdca and 41923ce to unbreak annoying PIN prompts
diff --git a/ssh-pkcs11.c b/ssh-pkcs11.c
index da176f6d..59332945 100644
--- a/ssh-pkcs11.c
+++ b/ssh-pkcs11.c
@@ -777,17 +777,15 @@ pkcs11_open_session(struct pkcs11_provider *p, CK_ULONG slotidx, char *pin,
CK_FUNCTION_LIST *f;
CK_RV rv;
CK_SESSION_HANDLE session;
- int login_required, have_pinpad, ret;
- char prompt[1024], *xpin = NULL;
+ int login_required, ret;
f = p->module->function_list;
si = &p->module->slotinfo[slotidx];
- have_pinpad = si->token.flags & CKF_PROTECTED_AUTHENTICATION_PATH;
login_required = si->token.flags & CKF_LOGIN_REQUIRED;
/* fail early before opening session */
- if (login_required && !have_pinpad && !pkcs11_interactive &&
+ if (login_required && !pkcs11_interactive &&
(pin == NULL || strlen(pin) == 0)) {
error("pin required");
return (-SSH_PKCS11_ERR_PIN_REQUIRED);
@@ -797,27 +795,8 @@ pkcs11_open_session(struct pkcs11_provider *p, CK_ULONG slotidx, char *pin,
error("C_OpenSession failed for slot %lu: %lu", slotidx, rv);
return (-1);
}
- if (login_required) {
- if (have_pinpad && (pin == NULL || strlen(pin) == 0)) {
- /* defer PIN entry to the reader keypad */
- rv = f->C_Login(session, CKU_USER, NULL_PTR, 0);
- } else {
- if (pkcs11_interactive) {
- snprintf(prompt, sizeof(prompt),
- "Enter PIN for '%s': ", si->token.label);
- if ((xpin = read_passphrase(prompt,
- RP_ALLOW_EOF)) == NULL) {
- debug("%s: no pin specified",
- __func__);
- return (-SSH_PKCS11_ERR_PIN_REQUIRED);
- }
- pin = xpin;
- }
- rv = f->C_Login(session, CKU_USER,
- (u_char *)pin, strlen(pin));
- if (xpin != NULL)
- freezero(xpin, strlen(xpin));
- }
+ if (login_required && pin != NULL && strlen(pin) != 0) {
+ rv = f->C_Login(session, user, (u_char *)pin, strlen(pin));
if (rv != CKR_OK && rv != CKR_USER_ALREADY_LOGGED_IN) {
error("C_Login failed: %lu", rv);
ret = (rv == CKR_PIN_LOCKED) ?
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@xxxxxxxxxxx
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
