[PATCH] pinpad input support improvement

NIIBE Yutaka gniibe at fsij.org
Wed Jun 19 02:54:48 CEST 2013


Hello,

This is an improvement of scdaemon for pinpad input.

It found that:

 (1) Pinpad input with pcscd doesn't work well for Gemalto GemPCPinpad
     SmartCard Reader and Covadis Vega Alpha Reader.

 (2) Pinpad input with internal ccid driver doesn't work well for
     Covadis Vega Alpha Reader.

For (1), those readers are the one which don't support variable length
pinpad input, and we need to identify PINMIN and PINMAX to compose
pinpad requests.  Recent PC/SC-lite implements
FEATURE_GET_TLV_PROPERTIES, and we use that.

For (2), Covadis Vega Alpha Reader requires "escape" command at
initialization of the reader for GnuPG.  This is because the reader
has a feature to display "the counts before lockdown" (PIN retry
counter, in terms of OpenPGP card), and it is implemented by the
reader using empty VERIFY command.  Since OpenPGP card doesn't support
empty VERIFY command, we need to disable the feature using "escape"
command.  At the initialization of the reader, the card is not yet
powered or it is not inserted yet.  We need to ignore errors of "card
inactive" or "no card".

Thanks to Michel who kindly tested the code.

Tested with both readers.

Here is a patch.

As it is now possible to identify readers by USB vendor/product ID, I
will further modify scd/apdu.c to match internal ccid driver.

======================

* scd/apdu.c (struct reader_table_s): New member PINMIN and PINMAX.
  (pcsc_vendor_specific_init): New.  Examine parameters at init.
  (check_pcsc_pinpad): Set ->minlen and ->maxlen.
  (pcsc_pinpad_verify, pcsc_pinpad_modify): Use more conservative
  settings for bNumberMessage and bMsgIndex*.

* scd/app-openpgp.c (parse_login_data): Free RELPTR on error.
  Fix parsing.

* scd/ccid-driver.c (ccid_vendor_specific_init): Fix initialization
for Covadis Vega Alpha reader.


diff --git a/scd/apdu.c b/scd/apdu.c
index 372932a..9b67eba 100644
--- a/scd/apdu.c
+++ b/scd/apdu.c
@@ -117,6 +117,8 @@ struct reader_table_s {
     pcsc_dword_t protocol;
     pcsc_dword_t verify_ioctl;
     pcsc_dword_t modify_ioctl;
+    int pinmin;
+    int pinmax;
 #ifdef NEED_PCSC_WRAPPER
     int req_fd;
     int rsp_fd;
@@ -220,9 +222,19 @@ static char (* DLSTDCALL CT_close) (unsigned short ctn);
 #define PCSC_W_REMOVED_CARD            0x80100069
 
 #define CM_IOCTL_GET_FEATURE_REQUEST (0x42000000 + 3400)
+#define CM_IOCTL_VENDOR_IFD_EXCHANGE (0x42000000 + 1)
+
 #define FEATURE_VERIFY_PIN_DIRECT        0x06
 #define FEATURE_MODIFY_PIN_DIRECT        0x07
+#define FEATURE_GET_TLV_PROPERTIES       0x12
+
 
+#define PCSCv2_PART10_PROPERTY_bEntryValidationCondition 2
+#define PCSCv2_PART10_PROPERTY_bTimeOut2                 3
+#define PCSCv2_PART10_PROPERTY_bMinPINSize               6
+#define PCSCv2_PART10_PROPERTY_bMaxPINSize               7
+#define PCSCv2_PART10_PROPERTY_wIdVendor                11
+#define PCSCv2_PART10_PROPERTY_wIdProduct               12
 
 /* The PC/SC error is defined as a long as per specs.  Due to left
    shifts bit 31 will get sign extended.  We use this mask to fix
@@ -424,6 +436,8 @@ new_reader_slot (void)
 #endif
   reader_table[reader].pcsc.verify_ioctl = 0;
   reader_table[reader].pcsc.modify_ioctl = 0;
+  reader_table[reader].pcsc.pinmin = -1;
+  reader_table[reader].pcsc.pinmax = -1;
 
   return reader;
 }
@@ -1687,6 +1701,115 @@ reset_pcsc_reader (int slot)
 }
 

+/* Examine reader specific parameters and initialize.  This is mostly
+   for pinpad input.  Called at opening the connection to the reader.  */
+static int
+pcsc_vendor_specific_init (int slot)
+{
+  unsigned char buf[256];
+  pcsc_dword_t len;
+  int sw;
+  int vendor = 0;
+  int product = 0;
+  pcsc_dword_t get_tlv_ioctl = (pcsc_dword_t)-1;
+  unsigned char *p;
+
+  len = sizeof (buf);
+  sw = control_pcsc (slot, CM_IOCTL_GET_FEATURE_REQUEST, NULL, 0, buf, &len);
+  if (sw)
+    {
+      log_error ("pcsc_vendor_specific_init: GET_FEATURE_REQUEST failed: %d\n",
+                 sw);
+      return SW_NOT_SUPPORTED;
+    }
+  else
+    {
+      p = buf;
+      while (p < buf + len)
+        {
+          unsigned char code = *p++;
+          int l = *p++;
+          unsigned int v = 0;
+
+          if (l == 1)
+            v = p[0];
+          else if (l == 2)
+            v = ((p[0] << 8) | p[1]);
+          else if (l == 4)
+            v = ((p[0] << 24) | (p[1] << 16) | (p[2] << 8) | p[3]);
+
+          if (code == FEATURE_VERIFY_PIN_DIRECT)
+            reader_table[slot].pcsc.verify_ioctl = v;
+          else if (code == FEATURE_MODIFY_PIN_DIRECT)
+            reader_table[slot].pcsc.modify_ioctl = v;
+          else if (code == FEATURE_GET_TLV_PROPERTIES)
+            get_tlv_ioctl = v;
+
+          if (DBG_CARD_IO)
+            log_debug ("feature: code=%02X, len=%d, v=%02X\n", code, l, v);
+
+          p += l;
+        }
+    }
+
+  if (get_tlv_ioctl == (pcsc_dword_t)-1)
+    return 0;
+
+  len = sizeof (buf);
+  sw = control_pcsc (slot, get_tlv_ioctl, NULL, 0, buf, &len);
+  if (sw)
+    {
+      log_error ("pcsc_vendor_specific_init: GET_TLV_IOCTL failed: %d\n", sw);
+      return SW_NOT_SUPPORTED;
+    }
+
+  p = buf;
+  while (p < buf + len)
+    {
+      unsigned char tag = *p++;
+      int l = *p++;
+      unsigned int v = 0;
+
+      /* Umm... here is little endian, while the encoding above is big.  */
+      if (l == 1)
+        v = p[0];
+      else if (l == 2)
+        v = ((p[1] << 8) | p[0]);
+      else if (l == 4)
+        v = ((p[3] << 24) | (p[2] << 16) | (p[1] << 8) | p[0]);
+
+      if (tag == PCSCv2_PART10_PROPERTY_bMinPINSize)
+        reader_table[slot].pcsc.pinmin = v;
+      else if (tag == PCSCv2_PART10_PROPERTY_bMaxPINSize)
+        reader_table[slot].pcsc.pinmax = v;
+      else if (tag == PCSCv2_PART10_PROPERTY_wIdVendor)
+        vendor = v;
+      else if (tag == PCSCv2_PART10_PROPERTY_wIdProduct)
+        product = v;
+
+      if (DBG_CARD_IO)
+        log_debug ("TLV properties: tag=%02X, len=%d, v=%08X\n", tag, l, v);
+
+      p += l;
+    }
+
+  if (vendor == 0x0982 && product == 0x0008) /* Vega Alpha */
+    {
+      /*
+       * Please read the comment of ccid_vendor_specific_init in
+       * ccid-driver.c.
+       */
+      const unsigned char cmd[] = { '\xb5', '\x01', '\x00', '\x03', '\x00' };
+      sw = control_pcsc (slot, CM_IOCTL_VENDOR_IFD_EXCHANGE,
+                         cmd, sizeof (cmd), NULL, 0);
+      if (sw)
+        return SW_NOT_SUPPORTED;
+    }
+
+  return 0;
+}
+
+
 /* Open the PC/SC reader without using the wrapper.  Returns -1 on
    error or a slot number for the reader.  */
 #ifndef NEED_PCSC_WRAPPER
@@ -1785,6 +1908,7 @@ open_pcsc_reader_direct (const char *portstr)
   reader_table[slot].send_apdu_reader = pcsc_send_apdu;
   reader_table[slot].dump_status_reader = dump_pcsc_reader_status;
 
+  pcsc_vendor_specific_init (slot);
   dump_reader_status (slot);
   unlock_slot (slot);
   return slot;
@@ -1989,6 +2113,8 @@ open_pcsc_reader_wrapped (const char *portstr)
   reader_table[slot].send_apdu_reader = pcsc_send_apdu;
   reader_table[slot].dump_status_reader = dump_pcsc_reader_status;
 
+  pcsc_vendor_specific_init (slot);
+
   /* Read the status so that IS_T0 will be set. */
   pcsc_get_status (slot, &dummy_status);
 
@@ -2028,56 +2154,23 @@ open_pcsc_reader (const char *portstr)
 static int
 check_pcsc_pinpad (int slot, int command, pininfo_t *pininfo)
 {
-  unsigned char buf[256];
-  pcsc_dword_t len = 256;
-  int sw;
+  int r;
 
-  (void)pininfo;      /* XXX: Identify reader and set pininfo->fixedlen.  */
+  pininfo->minlen = reader_table[slot].pcsc.pinmin;
+  pininfo->maxlen = reader_table[slot].pcsc.pinmax;
 
- check_again:
-  if (command == ISO7816_VERIFY)
-    {
-      if (reader_table[slot].pcsc.verify_ioctl == (pcsc_dword_t)-1)
-        return SW_NOT_SUPPORTED;
-      else if (reader_table[slot].pcsc.verify_ioctl != 0)
-        return 0;                       /* Success */
-    }
-  else if (command == ISO7816_CHANGE_REFERENCE_DATA)
-    {
-      if (reader_table[slot].pcsc.modify_ioctl == (pcsc_dword_t)-1)
-        return SW_NOT_SUPPORTED;
-      else if (reader_table[slot].pcsc.modify_ioctl != 0)
-        return 0;                       /* Success */
-    }
+  if ((command == ISO7816_VERIFY && reader_table[slot].pcsc.verify_ioctl != 0)
+      || (command == ISO7816_CHANGE_REFERENCE_DATA
+          && reader_table[slot].pcsc.modify_ioctl != 0))
+    r = 0;                       /* Success */
   else
-    return SW_NOT_SUPPORTED;
-
-  reader_table[slot].pcsc.verify_ioctl = (pcsc_dword_t)-1;
-  reader_table[slot].pcsc.modify_ioctl = (pcsc_dword_t)-1;
-
-  sw = control_pcsc (slot, CM_IOCTL_GET_FEATURE_REQUEST, NULL, 0, buf, &len);
-  if (sw)
-    return SW_NOT_SUPPORTED;
-  else
-    {
-      unsigned char *p = buf;
+    r = SW_NOT_SUPPORTED;
 
-      while (p < buf + len)
-        {
-          unsigned char code = *p++;
-
-          p++;                  /* Skip length */
-          if (code == FEATURE_VERIFY_PIN_DIRECT)
-            reader_table[slot].pcsc.verify_ioctl
-              = (p[0] << 24) | (p[1] << 16) | (p[2] << 8) | p[3];
-          else if (code == FEATURE_MODIFY_PIN_DIRECT)
-            reader_table[slot].pcsc.modify_ioctl
-              = (p[0] << 24) | (p[1] << 16) | (p[2] << 8) | p[3];
-          p += 4;
-        }
-    }
+  if (DBG_CARD_IO)
+    log_debug ("check_pcsc_pinpad: command=%02X, r=%d\n",
+               (unsigned int)command, r);
 
-  goto check_again;
+  return r;
 }
 

@@ -2124,7 +2217,7 @@ pcsc_pinpad_verify (int slot, int class, int ins, int p0, int p1,
   pin_verify[7] = 0x02; /* bEntryValidationCondition: Validation key pressed */
   if (pininfo->minlen && pininfo->maxlen && pininfo->minlen == pininfo->maxlen)
     pin_verify[7] |= 0x01; /* Max size reached.  */
-  pin_verify[8] = 0xff; /* bNumberMessage: Default */
+  pin_verify[8] = 0x01; /* bNumberMessage: One message */
   pin_verify[9] =  0x09; /* wLangId: 0x0409: US English */
   pin_verify[10] = 0x04; /* wLangId: 0x0409: US English */
   pin_verify[11] = 0x00; /* bMsgIndex */
@@ -2214,12 +2307,12 @@ pcsc_pinpad_modify (int slot, int class, int ins, int p0, int p1,
   pin_modify[10] = 0x02; /* bEntryValidationCondition: Validation key pressed */
   if (pininfo->minlen && pininfo->maxlen && pininfo->minlen == pininfo->maxlen)
     pin_modify[10] |= 0x01; /* Max size reached.  */
-  pin_modify[11] = 0xff; /* bNumberMessage: Default */
-  pin_modify[12] =  0x09; /* wLangId: 0x0409: US English */
+  pin_modify[11] = 0x03; /* bNumberMessage: Three messages */
+  pin_modify[12] = 0x09; /* wLangId: 0x0409: US English */
   pin_modify[13] = 0x04; /* wLangId: 0x0409: US English */
   pin_modify[14] = 0x00; /* bMsgIndex1 */
-  pin_modify[15] = 0x00; /* bMsgIndex2 */
-  pin_modify[16] = 0x00; /* bMsgIndex3 */
+  pin_modify[15] = 0x01; /* bMsgIndex2 */
+  pin_modify[16] = 0x02; /* bMsgIndex3 */
   pin_modify[17] = 0x00; /* bTeoPrologue[0] */
   pin_modify[18] = 0x00; /* bTeoPrologue[1] */
   pin_modify[19] = 2 * pininfo->fixedlen + 0x05; /* bTeoPrologue[2] */
diff --git a/scd/app-openpgp.c b/scd/app-openpgp.c
index 4af4e93..9186e18 100644
--- a/scd/app-openpgp.c
+++ b/scd/app-openpgp.c
@@ -629,7 +629,11 @@ parse_login_data (app_t app)
     if (*buffer == '\n')
       break;
   if (buflen < 2 || buffer[1] != '\x14')
-    return; /* No control sequences.  */
+    {
+      xfree (relptr);
+      return; /* No control sequences.  */
+    }
+
   buflen--;
   buffer++;
   do
@@ -676,14 +680,11 @@ parse_login_data (app_t app)
                       m = strtol (q, &q, 10);
                     }
 
-                  buffer = q;
                   if (buflen < ((unsigned char *)q - buffer))
-                    {
-                      buflen = 0;
-                      break;
-                    }
-                  else
-                    buflen -= ((unsigned char *)q - buffer);
+                    break;
+
+                  buflen -= ((unsigned char *)q - buffer);
+                  buffer = q;
 
                   if (buflen && !(*buffer == '\n' || *buffer == '\x18'))
                     goto next;
@@ -694,11 +695,11 @@ parse_login_data (app_t app)
             }
         }
     next:
-      for (; buflen && *buffer != '\x18'; buflen--, buffer++)
-        if (*buffer == '\n')
-          buflen = 1;
+      /* Skip to FS (0x18) or LF (\n).  */
+      for (; buflen && *buffer != '\x18' && *buffer != '\n'; buflen--)
+        buffer++;
     }
-  while (buflen);
+  while (buflen && *buffer != '\n');
 
   xfree (relptr);
 }
diff --git a/scd/ccid-driver.c b/scd/ccid-driver.c
index 42a219f..b812cb0 100644
--- a/scd/ccid-driver.c
+++ b/scd/ccid-driver.c
@@ -1537,6 +1537,7 @@ ccid_vendor_specific_init (ccid_driver_t handle)
 {
   if (handle->id_vendor == VENDOR_VEGA && handle->id_product == VEGA_ALPHA)
     {
+      int r;
       /*
        * Vega alpha has a feature to show retry counter on the pinpad
        * display.  But it assumes that the card returns the value of
@@ -1545,9 +1546,12 @@ ccid_vendor_specific_init (ccid_driver_t handle)
        * VERIFY command with empty data.  This vendor specific command
        * sequence is to disable the feature.
        */
-      const unsigned char cmd[] = "\xb5\x01\x00\x03\x00";
+      const unsigned char cmd[] = { '\xb5', '\x01', '\x00', '\x03', '\x00' };
 
-      return send_escape_cmd (handle, cmd, sizeof (cmd), NULL, 0, NULL);
+      r = send_escape_cmd (handle, cmd, sizeof (cmd), NULL, 0, NULL);
+      if (r != 0 && r != CCID_DRIVER_ERR_CARD_INACTIVE
+          && r != CCID_DRIVER_ERR_NO_CARD)
+        return r;
     }
 
   return 0;
-- 





More information about the Gnupg-devel mailing list