CVE-2020-7105: Abort if malloc() was unsuccessful

[lamby]

No description provided.

[lamby]

This is to address #747

[mnunberg]

Is there any advantage of crashing via abort vs crashing via segfault?

[lamby]

The segfault might be exploitable, and the abort is more debuggable outside of a security context.

[shouc]

I think a better way to fix this is to abstract out the handling function to header files. For example:
#define OOM_HANDLER abort() and use OOM_HANDLER to substitute abort(), which makes upstream developers easier to define their own try catch logic.

[lamby]

Ah, good idea. I was not aware of that particular idiom, thanks.

[lamby]

Something like this? :)

[shouc]

lgtm but I am not the owner

[mnunberg]

Perhaps just wrap the entire allocation in a macro/function?

[lamby]

Perhaps just wrap the entire allocation in a macro/function?

This could work. But:

• Is there a Don't Repeat Yourself version of doing this that can support malloc and calloc (used at least 9 times)
• It would be a fairly invasive code change (fine for master) but perhaps rather unwieldy to backport.

[mnunberg]

1. just wrap calloc, malloc, realloc, strdup, etc etc. I mean, this is what redis server does itself.
2. I personally don't think that a segfault on NULL is all that big a deal, but I think we should simply start first at those areas which have been identified. As far as the invasiveness, a one-line change in a bunch of places is going to be far less invasive than a multi-line change in those same areas.

[michael-grunder]

just wrap calloc, malloc, realloc, strdup, etc etc. I mean, this is what redis server does itself

That's what I would suggest as well. Given that we often do handle a NULL reply (e.g. returning REDIS_ERROR_OOM) we should probably name them something else.

What about hr_safe_malloc, hr_safe_realloc, hr_safe_calloc, and hr_safe_strdup?

If we're all good with that direction I'm happy to formalize (and actually test) the changes.

edit for correct links:

link to my POC branch
link to the diff

[lamby]

I quickly sketched out a POC over on my fork.

FYI this links to the "New PR" page and not the PR itself. Did you forget to push it? I don't see the branch in your fork... :)

[michael-grunder]

Apparently I can't link things properly 😄

link to my POC branch
link to the diff

[lamby]

Thanks; nice work. I suppose the other approach might be to emulate what Redis' src/zmalloc.c does by #define realloc(ptr,size) foo_realloc(ptr,size) etc.?

[michael-grunder]

I like that strategy as well, but we probably need two versions because there are a bunch of places where OOM is handled in a non-fatal way.

@mnunberg let us know if this strategy seems reasonable.

[michael-grunder]

As an alternative what do people think about getting #638 merged first?

That has some good stuff in it in addition to the malloc NULL checks. In particular the the redisAsync*Append functions.

[shouc]

lgtm, the only conflict is in async.c, where #638 uses following approach:

if (NULL != (dup = malloc(sizeof(*dup))))
        memcpy(dup, src, sizeof(*dup));

[lamby]

what do people think about getting #638 merged first?

I'd quite very like this one (or something similar) merged first as I am trying to ensure I can cherry-pick a minimal patch for older and already-shipped versions of hiredis, hence trying to reduce the number of changes. It would be difficult for me to ship out #638's wide-reaching changes in a safe and confident manner (and might even be difficult to backport in a pure code sesne as it changes quite a lot).

[mnunberg]

Re #638 I'm not a fan of mindlessly checking for NULL so much and so explicitly that it makes the actual code hard to read. I think that PR has too much boilerplate, though I do appreciate what he's trying to do over there.

@michael-grunder I'm in favor of a prefixed malloc, maybe hiredis_malloc or whatever... or just hi_malloc

[michael-grunder]

@mnunberg Seems reasonable to me. I'll package it up and open a PR for review.

Edit: @lamby I totally understand not wanting such a huge change just for a patch.

[michael-grunder]

Closing in favor of #754, please redirect all bikeshedding over there.

[stevelipinski]

Will a new release/tag be issued (0.15.0?) since this addresses a CVE?