Security review notes for followup

[randomdross]

Hi @blt, as requested I'm including security review notes here. (I'm not particularly Rust-savvy at the moment, so some of this may not be interesting in reality.) PTAL and assess if there's anything worth following up on.

---

• Some non-memory-safe functionality in hopper/src/deque.rs, though I don’t immediately see any red flags
• Good to see fuzzing in place, though I’m only just now learning about memory safety w.r.t. Rust. Looks like Rust is only unsafe in places explicitly marked as unsafe. Any further context on the fuzzing would be interesting to hear about.

---

[blt]

Some non-memory-safe functionality in hopper/src/deque.rs, though I don’t immediately see any red flags

Yep, yep. The code is manipulating an exploded Vec. It might actually be possible, these days, to deal with a Vec directly, assuming that it were pre-populated and any of its resize machinery were avoided. That'd be iffy, though, since we'd be depending on the incidental implementation of the Vec. But! When int generics come the deque can have an internal array and drop the pointer munging.

Good to see fuzzing in place, though I’m only just now learning about memory safety w.r.t. Rust. Looks like Rust is only unsafe in places explicitly marked as unsafe. Any further context on the fuzzing would be interesting to hear about.

There are two main ways you can crash a Rust program that is otherwise safe: integer over/underflow and memory allocation. The integer stuff can be worked around by using the wrapping, saturating operators but its easy to forget. With regard to allocation, the default strategy in Rust has long been to panic the program if allocation fails. This is slowly changing, but it's still a thing and fuzzing is quite helpful at catching accidentally large allocations.

In fact, panic if malloc fails makes fuzzing projects kind of difficult in Rust. If you check my commit history here you can see how much flailing I do around this issue, to the point where I made an allocator that just exits on allocation failure -- https://github.com/blt/bh_alloc/blob/master/src/fuzz/mod.rs -- and have started experimenting with a QuickCheck implementation that is panic friendly -- https://github.com/blt/rqc -- as time allows.

[randomdross]

Thanks! That suggests to me that memory corruption as is common in C / C++ is not really possible, though certainly an integer over/underflow could lead to a logic bug, and this could be revealed via fuzzing. I would imagine that allocation failure would at worst result in a non-exploitable crash, unless somehow it caused the program to fail in a non-secure state. I would expect that Rust (in "safe" mode) would prevent any OOB write to the heap in the case where allocation fails. So I think fuzzing is great & worthwhile, most critically for the unsafe code.

[blt]

I believe all of this to be accurate. David Ross wrote on 2/5/19 11:19 AM:

…

Thanks! That suggests to me that memory corruption as is common in C / C++ is not really possible, though certainly an integer over/underflow could lead to a logic bug, and this could be revealed via fuzzing. I would imagine that allocation failure would at worst result in a non-exploitable crash, unless somehow it caused the program to fail in a non-secure state. I would expect that Rust (in "safe" mode) would prevent any OOB write to the heap in the case where allocation fails. So I think fuzzing is great & worthwhile, most critically for the unsafe code. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAN-6oWBDjci6uWYJLVQS6HhWm3Hp6lks5vKdlQgaJpZM4afTGt>.