openssh bugs - Nov 2016 - [Bug 2638] New: Honor PKCS#11 CKA_ALWAYS_AUTHENTICATE attribute of the private objects

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

            Bug ID: 2638
           Summary: Honor PKCS#11 CKA_ALWAYS_AUTHENTICATE attribute of the
                     private objects
           Product: Portable OpenSSH
           Version: 7.3p1
          Hardware: Other
                OS: Linux
            Status: NEW
          Keywords: patch
          Severity: enhancement
          Priority: P5
         Component: Smartcard
          Assignee: unassigned-bugs at mindrot.org
          Reporter: jjelen at redhat.com

Created attachment 2890
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2890&action=edit
[PATCH] Honor PKCS#11 CKA_ALWAYS_AUTHENTICATE attribute of the private
objects

We don't need to care about always_authenticate attribute in case of
simple  ssh  connections, because the private key operation is
performed only once (immediately after login). But this is a problem in
 ssh-agent  which can authenticate more connections.

This patch introduces the additional login (the pin is requested using
SSH_ASKPASS if defined) if this attribute is not CK_FALSE.

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

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

--- Comment #1 from Damien Miller <djm at mindrot.org> ---
Comment on attachment 2890
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2890
[PATCH] Honor PKCS#11 CKA_ALWAYS_AUTHENTICATE attribute of the private
objects

>@@ -316,6 +359,7 @@ pkcs11_rsa_private_encrypt(int flen, const u_char *from,
u_char *to, RSA *rsa,
> 			return (-1);
> 		}
> 		si->logged_in = 1;
>+		login_performed = 1;
...
>+	} else if (!login_performed &&
>+	    pkcs11_always_authenticate(k11->provider, si, obj) < 0) {
>+		error("Failed to re-authenticate to access ALWAYS_AUTHENTICATE
object");
Can't we reuse si->logged_in here and skip the extra variable?

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

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

Jakub Jelen <jjelen at redhat.com> changed:

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

--- Comment #2 from Jakub Jelen <jjelen at redhat.com> ---
Created attachment 3033
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3033&action=edit
patch sharing the login code

We (In reply to Damien Miller from comment #1)
> Can't we reuse si->logged_in here and skip the extra variable?
We would need to reset the variable after the signing if you talk only
about variable sharing. It would work, but the actual
always-authenticate function would not get called for the second time.
It would call the original login before SignInit with
non-CONTEXT_SPECIFIC_LOGIN. It would work in some of the cases, but it
would not be according to the PKCS#11 specification. For example, if
the PINs are different, it would fail.
I don't see a way how to retain the same functionality without this
variable, but feel free to propose a solution.

Though after the second thought (year after), sharing the code for
C_Login, which is quite the same except the login type would make
sense.

I do not share the pkcs11_interactive check, because we need this
prompt from non-interactive ssh-agent process using askpass.

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

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

Orion Poplawski <orion at cora.nwra.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |orion at cora.nwra.com

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

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3033|0                           |1
        is obsolete|                            |
                 CC|                            |djm at mindrot.org

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
Created attachment 3225
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3225&action=edit
revised patch after PKCS#11 ECDSA support landed

-- 
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=2638

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2915

--- Comment #4 from Damien Miller <djm at mindrot.org> ---
Pity there seems no way to test this using softhsm2


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2915
[Bug 2915] Tracking bug for 8.0 release
-- 
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=2638

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |pkcs11

-- 
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=2638

--- Comment #5 from Jakub Jelen <jjelen at redhat.com> ---
Unfortunately ... but you can try that with your yubikey and with
OpenSC if you load the private key in the "SIGN KEY" slot 9c [0].

Note, that after [1] being merged in OpenSC last year, the trick with
only single login does not work anymore so in the proposed patch, we
should drop the did_login variable, otherwise it will not work (at
least with OpenSC pkcs11 module). Therefore, in the single-shot
connection, the pin is asked twice, which is unfortunate, but probably
closest to the PIV specification.

One note for the code style:

+       struct pkcs11_slotinfo  *si;
+       CK_FUNCTION_LIST        *f;
+       CK_BBOOL                flag = 0;
+       CK_ATTRIBUTE            attr;
+       CK_RV                    rv;
                                ^-- misaligned indentation (missing
space in flag, attr)

[0]
https://developers.yubico.com/PIV/Introduction/Certificate_slots.html
[1] https://github.com/OpenSC/OpenSC/pull/1256

-- 
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=2638

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #6 from Damien Miller <djm at mindrot.org> ---
This has been committed and will be in OpenSSH 8.0

-- 
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=2638

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #7 from Damien Miller <djm at mindrot.org> ---
closing resolved bugs as of 8.6p1 release

-- 
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=2638

Ahmed Sayeed <ahmedsayeed1982 at yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ahmedsayeed1982 at yahoo.com

--- Comment #8 from Ahmed Sayeed <ahmedsayeed1982 at yahoo.com> ---
------8<-------
 1 size_t fwrite(const void * __restrict ptr, size_t size,
http://www-look-4.com/category/travel/
 2               size_t nmemb, register FILE * __restrict stream)
 3 {
 4     size_t retval; https://komiya-dental.com/category/technology/
 5     __STDIO_AUTO_THREADLOCK_VAR;
 6  http://www.iu-bloomington.com/category/technology/
 7 >   __STDIO_AUTO_THREADLOCK(stream);
 8 
 9     retval = fwrite_unlocked(ptr, size, nmemb, stream);
10  https://waytowhatsnext.com/category/technology/
11     __STDIO_AUTO_THREADUNLOCK(stream);
12  http://www.wearelondonmade.com/category/travel/
13     return retval;
14 }
------>8-------
 http://www.jopspeech.com/category/travel/
Here, we are at line 7. Using the "next" command leads no where.
However,
setting a breakpoint on line 9 and issuing "continue" works.
http://joerg.li/category/travel/
Looking at the assembly instructions reveals that we're dealing with
the
critical section entry code [1] that should never be interrupted, in
this
case by the debugger's implicit breakpoints:
http://connstr.net/category/travel/

------8<-------
  ... http://embermanchester.uk/category/travel/
1 add_s   r0,r13,0x38
2 mov_s   r3,1
3 llock   r2,[r0]        <-.
4 brne.nt r2,0,14     --.  |
http://www.slipstone.co.uk/category/travel/
5 scond   r3,[r0]       |  |
6 bne     -10         --|--'
7 brne_s  r2,0,84     <-' http://www.logoarts.co.uk/category/travel/
  ...
------>8-------
 http://www.acpirateradio.co.uk/category/travel/
Lines 3 until 5 (inclusive) are supposed to be executed atomically.
Therefore,
GDB should never (implicitly) insert a breakpoint on lines 4 and 5,
else the http://www.compilatori.com/category/travel/ 
program will try to acquire the lock again by jumping back to line 3
and
gets stuck in an infinite loop.
https://www.webb-dev.co.uk/category/technology/

The solution is to make GDB aware of these patterns so it inserts
breakpoints
after the sequence -- line 6 in this example.

-- 
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.