[PATCH] Scute sometimes fails with GnuPG 2.1.0

[Damien Goutte-Gattat]

Hi folks,

This is a follow-up to a discussion that occured on this list a few months
ago [1], about Scute and the fact that the last released version didn’t work
with TLS 1.2.

At that time, Werner Koch proposed a patch against Scute [2], but it seemed
not to work with GnuPG 2.0.26.

With GnuPG 2.1.0 and Werner’s patch applied to Scute, it mostly works fine,
but Scute *sometimes* fails with a generic error (CKR_FUNCTION_FAILED).

As far as my understanding of the problem goes, it comes from the fact that
GnuPG Agent sometimes prepends a nul byte to the signature data it got from
Scdaemon, iff that signature begins with a byte with the 7th bit set. This
extra byte foils the parsing code inside Scute, which only expects the
signature to be 128- or 256-byte long.

The patch below attempts to fix the problem in Scute, first by reading the
actual size of the signature from the S-expression, and second by removing
the first nul byte, if present.

This is a rather crude attempt (although it works, for me anyway), and as
the comment in the code says, it would probably be better to actually parse
the S-expression, and then extract the signature data from it. What do you
think of using libgcrypt’s gcry_sexp_* functions to do that?

Regards,

Damien

[1] http://lists.gnupg.org/pipermail/gnupg-devel/2014-August/028709.html

[2] http://lists.gnupg.org/pipermail/gnupg-devel/2014-September/028750.html

---
 src/agent.c | 54 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/src/agent.c b/src/agent.c
index edf8d2d..1e4e0ca 100644
--- a/src/agent.c
+++ b/src/agent.c
@@ -981,13 +981,12 @@ pksign_cb (void *opaque, const void *buffer, size_t length)
 }
 
 
-#define SIG_PREFIX   "(7:sig-val(3:rsa(1:s128:"
-#define SIG_PREFIX_2 "(7:sig-val(3:rsa(1:s256:"
+#define SIG_PREFIX   "(7:sig-val(3:rsa(1:s"
 #define SIG_PREFIX_LEN (sizeof (SIG_PREFIX) - 1)
 #define SIG_POSTFIX ")))"
 #define SIG_POSTFIX_LEN (sizeof (SIG_POSTFIX) - 1)
-#define SIG_LEN 128
-#define SIG_LEN_2 256
+#define SIG_MIN_LEN 128
+#define SIG_MAX_LEN 256
 
 
 static unsigned char sha1_prefix[15] =   /* (1.3.14.3.2.26) */
@@ -1033,14 +1032,14 @@ scute_agent_sign (char *grip, unsigned char *data, int len,
   if (sig_result == NULL)
     {
       /* FIXME:  We return the largest supported size - is that correct?  */
-      *sig_len = SIG_LEN_2;
+      *sig_len = SIG_MAX_LEN;
       return 0;
     }
 
   if (len > MAX_DATA_LEN)
     return gpg_error (GPG_ERR_INV_ARG);
 
-  if (grip == NULL || sig_result == NULL || *sig_len < SIG_LEN)
+  if (grip == NULL || sig_result == NULL || *sig_len < SIG_MIN_LEN)
     return gpg_error (GPG_ERR_INV_ARG);
 
   snprintf (cmd, sizeof (cmd), "SIGKEY %s", grip);
@@ -1092,29 +1091,36 @@ scute_agent_sign (char *grip, unsigned char *data, int len,
     return err;
 
   /* FIXME: we need a real parser to cope with all kind of S-expressions.  */
-  if (sig.len == SIG_PREFIX_LEN + SIG_LEN_2 + SIG_POSTFIX_LEN)
+  if (!memcmp (sig.data, SIG_PREFIX, SIG_PREFIX_LEN))
     {
-      if (memcmp (sig.data, SIG_PREFIX_2, SIG_PREFIX_LEN))
-        return gpg_error (GPG_ERR_BAD_SIGNATURE);
-      if (memcmp (sig.data + sig.len - SIG_POSTFIX_LEN,
-                  SIG_POSTFIX, SIG_POSTFIX_LEN))
-        return gpg_error (GPG_ERR_BAD_SIGNATURE);
-      memcpy (sig_result, sig.data + SIG_PREFIX_LEN, SIG_LEN_2);
-      *sig_len = SIG_LEN_2;
-    }
-  else
-    {
-      if (sig.len != SIG_PREFIX_LEN + SIG_LEN + SIG_POSTFIX_LEN)
+      int data_len;
+      unsigned char *sig_begin;
+
+      data_len = atoi(sig.data + SIG_PREFIX_LEN);
+      sig_begin = strchr(sig.data + SIG_PREFIX_LEN, ':');
+      if (!sig_begin)   /* Invalid S-expression? */
         return gpg_error (GPG_ERR_BAD_SIGNATURE);
-      if (memcmp (sig.data, SIG_PREFIX, SIG_PREFIX_LEN))
+
+      sig_begin += 1;   /* Skip colon. */
+
+      if (sig.len != sig_begin - sig.data + data_len + SIG_POSTFIX_LEN)
         return gpg_error (GPG_ERR_BAD_SIGNATURE);
-      if (memcmp (sig.data + sig.len - SIG_POSTFIX_LEN,
-                  SIG_POSTFIX, SIG_POSTFIX_LEN))
+      if (memcmp (sig.data + sig.len - SIG_POSTFIX_LEN, SIG_POSTFIX, SIG_POSTFIX_LEN))
         return gpg_error (GPG_ERR_BAD_SIGNATURE);
-      memcpy (sig_result, sig.data + SIG_PREFIX_LEN, SIG_LEN);
-      *sig_len = SIG_LEN;
-    }
 
+      if ( *sig_begin == 0 && *(sig_begin+1) & 0x80 )
+        {
+          /* Remove the extra nul byte that was added to prevent
+           * the signature from being interpreted as a negative value. */
+          sig_begin += 1;
+          data_len -= 1;
+        }
+
+      memcpy (sig_result, sig_begin, data_len);
+      *sig_len = data_len;
+    }
+  else
+    return gpg_error( GPG_ERR_BAD_SIGNATURE );  /* Unexpected signature prefix. */
 
   return 0;
 }

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: </pipermail/attachments/20141207/36a5d009/attachment.sig>