Fix bug 977.
authorWerner Koch <wk@gnupg.org>
Tue, 2 Dec 2008 12:39:01 +0000 (12:39 +0000)
committerWerner Koch <wk@gnupg.org>
Tue, 2 Dec 2008 12:39:01 +0000 (12:39 +0000)
mpi/ChangeLog
mpi/mpi-pow.c
tests/ChangeLog
tests/mpitests.c

index 9ac3ab9..f584955 100644 (file)
@@ -1,6 +1,8 @@
 2008-12-02  Werner Koch  <wk@g10code.com>
 
        * mpi-pow.c (gcry_mpi_powm): Re-indent.
+       (gcry_mpi_powm): Simplified allocation of the result to fix a
+       double free bug.  This is bug#977.  Reported by Haakon Ringberg.
 
 2008-08-20  Werner Koch  <wk@g10code.com>
 
index dc75df5..2486554 100644 (file)
@@ -40,11 +40,15 @@ void
 gcry_mpi_powm (gcry_mpi_t res, 
                gcry_mpi_t base, gcry_mpi_t expo, gcry_mpi_t mod)
 {
+  /* Pointer to the limbs of the arguments, their size and signs. */
   mpi_ptr_t  rp, ep, mp, bp;
   mpi_size_t esize, msize, bsize, rsize;
   int               msign, bsign, rsign;
-  int        esec,  msec,  bsec,  rsec;
+  /* Flags telling the secure allocation status of the arguments.  */
+  int        esec,  msec,  bsec,  rsec;  
+  /* Size of the result including space for temporary values.  */
   mpi_size_t size;
+  /* Helper.  */
   int mod_shift_cnt;
   int negative_result;
   mpi_ptr_t mp_marker = NULL;
@@ -55,7 +59,6 @@ gcry_mpi_powm (gcry_mpi_t res,
   unsigned int bp_nlimbs = 0;
   unsigned int ep_nlimbs = 0;
   unsigned int xp_nlimbs = 0;
-  int assign_rp = 0;
   mpi_ptr_t tspace = NULL;
   mpi_size_t tsize = 0; 
 
@@ -127,54 +130,43 @@ gcry_mpi_powm (gcry_mpi_t res,
       goto leave;
     }
 
-  if (res->alloced < size)
+
+  /* Make BASE, EXPO and MOD not overlap with RES.  */
+  if ( rp == bp )
     {
-      /* We have to allocate more space for RES.  If any of the input
-         parameters are identical to RES, defer deallocation of the
-         old space.  */
-       if ( rp == ep || rp == mp || rp == bp ) 
-          {
-           rp = mpi_alloc_limb_space( size, rsec );
-           assign_rp = 1;
-          }
-       else
-          {
-           mpi_resize( res, size );
-           rp = res->d;
-          }
+      /* RES and BASE are identical.  Allocate temp. space for BASE.  */
+      gcry_assert (!bp_marker);
+      bp_nlimbs = bsec? bsize:0;
+      bp = bp_marker = mpi_alloc_limb_space( bsize, bsec );
+      MPN_COPY(bp, rp, bsize);
     }
-  else
-    { 
-      /* Make BASE, EXPO and MOD not overlap with RES.  */
-      if ( rp == bp )
-        {
-          /* RES and BASE are identical.  Allocate temp. space for BASE.  */
-          gcry_assert (!bp_marker);
-          bp_nlimbs = bsec? bsize:0;
-          bp = bp_marker = mpi_alloc_limb_space( bsize, bsec );
-          MPN_COPY(bp, rp, bsize);
-       }
-      if ( rp == ep )
-        {
-          /* RES and EXPO are identical.  Allocate temp. space for EXPO.  */
-          ep_nlimbs = esec? esize:0;
-          ep = ep_marker = mpi_alloc_limb_space( esize, esec );
-          MPN_COPY(ep, rp, esize);
-       }
-      if ( rp == mp ) 
-        {
-          /* RES and MOD are identical.  Allocate temporary space for MOD.*/
-          gcry_assert (!mp_marker);
-          mp_nlimbs = msec?msize:0;
-          mp = mp_marker = mpi_alloc_limb_space( msize, msec );
-          MPN_COPY(mp, rp, msize);
-       }
+  if ( rp == ep )
+    {
+      /* RES and EXPO are identical.  Allocate temp. space for EXPO.  */
+      ep_nlimbs = esec? esize:0;
+      ep = ep_marker = mpi_alloc_limb_space( esize, esec );
+      MPN_COPY(ep, rp, esize);
+    }
+  if ( rp == mp ) 
+    {
+      /* RES and MOD are identical.  Allocate temporary space for MOD.*/
+      gcry_assert (!mp_marker);
+      mp_nlimbs = msec?msize:0;
+      mp = mp_marker = mpi_alloc_limb_space( msize, msec );
+      MPN_COPY(mp, rp, msize);
+    }
+
+  /* Copy base to the result.  */
+  if (res->alloced < size)
+    {
+      mpi_resize (res, size);
+      rp = res->d;
     }
-  
   MPN_COPY ( rp, bp, bsize );
   rsize = bsize;
   rsign = bsign;
   
+  /* Main processing.  */
   {
     mpi_size_t i;
     mpi_ptr_t xp;
@@ -285,12 +277,8 @@ gcry_mpi_powm (gcry_mpi_t res,
             rsize++;
           }
       }
-    else
-      {
-        MPN_COPY( res->d, rp, rsize);
-        rp = res->d;
-      }
 
+    gcry_assert (res->d == rp);
     if ( rsize >= msize ) 
       {
         _gcry_mpih_divrem(rp + msize, 0, rp, rsize, mp, msize);
@@ -305,6 +293,7 @@ gcry_mpi_powm (gcry_mpi_t res,
     _gcry_mpih_release_karatsuba_ctx (&karactx );
   }
 
+  /* Fixup for negative results.  */
   if ( negative_result && rsize )
     {
       if ( mod_shift_cnt )
@@ -314,12 +303,11 @@ gcry_mpi_powm (gcry_mpi_t res,
       rsign = msign;
       MPN_NORMALIZE(rp, rsize);
     }
+  gcry_assert (res->d == rp);
   res->nlimbs = rsize;
   res->sign = rsign;
   
  leave:
-  if (assign_rp)
-    _gcry_mpi_assign_limb_space( res, rp, size );
   if (mp_marker)
     _gcry_mpi_free_limb_space( mp_marker, mp_nlimbs );
   if (bp_marker)
index 703a816..b6adb76 100644 (file)
@@ -1,3 +1,7 @@
+2008-12-02  Werner Koch  <wk@g10code.com>
+
+       * mpitests.c (mpi_powm): New.
+
 2008-11-28  Werner Koch  <wk@g10code.com>
 
        * fips186-dsa.c: New.
index d508da7..ac93697 100644 (file)
@@ -34,6 +34,19 @@ static int verbose;
 static int debug;
 
 
+static void
+die (const char *format, ...)
+{
+  va_list arg_ptr;
+
+  va_start (arg_ptr, format);
+  vfprintf (stderr, format, arg_ptr);
+  va_end (arg_ptr);
+  exit (1);
+}
+
+
+
 /* Set up some test patterns */
 
 /* 48 bytes with value 1: this results in 8 limbs for 64bit limbs, 16limb for 32 bit limbs */
@@ -157,6 +170,110 @@ test_mul (void)
 }
 
 
+/* What we test here is that we don't overwrite our args and that
+   using thne same mpi for several args works.  */
+static int
+test_powm (void)
+{
+  int b_int = 17;
+  int e_int = 3;
+  int m_int = 19;  
+  gcry_mpi_t base = gcry_mpi_set_ui (NULL, b_int);
+  gcry_mpi_t exp = gcry_mpi_set_ui (NULL, e_int);
+  gcry_mpi_t mod = gcry_mpi_set_ui (NULL, m_int);
+  gcry_mpi_t res = gcry_mpi_new (0);
+
+  gcry_mpi_powm (res, base, exp, mod);
+  if (gcry_mpi_cmp_ui (base, b_int))
+    die ("test_powm failed for base at %d\n", __LINE__);
+  if (gcry_mpi_cmp_ui (exp, e_int))
+    die ("test_powm_ui failed for exp at %d\n", __LINE__);
+  if (gcry_mpi_cmp_ui (mod, m_int))
+    die ("test_powm failed for mod at %d\n", __LINE__);
+
+  /* Check using base for the result.  */
+  gcry_mpi_set_ui (base, b_int);
+  gcry_mpi_set_ui (exp, e_int);
+  gcry_mpi_set_ui(mod, m_int);
+  gcry_mpi_powm (base, base, exp, mod);
+  if (gcry_mpi_cmp (res, base))
+    die ("test_powm failed at %d\n", __LINE__);
+  if (gcry_mpi_cmp_ui (exp, e_int))
+    die ("test_powm_ui failed for exp at %d\n", __LINE__);
+  if (gcry_mpi_cmp_ui (mod, m_int))
+    die ("test_powm failed for mod at %d\n", __LINE__);
+
+  /* Check using exp for the result.  */
+  gcry_mpi_set_ui (base, b_int);
+  gcry_mpi_set_ui (exp, e_int);
+  gcry_mpi_set_ui(mod, m_int);
+  gcry_mpi_powm (exp, base, exp, mod);
+  if (gcry_mpi_cmp (res, exp))
+    die ("test_powm failed at %d\n", __LINE__);
+  if (gcry_mpi_cmp_ui (base, b_int))
+    die ("test_powm failed for base at %d\n", __LINE__);
+  if (gcry_mpi_cmp_ui (mod, m_int))
+    die ("test_powm failed for mod at %d\n", __LINE__);
+
+  /* Check using mod for the result.  */
+  gcry_mpi_set_ui (base, b_int);
+  gcry_mpi_set_ui (exp, e_int);
+  gcry_mpi_set_ui(mod, m_int);
+  gcry_mpi_powm (mod, base, exp, mod);
+  if (gcry_mpi_cmp (res, mod))
+    die ("test_powm failed at %d\n", __LINE__);
+  if (gcry_mpi_cmp_ui (base, b_int))
+    die ("test_powm failed for base at %d\n", __LINE__);
+  if (gcry_mpi_cmp_ui (exp, e_int))
+    die ("test_powm_ui failed for exp at %d\n", __LINE__);
+
+  /* Now check base ^ base mod mod.  */
+  gcry_mpi_set_ui (base, b_int);
+  gcry_mpi_set_ui(mod, m_int);
+  gcry_mpi_powm (res, base, base, mod);
+  if (gcry_mpi_cmp_ui (base, b_int))
+    die ("test_powm failed for base at %d\n", __LINE__);
+  if (gcry_mpi_cmp_ui (mod, m_int))
+    die ("test_powm failed for mod at %d\n", __LINE__);
+
+  /* Check base ^ base mod mod with base as result.  */
+  gcry_mpi_set_ui (base, b_int);
+  gcry_mpi_set_ui(mod, m_int);
+  gcry_mpi_powm (base, base, base, mod);
+  if (gcry_mpi_cmp (res, base))
+    die ("test_powm failed at %d\n", __LINE__);
+  if (gcry_mpi_cmp_ui (mod, m_int))
+    die ("test_powm failed for mod at %d\n", __LINE__);
+
+  /* Check base ^ base mod mod with mod as result.  */
+  gcry_mpi_set_ui (base, b_int);
+  gcry_mpi_set_ui(mod, m_int);
+  gcry_mpi_powm (mod, base, base, mod);
+  if (gcry_mpi_cmp (res, mod))
+    die ("test_powm failed at %d\n", __LINE__);
+  if (gcry_mpi_cmp_ui (base, b_int))
+    die ("test_powm failed for base at %d\n", __LINE__);
+
+  /* Now check base ^ base mod base.  */
+  gcry_mpi_set_ui (base, b_int);
+  gcry_mpi_powm (res, base, base, base);
+  if (gcry_mpi_cmp_ui (base, b_int))
+    die ("test_powm failed for base at %d\n", __LINE__);
+
+  /* Check base ^ base mod base with base as result.  */
+  gcry_mpi_set_ui (base, b_int);
+  gcry_mpi_powm (base, base, base, base);
+  if (gcry_mpi_cmp (res, base))
+    die ("test_powm failed at %d\n", __LINE__);
+
+  /* Fixme: We should add the rest of the cases of course.  */
+
+
+
+  return 1;
+}
+
+
 int 
 main (int argc, char* argv[])
 {
@@ -175,6 +292,7 @@ main (int argc, char* argv[])
   test_add ();
   test_sub ();
   test_mul ();
+  test_powm ();
 
   return 0;
 }