Warning: Implicit Backdoor(flak.tedunangst.com)
flak.tedunangst.com
Warning: Implicit Backdoor
https://flak.tedunangst.com/post/warning-implicit-backdoor
105 comments
> 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›.
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›.
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.
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.
I'd also add range types ala Ada.
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
https://en.m.wikipedia.org/wiki/Numerical_tower
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.
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.
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?
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:
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))
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.
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.
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.
[deleted]
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.
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.
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
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.
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.
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:
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.
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.
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.
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?
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?
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):
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;
}
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...
> This depends, obviously, on the code calling allocatebufs
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 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.
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.
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?
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?
[deleted]
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.
Just a small nit: underflow is something else; this is still overflow (or wrap-around).
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
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.
That's the whole point of the article. Risky, but not wrong. A subtle way to introduce a backdoor.
[deleted]
Great explanation.
I'm confused why the author would just assume their readers would know this without any kind of 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.
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!
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.
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!
In my opinion, the interactions between unsigned integers and signed integers are too hard for a programmer like myself to handle correctly in practice, especially being consistently vigilant all the time.
I admit that I didn't understand how the code in the article worked (i.e. why the unsigned check was correct and why the signed check was incorrect) until I read the answer in this comment thread.
I like Java's design where all integer types (except char) are signed, and all arithmetic is done in signed mode. It simplifies the mental model and reduces the opportunities for logic errors.
Signed integers are definitely useful sometimes (e.g. difference of two unsigned indexes). A wide-enough signed type (e.g. int32) can hold all values of a narrower unsigned type (e.g. uint16). So it is possible to design a language without unsigned integer types without losing functionality.
I admit that I didn't understand how the code in the article worked (i.e. why the unsigned check was correct and why the signed check was incorrect) until I read the answer in this comment thread.
I like Java's design where all integer types (except char) are signed, and all arithmetic is done in signed mode. It simplifies the mental model and reduces the opportunities for logic errors.
Signed integers are definitely useful sometimes (e.g. difference of two unsigned indexes). A wide-enough signed type (e.g. int32) can hold all values of a narrower unsigned type (e.g. uint16). So it is possible to design a language without unsigned integer types without losing functionality.
My current position is that a high level language should have either explicit numeric conversions with very strong typing (like Haskell), or a single integral type (like Python). Anything else is unecesary complexity that won't even bring any sizeable gains. Strong types with implicit conversions (like SQL) isn't too bad, but Java's system is.
But C isn't a high level language, so none of the above applies.
But C isn't a high level language, so none of the above applies.
> C isn't a high level language
Depends on your viewpoint, of course :)
Depends on your viewpoint, of course :)
I agree. This is the reason the Google C++ style guide bans use of "unsigned" wherever it can be avoided.
The extra bit of headroom is not worth it, and it's clearer to explicitly check for arguments that are out of range.
The extra bit of headroom is not worth it, and it's clearer to explicitly check for arguments that are out of range.
[deleted]
This was potentially in response to this event, posted to HN earlier today: https://news.ycombinator.com/item?id=20874470 (a Spectre Linux kernel patch rendered ineffective by LTS maintainers in an attempt to silence a compiler warning).
The scenario described in this submission was the first thing that came to my mind upon reading about that patch.
The scenario described in this submission was the first thing that came to my mind upon reading about that patch.
To me, as someone that doesn't use C in their day to day life, C appears to be a security nightmare. I cannot imagine using it in a production environment that takes any amount of user input.
Yep. It's insane to use tools/languages susceptible to problems like these in 2019.
To underscore your general point: You're totally right. But... a but:
Implicit type conversions in C are cumbersome. In javascript for example, they are way more. If javascript had a similar role (i.e. bare metal code), the world would be way messier than it already is.
Languages without strict type checking are in general open to problems like this, more or less depending on the leniency to check potential type error. C, being a bare-metal language is especially ugly since it allows to cast anything to anything else, granted - but is is spottable, reviewable and, for new code, you won't get away with ugly casts. In javascript you don't even see casts, they just happen.
Still, javascript doesn't have an unsigned integer, so this attack vector would not work.
Implicit type conversions in C are cumbersome. In javascript for example, they are way more. If javascript had a similar role (i.e. bare metal code), the world would be way messier than it already is.
Languages without strict type checking are in general open to problems like this, more or less depending on the leniency to check potential type error. C, being a bare-metal language is especially ugly since it allows to cast anything to anything else, granted - but is is spottable, reviewable and, for new code, you won't get away with ugly casts. In javascript you don't even see casts, they just happen.
Still, javascript doesn't have an unsigned integer, so this attack vector would not work.
More to the point, memory-safe languages cannot have these types of vulnerabilities regardless of other aspects of the type system.
That's assuming, of course, that the runtimes for those languages are correct: as numerous web browser exploits have taught us, this isn't always the case ;)
[deleted]
I’m quite sure this is a response to https://news.ycombinator.com/item?id=20874470 submitted earlier today. I don’t believe there was any malicious intent there, but it’s yet another safety issue in C that anybody tells me they are aware of, that it would never happen to them and that memory safety issues are things that only happen to other less experienced programmers.
I must only know the cream of the crop of C programmers /s
I must only know the cream of the crop of C programmers /s
Thanks. I was wondering what he was ~pointing at.
As someone who doesn't understand the language's nuances - what does adding a cast to a value being assigned in the function open? Will it return NULL more often?
Sign confusion: malloc takes size_t, so negative numbers are just really large numbers to malloc. A negative int passes the check after adding the cast, because we’re now comparing with signs and now a negative number is smaller than a positive one, so malloc will be called with the larger value. Since it gets multiplied you will also overflow and get pretty much any size. There’s a DoS opportunity at least.
The correct fix is to cast the input to unsigned, I think.
The correct fix is to cast the input to unsigned, I think.
The correct fix is to change the type of the argument to allocatebufs to size_t. Casts are a code smell, and often the best way to fix these warnings is to work your way backward through the code fixing up the types to something more appropriate. Ideally you'd reach the point where someone could induce a negative value, but in any event simply changing the argument type would have sufficed to safeguard the limit check.
That said, the latest release of glibc would have failed on a negative expression promoted to unsigned:
> Memory allocation functions malloc, calloc, realloc, reallocarray, valloc, pvalloc, memalign, and posix_memalign fail now with total object size larger than PTRDIFF_MAX. This is to avoid potential undefined behavior with pointer subtraction within the allocated object, where results might overflow the ptrdiff_t type.
https://sourceware.org/ml/libc-alpha/2019-08/msg00029.html
The new glibc behavior doesn't help with the multiplication overflow, though, as it could have wrapped to a positive value (notwithstanding the fact that such wrapping is undefined). Which is why if I saw code like this I would've worked my way back to all the call sites to understand why the argument to allocatebufs wasn't size_t to begin with. Maybe they were C++ programmers--for decades "best practice" in C++ was to use int for the size of objects.
Arithmetic overflow leading to potential buffer overflow is still a problem even in languages like Rust. Nobody should ever add an integer cast somewhere without having done their due diligence. Which is why stricter arithmetic type checking can sometimes be a double-edged sword--too many people will add an explicit cast and move on.
That said, the latest release of glibc would have failed on a negative expression promoted to unsigned:
> Memory allocation functions malloc, calloc, realloc, reallocarray, valloc, pvalloc, memalign, and posix_memalign fail now with total object size larger than PTRDIFF_MAX. This is to avoid potential undefined behavior with pointer subtraction within the allocated object, where results might overflow the ptrdiff_t type.
https://sourceware.org/ml/libc-alpha/2019-08/msg00029.html
The new glibc behavior doesn't help with the multiplication overflow, though, as it could have wrapped to a positive value (notwithstanding the fact that such wrapping is undefined). Which is why if I saw code like this I would've worked my way back to all the call sites to understand why the argument to allocatebufs wasn't size_t to begin with. Maybe they were C++ programmers--for decades "best practice" in C++ was to use int for the size of objects.
Arithmetic overflow leading to potential buffer overflow is still a problem even in languages like Rust. Nobody should ever add an integer cast somewhere without having done their due diligence. Which is why stricter arithmetic type checking can sometimes be a double-edged sword--too many people will add an explicit cast and move on.
Fair, I only meant to fix the warning, not improve the code quality. I think the problem here is probably something more along the lines of a bad abstraction, as I get the feeling the idea is "I have an int, I need to safely allocate this much memory within limits." If you just change the interface to size_t, that would push the problem down further, but at some point I'd ask why bother with this helper function.
That doesn't make sense -- how would a negative value of num be greater than the positive value that is (int)limit? It must be something else...
Edit: Oh, I see what I missed -- I was thinking of the danger as directly passing too large a number to malloc, when actually the danger is passing a negative number, which then gets implicitly converged to a large positive number. Oops.
Edit: Oh, I see what I missed -- I was thinking of the danger as directly passing too large a number to malloc, when actually the danger is passing a negative number, which then gets implicitly converged to a large positive number. Oops.
Because C's implicit integer promotion rules are horrible. If you are operating on two signed types, or two unsigned types, it does what you would expect and converts both to the type with greater rank (essentially more bits). Likewise, if you have one signed type and one unsigned type and the signed type has greater rank then it does the sensible thing and converts them both to the signed type.
However, if you have one signed type and one unsigned type, and the unsigned type has greater or equal rank, then it converts to the unsigned type, despite the fact that the unsigned type can't represent any of the negative values of the signed type, so they get wrapped!
This is the cause of so many bugs.
However, if you have one signed type and one unsigned type, and the unsigned type has greater or equal rank, then it converts to the unsigned type, despite the fact that the unsigned type can't represent any of the negative values of the signed type, so they get wrapped!
This is the cause of so many bugs.
But the irony is that the [immediate] bug in this case was adding an explicit cast, which is also exactly what people would have done in languages that didn't implicitly promote to unsigned.
The cleverness of this implicit backdoor hypothetical is that it would have worked substantially the same way in most other low-level languages, including Rust. Modern C compilers literally complain in the same way as stricter languages, which had the effect of inducing a poorly considered cast.
The cleverness of this implicit backdoor hypothetical is that it would have worked substantially the same way in most other low-level languages, including Rust. Modern C compilers literally complain in the same way as stricter languages, which had the effect of inducing a poorly considered cast.
Yeah, it frustrates me that both Go and Rust have decided to require casting for all numeric conversions, whether there is a chance of information loss or not. This causes casting to proliferate, and you grow numb to it. If lossless conversions were implicit, then you would only need casts in potentially lossy situations so they would stand out more, and hopefully make you think harder about correctness.
The problem arises because doing a signed comparison is just fine, but when you call malloc it gets converted to an unsigned integer and allocated.
Meaning -1, which passes the signed check fine, becomes 0xFFFFFFFF as a 32 bit unsigned integer...
Now, theoretically a well configured compiler that complains about one would also complain about the other, catching the signed value going to malloc as a warning.
Meaning -1, which passes the signed check fine, becomes 0xFFFFFFFF as a 32 bit unsigned integer...
Now, theoretically a well configured compiler that complains about one would also complain about the other, catching the signed value going to malloc as a warning.
The multiplication can end up turning that large negative number into a small positive one, likely giving you a chance to smash the heap later on (depending on exactly how the int is used).
[deleted]
Exactly right. Here's a runnable sample!
https://repl.it/repls/ShinyQueasyLocks
Ouf, I wish C had better type checking. I'm working in PHP and this would've been caught in strict mode there - why wouldn't malloc raise an objection over being passed a signed value?
C implicitly changes among numeric types without requiring any conversion operator.
Parameter passing works much like assignment. Just like you can assign an int value to an unsigned int variable, you can pass an int argument to an unsigned int parameter.
There is free conversion among char, int, long, and the floating-point types. For instance int x = 1.0 is valid.
Many of these converisons have implementation-defined behavior if the value doesn't fit into the range of the destination type. Assigning a long long to a char where the value does not fit, for instance.
In an integer-floating conversion or vice versa, if the value does not fit, the behavior is undefined. No diagnostic required. E.g. the program can die with a "floating point exception", or just putter along with a random incorrect value.
The conversion rules themselves are implementation-specific because the sizes and ranges of the type vary, as well as certain details like whether plain char is signed or not.
For instance, if int and long have the same range (e.g. both are 32 bits wide), then converting any value between them is well-defined. If they don't (e.g. 16 bit int, 32 bit long), then converting long to int can truncate.
Some things can be counted upon: signed char will convert to short without loss; short to int, int to long, long to long long. Also float to double.
Parameter passing works much like assignment. Just like you can assign an int value to an unsigned int variable, you can pass an int argument to an unsigned int parameter.
There is free conversion among char, int, long, and the floating-point types. For instance int x = 1.0 is valid.
Many of these converisons have implementation-defined behavior if the value doesn't fit into the range of the destination type. Assigning a long long to a char where the value does not fit, for instance.
In an integer-floating conversion or vice versa, if the value does not fit, the behavior is undefined. No diagnostic required. E.g. the program can die with a "floating point exception", or just putter along with a random incorrect value.
The conversion rules themselves are implementation-specific because the sizes and ranges of the type vary, as well as certain details like whether plain char is signed or not.
For instance, if int and long have the same range (e.g. both are 32 bits wide), then converting any value between them is well-defined. If they don't (e.g. 16 bit int, 32 bit long), then converting long to int can truncate.
Some things can be counted upon: signed char will convert to short without loss; short to int, int to long, long to long long. Also float to double.
If you run it with `-Wsign-conversion`, it will catch this.
https://stackoverflow.com/questions/27490762/how-can-i-conve...
https://stackoverflow.com/questions/27490762/how-can-i-conve...
Technically, malloc doesn't get passed a signed value. malloc takes a size_t (which is unsigned). The conversion happens at the call site.
The compiler (or linker I guess) should have visibility of this conversion. I guess to rephrase, why is this conversion being necessary not a compile warning by default, you are changing the range the type can store possibly leading to truncation or misinterpretation.
That would be the compiler, not the linker. Regarding the warnings, compilers must strike a balance between positives and false positives (just that a number is of type int need not necessarily mean that it is negative). With most compilers you can select a warning level or even enable or disable specific warnings, like "sign conversions".
Because malloc does not throw exceptions (in C++), in C there is no concept of exceptions at all, so there is no objection being raised. The developer is treated as responsible for verifying the memory and overflows themselves. The only thing you are promised is that if the malloc operation itself fails you will get a result of NULL;
munk-a’s question was more along the lines of “why would the language allow a signed int to be silently converted to an unsigned int” than “what should malloc return for negative input”.
malloc has no idea what negative input is, because it takes an (unsigned) size_t. Negative numbers just end up being large positive numbers (many allocators will check for this, though).
The compiler is aware that a function taking an unsigned size_t is being given a signed value though - it could mandatorily throw up a warning (and apparently there is a switch for this) which, if it were on, actually protects against the issue raised in this specific security vulnerability.
Oh, perhaps I misunderstood the intent then.
> The correct fix is to cast the input to unsigned, I think.
I think the correct fix is rather to check the input (e.g. assert()). What you suggest means reinterpreting what the caller wanted, and that leads to nasty surprises down the road.
I think the correct fix is rather to check the input (e.g. assert()). What you suggest means reinterpreting what the caller wanted, and that leads to nasty surprises down the road.
Iirc size_t is unsigned and int is signed, normally, but raw casting in c compares bits, so limit will always be very large. Take with a grain of of salt this info, I can't verify right now.
Edit: proper explanation above. I got some things wrong.
Edit: proper explanation above. I got some things wrong.
I feel that it says something about the problem area when over forty comments in the thread are dedicated to the discussion of how exactly the five lines of code work.
Half of the problem is programmers putting their head in the sand. Half of the problem is lack of real leadership from people that know better.
Computers are not securable. Not from these types of attacks, anyway. State intelligence agencies are working overtime trying to stop the fires from spreading, but at the end of the day we've had Petya, NotPetya, and WannaCry. We have Schneier with a book called "Click Here to Kill Everyone" and we still have computers running everything.
Ok fine. But one day we're going to get an event and people are going to say: "Oh the humanity, how did this happen? How could we have known?!" I don't know what it is going to be, but I know it's going to be just like 2008 all over again. Industry insiders know the score. If it does happen, my deepest hope is that it ends up being strictly financial, but my gut says that it will probably be cyberphysical. The financial sector has had decades to harden their systems.
Computers are not securable. Not from these types of attacks, anyway. State intelligence agencies are working overtime trying to stop the fires from spreading, but at the end of the day we've had Petya, NotPetya, and WannaCry. We have Schneier with a book called "Click Here to Kill Everyone" and we still have computers running everything.
Ok fine. But one day we're going to get an event and people are going to say: "Oh the humanity, how did this happen? How could we have known?!" I don't know what it is going to be, but I know it's going to be just like 2008 all over again. Industry insiders know the score. If it does happen, my deepest hope is that it ends up being strictly financial, but my gut says that it will probably be cyberphysical. The financial sector has had decades to harden their systems.
> State intelligence agencies are working overtime trying to stop the fires from spreading
I think you're mistaken. Intelligence agencies do the opposite by hoarding exploits and purposely weakening security standards.
I think you're mistaken. Intelligence agencies do the opposite by hoarding exploits and purposely weakening security standards.
The code is 'totally safe and secure' in a nasty, obfuscated way. It takes too much language / system knowledge to tell that negative numbers are covered. Plus, here isn't an extra block whose purpose is confusing that is responsible for negative numbers. Instead the behavior is hidden as part of another block that has a simple face-value interpretation. This is (1) fragile but (2) not self-aware that it's fragile. If you don't refactor to avoid fragility at least leave a comment. Or how about unit tests? If you don't question the motives of the author you should at least question their coding ability.
Obligatory mention of early attempt to really understand and address subversion as a threat plus a follow-up by pioneers that built on that:
http://seclab.cs.ucdavis.edu/projects/history/papers/myer80....
https://apps.dtic.mil/dtic/tr/fulltext/u2/a435312.pdf
The current state-of-the-art in both formal methods, automated analyses, and test generators can get us pretty close to their goals with such designs, too. Way less cost-prohibitive than it used to be. Although, high-assurance security today has moved far away from security kernels they advocated to focus on securing hardware/software and distributed systems in ways that allow expressing more flexible and useful policies. The design and assurance methods of the past still work, though. Stuff like Rust, SPARK Ada, RV-Match, Why3, and CompCert handle more code-level problems than older methods, too.
http://seclab.cs.ucdavis.edu/projects/history/papers/myer80....
https://apps.dtic.mil/dtic/tr/fulltext/u2/a435312.pdf
The current state-of-the-art in both formal methods, automated analyses, and test generators can get us pretty close to their goals with such designs, too. Way less cost-prohibitive than it used to be. Although, high-assurance security today has moved far away from security kernels they advocated to focus on securing hardware/software and distributed systems in ways that allow expressing more flexible and useful policies. The design and assurance methods of the past still work, though. Stuff like Rust, SPARK Ada, RV-Match, Why3, and CompCert handle more code-level problems than older methods, too.
[deleted]
Or just introduce a format string vulnerability by removing the format specifier from printf et all functions ... or an off by one or double free bug if you want to get more genuine
How about this in stdlib.h:
Edit: markdown
void *malloc(signed long long int untrusted_size) {
size_t size;
if (untrusted_size < 0) {
fprintf(stderr, "warning: exploit "
"detected. Click <ok> to cancel");
}
size = (size_t)untrusted_size;
etc.
}
Edit: markdown
The hack depends on numeric overflow in the caller that flips negative number into positive, so malloc can't possibly know it got the wrong size.
It's even funnier that signed overflow is Undefined Behavior in C, so the compiler is allowed to assume it can never happen (and thus let it happen, and even remove non-kosher overflow checks).
It's even funnier that signed overflow is Undefined Behavior in C, so the compiler is allowed to assume it can never happen (and thus let it happen, and even remove non-kosher overflow checks).
> The hack depends on numeric overflow in the caller that flips negative number into positive, so malloc can't possibly know it got the wrong size.
Look again at the hack-- Ted is sending a negative number to malloc. If I change malloc's interface to accept signed numbers, then I can check inside the definition for negative numbers and report to the user that something bad has happened.
Look again at the hack-- Ted is sending a negative number to malloc. If I change malloc's interface to accept signed numbers, then I can check inside the definition for negative numbers and report to the user that something bad has happened.
This might be what you want, from glibc: https://sourceware.org/bugzilla/show_bug.cgi?id=23741
Making malloc take a long long on 32b archs is a _real_ perf hit. At least make it an ssize_t.
Is this bug exposed only on systems with 1 byte words? I'm struggling a bit to understand the exposed bug and, while I understand the whole point is that there are lots of issues of this kind that can hide in code pretty innocently it'd be nice to actually comprehend this one.
You can pass a negative number such that num * 64 will underflow and result in a large positive number. (Technically it's UB, but in practice it will probably underflow).
Or as someone else mentions, even without underflow, it will be converted to unsigned since malloc takes a size_t.
Or as someone else mentions, even without underflow, it will be converted to unsigned since malloc takes a size_t.
I wonder if you could trick an auto linter git hook to do the malicious commit for you.
I’d be interested to see such an example with a more secure language like Rust.
This specific case simply doesn't compile:
if num > limit {}
^^^^^ expected i32, found usize
The point is not really about the specific mechanism shown in the post, but more about plausible deniability and how a malicious actor might introduce a vulnerability like this in a way that makes their innocence seem unimpeachable.
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