12 ms·
Warning: Implicit Backdoor
- SketchySeaBeast 7y agoAs 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?
- jchw 7y agoSign 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.
- munk-a 7y agoOuf, 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?
- AlexCoventry 7y agoIf you run it with `-Wsign-conversion`, it will catch this. https://stackoverflow.com/questions/27490762/how-can-i-convert-to-size-t-from-int-safely https://stackoverflow.com/questions/27490762/how-can-i-conve...
- Rietty 7y agoBecause 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;
- DougBTX 7y agomunk-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”.
- Rietty 7y agoOh, perhaps I misunderstood the intent then.
- saagarjha 7y agomalloc 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).
- munk-a 7y agoThe 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.
- jstimpfle 7y agoTechnically, malloc doesn't get passed a signed value. malloc takes a size_t (which is unsigned). The conversion happens at the call site.
- munk-a 7y agoThe 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.
- jstimpfle 7y agoThat 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".
- kazinator 7y agoC 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.
- Sniffnoy 7y agoThat 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.
- deleted 7y ago[deleted]
- Rietty 7y ago#include <stdio.h> int main(void) { unsigned int plus_one = 1; int minus_one = -1; if(plus_one < minus_one) printf("1 < -1"); else printf("boring"); return 0; } Gotten from S/O.
- firethief 7y agoThis is an example of code that would trigger what the OP describes as a "harmless warning"
- buildzr 7y agoThe 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.
- pavon 7y agoBecause 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.
- InnocentB 7y agoExactly right. Here's a runnable sample! https://repl.it/repls/ShinyQueasyLocks https://repl.it/repls/ShinyQueasyLocks
- jstimpfle 7y ago> 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.
- wahern 7y agoThe 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 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.
- jchw 7y agoFair, 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.
- alanfranz 7y agoIirc 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.
- munk-a 7y agoIs 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.
- jstimpfle 7y agoYou 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.
- gyanchawdhary 7y agoOr 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
- comex 7y agoFor those wondering how this works: 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): if (num < 0 || num > limit) 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.8.html - http://c0x.coding-guidelines.com/6.3.1.1.html http://c0x.coding-guidelines.com/6.3.1.1.html
- Liquix 7y agoThank you for the informative and thorough explanation!
- thegeomaster 7y agoI 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?
- pilif 7y agoI’m quite sure this is a response to https://news.ycombinator.com/item?id=20874470 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
- mirimir 7y agoThanks. I was wondering what he was ~pointing at.
- windsurfer 7y agoTo 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.
- carapace 7y agoYep. It's insane to use tools/languages susceptible to problems like these in 2019.
- chupa-chups 7y agoTo 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.
- tsimionescu 7y agoMore to the point, memory-safe languages cannot have these types of vulnerabilities regardless of other aspects of the type system.
- saagarjha 7y agoThat'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 7y ago[deleted]
- nayuki 7y agoIn 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.
- deleted 7y ago[deleted]
- marcosdumay 7y agoMy 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.
- saagarjha 7y ago> C isn't a high level language Depends on your viewpoint, of course :)
- w0mbat 7y agoI 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.
- twiceaday 7y agoThe 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.
- nickpsecurity 7y agoObligatory 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.pdf http://seclab.cs.ucdavis.edu/projects/history/papers/myer80.... https://apps.dtic.mil/dtic/tr/fulltext/u2/a435312.pdf 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.
- baby 7y agoI’d be interested to see such an example with a more secure language like Rust.
- pornel 7y agoThis specific case simply doesn't compile: if num > limit {} ^^^^^ expected i32, found usize The whole trick isn't really translatable to idiomatic Rust, because `malloc` is not used beyond specific C interoperability needs. If you want to store `num` elements, you use `Vec::with_capacity(num)` or such, instead of allocating the hard way.
- draw_down 7y agoThe 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.
- aasasd 7y agoI 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.
- 3pt14159 7y agoHalf 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.
- adrianN 7y ago> 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.
- 5- 7y agoThis was potentially in response to this event, posted to HN earlier today: https://news.ycombinator.com/item?id=20874470 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.
- jancsika 7y agoHow about this in stdlib.h: 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. } Or better yet redefine malloc with a wrapper and include the __line__ and __file__ in the exploit error. Edit: markdown
- saagarjha 7y agoThis might be what you want, from glibc: https://sourceware.org/bugzilla/show_bug.cgi?id=23741 https://sourceware.org/bugzilla/show_bug.cgi?id=23741
- pornel 7y agoThe 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).
- jancsika 7y ago> 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.
- verall 7y agoMaking malloc take a long long on 32b archs is a _real_ perf hit. At least make it an ssize_t.
- crisnoble 7y agoI wonder if you could trick an auto linter git hook to do the malicious commit for you.
- deleted 7y ago[deleted]