scd: Generating CSR for NetKey card key fails
Closed, ResolvedPublic

Description

Generating a CSR for the signing key of a NetKey card fails.

How to reproduce:

  • Put the following key parameters into a file (e.g. keyparams.txt)
Key-Type:card:NKS-NKS3.4531
Key-Usage:sign
Name-DN:CN=Otto Example,O=Example,C=DE
Name-Email:otto@example.net
  • Run
$ gpgsm --debug=ipc --gen-key --armor --batch <keyparams.txt
gpgsm: reading options from '/home/ingo/.gnupg/gpgsm.conf'
gpgsm: enabled debug flags: ipc
gpgsm: DBG: chan_3 <- OK Pleased to meet you, process 29094
gpgsm: DBG: connection to agent established
gpgsm: DBG: chan_3 -> RESET
gpgsm: DBG: chan_3 <- OK
gpgsm: DBG: chan_3 -> OPTION ttyname=/dev/pts/40
gpgsm: DBG: chan_3 <- OK
gpgsm: DBG: chan_3 -> OPTION ttytype=xterm-256color
gpgsm: DBG: chan_3 <- OK
gpgsm: DBG: chan_3 -> OPTION display=:0
gpgsm: DBG: chan_3 <- OK
gpgsm: DBG: chan_3 -> OPTION xauthority=/run/user/1000/xauth_NoUfTI
gpgsm: DBG: chan_3 <- OK
gpgsm: DBG: chan_3 -> OPTION putenv=XMODIFIERS=@im=ibus
gpgsm: DBG: chan_3 <- OK
gpgsm: DBG: chan_3 -> OPTION putenv=GTK_IM_MODULE=ibus
gpgsm: DBG: chan_3 <- OK
gpgsm: DBG: chan_3 -> OPTION putenv=DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1000/bus
gpgsm: DBG: chan_3 <- OK
gpgsm: DBG: chan_3 -> OPTION putenv=QT_IM_MODULE=ibus
gpgsm: DBG: chan_3 <- OK
gpgsm: DBG: chan_3 -> OPTION lc-ctype=de_DE.UTF-8
gpgsm: DBG: chan_3 <- OK
gpgsm: DBG: chan_3 -> OPTION lc-messages=de_DE.UTF-8
gpgsm: DBG: chan_3 <- OK
gpgsm: DBG: chan_3 -> GETINFO version
gpgsm: DBG: chan_3 <- D 2.2.25
gpgsm: DBG: chan_3 <- OK
gpgsm: DBG: chan_3 -> OPTION allow-pinentry-notify
gpgsm: DBG: chan_3 <- OK
gpgsm: DBG: chan_3 -> RESET
gpgsm: DBG: chan_3 <- OK
gpgsm: DBG: chan_3 -> SCD READKEY NKS-NKS3.4531
gpgsm: DBG: chan_3 <- [ 44 20 28 31 30 3a 70 75 62 6c 69 63 2d 6b 65 79 ...(291 byte(s) skipped) ]
gpgsm: DBG: chan_3 <- OK
gpgsm: about to sign the CSR for key: &39400430E38BB96F105B740A7119FE113578B59D
gpgsm: DBG: chan_3 -> SCD SETDATA 65960DFECD593A6EB9B4798E94B25005D508AB048612784024D176CDE57BF3D1
gpgsm: DBG: chan_3 <- OK
gpgsm: DBG: chan_3 -> SCD PKSIGN --hash=sha256 NKS-NKS3.4531
gpgsm: DBG: chan_3 <- ERR 100663351 Invalid value <SCD>
gpgsm: signing failed: Invalid value
gpgsm: error creating certificate request: Invalid value <SCD>
secmem usage: 0/16384 bytes in 0 blocks

The problem is that the raw SHA-256 message digest is passed to do_sign() (app-nks.c), but unlike the corresponding functions in app-openpgp.c and app-piv.c the function in app-nks.c seems to expect an ASN.1 encoded hash (or a raw SHA-1 or RMD160 digest). If have fixed this locally by adding support for a raw SHA-256 message digest, but I guess we also want to support all other SHA algorithm (as in app-openpgp.c):

diff --git a/scd/app-nks.c b/scd/app-nks.c
index 6f24e6e83..0479f8b1c 100644
--- a/scd/app-nks.c
+++ b/scd/app-nks.c
@@ -1665,6 +1665,10 @@ do_sign (app_t app, ctrl_t ctrl, const char *keyidstr, int hashalgo,
   static unsigned char rmd160_prefix[15] = /* Object ID is 1.3.36.3.2.1 */
     { 0x30, 0x21, 0x30, 0x09, 0x06, 0x05, 0x2b, 0x24, 0x03,
       0x02, 0x01, 0x05, 0x00, 0x04, 0x14 };
+  static unsigned char sha256_prefix[19] = /* (2.16.840.1.101.3.4.2.1) */
+    { 0x30, 0x31, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86,
+      0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01, 0x05,
+      0x00, 0x04, 0x20  };
   gpg_error_t err;
   int idx;
   int pwid;
@@ -1677,8 +1681,10 @@ do_sign (app_t app, ctrl_t ctrl, const char *keyidstr, int hashalgo,
 
   switch (indatalen)
     {
-    case 16: case 20: case 35: case 47: case 51: case 67: case 83: break;
-    default: return gpg_error (GPG_ERR_INV_VALUE);
+    case 20: case 32: case 35: case 47: case 51: case 67: case 83: break;
+    default:
+      log_debug ("invalid length of input data: %zu\n", indatalen);
+      return gpg_error (GPG_ERR_INV_VALUE);
     }
 
   err = find_fid_by_keyref (app, keyidstr, &idx, NULL);
@@ -1723,6 +1732,19 @@ do_sign (app_t app, ctrl_t ctrl, const char *keyidstr, int hashalgo,
       memcpy (data, indata, indatalen);
       datalen = 35;
     }
+  else if (indatalen == 32)
+    {
+      size_t prefixlen;
+      if (hashalgo == GCRY_MD_SHA256)
+        {
+          prefixlen = sizeof sha256_prefix;
+          memcpy (data, sha256_prefix, prefixlen);
+        }
+      else
+        return gpg_error (GPG_ERR_UNSUPPORTED_ALGORITHM);
+      memcpy (data+prefixlen, indata, indatalen);
+      datalen = prefixlen+indatalen;
+    }
   else if (indatalen == 20)
     {
       if (hashalgo == GCRY_MD_SHA1)

Note: This patch also removes case 16: from the above switch because an indatalen of 16 isn't handled and would anyway result in a GPG_ERR_INV_VALUE further down.

gniibe added a subscriber: gniibe.Dec 18 2020, 7:54 AM

IIUC, for completeness, it would be good to add the lines like:

else if (indatalen == 51)
  {
    if (hashalgo == GCRY_MD_SHA256 && !memcmp (indata, sha256_prefix, sizeof sha256_prefix))
      ;
    else
      return gpg_error (GPG_ERR_UNSUPPORTED_ALGORITHM);
    memcpy (data, indata, indatalen);
    datalen = 15;
  }

Yes, makes sense. Although, you should use datalen = indatalen; in the last line (to prevent typos in the numbers).

Note that the case indatalen == 51 is already handled in

/* Prepare the DER object from INDATA.  */
if (app->appversion > 2 && (indatalen == 35
                            || indatalen == 47
                            || indatalen == 51
                            || indatalen == 67
                            || indatalen == 83))
  {
    /* The caller send data matching the length of the ASN.1 encoded
       hash for SHA-{1,224,256,384,512}.  Assume that is okay.  */
    log_assert (indatalen <= sizeof data);
    memcpy (data, indata, indatalen);
    datalen = indatalen;
  }

In app-openpgp.c some macros are used to reduce code duplication and risk of copy&paste errors.

werner triaged this task as High priority.Tue, Jan 5, 9:13 AM
werner claimed this task.
werner reassigned this task from werner to ikloecker.Tue, Jan 5, 11:08 AM
werner added a subscriber: werner.

It seems you have a pretty good understanding and also test cases at hand. May I ask you to apply the suggested pacthes to master?

(The length of 16 is likely a relict from the very od MD5 support we used to have a long time ago - indeed no need for it).

ikloecker closed this task as Resolved.Wed, Jan 6, 12:22 PM