[gnutls-dev] Buggy RSA/DSA signature verification

[Simon Josefsson]

ludovic.courtes at laas.fr (Ludovic Courtès) writes:

> Hi,
>
> There seems to be a bug in `_gnutls_pkcs1_rsa_verify_sig ()': Basically,
> when verifying a DSA signature, it wrongfully assumes that the SHA1 hash
> is located at `&hash_concat->data[16]'.  In some cases, as visible in
> `_gnutls_verify_sig_params ()', the SHA1 hash is actually located
> `&hash_concat->data[15]' instead, because the PKCS#1 algorithm
> identifier for SHA1 is 15-octet-long, not 16.  In those cases,
> `_gnutls_pkcs1_rsa_verify_sig ()' fails to verify the signature and
> performs an off-by-one memory access.

Ah, right.  An important condition for this to happen is that TLS 1.2
is used, though.

> I don't know what the best way to fix `_gnutls_pkcs1_rsa_verify_sig ()'
> is.  Perhaps it could decode the header of HASH_CONCAT in order to
> determine the exact location of the hash value.  Alternatively, since
> the function is only used internally, we could change callers so that
> they provide it directly with the hash value in the `GNUTLS_PK_DSA'
> case.
>
> I'd be glad to help fix this based on your comments if you don't have
> time to do it.

I think we should change both the function parameters and the name of
the function -- it is quite confusing for the function to be called
_gnutls_pkcs1_rsa_verify_sig when it is actually responsible for
verifying both RSA and DSA signatures.  I have installed the patch
below.

When we support more than SHA-1, this will have to be revisited again,
but at least this will solve the immediate problem.

Thanks,
Simon

--- gnutls_sig.c	26 Nov 2006 12:10:10 +0100	2.54
+++ gnutls_sig.c	27 Dec 2006 21:28:44 +0100	
@@ -259,9 +259,10 @@
 
 
 static int
-_gnutls_pkcs1_rsa_verify_sig (gnutls_cert * cert,
+_gnutls_verify_sig (gnutls_cert * cert,
 			      const gnutls_datum_t * hash_concat,
-			      gnutls_datum_t * signature)
+		    gnutls_datum_t * signature,
+		    size_t sha1pos)
 {
   int ret;
   gnutls_datum_t vdata;
@@ -302,7 +303,7 @@
       break;
     case GNUTLS_PK_DSA:
 
-      vdata.data = &hash_concat->data[16];
+      vdata.data = &hash_concat->data[sha1pos];
       vdata.size = 20;		/* sha1 */
 
       /* verify signature */
@@ -380,7 +381,7 @@
   dconcat.data = concat;
   dconcat.size = 20 + 16;	/* md5+ sha */
 
-  ret = _gnutls_pkcs1_rsa_verify_sig (cert, &dconcat, signature);
+  ret = _gnutls_verify_sig (cert, &dconcat, signature, 16);
   if (ret < 0)
     {
       gnutls_assert ();
@@ -461,7 +462,7 @@
 
   dconcat.data = concat;
 
-  ret = _gnutls_pkcs1_rsa_verify_sig (cert, &dconcat, signature);
+  ret = _gnutls_verify_sig (cert, &dconcat, signature, dconcat.size - 20);
   if (ret < 0)
     {
       gnutls_assert ();