5 ms·
// SECURITY IMPROVEMENTS REQUIRED. VULNERABLE TO INTEGER AND BUFFER OVERFLOWS. well at least it's pretty clear up front. No malloc result checks, invalid forma
by audidude 10y ago
// SECURITY IMPROVEMENTS REQUIRED. VULNERABLE TO INTEGER AND BUFFER OVERFLOWS.
well at least it's pretty clear up front. No malloc result checks, invalid format in printfs, probably more.
(I don't mean to be disparaging, please don't take it that way OP. We clearly need more/better tooling in this area in C).
(Edited for clarity)
- blechschmidt 10y agoMalloc results are checked by the safe_malloc function in security.h. Could you point out a line that is prone to integer overflows?
- audidude 10y agostream_buffer_init() calls malloc directly, so it's not protected by safe_malloc(). (Unless there is some funky re-define stuff I didn't see). %x takes unsigned int, not a char. (Although I guess you could just rely on promotion here if you can rely on the unsignedness of char). > Could you point out a line that is prone to integer overflows? Never said that it was an overflow (just C&P that comment from the file). Just said its invalid use. Which I'm totally willing to agree on the compiler mostly getting correct behavior. Just not really something I'd want to see landing in something that we all need a really good, secure implementation of. Best of luck though, I hope you iterate on this to the point we can have a really good DNS implementation in C, in user-space, with optional async processing.
- blechschmidt 10y agoThank you for pointing this out. This file actually contained some unused code that was not supposed to make it into the repository so I just removed it. The "%x" format specifiers were located in debug comments that have been commented out. They have also been removed now.
- userbinator 10y agoAlthough I guess you could just rely on promotion here if you can rely on the unsignedness of char If compilers did not do promotion, they would be very obviously not compliant with the C standard and unable to compile correctly most existing code, so I'm willing to bet that is something you can rely on. It's defined by the standard and widely used.
- audidude 10y agoSure, but in this case it was signed to unsigned promotion.
- blechschmidt 10y agoWhy should that matter in this case? The worst case would have been the output of a wrong value and it was some commented out debug output which I have used in a very specific case.
- audidude 10y agoTotally, just wrong value. This thread is already making a bigger deal of it than I expected it to.
- tptacek 10y agoMalloc result checks are generally a bad idea; they're error prone. Better just to run with the allocator configured to abort on failure.
- jjnoakes 10y agoUnless, of course, one can recover from malloc failing.
- pcwalton 10y agoEven if you can in theory, it's still a bad idea to try to do that. Statistically speaking, you will get it wrong.
- jjnoakes 10y agoIt may be a bad idea for an average developer to try it, sure. Plenty of advanced things fall in to that category though.
- pcwalton 10y agoIt is a bad idea for any developer to try it. Statistically speaking, it is the best programmers who create the most widespread security flaws.
- briansmith 10y agoWhich statistics are you referring to?
- pcwalton 10y agoWell, there can't be any objective quantification of "best", so it's impossible to prove this without being subjective. But the most widespread vulnerabilities tend to be in software that a lot of people use. Software that a lot of people use is usually written by good programmers, because good programmers (for typical definitions of "good") are the most productive ones. The people who write the Linux kernel, for example, are by and large good programmers, because kernel programming is difficult and becoming a prolific contributor even more so. They've been responsible for the majority of the security bugs in the Linux kernel. That doesn't make them bad programmers.
- userbinator 10y agoNo malloc result checks The last time I worked with the DNS protocol, I seem to remember that everything had a well-defined (and small) limit on its size, and it wasn't really necessary to use dynamic allocation at all. Strings are length-prefixed, not null-terminated, and the longest length fields are only 16 bits. A look at the RFC shows the strict limits on size: https://www.ietf.org/rfc/rfc1035.txt https://www.ietf.org/rfc/rfc1035.txt In comparison to text protocols like HTTP, I see far less opportunities to make errors like buffer overflows with DNS. It's a simple and relatively easy to parse binary protocol.
- legulere 10y agoJust because strings are length prefixed, doesn't mean that they don't need to be dynamically allocated. The reason they can be put on the stack is that the prefix is just one byte and thus the maximum length 255 bytes for the strings. Prefixed strings can even be a source of buffer overflows, if you don't check if the length is smaller than the actual length of the rest of the data. In fact this was actually the source of the heartbleed bug.
- technion 10y agoWe clearly need more/better tooling in this area in C). I think the tooling we have nowadays is better than we give it credit for. Between clang's analyser, ASAN, Infer and tis-interpreter, I often find when someone points out a vulnerability, I can get one of these tools to make an issue of it. Note I'm not suggesting this is enough to make C safe. Edit: A quick demonstration, based on 30 seconds with valgrind. If I run with ./bin/massdns --help, it makes a malloc() on line 595, then exits on 605 without free()ing. And yes I know this isn't really an issue in practice, but it demonstrates that the tools can see issues.