[tor-commits] [tor/master] Fix unreachable heap corruption in base64_decode()

[nickm at torproject.org]

commit a23fd1578612051a3ac804c12a629f6a5cfa296e
Author: Hans Jerry Illikainen <hji at dyntopia.com>
Date:   Sun Dec 11 20:17:49 2016 +0000

    Fix unreachable heap corruption in base64_decode()
    
    Give size_mul_check() external linkage and use it in base64_decode() to
    avoid a potential integer wrap.
    
    Closes #19222
---
 src/common/util.c           | 11 +----------
 src/common/util.h           |  4 +---
 src/common/util_format.c    |  2 +-
 src/test/test_util.c        | 30 +++++++++++++++---------------
 src/test/test_util_format.c |  3 +++
 5 files changed, 21 insertions(+), 29 deletions(-)

diff --git a/src/common/util.c b/src/common/util.c
index 9d134c1..d02eb66 100644
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -188,7 +188,7 @@ tor_malloc_zero_(size_t size DMALLOC_PARAMS)
 #define SQRT_SIZE_MAX_P1 (((size_t)1) << (sizeof(size_t)*4))
 
 /** Return non-zero if and only if the product of the arguments is exact. */
-static inline int
+inline int
 size_mul_check(const size_t x, const size_t y)
 {
   /* This first check is equivalent to
@@ -202,15 +202,6 @@ size_mul_check(const size_t x, const size_t y)
           x <= SIZE_MAX / y);
 }
 
-#ifdef TOR_UNIT_TESTS
-/** Exposed for unit tests only */
-int
-size_mul_check__(const size_t x, const size_t y)
-{
-  return size_mul_check(x,y);
-}
-#endif
-
 /** Allocate a chunk of <b>nmemb</b>*<b>size</b> bytes of memory, fill
  * the memory with zero bytes, and return a pointer to the result.
  * Log and terminate the process on error.  (Same as
diff --git a/src/common/util.h b/src/common/util.h
index 2b3e48e..ff56615 100644
--- a/src/common/util.h
+++ b/src/common/util.h
@@ -553,9 +553,7 @@ STATIC int format_helper_exit_status(unsigned char child_state,
 
 #endif
 
-#ifdef TOR_UNIT_TESTS
-int size_mul_check__(const size_t x, const size_t y);
-#endif
+int size_mul_check(const size_t x, const size_t y);
 
 #define ARRAY_LENGTH(x) ((sizeof(x)) / sizeof(x[0]))
 
diff --git a/src/common/util_format.c b/src/common/util_format.c
index aef9db8..6e0a045 100644
--- a/src/common/util_format.c
+++ b/src/common/util_format.c
@@ -398,7 +398,7 @@ base64_decode(char *dest, size_t destlen, const char *src, size_t srclen)
    * Number of bytes required to hold all bits == (srclen*6)/8.
    * Yes, we want to round down: anything that hangs over the end of a
    * byte is padding. */
-  if (destlen < (srclen*3)/4)
+  if (!size_mul_check(srclen, 3) || destlen < (srclen*3)/4)
     return -1;
   if (destlen > SIZE_T_CEILING)
     return -1;
diff --git a/src/test/test_util.c b/src/test/test_util.c
index 86e3fea..fafb84f 100644
--- a/src/test/test_util.c
+++ b/src/test/test_util.c
@@ -5479,26 +5479,26 @@ test_util_calloc_check(void *arg)
 {
   (void) arg;
   /* Easy cases that are good. */
-  tt_assert(size_mul_check__(0,0));
-  tt_assert(size_mul_check__(0,100));
-  tt_assert(size_mul_check__(100,0));
-  tt_assert(size_mul_check__(100,100));
+  tt_assert(size_mul_check(0,0));
+  tt_assert(size_mul_check(0,100));
+  tt_assert(size_mul_check(100,0));
+  tt_assert(size_mul_check(100,100));
 
   /* Harder cases that are still good. */
-  tt_assert(size_mul_check__(SIZE_MAX, 1));
-  tt_assert(size_mul_check__(1, SIZE_MAX));
-  tt_assert(size_mul_check__(SIZE_MAX / 10, 9));
-  tt_assert(size_mul_check__(11, SIZE_MAX / 12));
+  tt_assert(size_mul_check(SIZE_MAX, 1));
+  tt_assert(size_mul_check(1, SIZE_MAX));
+  tt_assert(size_mul_check(SIZE_MAX / 10, 9));
+  tt_assert(size_mul_check(11, SIZE_MAX / 12));
   const size_t sqrt_size_max_p1 = ((size_t)1) << (sizeof(size_t) * 4);
-  tt_assert(size_mul_check__(sqrt_size_max_p1, sqrt_size_max_p1 - 1));
+  tt_assert(size_mul_check(sqrt_size_max_p1, sqrt_size_max_p1 - 1));
 
   /* Cases that overflow */
-  tt_assert(! size_mul_check__(SIZE_MAX, 2));
-  tt_assert(! size_mul_check__(2, SIZE_MAX));
-  tt_assert(! size_mul_check__(SIZE_MAX / 10, 11));
-  tt_assert(! size_mul_check__(11, SIZE_MAX / 10));
-  tt_assert(! size_mul_check__(SIZE_MAX / 8, 9));
-  tt_assert(! size_mul_check__(sqrt_size_max_p1, sqrt_size_max_p1));
+  tt_assert(! size_mul_check(SIZE_MAX, 2));
+  tt_assert(! size_mul_check(2, SIZE_MAX));
+  tt_assert(! size_mul_check(SIZE_MAX / 10, 11));
+  tt_assert(! size_mul_check(11, SIZE_MAX / 10));
+  tt_assert(! size_mul_check(SIZE_MAX / 8, 9));
+  tt_assert(! size_mul_check(sqrt_size_max_p1, sqrt_size_max_p1));
 
  done:
   ;
diff --git a/src/test/test_util_format.c b/src/test/test_util_format.c
index 1d58ba2..21a6923 100644
--- a/src/test/test_util_format.c
+++ b/src/test/test_util_format.c
@@ -202,6 +202,9 @@ test_util_format_base64_decode(void *ignored)
   res = base64_decode(dst, SIZE_T_CEILING+1, src, 10);
   tt_int_op(res, OP_EQ, -1);
 
+  res = base64_decode(dst, 1, real_src, SIZE_MAX/3+1);
+  tt_int_op(res, OP_EQ, -1);
+
   const char *s = "T3BhIG11bmRv";
   res = base64_decode(dst, 9, s, strlen(s));
   tt_int_op(res, OP_EQ, 9);