From: Dominique Brezinski <dominique.brezinski@...>
Date: 2006-05-04T10:14:00+09:00
Subject: (security-related) patch to ALLOC macros to prevent integer overflow bugs

While fixing the integer overflow in rb_ary_fill(), it occurred to me
it would be better to fix the *ALLOC* macros in ruby.h. The patch is
not perfect (though make test-all passes), and I will enumerate the
issues I see with it (most likely a subset of the true issues). Here
is the patch against 1.8.4:

--- ruby.h.org  Wed Oct 26 00:05:58 2005
+++ ruby.h      Thu May  4 00:03:36 2006
@@ -459,11 +459,11 @@
 #define OBJ_FROZEN(x) FL_TEST((x), FL_FREEZE)
 #define OBJ_FREEZE(x) FL_SET((x), FL_FREEZE)

-#define ALLOC_N(type,n) (type*)xmalloc(sizeof(type)*(n))
+#define ALLOC_N(type,n) (sizeof(type)*(n)) / sizeof(type) == (n) ?
(type*)xmalloc(sizeof(type)*(n)) : NULL
  #define ALLOC(type) (type*)xmalloc(sizeof(type))
-#define REALLOC_N(var,type,n)
(var)=(type*)xrealloc((char*)(var),sizeof(type)*(n))
+#define REALLOC_N(var,type,n) (sizeof(type)*(n)) / sizeof(type) == (n) ? (var)=
(type*)xrealloc((char*)(var),sizeof(type)*(n)) :
rb_raise(rb_eNoMemError, "integer overflow")

-#define ALLOCA_N(type,n) (type*)alloca(sizeof(type)*(n))
+#define ALLOCA_N(type,n) (sizeof(type)*(n)) / sizeof(type) == (n) ?
(type*)alloca(sizeof(type)*(n)) : NULL

  #define MEMZERO(p,type,n) memset((p), 0, sizeof(type)*(n))
  #define MEMCPY(p1,p2,type,n) memcpy((p1), (p2), sizeof(type)*(n))

1) For REALLOC_N I am able to substitute rb_raise in place of xrealloc
if an integer overflow is detected, since no return value is expected.
I verified the exception gets raised when I cause the over flow in
array.fill. For ALLOC_N and ALLOCA_N, a return value is expected, so
substituting rb_raise is not a clean option. This patch returns NULL
when an overflow is detected. The problem with returning NULL is that
almost all the ruby code expects the functions to return valid memory
and do not check for a NULL value. This will effectively turn an
integer overflow in those functions into a NULL-pointer de-reference,
which is safer, but still not desirable. I would assert that NULL
should be tested for after every *ALLOC* call and handled gracefully,
but others might not like that ;)

2) The actual test for the integer overflow in the multiplication uses
division, which is slower than other options for 32 bit integers but
required for 64 bit ints AFAIK. Many people has argued about the best
way to test for integer overflows, and this may not be the optimal
test for these cases.

3) The MEM* macros also perform sizeof(type)*(n) calculations that are
subject to overflow, but the overflows here will cause truncated
comparisons, moves and copies. Though still a source of some concern
and hard to debug errors, at least the chance of code injection is
generally not present.

Humbly,
Dom