4 ms·
I'm not a fan of rust either, not by a long mile. But current kernel approach to memory safety is a complete, utter, demonstrated failure. Look at last week's C
by nousermane 4y ago
I'm not a fan of rust either, not by a long mile. But current kernel approach to memory safety is a complete, utter, demonstrated failure. Look at last week's CVE-2022-41674:
https://seclists.org/oss-sec/2022/q4/23 https://seclists.org/oss-sec/2022/q4/23
This is a catastrophic bug, that (after some work on developing an actual RCE) lets anybody within wifi range to get root on your laptop (or phone, or access point). And all it took is this one line:
https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless.git/commit/?id=aebe9f4639 https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wir...
We've all been repeating the "1000 eyes - all bugs are shallow" mantra for far too long. This one was in the mainline for more than 3 years, and nobody noticed. How many more are lurking there?
- JoeAltmaier 4y agoConfused by that bug. Adding 2 to a U8 causing overflow makes it smaller, not larger. It will copy too little? not too much.
- planede 4y ago"The subsequent memcpy" is this: memcpy(pos, mbssid + cpy_len, ((ie + ielen) - (mbssid + cpy_len)));
- abainbridge 4y agoHow would Rust fix this? Here's a Rust program that overflows a u8, with no compile error or warning. https://godbolt.org/z/ME3e9KeMe https://godbolt.org/z/ME3e9KeMe You can turn on runtime overflow checking with the Rust compiler, but you can do that with gcc compiling C too. I guess the answer is that you wouldn't do the memcpy like that in idiomatic Rust - you'd use some higher level construct that gives the compiler more chance to catch your errors. Could anyone comment on how this works in a case like this? edit: Can't reply to repliers. Defined overflow doesn't help here. A checked_add() function could be used in the Linux kernel in C, just as easily as in Rust. Forcing checked_add() to be used in the Rust compiler would help, but would also have performance and readability impact, which is presumably why it is still being debated.
- selfmodruntime 4y agoThat is because you're using a normal add instead of `checked_add`. There are discussions about forcing the compiler to only allow `checked_x` and `wrapping_x` operations for any mathematical operation within your code.
- laweijfmvo 4y agoI thought the idea was that Rust’s behavior in cases of overflow was defined, whereas c/c++ is not defined by the language.
- tialaramex 4y agoThe C language says the unsigned integers wrap. That is, C's 8-bit unsigned integer (e.g. uint8_t or in Linux source u8) behaves the same as Rust's Wrapping<u8> type. In Rust the u8 type wraps in default release builds, but panics on overflow in debug builds, but this is kernel code so it will definitely be built in release mode with wrapping.
- tialaramex 4y agoIn C when you use an inappropriate type (such as u8 here for a size) it's just coerced. In Rust the wrong type doesn't compile. So, I think the programmer is less likely to choose u8 in the first place.
- cesarb 4y ago> How would Rust fix this? [...] I guess the answer is that you wouldn't do the memcpy like that in idiomatic Rust - you'd use some higher level construct that gives the compiler more chance to catch your errors. Yes, that's the trick: it will be caught not in the u8 overflow, but either in the memcpy equivalent (which is "dst.copy_from_slice(src)"), or in the slice manipulation before it. What happens is that slices in Rust are represented by a "fat pointer", a pair of the starting address and the length, and both the "copy_from_slice" method and the index/index_mut operator check the bounds before doing the operation. (You could do things using "unsafe" and raw pointers, which have only the starting address without the length, but in idiomatic Rust you'd use slices most of the time.)
- baybal2 4y ago
- beanjuiceII 4y agois this really that bad of a bug though? i need to be using that driver, and someone near me has to be actively knowing and trying to inject... and it's a DoS ? i'm sincerely asking btw
- nousermane 4y agoDriver in question is cfg80211. Majority of popular wireless cards use it. DoS is the only publicly known exploit, right now. There is understanding that RCE is possible with additional specialist work. Local packet injection is an implementation detail of the first public PoC. There are other implementations of the same exploit, that deliver packets over the air instead. Including one that runs on ESP32, and attacks unmodified Linux nearby: https://github.com/jo-m/linux-wifi-ota-crash https://github.com/jo-m/linux-wifi-ota-crash
- rbanffy 4y ago> and someone near me has to be actively knowing and trying to inject Just leave a small board, a battery, and a solar panel on a roof or tree near your office. If RCE is possible with this (people think it is), this would be very valuable. On a tree near my office, you'd gain access to our network, Microsoft's and a European central bank. Not bad at all.
- nyanpasu64 4y agoSeparately I feel "with enough eyes, all bugs are shallow" fails to apply in codebases with many users but most of whom consume it as a black box without looking inside. Some issues making it harder for me to read the code I use: - The Linux kernel and issuance .so files lack a "view source" button on compiled binaries. And even checking out the matching source, building a replacement binary, and diffing your local changes from the matching source is an arduous progress to setup per program/library from a tarball/Git tag, wait for the computer to finish, install dependency .so files globally, ensure symbols are present, ensure you can breakpoint static functions... - Dynamic dispatch and generic code might help maintainers and code extensibility but (in my experience) definitely impede external eyeballs from understanding code.
- unsafecast 4y agoYes! That's why I adore the way plan 9 has the whole source tree in /sys/src. Everything is there. To a smaller extent, netbsd has pkgsrc, and most BSD friends have something like that too.
- nyanpasu64 4y agos/issuance/userspace, s/computer/compiler
- deng 4y ago> (after some work on developing an actual RCE) While I agree that bug is serious, that "some" is doing pretty heavy lifting here. Is there an RCE for this bug?
- amluto 4y agoThere are almost certainly some private ones. This is an extremely high impact bug.
- thomasahle 4y agoDoes Rust help solve overflow errors? Don't you need to explicitly used "saturated_add" or "checked_add"?
- steveklabnik 4y agoRust doesn't directly solve overflow errors. What Rust does do is turn them into logic errors, rather than memory issues. To break it down: * Overflow with the default operators is not UB in Rust, it will either panic or two's compliment wrap, depending on various things (including things you can set to choose this global behavior). This already prevents various issues. * You can also explicitly choose to do various operations with whatever overflow semantics you want, as you mention with saturated_add and friends. * Because indexing is bounds checked, where in some languages an overflowed integer would lead to incorrect indexing and therefore possible memory problems, you'll either get a panic or a logic bug, not a memory bug. * If your integer isn't being used for indexing, you'll end up with some sort of error, but again, at worst a logical error, not a memory error.