[gnutls-dev] gnutls_pk.c uninitialised memory

[Martijn Koster]

Hi all,

I was using valgrind on gnutls-cli-debug, and it found a bug.
I tested gnutls-1.0.8, but that code has not changed in HEAD.

  valgrind --tool=memcheck \
    `which gnutls-cli-debug` pics.half.ebay.com

reported:

  Conditional jump or move depends on uninitialised value(s)
    at 0x3C294E16: _gnutls_pkcs1_rsa_encrypt (gnutls_pk.c:122)
    by 0x3C2938B1: _gnutls_gen_rsa_client_kx (auth_rsa.c:313)
    by 0x3C28D76C: _gnutls_send_client_kx_message (gnutls_kx.c:187)
    by 0x3C28846F: _gnutls_handshake_client (gnutls_handshake.c:2035)
    by 0x3C2880A7: gnutls_handshake (gnutls_handshake.c:1897)

That refers to the last line of this code:

   opaque rnd[3];

   /* Read three random bytes that will be
    * used to replace the zeros.
    */
   if ( (ret=_gnutls_get_random( rnd, 3, GNUTLS_STRONG_RANDOM)) < 0) {
                                gnutls_assert();
                                gnutls_afree(edata);
                                return ret;
   }
   /* use non zero values for 
    * the first two.
    */
   if (rnd[0]==0) rnd[0] = 0xaf;
   if (rnd[1]==0) rnd[1] = 0xae;

   if (ps[i] == 0) {
       /* If the first one is zero then set it to rnd[0].
        * If the second one is zero then set it to rnd[1].
        * Otherwise add (mod 256) the two previous ones plus rnd[3], or
use
        * rnd[1] if the value == 0.
        */
       if (i<2) ps[i] = rnd[i];
       else ps[i] = GMAX( rnd[3] + ps[i-1] + ps[i-2], rnd[1]);

rnd is a 3-byte array, and get_random fills it with 3 bytes,
but then in the last line rnd[3] is used, which is outside
the array, and uninitialised.

Current code in CVS has the same thing:

http://cvs.gnupg.org/cgi-bin/viewcvs.cgi/gnutls/lib/gnutls_pk.c?rev=HEAD&cvsroot=GNU+TLS+Library&content-type=text/vnd.viewcvs-markup

I presume that rnd[3] should be rnd[2]? With that change the warning
goes away. But I don't know what this code does, so I don't know if it's
the right fix. I've appended a patch.

Regards,

-- Martijn

--- gnutls_pk.c.orig    2004-05-18 14:58:03.569633016 +0100
+++ gnutls_pk.c 2004-05-18 14:58:27.360016328 +0100
@@ -115,11 +115,11 @@
                        if (ps[i] == 0) {
                                /* If the first one is zero then set it to rnd[0].
                                 * If the second one is zero then set it to rnd[1].
-                                * Otherwise add (mod 256) the two previous ones plus rnd[3], or use
+                                * Otherwise add (mod 256) the two previous ones plus rnd[2], or use
                                 * rnd[1] if the value == 0.
                                 */
                                if (i<2) ps[i] = rnd[i];
-                               else ps[i] = GMAX( rnd[3] + ps[i-1] + ps[i-2], rnd[1]);
+                               else ps[i] = GMAX( rnd[2] + ps[i-1] + ps[i-2], rnd[1]);
                        }
                }
                break;