Limit the use of unsafe Rust code #499
Description
Rust-protobuf currently has 33 uses of unsafe Rust code.
Most of them, although not all, appear to be for performance reasons.
As this crate can be used to parse untrusted protobuf inputs, security is an important topic.
It is also a general recommendation in the Rust community to reduce unsafe code as much as possible.
Ideally, every use of unsafe should be clearly commented with an explanation of why it is safe and necessary.
As I went through the code, a lot of them couldn't be immediately understood.
I am not saying that this crate currently has security issues, but right now it is very difficult to audit it to show that it doesn't.
Below is a run-through all the unsafe uses I found (I only checked on the master branch, as the latest version 2.15.1 still uses unsafe in generated code).
- ./protobuf/src/buf_read_iter.rs: 7 uses
./protobuf/src/stream.rs: 5 uses
These two are obviously for performance reasons, although I believe they can be rewritten in safe code without impacting performance at all.
I am willing to try and work on it if wanted.
- ./protobuf/src/chars.rs: 2 uses
./protobuf/src/json/base64.rs: 1 use
These are examples of "good" uses of unsafe.
I recommend adding a short comment for each one explaining the reason behind it.
- ./protobuf-codegen-pure/src/convert.rs: 2 uses
Can be replaced trivially by f64::to_bits() and f32::to_bits()
- ./protobuf/src/lazy.rs: 4 uses
After reading the code, I am not able to understand the use behind this lazy initialization code, as it is never used in any of the 'hot' code paths (i.e. parsing or writing).
This is also storing global state, which is usually frowned upon in Rust, and not something that users expect from a parsing library.
Either way, using an dedicated crate, like lazy-static or once_cell, would make this code much easier to audit.
- ./protobuf/src/core.rs: 4 uses
./protobuf/src/misc.rs: 3 uses
./protobuf/src/reflect/enums.rs: 4 uses
./protobuf/src/reflect/runtime_type_dynamic.rs: 1 use
./protobuf/src/reflect/transmute_eq.rs: 1 use
That is a lot of transmutes and raw pointer dereferences, and the whole code appears to be built around it.
Rust is usually not a dynamically typed language, but this use of std::any everywhere makes it look like one.
In addition, because the code is using raw dyn traits everywhere, and not Box<dyn Trait>, the whole downcast and transmute code was re-implemented instead of using the standard Box::downcast(), which would have been safe.
Overall, I believe this to be the most problematic part for anyone trying to perform a security audit of this library.
Using dyn Trait everywhere (instead of impl Trait) also has huge performance impacts, as virtual function calls are slow...