am i the only one that thinks this is a total non-issue?
if you're going to overflow a size_t, you're already in trouble. realloc is used for.. um, reallocating data. it's probably grown many times before you come close to exhausting 32 bits of size (assuming i386). if you are coming close to 4gb allocations, you're already likely running out memory. as such, you're program is probably going to be killed for using too much memory before you are in any risk of overflowing size_t.
similarly, in a 64bit runtime.. well, you will certainly be killed by the kernel before you come close to overflowing size_t.
if you're reallocing based on untrusted user input.. well, again, you're already in trouble. "fixing" realloc will only make problems occur earlier (which i suppose is good).
EDIT: what i really should have said, is "make problems occur somewhere else".
finally, returning NULL for failure.. well, malloc doesn't fail. most (sane) codes don't check the return value of malloc because it doesn't make sense to. the kernel doesn't allocate memory until first access, at which point you're going to have trouble regardless.
colour me sceptical, but this seems like a fix to a non-existent problem.
1) malloc absolutely returns NULL on Linux, even with overcommit. Using setrlimit (which properly sandboxed daemons should be doing), it's trivial to get malloc to return NULL.
2) The issue isn't whether malloc returns NULL. Technically speaking, unsigned arithmetic doesn't even "overflow", which in C is a term meaning the result is undefined. It returns the value modulo 2^N. In other words, in the vast majority of situations malloc will succeed, but return a buffer smaller than you requested.
3) According to your logic, people shouldn't check object boundaries, either. Many buffer overflow exploits utilize arithmetic overflow, because usually the memory you're interested is nowhere near the object you're overflowing. And we know these overflows happen and can be triggered even when the application isn't using very much memory.
People with your opinions shouldn't be allowed anywhere near C code.
>"fixing" realloc will only make problems occur earlier (which i suppose is good).
Yes, you should certainly suppose it's good, because it means the difference between an early out-of-memory error and a serious security vulnerability that can result in arbitrary code execution, like a buffer overflow.
That is the point of wrappers like these: to improve security.
>if you're reallocing based on untrusted user input.. well, again, you're already in trouble.
You are in trouble, but the trouble will be considerably less severe if the untrusted input hits this function instead of hitting malloc or realloc.
> Yes, you should certainly suppose it's good, because it means the difference between an early out-of-memory error and a serious security vulnerability that can result in arbitrary code execution, like a buffer overflow.
my point is that this scenario is so improbable that it's not worth worrying about.
> You are in trouble, but the trouble will be considerably less severe if the untrusted input hits this function instead of hitting malloc or realloc
heartbleed, SQL injection, and hundreds of other vulnerabilities are more likely to occur from untrusted user input. if you overflow a realloc, you don't know where the destination address is. you're far away from the stack, too. there is very little (if any) risk. i have never come across a realloc based exploit.
there is no need to apologise. if i am wrong, i am wrong.
i must say, this is the first (or second..) time i've heard of such an attack. and, whilst i'm not saying you're incorrect, i will say this - if you take user input and act upon it without validation, you're going to have a bad time. realloc does exactly what it says it will do.
is this a fault of realloc, or the programmer that wrote that code? my opinion is that it's a fault of the programmer. and i can already see that others disagree with that.
Yes, it is programmer error, but the key takeaway is that it's pretty damn easy to make a subtle yet critical error when writing in C or C++. Even if you're a developer at Google and have been writing C/C++ for many years.
Tools and wrappers like these try to mitigate the risk of these errors.
You appear to be vehemently denying the existence of a class of bugs that has plagued programs for longer than you've been programming. Perhaps reconsider how much experience you actually have.
even though, prior to this thread, i'd never heard of 2 bugs exploiting this issue, i'm not denying their existence at all. only their likelihood of occurring.
10 years since i started programming, 7 of which i consider good enough to be "experience", is the answer to your statement. less than many here, but i'm certainly not a novice.
"Likelihood" is the wrong way to think about security vulnerabilities. Once an exploit such as this is known to be possible, attackers can contrive scenarios to exploit it.
Whether or not mmap and malloc nicely return null, or whether you get a segfault on first access, depends on what OS you're using and how it is configured. You can configure Linux not to ovecommit memory. I don't think it's good practice to write user space code on the assumption that "modern OS's overcommit anyway, so we will never see null bubble out of malloc". There are conditions where that may be justified, but one example where that doesn't fly is writing portable, reusable components.
Defence in depth. This single overflow check could very well be the difference between denial-of-service-via-OOM and heap overflow.
Should we then abandon ASLR or PIE or other defensive measures as well, just because some code is "already in trouble" due to a bad strcpy or something similar?
> This single overflow check could very well be the difference between denial-of-service-via-OOM and heap overflow
OOM in general cannot be used for remote code execution. heap overflow can.
> Should we then abandon ASLR or PIE or other defensive measures as well, just because some code is "already in trouble" due to a bad strcpy or something similar
certainly not, ASLR is so general that it doesn't make sense to not do it. more importantly, they're entirely transparent to the programmer, and can be used on existing codes.
> OOM in general cannot be used for remote code execution. heap overflow can.
That was exactly my point. Using reallocarray() instead of malloc(x * sizeof(foo)) can prevent a heap overflow bug. For a concrete example (assuming 32-bit size_t):
size_t user_input = ...;
size_t* ptr = malloc(user_input * sizeof(size_t));
if (!ptr)
return "FAIL: Out of memory";
for (size_t i = 0; i < user_input; i++)
ptr[i] = whatever();
Now, a user_input of 2^30 + 1 overflows the multiplication so only 4 bytes is allocated, and then the for loop causes a heap overflow. Boom.
But if the code was this:
size_t user_input = ...;
size_t* ptr = reallocarray(NULL, user_input, sizeof(size_t));
if (!ptr)
return "FAIL: Out of memory";
for (size_t i = 0; i < user_input; i++)
ptr[i] = whatever();
Now, a user_input of 2^30 + 1 would cause reallocarray() to return NULL, and the function returns. Heap overflow avoided by using reallocarray().
Of course, that code is still somewhat bad, since a user_input of 2^30 - 1 will cause the program to use large amounts for memory.
as you correctly state, user input of 2^30 is.. well, it's unlikely. more likely, is a user being able cause a program to do billions of reallocs. in which case, OOM is going to happen before overflow and exploitation.
The Linux kernel doesn't allocate memory until first access. Most other Unixes actually allocate when you ask them to allocate. So on BSDs, etc, malloc can and will fail when you ask the computer to allocate more memory than it can actually access. Overcommitting RAM is a rather silly "optimization" and tends to cause more trouble than it actually solves.
darwin, NT, and linux all do this. that is the majority of systems. it's not a silly optimisation at all. at worst, it performs exactly as well as malloc. at best, it saves allocating memory. which in the mobile platform (again, most of computing), is a big deal.
Imagine that this allocation is for a buffer that is supposed to take user input. If the size is too small, you could easily be exposed to the classic buffer-overflow style of attacks. (That is, users know it's possible, craft a scenario to induce it, then deliver payloads.) So, yes, it's an issue.
well the solution is to validate user input. user input should certainly be limited to 4gb, at least. a wrapper around an allocator is definitely not the solution.
If you're going to validate user input, you first need to get it into kernel space, which requires allocating memory for it.
User input is not limited to 4 GB. Users can provide whatever they want; that's the point. You have to code for malicious users who are pushing your system towards its edge cases, trying to find holes to break through.
> If you're going to validate user input, you first need to get it into kernel space, which requires allocating memory for it.
i don't buy this - if you are going to allocate space, you must know the size of the space you are allocating. which you can validate, and certainly does not require going into kernel space to do.
Oh come on, what do you think the purpose of reallocarray is? It's exactly to include that boilerplate for you so that you don't have write your own "validation" before every alloc.
Letting the user (try) use as much as he needs can be perfectly okay. Even if it weren't, it doesn't necessarily mean the program itself needs to impose some limit to check the inputs against. There are other means, e.g. rlimits (which are enabled by default on OpenBSD).
What reallocarray does is the bare minimum validation that shoud always be done. And in many cases that is enough, though not always.
> am i the only one that thinks this is a total non-issue?
OpenBSD's slogan boasts that there have only ever been "two remote holes in the default install", one of those holes would have been prevented by this.
whilst i appreciate the prudence of this measure, it doesn't do anything for existing programs. this will not fix any codes in OpenBSD that currently ship with it.
either you mean intercepting malloc/realloc calls, in which case can't happen as the function signature is different, or you mean literally modifying the source code of existing programs, and releasing that.
which, lets face it, in the BSD is space (and particularly with regard to memory allocations from user input), is highly unlikely to change any time soon.
> or you mean literally modifying the source code of existing programs, and releasing that.
>
> which, lets face it, in the BSD is space (and particularly with regard to memory allocations from user input), is highly unlikely to change any time soon.
I'm not sure how to parse your sentence. Are you saying OpenBSD (which should be known for its mass audits) isn't likely to make changes in its source code any soon?
if you're going to overflow a size_t, you're already in trouble. realloc is used for.. um, reallocating data. it's probably grown many times before you come close to exhausting 32 bits of size (assuming i386). if you are coming close to 4gb allocations, you're already likely running out memory. as such, you're program is probably going to be killed for using too much memory before you are in any risk of overflowing size_t.
similarly, in a 64bit runtime.. well, you will certainly be killed by the kernel before you come close to overflowing size_t.
if you're reallocing based on untrusted user input.. well, again, you're already in trouble. "fixing" realloc will only make problems occur earlier (which i suppose is good).
EDIT: what i really should have said, is "make problems occur somewhere else".
finally, returning NULL for failure.. well, malloc doesn't fail. most (sane) codes don't check the return value of malloc because it doesn't make sense to. the kernel doesn't allocate memory until first access, at which point you're going to have trouble regardless.
colour me sceptical, but this seems like a fix to a non-existent problem.