Hacker News Reader

> 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.)

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›.

Yep. Also, if both operands have types smaller than int, they’ll both be converted to int as part of the “integer promotions” step. This is less likely to lead to surprising results since the conversion is value-preserving, but it’s inconsistent with the behavior of int-and-larger types.

I've heard a lot about NULL being considered a multi-billion-dollar mistake, but not a lot about the wishy-washy integer types hierarchy.

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.

Yeah things I would get rid of in c is 'int' and the wishy washy integer type non sense that goes with it. The whole thing was a big pain in the ass until pure 32 bit machines came along and everyone forgot. Now it's a pain in the ass again.

I'd also add range types ala Ada.

Range types were one of my favourite features of the Ada type system. Such a simple idea, but super effective, and really useful when doing static analysis. Being able to push constraints to the interface simplified a lot of things.

Something like that is what I am hoping we get with contracts when they make it into C++. Although the versions so far only add constraints to functions, having them on types as well sounds very useful.

It's great. It allows you to push the responsibility for checking values are in type right out to the earliest point, reducing the need to sprinkle bounds checking code all over the place. If you are using static analysis like SPARK or one of the others, it then lets you do a load more of it automatically, the constraints it places on values makes it much easier to statically demonstrate runtime properties of the code before.

Yet one more thing that LISP/Scheme got right: Number Towers. Avoid all of this craziness, and just works the way the user intends.

https://en.m.wikipedia.org/wiki/Numerical_tower

Explicit is better than implicit, viva Guido!

I still don't understand what the security issue is. In case of num < 0, malloc will be called with a negative number, which will be converted to size_t, resulting in a huge number which will most likely fail to allocate, returning NULL. Even if it doesn't, I don't see any way to exploit this further. What am I missing?

Agreed; the explanation does not clarify why the first snippet is considered "totally safe" and the second one is not. How does the second snippet introduce a new security issue? A negative value for num passed into the function will always be forwarded to malloc (after going through the size_t conversion), no?

Without the cast, a negative value for num wouldn't have made it past the limit constraint as the operand `num` would've been implicitly promoted to size_t (an unsigned type) in the comparison. That promotion would've reliably caused the negative value to be outside the constraint range in this case (because C has well-defined rules for signed->unsigned conversions), or in any event (if limit was >SSIZE_MAX) preserving the intended semantics viz-a-viz malloc.

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.

> Though this is contingent on arithmetic bugs in caller code elsewhere in the hypothetical program

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.

The negative number is multiplied by 64, so it can then underflow and become positive again.

Ah interesting... but then what could happen? Is the risk that the program could try allocating too much memory and crash? Calling this a "backdoor" seems excessive, no? -- at worst it's just a denial of service.

If you take (1 << 63) + 1 as size (which is a large negative int64), multiply that by 64 and you get 64. So it will call malloc(64) which will succeed. If other parts do the correct unsigned comparison then you have a heap overflow.

This makes sense, but it also implies that there is no actual backdoor in TFA. The code as shown in the article is not exploitable without assuming more (actually exploitable) bugs somwhere else in the callsite (which the article doesn't mention). Or maybe we haven't figured out the actual vulnerability yet...

Well, the rest of the code is also correct on its own though. If I tell the function to allocate X bytes, it should either fail or return a buffer that has space for X bytes. If the function returns less than X bytes if X happens to be a large unsigned int (that is a negative value if interpreted signed) then that is a bug and is exploitable.

But that is not what happens in the code shown in TFA.

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.

It's hypothetical, but other code may expect a bigger buffer than was actually allocated.

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.

You would be right if this were an honest mistake, but a malicious actor can take advantage of this for all kinds of heap-based attacks. https://en.wikipedia.org/wiki/Heap_overflow

On Linux malloc doesn't crash if you give it a too large size. The cash only happens later once you're accessing a memory page that the kernel cannot allocate.

While malloc() won't "crash", it can and will, in some circumstances, return NULL. E.g., the following:

  printf("%p\n", malloc(0x7fffffffffffffff));

prints

  (nil)

on my amd64 machine, as there is no way for the kernel to allocate that much virtual address space. (Even if it wouldn't back it w/ physical RAM.)

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.

But how does that result in a vulnerability? At best you can call the function with garbage input (negative number) and still receive a valid buffer from malloc, no?

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?

You get a buffer that's too small for whatever data gets written there. That data (presumably controllable by the attacker) overwrites whatever other data is right after the allocated (too small) buffer; in many cases this can be leveraged to gain control of the program in ways that will depend the rest of the code around that backdoor.

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.

I don't think that what you are saying is correct.

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?

This depends, obviously, on the code calling allocatebufs. The implication is that the calling code will most likely assume that a buffer of the right size (i.e. "num" elements) has been allocated, while the real underlying buffer can be of any size depending on the low bits of "num".

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;
    }

This code isn't really safe because it passes an unsigned int to allocatebufs, but by default you won't see a warning for this. In the previous version of the code it would work fine - reject anything above 256. In the new "fixed" code, if count = 0x20000001 (for example) this will allocate 64 bytes and proceed to read up to 34 GB of data into the buffer. (A clever attacker can probably cause read_buf to fail early to avoid running off the end of the heap).

Even if read_buf doesn't fail, an even more clever attacker may be able to still exploit a bug like this in a multithreaded environment: https://googleprojectzero.blogspot.com/2015/03/taming-wild-c...

It will become positive anyhow, since malloc takes a size_t as argument. My question still stands - how is the ability to coerce a program to malloc an arbitrarily large block a backdoor?

The caller of allocatebufs will probably expect that allocatebufs returns num * 64 bytes, but due to overflow it could return fewer. Thus, it could end up reading or writing out of bounds.

Just a small nit: underflow is something else; this is still overflow (or wrap-around).

Signed overflow is undefined behavior in C.

It is, but in this case the compiler will assume it never occurs and generate assembly that will function as the parent describes. Which, from a security perspective, is what matters in this case.

This alone is not a backdoor but it might lead to one.

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))

If the limit-check fails and the supplied size is in a valid range for malloc, it would be a typical buffer overflow.

My favorite example to trot out of how C behavior can lead to security problems is a Cap'n'Proto problem a couple years back, reported here.[1] While not entirely the same, there are some some similarities. The article submitted in that discussion does a good job explaining the problem, and the discussion covers even more possible issues. It pays to be very careful how these checks are done in C.

1: https://news.ycombinator.com/item?id=14163111

> Taking `int` is risky even without the comparison bug:

That's the whole point of the article. Risky, but not wrong. A subtle way to introduce a backdoor.

Great explanation.

I'm confused why the author would just assume their readers would know this without any kind of explanation.

It made me stop and think about it for a while.

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.

> in case of a tie, unsigned wins over signed

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!

TBH, Rust's other extreme of never converting any numeric type anywhere is also annoying, and leads to overuse of casts and/or accidental truncation.

Fortunately, memory allocation in Rust is easier, and the whole `malloc(elem * size)` chore just doesn't exist.

Thank you for the informative and thorough explanation!