Skip to content

Commit

Permalink
Fix for integer/buffer overflow CVE-2021-32765
Browse files Browse the repository at this point in the history
This fix prevents hiredis from trying to allocate more than `SIZE_MAX`
bytes, which would result in a buffer overrun.

[Full Details](GHSA-hfm9-39pp-55p2)
  • Loading branch information
yossigo authored and michael-grunder committed Oct 4, 2021
1 parent d5b4c69 commit 76a7b10
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 0 deletions.
1 change: 1 addition & 0 deletions hiredis.c
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ 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);
Expand Down
14 changes: 14 additions & 0 deletions test.c
Original file line number Diff line number Diff line change
Expand Up @@ -493,6 +493,20 @@ static void test_reply_reader(void) {
freeReplyObject(reply);
redisReaderFree(reader);

test("Multi-bulk never overflows regardless of maxelements: ");
size_t bad_mbulk_len = (SIZE_MAX / sizeof(void *)) + 3;
char bad_mbulk_reply[100];
snprintf(bad_mbulk_reply, sizeof(bad_mbulk_reply), "*%llu\r\n+asdf\r\n",
(unsigned long long) bad_mbulk_len);

reader = redisReaderCreate();
reader->maxelements = 0; /* Don't rely on default limit */
redisReaderFeed(reader, bad_mbulk_reply, strlen(bad_mbulk_reply));
ret = redisReaderGetReply(reader,&reply);
test_cond(ret == REDIS_ERR && strcasecmp(reader->errstr, "Out of memory") == 0);
freeReplyObject(reply);
redisReaderFree(reader);

#if LLONG_MAX > SIZE_MAX
test("Set error when array > SIZE_MAX: ");
reader = redisReaderCreate();
Expand Down

2 comments on commit 76a7b10

@setharnold
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, this patch surprises me a little:

  • Normally applications assume calloc() is reasonable and performs the overflow check itself. I'd argue any implementation of calloc() that neglects this check deserves a CVE so that it can be fixed, once, for all applications using that implementation, and applications can skip doing these checks themselves.
  • If Redis must work on platforms with a broken calloc() that cannot be fixed, why not put this check into the hi_calloc() wrapper function? This would save the Redis developers from needing to annotate every call site with this check.

Thanks

@michael-grunder
Copy link
Collaborator

@michael-grunder michael-grunder commented on 76a7b10 Oct 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

  • We agree that any sane calloc will perform this check itself, but we received a formal report from MSVR who recommended the hardening.

  • Good point, I think the wrapper function is a better place for the logic. I'll make a point to do that before the next release.

Edit: cc @setharnold Branch with refactored logic for reference

Please sign in to comment.