ECC encryption key on-card generation broken
Closed, ResolvedPublic

Description

Commits ea09b6cded9d31a8ebd91878553c3eaa2b76e817 and acb300543422c660c87ac2f0211a42f792a65cc4 break the possibility to generate ECC encryption key on-card when any curve other than 25519 is used.

The problem comes from the fact that PUBKEY_ALGO_ECDH is both used for 25519 and non-25519 curves.
The problem does not appear for signature and authentication keys as PUBKEY_ALGO_EDDSA is used for 25519 and PUBKEY_ALGO_ECDSA for non-25519 curves. The patch below fixes the problem, but will break the "magic trick" introduced for GnuK...

A possible solution is to use another constant such as PUBKEY_ALGO_EDDH (with a new value to identify this case such as 23 for instance...) for encryption with 25519, and leave PUBKEY_ALGO_ECDH for non-25519 curves, as previously.

Index: gnupg/g10/card-util.c
===================================================================
--- gnupg.orig/g10/card-util.c
+++ gnupg/g10/card-util.c
@@ -1486,7 +1486,7 @@ generate_card_keys (ctrl_t ctrl)
       for (keyno = 0; keyno < DIM (info.key_attr); keyno++)
         {
           if (info.key_attr[keyno].algo == PUBKEY_ALGO_RSA
-              || info.key_attr[keyno].algo == PUBKEY_ALGO_ECDH
+              //|| info.key_attr[keyno].algo == PUBKEY_ALGO_ECDH
               || info.key_attr[keyno].algo == PUBKEY_ALGO_EDDSA)
             {
               if (info.key_attr[keyno].algo == PUBKEY_ALGO_RSA)
@@ -1572,7 +1572,7 @@ card_generate_subkey (ctrl_t ctrl, kbnod
   if (info.is_v2 && info.extcap.aac)
     {
       if (info.key_attr[keyno-1].algo == PUBKEY_ALGO_RSA
-          || info.key_attr[keyno].algo == PUBKEY_ALGO_ECDH
+          //|| info.key_attr[keyno].algo == PUBKEY_ALGO_ECDH
           || info.key_attr[keyno].algo == PUBKEY_ALGO_EDDSA)
         {
           unsigned int nbits;
Arnaud created this task.Feb 6 2018, 6:10 PM
Arnaud updated the task description. (Show Details)
Arnaud updated the task description. (Show Details)Feb 6 2018, 6:12 PM
gniibe claimed this task.Feb 13 2018, 5:33 AM
werner added a subscriber: werner.Mar 5 2018, 9:39 AM

So should we revert this patch and replace it by an explicit command to switch the card to ECC?

werner added a comment.Mar 5 2018, 9:40 AM

This has also the advantage that we could list the possible curves and let the user select them.

This would be a good solution.

gniibe triaged this task as Normal priority.Mar 29 2018, 6:10 AM

I changed the interaction so that user can specify RSA or ECC, then when it's for ECC, specifying curve.

Furthermore, I changed to have an explicit command: key-attr

gniibe changed the task status from Open to Testing.Mar 30 2018, 4:52 AM
gniibe closed this task as Resolved.Apr 11 2018, 1:58 AM

Fixed in 2.2.6.

Hi,

works just fine, thx!

I wondered if we could enable brainpoolP384r1 or alike for non-expert mode as well, as this is one of the two possible curves for OpenPGP Card v3.

It could look like:

Please select which elliptic curve you want:

(1) Curve 25519
(4) NIST P-384
(7) Brainpool P-384

Kind regards
Alex

Neither Brainpool nor NIST curves make any sense unless there is an organizational policy requirement. Thus the --expert requirement is the Right Thing (tm).

Well, I surely would agree (and this is only a proposal anyway), but my point here is, that OpenPGP Card does not support Curve 25519, so that one *have to* choose between those other two. Considering me a tinfoil hat person, I would rather not choose NIST, as many others wouldn't too.

That is the reason I'd like to see Brainpool here as well, so that OpenPGP Card v3 users could choose between NIST and Brainpool, their only two options (at least for curves)...

@nitroalex Perhaps, creating new ticker is better for this topic.
In the current OpenPGP card specification, there is no way for an application (except having a list of card implementation information) to know wich algo and which curve is supported or not.
So, what an application does is try and error.
I don't like this situation, but I don't know how we can modify the specification.