Created attachment 2718 [details] 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.
Created attachment 2724 [details] 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.
Created attachment 2728 [details] 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.
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@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=
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/
Bump! Would be really nice to have. Thanks!
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.
Created attachment 3069 [details] Updated for 7.6p1 Adding a patch that applies cleanly on 7.6p1
I can confirm that the new patch works just fine with 7.6p1 when using ECDSA from a Yubikey 4.
Created attachment 3093 [details] 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!)
Created attachment 3094 [details] Fifth Iteration off 7.6p1 corrected sorry, the previous attached patch "Fifth Iteration off 7.6p1" was incomplete, replacing with a full one.
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!
Created attachment 3095 [details] 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>).
The latest patch update was done after bug reports and tests from Peter Ankerstål and Hasso Tepper.
(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?
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
Created attachment 3101 [details] Seventh iteration Fix of a fix
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); + }
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
Created attachment 3107 [details] Eigth iteration An updated patch that includes the latest fix Dmitry (and his colleagues)
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?
Created attachment 3122 [details] 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.
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 ,
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 !
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.
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?
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/
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.
closing resolved bugs as of 8.6p1 release