How can buffer overflows still be happening in this day and age?
In this context, “memory-safe” means it does bounds checking on an array when you try to access an element. But the webp code does bounds checks up-front so that array accesses can be non-checked, to help performance. (If they didn’t want this performance, they could have easily used a std::vector and used bounds-checked access.) The vulnerability happened because this up-front bounds checking logic was incorrect.
If we were to hypothesize a counterfactual world where webp was written in rust, presumably the devs would have wanted a similar optimization where they did the bounds checking up-front still, and they would have put the actual array access in an unsafe block to have the same perf optimization. The same bug would have thus happened.
The lesson here is that bounds checking on every element access is probably a worthwhile overhead, no matter what the language is. C++ has safe ways to do this (well, safe-ish… safe for the purposes of this bug at least) with std::vector, and maybe they should just switch to that and eat the performance overhead.
https://security.googleblog.com/2022/12/memory-safe-language... shows improvements at scale.
It looks like dropbox experimented with disabling bounds checks in their huffman coding impl, and found that using the unsafe pattern increased throughput from 224 MB/s to 249 MB/s (11%-ish faster.) We don’t even need to hypothesize about whether webp would have elminated bounds checking, we can see that other companies arrived at the same conclusion: Disabling it can be worth it if you’re quite sure you’ve gotten the up-front checking right. We can imagine that if Dropbox went to prod with the unchecked huffman implementation (never mind that that article isn’t about webp in particular), we could imagine they could easily have the same bug. And I don’t think a naive code review saying “unsafe is bad” would have stopped them from doing it: they clearly did the work to show why it’s worth it.
It's already common for PC apps to split potentially unsafe rendering into subprocesses, like in Chrome. If you don't want to pay the full IPC toll, there's shared memory. In theory should be about the same speed as inlined unsafe code, right? What if Rust's "unsafe" blocks could do this for you?
Rust has a pattern of isolating unsafety into small components behind a safe interface, so that the component can be understood and tested in isolation. For example, if you need some adventurous pointer arithmetic, you write an Iterator for it, rather than do it in the middle of a complex algorithm. This way the complicated logic can be in safe code.
It's sort of like Lego, where you build from safe higher-level blocks, but you can design custom blocks if you need.
I can't find the actual code causing the libwebp vulnerability, so idk if mixed safe/unsafe Rust code would've been any better here. Maybe what we really need is an "unsafe-jail" block in Rust that uses a child process limited to a piece of shared mem, and you put big pieces in there to avoid overhead. Like, libwebp can screw up all it wants, just don't touch the rest of my app.
Am I missing something else, like shared mem being slower? Maybe the inability to share CPU caches?
Scheduling, I dunno. Would imagine it's not bad as long as you don't spawn a ton of these.
C's type system is not nearly expressive enough for this. You can barely declare a pointer non-null with extensions, but you can't express ownership. You can't force callers of your function to check for error before using the returned value. You can't force correct lifecycle of objects (e.g. Rust can have methods that can be called once, and no more, and then statically forbid further uses of the object. Great for cleanup without double-free.)
C doesn't have ability to turn off Undefined Behavior for a section of code. You can't just forbid dangling pointers.
For a foolproof API the best you can do is use handles and opaque objects, and a ton of run-time defenses. But in Rust that is unnecessary. Use of the API can be guaranteed safe, and a lot of that is guaranteed at compile time, with zero run-time overhead.
For example, a mutable slice in Rust (a buffer) has a guarantee that the pointer is not null, is a valid allocation for the length of the slice, is aligned, points to initialized memory, is not aliased with any other pointer, and will not be freed for as long as you use it. And the compiler enforces that the safe Rust code can't break these guarantees, even if it's awful code written by the most incompetent amateur while drunk. And at run time this is still just a pointer and a length.
In C you don't get this compartmentalization and low- or zero-overhead layering. Instead of unsafe + actually enforced safe, you have unsafe + more unsafe.
These decent C programmers are like True Scotsmen. When top software companies keep getting pwned, even in their most security-sensitive projects, it's because they hire crap programmers.
Even basic boring C can be exploitable. Android was hit by an integer overflow in `malloc(items * size)` (stagefright). Mozilla's NSS had vulnerability due to a wrong buffer size, which fuzzing did not catch (BigSig).
This is a big presumption. Yes, it could happen. In practice, doing this isn't even the first tool you'd reach for in this circumstance; the compiler can and will eliminate duplicate bounds checks, so if you've hoisted it early, you shouldn't be using unchecked accesses, even if you care about performance, until you've demonstrated why the compiler isn't okay with removing them. The extra ceremony ("unsafe { foo.get_unchecked(n) {" vs "foo[n]") makes this even simpler to catch in code review.
Right, and in said code review, the webp author could have easily said “yup, we want unsafe here because we already checked up front that the buffer shall not exceed k elements”. Sure it’s easier to see that unchecked access is happening, but when the whole point of large sections of the huffman table code in webp is to make this very thing work, it wouldn’t cause any additional scrutiny in a code review. In other words, it would already be super clear to the reviewer that bounds checking is being disabled for performance sake, seeing the word `unsafe` as ceremony isn’t really adding any information here.
It’s possible webp could have been implemented in rust with a naive approach to early bounds checking and relying on the optimizer to elide it, but having looked at the code, with all the buffer sizes they’re passing around, it looks unlikely that equivalent rust code would have been able to auto-optimize it. I don’t think it’s unlikely at all that, in this parallel universe, they would have found the bounds checking to be a decent enough overhead that they would have reached for unchecked access, and would have passed code review the same way the current code did.
> it wouldn’t cause any additional scrutiny in a code review.
I would hope that it would at least cause a "demonstrate that this is actually necessary" review. People do do this! and sometimes, you do have to use unchecked. A famous example of this is litearally huffman coding, by Dropbox, back in 2016. They ended up providing both bounds checked and unchecked options as a flag, because they actually did measure and demonstrate that things were causing performance drops. I am curious if they'd still have the same issues today, given that a lot of time has passed, and also there were some other factors that would cause me to wonder a bit, but haven't followed up on myself. Regardless, this scenario will end up happening for good reasons at some point, the goal is to get them to happen only when they need to, and not before.
That was for Dropbox specific use case, and 11% seems too much in any case.
For example, what improvements has had rustc in the meantime to better optimize bounds checking, it is still the same, it is less, does it actually matter on the acceptance criteria regarding the definition of done on the story.
You don't have to hypothesize. It's here:
https://github.com/image-rs/image/blob/master/src/codecs/web...
and it doesn't use unsafe indexing.
Meanwhile the real code that exists doesn’t work like that. It’s not just a matter of personal style. Rust has constructs that help avoid bounds checks. It has design patterns that move unsafe code into smaller, easier to verify helpers. It has a culture of caring about safety – people don’t rewrite projects in Rust to just give up on its biggest selling point.
It is not a stretch whatsoever to assume that a direct rewrite of the webp library into rust would have uncovered the same perf findings that Dropbox did, and decided that the pattern of “up front bounds checks, disabled bounds checks at element access” is a reasonable perf enhancement, especially for a Huffman table, where you’re continually accessing the compression table over and over when expanding it.
Edit: the more I think about it, you’re probably right. I had mistakenly thought I was reading C++ code when I was looking at the original patch to the vulnerability. The fact that it’s actually written in C, erodes my argument quite a bit… my logic in my head went something like, “if they wanted bounds checking they would have used std::vector”, but now I realize that’s impossible since it’s not C++. I’ll concede that there’s no real reason to assume that they would have skipped bounds checking if written in a safe language.