From e489846b7226958718ae91fa0c4999b420c706e2 Mon Sep 17 00:00:00 2001
From: michael-grunder <michael.grunder@gmail.com>
Date: Thu, 7 Oct 2021 14:47:11 -0700
Subject: Minor refactor of CVE-2021-32765 fix.

Since `hi_calloc` always passes through one of our wrapper functions,
we can perform this overflow in the wrapper, and get protection
everywhere.

Previous commit: 76a7b10005c70babee357a7d0f2becf28ec7ed1e

Related vuln ID: CVE-2021-32765
[Full Details](https://github.com/redis/hiredis/security/advisories/GHSA-hfm9-39pp-55p2)
---
 alloc.c   |  4 ++++
 alloc.h   |  5 +++++
 hiredis.c |  1 -
 test.c    | 16 ++++++++++++++++
 4 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/alloc.c b/alloc.c
index 7fb6b35..0902286 100644
--- a/alloc.c
+++ b/alloc.c
@@ -68,6 +68,10 @@ void *hi_malloc(size_t size) {
 }
 
 void *hi_calloc(size_t nmemb, size_t size) {
+    /* Overflow check as the user can specify any arbitrary allocator */
+    if (SIZE_MAX / size < nmemb)
+        return NULL;
+
     return hiredisAllocFns.callocFn(nmemb, size);
 }
 
diff --git a/alloc.h b/alloc.h
index 34a05f4..771f9fe 100644
--- a/alloc.h
+++ b/alloc.h
@@ -32,6 +32,7 @@
 #define HIREDIS_ALLOC_H
 
 #include <stddef.h> /* for size_t */
+#include <stdint.h>
 
 #ifdef __cplusplus
 extern "C" {
@@ -59,6 +60,10 @@ static inline void *hi_malloc(size_t size) {
 }
 
 static inline void *hi_calloc(size_t nmemb, size_t size) {
+    /* Overflow check as the user can specify any arbitrary allocator */
+    if (SIZE_MAX / size < nmemb)
+        return NULL;
+
     return hiredisAllocFns.callocFn(nmemb, size);
 }
 
diff --git a/hiredis.c b/hiredis.c
index 15de4ad..7e7af82 100644
--- a/hiredis.c
+++ b/hiredis.c
@@ -178,7 +178,6 @@ static void *createArrayObject(const redisReadTask *task, size_t elements) {
         return NULL;
 
     if (elements > 0) {
-        if (SIZE_MAX / sizeof(redisReply*) < elements) return NULL;  /* Don't overflow */
         r->element = hi_calloc(elements,sizeof(redisReply*));
         if (r->element == NULL) {
             freeReplyObject(r);
diff --git a/test.c b/test.c
index 9c91107..91feaed 100644
--- a/test.c
+++ b/test.c
@@ -59,6 +59,8 @@ struct pushCounters {
     int str;
 };
 
+static int insecure_calloc_calls;
+
 #ifdef HIREDIS_TEST_SSL
 redisSSLContext *_ssl_ctx = NULL;
 #endif
@@ -765,6 +767,11 @@ static void *hi_calloc_fail(size_t nmemb, size_t size) {
     return NULL;
 }
 
+static void *hi_calloc_insecure(size_t nmemb, size_t size) {
+    insecure_calloc_calls++;
+    return (void*)0xdeadc0de;
+}
+
 static void *hi_realloc_fail(void *ptr, size_t size) {
     (void)ptr;
     (void)size;
@@ -772,6 +779,8 @@ static void *hi_realloc_fail(void *ptr, size_t size) {
 }
 
 static void test_allocator_injection(void) {
+    void *ptr;
+
     hiredisAllocFuncs ha = {
         .mallocFn = hi_malloc_fail,
         .callocFn = hi_calloc_fail,
@@ -791,6 +800,13 @@ static void test_allocator_injection(void) {
     redisReader *reader = redisReaderCreate();
     test_cond(reader == NULL);
 
+    /* Make sure hiredis itself protects against a non-overflow checking calloc */
+    test("hiredis calloc wrapper protects against overflow: ");
+    ha.callocFn = hi_calloc_insecure;
+    hiredisSetAllocators(&ha);
+    ptr = hi_calloc((SIZE_MAX / sizeof(void*)) + 3, sizeof(void*));
+    test_cond(ptr == NULL && insecure_calloc_calls == 0);
+
     // Return allocators to default
     hiredisResetAllocators();
 }
-- 
cgit v1.2.3