Indeed. The pathological example where both operands get converted to a different type is like this: Assume that ‹int› is 32 bits and ‹long› is 32 bits. Then ‹unsigned int› + ‹long› becomes ‹unsigned long›. This is because ‹long› has a higher rank than ‹int›, so the result must have the rank of long. But because ‹long› cannot hold all values of ‹unsigned int›, we must use ‹unsigned long›.
The more explicit stdint types (e.g. uint32_t) are both safer and more portable. Modern languages like Rust seem to agree... (u8, i32, etc).
The argument I've always heard was that you want your program to use the sizes that the architecture can handle most efficiently. But actually, I don't. I'd rather my program run correctly, the way I intended, even if that means it runs a little slower on a sparc or powerpc.
The combinatoric explosion of size hierarchies is basically impossible for normal people to reason about or test for.
I'd also add range types ala Ada.
This assumes a scenario where an attacker is in full control of "num".
For example: A lot of protocols have a structure like "<SIZE><PAYLOAD>" where the sending side (the attacker) specifies SIZE and the receiving side allocates a buffer of that size).
The code calling that function assumes it will never create a buffer that is larger then 256 bytes, so perhaps it is doing something like this:
char buf[256];
strcpy (buf, allocatebufs (ATTACKER_SIZE))
With the cast of limit to int, num is never promoted. A negative value can then reach the malloc expression, where it will be implicitly promoted to unsigned anyhow--because malloc takes a size_t argument. Worse, the constraint check had the side-effect of preventing multiplicative overflow in the malloc expression, which means if an attacker can control the value of num, they can cause malloc to return a block of memory smaller than what the caller expected (though this is contingent on arithmetic bugs in caller code elsewhere in the hypothetical program).
The real bug here is that allocatebufs takes an int argument rather than a size_t argument. And the real issue with C's arithmetic typing isn't promotions in normal expressions (promotion to unsigned almost always preserves the intended semantics of size constraints) so much as the ability to pass arguments of the wrong signedness to functions, which has the side-effect of hiding the fact that caller and callee aren't on the same page wrt value semantics and arithmetic limits.
I'm on the fence regarding C's behavior here because almost everybody in the comments instinctively were okay with casting, just that they would've casted some other expression. But the real problem here was the type of the argument to allocatebufs. It should've never been signed because negative values for object sizes are non-sensical. But because everybody's instinct is to cast, what good does stricter arithmetic type checking do for callers? Especially when simply leaving C's existing promotion rules in place, particularly implicit promotion to unsigned, would've resulted in the correct behavior. This eagerness to simply cast arithmetic values to quiet compiler diagnostics is also a problem in other languages with formally stricter type checking.
That was my point. The article claims to show how easy it is to introduce such a bug in the second snippet, but that isn't true. You need to introduce more bugs to get a security vulnerability.
Suppose we're on a 32 bit platform and num is 0xF0000001. When multiplied by 64 (0x40), we'll end up allocating a 64 (0x40) byte buffer. But other code converting num to unsigned may be expecting a buffer large enough for 0xF0000001 64 byte records. After all, that was probably the reason for multiplying by 64.
Passing a large value into the method shown in the article will do nothing nefarious (assuming sizeof(size_t) >= sizeof(int)). It will either return a large allocation, or, more likely, fail because the amount of requested memory is too large.
If you have a narrowing/casting bug somewhere else in your program, which BTW would produce an obvious warning, that would of course cause trouble as you have described.
And while mixing signed and unsigned arithmetic for buffer sizes is, of course, a recipe for desaster, I think it's incorrect to claim that the allocatebufs method shown in TFA has a "backdoor" because of this. I feel that is a bit like saying memcpy has a backdoor because you might get your pointer arithmetic wrong when calling it.
printf("%p\n", malloc(0x7fffffffffffffff));
(nil)
The scenario being discussed in this subthread — having a negative number get inadvertently converted into a `size_t` and passed to `malloc()` is exactly the sort of way you end up getting a NULL back. But this is also not guaranteed.
TFA calls this a "backdoor"; so how do you actually "get in" after you managed to get the backdoor through code review and deployed into production?
For example, suppose the code was parsing user input as follows (a fairly common pattern):
unsigned int count = read_int();
struct buf *bufs = allocatebufs(count);
if(!bufs) goto fail;
for(unsigned int i=0; i<count; i++) {
bufs[i] = read_buf();
if(!bufs[i]) break;
}
That was my point. The article claims to introduce a backdoor with the tiny change in the second example, i.e. "commit the change from the second example an you're in". But that just isn't true without assuming some other vulnerable code at the callsite.
And arguably, the assumed bug is a bug of the assumed callsite and not a bug of the allocatebufs method!
"See how easy it is to introduce a backdoor into C code which just one small change that looks completely harmless" might generally be true (debatable), but claiming that the change shown in the article (on it's own) is an example of this is incorrect and looks a bit like fearmongering.
That code doesn't need to be buggy - simple, correct code that traverses e.g. a linked list can be abused to gain arbitrary memory writes and reads if you can overwrite the pointers in that linked list with values under your control; and these arbitrary memory writes can be abused to gain arbitrary code execution.
Exploiting a heap overflow is not as straightforward as a stack overflow, but certainly possible, there are many real world code execution vulnerabilities that arise from a buffer overflow in heap.
If you ask the method to allocate a negative number of bytes and the method returns a buffer which is greater than zero, that doesn't seem like a backdoor in the allocation method!
Saying you can get a return buffer that is smaller than whatever amount of bytes you requested is wrong I think. Can you give an input to the second method that will result in a buffer smaller than the input value?
That's the whole point of the article. Risky, but not wrong. A subtle way to introduce a backdoor.
I'm confused why the author would just assume their readers would know this without any kind of explanation.
It feels like I learned on a deeper level than if s/he had just explained it.
. . .
But the takeaway here is, I think, not to make us better C programmers. I think the takeaway is that C sucks because it just smiles at esoteric bugs like this. We need to replace it, or forever suffer from crashes and RCEs, et.c.
As a career C programmer, this drives me crazy. I kind of get it from a object storage size perspective, but the side-effects are awful.
But of course, it's too late to fix it. Better just burn it all down and switch to something completely different. Oh hey, Rust!
Fortunately, memory allocation in Rust is easier, and the whole `malloc(elem * size)` chore just doesn't exist.
In C, like most languages, all binary operators on numbers – including arithmetic as well as comparisons – 'natively' require two operands of the same type. When you pass operands of different types, the language usually converts one of the operands to the other's type. (Sometimes it converts both operands to a different type instead.)
In this case, without the cast, the comparison will be performed by converting the `int` to `size_t`. The dangerous case is when `num` is negative. Since `size_t` is unsigned, the conversion will turn negative numbers into very high positive numbers, so `num > limit` will be true, and the function will safely reject the allocation. On the other hand, with `num > (int)limit`, no conversion occurs; the comparison is performed between two signed integers, and `num > limit` will be false if `num` is negative.
It would be better to be more explicit. Perhaps use two comparisons (the compiler will optimize them into one if possible):
Or, better yet, make `allocatebufs` take `size_t` instead of `int` in the first place. Taking `int` is risky even without the comparison bug: if someone calls the function with a `size_t` on a platform where `size_t` is larger than `int` (i.e. 64-bit platforms), the upper bits will be truncated. For instance, if the passed-in value is 0x100000001, it will be truncated to 0x1, causing allocatebufs to return a much smaller buffer than expected.If you're wondering about the exact rules for what types get converted to what, they're very complicated, unnecessarily so. It mostly boils down to "larger type wins; in case of a tie, unsigned wins over signed". But there are exceptions. Here's the spec language:
- http://c0x.coding-guidelines.com/6.3.1.8.html
- http://c0x.coding-guidelines.com/6.3.1.1.html