urschrei / lonlat_bng

A multithreaded Rust library with FFI for converting WGS84 longitude and latitude coordinates into BNG (OSGB36) Eastings and Northings and vice versa (using OSTN15)
https://docs.rs/lonlat_bng
MIT License
25 stars 3 forks source link

Unsoundness in `From` implementation #19

Closed shinmao closed 1 year ago

shinmao commented 1 year ago

The source of soundness

https://github.com/urschrei/lonlat_bng/blob/c66eca1c878c8d7eb86b53463ccaf56e86dac7bb/src/ffi.rs#L64-L79 With from at line 66, we can cast arbitrary type as Array.data; with from at line 76, we can create a slice of f64. It is unsound because there is no any trait bound on T, then we can indirectly cast arbitrary types to f64 slice. If we pass any type with smaller alignment such as f32, it would lead to out-of-bounds read when slice::from_raw_parts_mut tries to call &mut *ptr::slice_from_raw_parts_mut(). The reason is that it would create a slice of 8 bytes * arr.len while the actual data only has size of 4 bytes * arr.len. Another point is that: Even though we could change the arr.len here to the one based on f32 (e.g., arr.len / 2), it is still UB with accessing misaligned memory.

To reproduce the bug

fn main() {
    let mut arr = [1.5_f32; 2];
    let sa_A = Array::from(&arr);
    let f64_A: &mut [f64] = sa_A.into();
    println!("{:?}", f64_A);
}
urschrei commented 1 year ago

Thanks!

shinmao commented 1 year ago

Would you mind report to RUSTSEC advisories so that user can update via cargo audit? Or I can help report it?

urschrei commented 1 year ago

I had no idea that unsoundness rose to the level of security advisories, but by all means…

shinmao commented 1 year ago

Yes. UB means that we can't guarantee what would happen on any target at anytime. UB should not happen with safe function. In this case, uninitialized memory read might also cause to security issues.