Rust: various unsafe traits/functions meant for gencode in public API

[TethysSvensson]

The following safe code will segfault:

fn main() {
    let ptr: &&u8 = flatbuffers::follow_cast_ref(&[1, 2, 3, 4, 5, 6, 7, 8], 0);
    println!("{}", **ptr);
}

[CasperN]

Thanks for documenting these. I'm going to close the other issues and combine them with this one:

• Rust: The impl of Follow<'a> for &'a str is unsound #6628 - Follow<'a> for &'a str
• Rust: Unsound creation of unaligned slices #6629 - Follow for slices
• Rust: SafeSliceAccess should be an unsafe trait #6630 - SafeSliceAccess should be unsafe.
• Rust: emplace_scalar_array should probably be unsafe #6631 - emplace_scalar_array
• Rust: The Push impls for integers are unsound #6632 - Push for scalars

For most of these, since these are typically used within the Flatbuffers library functions or generated code and I think the usual usage is safe, I'm not going to make fixing this the highest priority. The safety model we're going for is that flatbuffers are constructed with a FlatBufferBuilder and read from the root_* variants -- those APIs must be safe. We could mark the above types/traits them as unsafe to be rigorous, and/or move them to a module like flatbuffers::for_gencode_only which will make their internal use more obvious (though obviously that'd still be sub optimal).

I do want to deprecate SafeSliceAccess, though. That one was an especially bad idea, and getting unaligned access is in the public API (sigh).

[CasperN]

Let's try to fix this for the 3.0 release #6636

[jorgecarleitao]

This is quite serious:

namespace MyGame.Sample;

enum Color:byte { Red = 0, Green, Blue = 2 }

is compiled to

// automatically generated by the FlatBuffers compiler, do not modify



use std::mem;
use std::cmp::Ordering;

extern crate flatbuffers;
use self::flatbuffers::{EndianScalar, Follow};

#[allow(unused_imports, dead_code)]
pub mod my_game {

  use std::mem;
  use std::cmp::Ordering;

  extern crate flatbuffers;
  use self::flatbuffers::{EndianScalar, Follow};
#[allow(unused_imports, dead_code)]
pub mod sample {

  use std::mem;
  use std::cmp::Ordering;

  extern crate flatbuffers;
  use self::flatbuffers::{EndianScalar, Follow};

#[deprecated(since = "2.0.0", note = "Use associated constants instead. This will no longer be generated in 2021.")]
pub const ENUM_MIN_COLOR: i8 = 0;
#[deprecated(since = "2.0.0", note = "Use associated constants instead. This will no longer be generated in 2021.")]
pub const ENUM_MAX_COLOR: i8 = 2;
#[deprecated(since = "2.0.0", note = "Use associated constants instead. This will no longer be generated in 2021.")]
#[allow(non_camel_case_types)]
pub const ENUM_VALUES_COLOR: [Color; 3] = [
  Color::Red,
  Color::Green,
  Color::Blue,
];

#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Default)]
#[repr(transparent)]
pub struct Color(pub i8);
#[allow(non_upper_case_globals)]
impl Color {
  pub const Red: Self = Self(0);
  pub const Green: Self = Self(1);
  pub const Blue: Self = Self(2);

  pub const ENUM_MIN: i8 = 0;
  pub const ENUM_MAX: i8 = 2;
  pub const ENUM_VALUES: &'static [Self] = &[
    Self::Red,
    Self::Green,
    Self::Blue,
  ];
  /// Returns the variant's name or "" if unknown.
  pub fn variant_name(self) -> Option<&'static str> {
    match self {
      Self::Red => Some("Red"),
      Self::Green => Some("Green"),
      Self::Blue => Some("Blue"),
      _ => None,
    }
  }
}
// ...
impl<'a> flatbuffers::Follow<'a> for Color {
  type Inner = Self;
  #[inline]
  fn follow(buf: &'a [u8], loc: usize) -> Self::Inner {
    let b = unsafe {
      flatbuffers::read_scalar_at::<i8>(buf, loc)
    };
    Self(b)
  }
}

impl flatbuffers::Push for Color {
    type Output = Color;
    #[inline]
    fn push(&self, dst: &mut [u8], _rest: &[u8]) {
        unsafe { flatbuffers::emplace_scalar::<i8>(dst, self.0); }
    }
}
// ...

which allows both read and write out of bounds in safe code. Specifically, the following
tests do not pass miri:

mod generated;

#[cfg(test)]
mod test {
    use super::*;

    use flatbuffers::Follow;
    use flatbuffers::Push;

    #[test]
    fn write() {
        let color = generated::Color::Blue;
        color.push(&mut [], &[]);
    }

    #[test]
    fn read() {
        generated::Color::follow(&[1], 1);
    }
}

and constitute CWE-787 and CWE-125 respectively, ranking 1st and 3rd in the most dangerous vulnerabilities at https://cwe.mitre.org/top25/archive/2021/2021_cwe_top25.html.

[CasperN]

Yep a bunch of the traits, especially Follow, are unsafe and should be refactored as such. Using it outside the generated code is not intended and a great way of shooting yourself in the foot. @aardappel, I'm not going to be available to contribute for a while, but marking Follow and friends stuff unsafe is an easy first contribution if you find anyone willing.

[aardappel]

Not sure what rustaceans are around to take this on..
@rw