openssh bugs - Sep 2015 - [Bug 2474] New: Enabling ECDSA in PKCS#11 support for ssh-agent

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

            Bug ID: 2474
           Summary: Enabling ECDSA in PKCS#11 support for ssh-agent
           Product: Portable OpenSSH
           Version: 7.1p1
          Hardware: All
                OS: All
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: ssh-agent
          Assignee: unassigned-bugs at mindrot.org
          Reporter: mathias at brossard.org

Created attachment 2718
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2718&action=edit
First iteration

I have made a patch for enabling the use of ECDSA keys in the PKCS#11
support of ssh-agent which will be of interest to other users.

I have tested it with P-256 keys. P-384 and P-521 should work
out-of-the box. The code is ready for non-FIPS curves (named or
explicit), but OpenSSH currently limits ECDSA to those 3 curves.

At high level it works like the support for RSA, but because of
differences in OpenSSL between RSA and EC_KEY, implementation has a
few differences. The RSA and RSA_METHOD structures are exposed and the
existing ssh-pkcs11 code uses that to create an RSA_METHOD object for
each key.

Because of APIs (in addition to ECDSA support) needed by the patch
this currently works with:

- LibreSSL >= 2.2.2: until LibreSSL 2.1.2 (which is the what I am
  testing for), the ECDSA_METHOD structure was defined in a private
  header. But the LIBRESSL_VERSION_NUMBER constant was not updated
  until 2.2.2.

- OpenSSL >= 1.0.2: creating your own ECDSA_METHOD is not possible
  before because the ECDSA_METHOD structure if opacified. In OpenSSL
  1.0.2, they added the option to create new ECDSA_METHOD object if
  this is detectable with the ECDSA_F_ECDSA_METHOD_NEW define.

A few notes to understand the patch:

- A few places assumed RSA keys, I added a key type field and use it
  to handle the differences. I also renamed some function to reflect
  their link to RSA.

- I moved some code out of pkcs11_rsa_private_encrypt into a separate
  function pkcs11_login to share it with pkcs11_ecdsa_sign

- For EC_KEY, the pointer to the struct pkcs11_key object is not in
  the method but in the EC_KEY itself using ECDSA_set_ex_data and
  ECDSA_set_ex_data. This allows having a single ECDSA_METHOD for
  all keys.

- Unlike the RSA_METHOD, ECDSA_METHOD does not include a "finish"
  method to clean up the associated data. This was only a problem for
  ssh-pkcs11-helper.c that called key_free on struct sshkey objects
  created by ssh-pkcs11.c. To work around that I added a function
  pkcs11_del_key(struct sshkey *) to the list of functions exported by
  ssh-pkcs11.c that allows us to properly clean up ECDSA keys.

I tried to:
- be as consistent as possible with the RSA part,
- minimize the size of the patch and the number of locations,
- document some of the additional quirks specific to ECDSA.

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

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

Mathias <mathias at brossard.org> changed:

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

--- Comment #1 from Mathias <mathias at brossard.org> ---
Created attachment 2724
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2724&action=edit
Second iteration

The second iteration fixes a compilation problem in the case the
OpenSSL/LibreSSL library doesn't meet the requirement to support ECDSA
on PKCS#11.

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

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

Mathias <mathias at brossard.org> changed:

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

--- Comment #2 from Mathias <mathias at brossard.org> ---
Created attachment 2728
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2728&action=edit
Third iteration

This third iteration contains two bug fixes and has been tested
successfully with 2 different smart-cards with P-256 and a software
token with P-256, P-384 and P-521.

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

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

Peter <peter at pean.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |peter at pean.org

--- Comment #3 from Peter <peter at pean.org> ---
I can confirm that this patch works on FreeBSD 11-Current using the
smart card on a Yubikey NEO with a ECCP256 key.

FreeBSD mobius 11.0-CURRENT FreeBSD 11.0-CURRENT #9 r290134M: Thu Oct
29 10:46:05 CET 2015     peter at mobius:/usr/obj/usr/src/sys/GENERIC 
amd64

OpenSSL 1.0.2d-freebsd 9 Jul 2015
> ./ssh-keygen -D /usr/local/lib/opensc-pkcs11.so
C_GetAttributeValue failed: 18
ecdsa-sha2-nistp256
AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBAJGTLvC9BHSNPAfOw3s4lEix3zKLBKRgZlQ9kSxyttSG8XZ/NIoxm+ZYGbkYxji1kN7brMff21mgXGUzfxp58M
-- 
You are receiving this mail because:
You are watching the assignee of the bug.

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

Jan V??el??k <jvcelak at fedoraproject.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jvcelak at fedoraproject.org

--- Comment #4 from Jan V??el??k <jvcelak at fedoraproject.org> ---
Is there a chance this patch will be merged any time soon?

I'm using the patch for about six months and it works perfectly with my
Yubikey NEO.

Here are my builds with patched OpenSSL for Fedora:
https://copr.fedorainfracloud.org/coprs/jvcelak/openssh/

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

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

misc at zarb.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |misc at zarb.org

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

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

A. Karl Kornel <karl at kornel.us> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |karl at kornel.us

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

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

--- Comment #5 from Peter <peter at pean.org> ---
Bump! Would be really nice to have. Thanks!

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

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

Hasso Tepper <hasso.tepper at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hasso.tepper at gmail.com

--- Comment #6 from Hasso Tepper <hasso.tepper at gmail.com> ---
To mitigate Infineon RSA vulnerability (ROCA) many smartcard users are
forced to switch to ECDSA (it includes 750000 Estonian national ID-card
users for example). Not all of them use OpenSSH, but many do and this
move breaks it for them. So please consider merging this ASAP.

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

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

--- Comment #7 from Mathias <mathias at brossard.org> ---
Created attachment 3069
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3069&action=edit
Updated for 7.6p1

Adding a patch that applies cleanly on 7.6p1

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

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

--- Comment #8 from Peter <peter at pean.org> ---
I can confirm that the new patch works just fine with 7.6p1 when using
ECDSA from a Yubikey 4.

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

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

Dmitry Savintsev <dsavints at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3069|0                           |1
        is obsolete|                            |
                 CC|                            |dsavints at gmail.com

--- Comment #9 from Dmitry Savintsev <dsavints at gmail.com> ---
Created attachment 3093
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3093&action=edit
Fifth Iteration off 7.6p1

I believe there is a small bug in the previous version of the patch
("Updated for 7.6p1" - 2017-10-20 15:48 EST) with missing zero check
on
k11->keyid_len before calling xmalloc in pkcs11_ecdsa_wrap. This leads
to ssh-pkcs11-helper crashing when trying to add a SoftHSM
(https://www.opendnssec.org/softhsm/) card with an ECDSA key (though it
works fine with only RSA keys present).  The check "if
(k11->keyid_len
> 0) {" is present in the pkcs11_rsa_wrap function, now added also in
pkcs11_ecdsa_wrap.  I also uploaded the 7.6p1 version with the previous
("Updated for 7.6p1") patch to
https://github.com/dmitris/openssh-portable/tree/7.6p1-bug2474-patch,
the version with the current fix is in
https://github.com/dmitris/openssh-portable/tree/7.6p1-bug2474-patch-fix
and the diff can be seen in the demo PR
https://github.com/dmitris/openssh-portable/pull/1/files.  

With the fix applied, I was able to successfully add the SoftHSM
"card"
with ECDSA keys with "ssh-add -s
/usr/local/lib/softhsm/libsofthsm2.so".  (Thanks so much Mathias for
creating the patch and making this possible!)

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

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

Dmitry S. <dsavints at gmail.com> changed:

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

--- Comment #10 from Dmitry S. <dsavints at gmail.com> ---
Created attachment 3094
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3094&action=edit
Fifth Iteration off 7.6p1 corrected

sorry, the previous attached patch "Fifth Iteration off 7.6p1" was
incomplete, replacing with a full one.

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

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

--- Comment #11 from Dmitry S. <dsavints at gmail.com> ---
Here's the comparison of the proposed patch with the master version:
https://github.com/openssh/openssh-portable/pull/80

It would be great to add it to the next release, if possible.  If
there's anything I could do to help this happen, I would be happy to,
please let me know!

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

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

Mathias <mathias at brossard.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2728|0                           |1
        is obsolete|                            |
   Attachment #3094|0                           |1
        is obsolete|                            |

--- Comment #12 from Mathias <mathias at brossard.org> ---
Created attachment 3095
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3095&action=edit
Sixth iteration

I've updated my patch with the fix from Dmitry and a change to avoid
error messages (notably when using ssh-keygen -D <pkcs11.so>).

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

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

--- Comment #13 from Mathias <mathias at brossard.org> ---
The latest patch update was done after bug reports and tests from Peter
Ankerst?l and Hasso Tepper.

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

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

--- Comment #14 from Dmitry S. <dsavints at gmail.com> ---
(In reply to Mathias from comment #12)
> Created attachment 3095 [details]
> Sixth iteration
> 
> I've updated my patch with the fix from Dmitry [...]
Thanks Mathias - but I wonder if the fix was properly propagated.  In
my patch, I had the following in ssh-pkcs11.c function
pkcs11_ecdsa_wrap:

+       /* identify key object on smartcard */
+       k11->keyid_len = keyid_attrib->ulValueLen;
+       if (k11->keyid_len > 0) {
+         k11->keyid = xmalloc(k11->keyid_len);
+       }

in your latest ("Sixth iteration") patch I see the statements in
different order:

+       /* identify key object on smartcard */
+       if (k11->keyid_len > 0) {
+               k11->keyid_len = keyid_attrib->ulValueLen;
+               k11->keyid = xmalloc(k11->keyid_len);
+    }

Is it a typo or have you done it for a reason that I'm missing?  Should
not we first extract the value from keyid_attrib->ulValueLen and assign
it to k11->keyid_len and only then use it in the if condiftion for zero
check?  This is how it is done in pkcs11_rsa_wrap in master ov
V_7_6_p1:
https://github.com/openssh/openssh-portable/blob/V_7_6_P1/ssh-pkcs11.c#L324-L325

I believe in your code the k11->keyid_len is uninitialized and
therefore can take arbitrary values leading to undefined behavior. 
Please let me know if I'm missing something here.  If it is a bug, I
wonder if we could add a test to catch it, so it would fail on the
current patch and succeed with a fix?

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

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

--- Comment #15 from Dmitry S. <dsavints at gmail.com> ---
I copied the latest patches to
https://github.com/dmitris/openssh-portable-patches - the code snippets
in question are:
https://github.com/dmitris/openssh-portable-patches/blob/master/fifth-iteration.patch#L431-L433
vs
https://github.com/dmitris/openssh-portable-patches/blob/master/sixth-iteration.patch#L431-L433

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

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

Mathias <mathias at brossard.org> changed:

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

--- Comment #16 from Mathias <mathias at brossard.org> ---
Created attachment 3101
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3101&action=edit
Seventh iteration

Fix of a fix

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

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

--- Comment #17 from Mathias <mathias at brossard.org> ---
Thanks for catching that, Dmitry.  I don't know how I ended up with the
sixth iteration patch, but I intended to improve to the following
(moving the memcpy inside the if):

+       /* identify key object on smartcard */
+       k11->keyid_len = keyid_attrib->ulValueLen;
+       if (k11->keyid_len > 0) {
+               k11->keyid = xmalloc(k11->keyid_len);
+               memcpy(k11->keyid, keyid_attrib->pValue,
k11->keyid_len);
+    }

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

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

--- Comment #18 from Dmitry S. <dsavints at gmail.com> ---
Hi Mathias - my colleagues identified a problem with the ECDSA
signatures in the process_sign() function which happens when r and s in
the signature are smaller than the order size.  This does not happen
most the times but is especially noticeable when a large number of
signing operations are performed.

We have come up with this fix:
https://github.com/dmitris/openssh-portable/pull/3/files

Could you please check it out and let me know if you have any
questions, or otherwise incorporate it in the next version of your
patch?  Thanks.

Regards,

- Dmitry

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

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

--- Comment #19 from Mathias <mathias at brossard.org> ---
Created attachment 3107
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3107&action=edit
Eigth iteration

An updated patch that includes the latest fix Dmitry (and his
colleagues)

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

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

Mathias <mathias at brossard.org> changed:

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

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

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

popovec <popovec.peter at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |popovec.peter at gmail.com

--- Comment #20 from popovec <popovec.peter at gmail.com> ---
Hi

I can confirm functionality of last patch (tested with opensc, MyEID
card, with nist256v1 and secp384r1). There is already opensc patch
(https://github.com/OpenSC/OpenSC/pull/1233) for extracting ECC key
from card in ssh format available. This patch is waiting for ECDSA
support in openssh pkcs11 interface. Is there a time plan when this
patch can be merged into official openssh release?

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

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

Jakub Jelen <jjelen at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jjelen at redhat.com

--- Comment #21 from Jakub Jelen <jjelen at redhat.com> ---
Created attachment 3122
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3122&action=edit
load ECDSA public key from certificates

I was trying to build it against OpenSSL 1.1.0 and hit several issues
with the eighth iteration patch:

 * I would say that the OpenSSL version and functions detection step
should happen during the configure phase, rather than during build on
top of each file using ECDSA keys.

 * OpenSSL 1.1.0 obsoletes most of the ECDSA_* structures and functions
in favor of generic EC_KEY*. The OpenSSL 1.1.0 method structure is
called EC_KEY_METHOD. Doing this in compatible manner will be pain.

 * The functionality of pulling the public key from X.509 certificate
is completely missing. 

In the attached patch, there is a snippet to load ECDSA public keys
from certificates as my small contribution. Feel free to include it in
your patch.

I somehow solved the OpenSSL 1.1.0 compatibility, but probably for the
price of breaking backward compatibility with OpenSSL 1.0.x or
LibreSSL.

I put together some modifications to the original patch and built it in
copr for Fedora if somebody is interested and would like to try:

https://copr.fedorainfracloud.org/coprs/jjelen/openssh-pkcs11/

So far my testing looks fine, keys on my yubikey are recognized and I
can authenticate using them successfully.

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

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

--- Comment #22 from Jakub Jelen <jjelen at redhat.com> ---
I also noticed, that the ssh-keygen manual page needs to be updated:

--- a/ssh-keygen.1
+++ b/ssh-keygen.1
@@ -269,7 +269,7 @@ newer OpenSSH format.
 The program will prompt for the file containing the private keys, for
 the passphrase if the key has one, and for the new comment.
 .It Fl D Ar pkcs11
-Download the RSA public keys provided by the PKCS#11 shared library
+Download the public keys provided by the PKCS#11 shared library
 .Ar pkcs11 .
 When used in combination with
 .Fl s ,

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

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

Kyle Marek <psppsn96 at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |psppsn96 at gmail.com

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

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

Andy Sayler <andy.sayler at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |andy.sayler at gmail.com

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

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

b631093f-779b-4d67-9ffe-5f6d5b1d3f8a at protonmail.ch changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |b631093f-779b-4d67-9ffe-5f6
                   |                            |d5b1d3f8a at protonmail.ch

--- Comment #23 from b631093f-779b-4d67-9ffe-5f6d5b1d3f8a at protonmail.ch ---
I just wanted to add my 2 cents worth that this REALLY REALLY needs to
be pulled into the official codebase.  This has been going on for years
now, time to fix PKCS11 support !

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

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

--- Comment #24 from Peter <peter at pean.org> ---
Once again I would like to add that this feature is rally needed! We
use smartcards heavily at work and would really benefit from faster key
operations.

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

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

--- Comment #25 from Andy Sayler <andy.sayler at gmail.com> ---
I've started maintaining a Ubuntu PPA for 16.04 and 18.04 that carries
these patches at
https://launchpad.net/~andy.sayler/+archive/ubuntu/openssh-pkcs11-ecdsa.

That said, I don't love shipping non-upstreamed patches for something
as sensitive as openssh, so I'd love to see these properly upstreamed
as well.

Seems this ticket has stalled out. Any thoughts on how best to move it
toward proper upstreaming?

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

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

--- Comment #26 from Jakub Jelen <jjelen at redhat.com> ---
Welcome onboard. We ship this since Fedora 28 [1]. Hopefully more
testing and reviews from more people understanding the PKCS#11 can help
upstream take it.

[1]
https://fedoramagazine.org/fedora-28-better-smart-card-support-openssh/

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

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

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
                 CC|                            |djm at mindrot.org
             Status|NEW                         |RESOLVED
             Blocks|                            |2915

--- Comment #27 from Damien Miller <djm at mindrot.org> ---
Markus has added support for ECDSA in PKCS#11 tokens and some
regression tests against softhsm2. This is planned to be in the OpenSSH
8.0 release.


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 someone on the CC list of the bug.
You are watching the assignee of the bug.

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

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #28 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.