Commit e489846b authored by michael-grunder's avatar michael-grunder Committed by Michael Grunder
Browse files

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: 76a7b100

Related vuln ID: CVE-2021-32765
[Full Details](https://github.com/redis/hiredis/security/advisories/GHSA-hfm9-39pp-55p2)
parent 51c74082
...@@ -68,6 +68,10 @@ void *hi_malloc(size_t size) { ...@@ -68,6 +68,10 @@ void *hi_malloc(size_t size) {
} }
void *hi_calloc(size_t nmemb, 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); return hiredisAllocFns.callocFn(nmemb, size);
} }
......
...@@ -32,6 +32,7 @@ ...@@ -32,6 +32,7 @@
#define HIREDIS_ALLOC_H #define HIREDIS_ALLOC_H
#include <stddef.h> /* for size_t */ #include <stddef.h> /* for size_t */
#include <stdint.h>
#ifdef __cplusplus #ifdef __cplusplus
extern "C" { extern "C" {
...@@ -59,6 +60,10 @@ static inline void *hi_malloc(size_t size) { ...@@ -59,6 +60,10 @@ static inline void *hi_malloc(size_t size) {
} }
static inline void *hi_calloc(size_t nmemb, 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); return hiredisAllocFns.callocFn(nmemb, size);
} }
......
...@@ -178,7 +178,6 @@ static void *createArrayObject(const redisReadTask *task, size_t elements) { ...@@ -178,7 +178,6 @@ static void *createArrayObject(const redisReadTask *task, size_t elements) {
return NULL; return NULL;
if (elements > 0) { if (elements > 0) {
if (SIZE_MAX / sizeof(redisReply*) < elements) return NULL; /* Don't overflow */
r->element = hi_calloc(elements,sizeof(redisReply*)); r->element = hi_calloc(elements,sizeof(redisReply*));
if (r->element == NULL) { if (r->element == NULL) {
freeReplyObject(r); freeReplyObject(r);
......
...@@ -59,6 +59,8 @@ struct pushCounters { ...@@ -59,6 +59,8 @@ struct pushCounters {
int str; int str;
}; };
static int insecure_calloc_calls;
#ifdef HIREDIS_TEST_SSL #ifdef HIREDIS_TEST_SSL
redisSSLContext *_ssl_ctx = NULL; redisSSLContext *_ssl_ctx = NULL;
#endif #endif
...@@ -765,6 +767,11 @@ static void *hi_calloc_fail(size_t nmemb, size_t size) { ...@@ -765,6 +767,11 @@ static void *hi_calloc_fail(size_t nmemb, size_t size) {
return NULL; 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) { static void *hi_realloc_fail(void *ptr, size_t size) {
(void)ptr; (void)ptr;
(void)size; (void)size;
...@@ -772,6 +779,8 @@ static void *hi_realloc_fail(void *ptr, size_t size) { ...@@ -772,6 +779,8 @@ static void *hi_realloc_fail(void *ptr, size_t size) {
} }
static void test_allocator_injection(void) { static void test_allocator_injection(void) {
void *ptr;
hiredisAllocFuncs ha = { hiredisAllocFuncs ha = {
.mallocFn = hi_malloc_fail, .mallocFn = hi_malloc_fail,
.callocFn = hi_calloc_fail, .callocFn = hi_calloc_fail,
...@@ -791,6 +800,13 @@ static void test_allocator_injection(void) { ...@@ -791,6 +800,13 @@ static void test_allocator_injection(void) {
redisReader *reader = redisReaderCreate(); redisReader *reader = redisReaderCreate();
test_cond(reader == NULL); 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 // Return allocators to default
hiredisResetAllocators(); hiredisResetAllocators();
} }
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment