Enforce memory safety of the Cmp/Immediate deserialization step by limiting such operations to byte arrays

2 files changed, 114 insertions, 64 deletions

diff --git a/rustables/src/expr/cmp.rs b/rustables/src/expr/cmp.rs
index e4cfb0f..11825d6 100644
--- a/rustables/src/expr/cmp.rs
+++ b/rustables/src/expr/cmp.rs
@@ -55,7 +55,7 @@ impl CmpOp {
 
 /// Comparator expression. Allows comparing the content of the netfilter register with any value.
 #[derive(Debug, PartialEq)]
-pub struct Cmp<T: ToSlice> {
+pub struct Cmp<T> {
     op: CmpOp,
     data: T,
 }
@@ -68,42 +68,11 @@ impl<T: ToSlice> Cmp<T> {
     }
 }
 
-impl<T: ToSlice + Copy> Expression for Cmp<T> {
+impl<T: ToSlice> Expression for Cmp<T> {
     fn get_raw_name() -> *const c_char {
         b"cmp\0" as *const _ as *const c_char
     }
 
-    fn from_expr(expr: *const sys::nftnl_expr) -> Option<Self>
-    where
-        Self: Sized,
-    {
-        unsafe {
-            let ref_len = std::mem::size_of::<T>() as u32;
-            let mut data_len = 0;
-            let data = sys::nftnl_expr_get(
-                expr,
-                sys::NFTNL_EXPR_CMP_DATA as u16,
-                &mut data_len as *mut u32,
-            );
-
-            if data.is_null() {
-                return None;
-            } else if data_len != ref_len {
-                debug!("Invalid size requested for deserializing a 'cmp' expression: expected {} bytes, got {}", ref_len, data_len);
-                return None;
-            }
-
-            // Warning: this is *very* dangerous safety wise if the user supply us with
-            // a type that have the same size as T but a different memory layout.
-            // Is there a better way? And if there isn't, shouldn't we gate this behind
-            // an "unsafe" boundary?
-            let data = *(data as *const T);
-
-            let op = CmpOp::from_raw(sys::nftnl_expr_get_u32(expr, sys::NFTNL_EXPR_CMP_OP as u16));
-            op.map(|op| Cmp { op, data })
-        }
-    }
-
     fn to_expr(&self, _rule: &Rule) -> *mut sys::nftnl_expr {
         unsafe {
             let expr = try_alloc!(sys::nftnl_expr_alloc(Self::get_raw_name()));
@@ -129,6 +98,49 @@ impl<T: ToSlice + Copy> Expression for Cmp<T> {
     }
 }
 
+impl<const N: usize> Expression for Cmp<[u8; N]> {
+    fn get_raw_name() -> *const c_char {
+        Cmp::<u8>::get_raw_name()
+    }
+
+    // As casting bytes to any type of the same size as the input would
+    // be *extremely* dangerous in terms of memory safety,
+    // rustables only accept to deserialize expressions with variable-size data
+    // to arrays of bytes, so that the memory layout cannot be invalid.
+    fn from_expr(expr: *const sys::nftnl_expr) -> Option<Self> {
+        unsafe {
+            let ref_len = std::mem::size_of::<[u8; N]>() as u32;
+            let mut data_len = 0;
+            let data = sys::nftnl_expr_get(
+                expr,
+                sys::NFTNL_EXPR_CMP_DATA as u16,
+                &mut data_len as *mut u32,
+            );
+
+            if data.is_null() {
+                return None;
+            } else if data_len != ref_len {
+                debug!("Invalid size requested for deserializing a 'cmp' expression: expected {} bytes, got {}", ref_len, data_len);
+                return None;
+            }
+
+            let data = *(data as *const [u8; N]);
+
+            let op = CmpOp::from_raw(sys::nftnl_expr_get_u32(expr, sys::NFTNL_EXPR_CMP_OP as u16));
+            op.map(|op| Cmp { op, data })
+        }
+    }
+
+    // call to the other implementation to generate the expression
+    fn to_expr(&self, rule: &Rule) -> *mut sys::nftnl_expr {
+        Cmp {
+            data: self.data.as_ref(),
+            op: self.op,
+        }
+        .to_expr(rule)
+    }
+}
+
 #[macro_export(local_inner_macros)]
 macro_rules! nft_expr_cmp {
     (@cmp_op ==) => {
@@ -160,12 +172,6 @@ pub trait ToSlice {
     fn to_slice(&self) -> Cow<'_, [u8]>;
 }
 
-impl<'a> ToSlice for [u8; 0] {
-    fn to_slice(&self) -> Cow<'_, [u8]> {
-        Cow::Borrowed(&[])
-    }
-}
-
 impl<'a> ToSlice for &'a [u8] {
     fn to_slice(&self) -> Cow<'_, [u8]> {
         Cow::Borrowed(self)
diff --git a/rustables/src/expr/immediate.rs b/rustables/src/expr/immediate.rs
index e8823b1..0e26bb4 100644
--- a/rustables/src/expr/immediate.rs
+++ b/rustables/src/expr/immediate.rs
@@ -1,4 +1,4 @@
-use super::{Expression, Register, Rule};
+use super::{Expression, Register, Rule, ToSlice};
 use rustables_sys as sys;
 use std::ffi::c_void;
 use std::mem::size_of_val;
@@ -18,18 +18,45 @@ impl<T> Immediate<T> {
     }
 }
 
-// The Copy requirement is present to allow us to dereference the newly created raw pointer in `from_expr`
-impl<T: Copy> Expression for Immediate<T> {
+impl<T: ToSlice> Expression for Immediate<T> {
     fn get_raw_name() -> *const c_char {
         b"immediate\0" as *const _ as *const c_char
     }
 
-    fn from_expr(expr: *const sys::nftnl_expr) -> Option<Self>
-    where
-        Self: Sized,
-    {
+    fn to_expr(&self, _rule: &Rule) -> *mut sys::nftnl_expr {
         unsafe {
-            let ref_len = std::mem::size_of::<T>() as u32;
+            let expr = try_alloc!(sys::nftnl_expr_alloc(Self::get_raw_name()));
+
+            sys::nftnl_expr_set_u32(
+                expr,
+                sys::NFTNL_EXPR_IMM_DREG as u16,
+                self.register.to_raw(),
+            );
+
+            sys::nftnl_expr_set(
+                expr,
+                sys::NFTNL_EXPR_IMM_DATA as u16,
+                &self.data.to_slice() as *const _ as *const c_void,
+                size_of_val(&self.data) as u32,
+            );
+
+            expr
+        }
+    }
+}
+
+impl<const N: usize> Expression for Immediate<[u8; N]> {
+    fn get_raw_name() -> *const c_char {
+        Immediate::<u8>::get_raw_name()
+    }
+
+    // As casting bytes to any type of the same size as the input would
+    // be *extremely* dangerous in terms of memory safety,
+    // rustables only accept to deserialize expressions with variable-size data
+    // to arrays of bytes, so that the memory layout cannot be invalid.
+    fn from_expr(expr: *const sys::nftnl_expr) -> Option<Self> {
+        unsafe {
+            let ref_len = std::mem::size_of::<[u8; N]>() as u32;
             let mut data_len = 0;
             let data = sys::nftnl_expr_get(
                 expr,
@@ -44,11 +71,7 @@ impl<T: Copy> Expression for Immediate<T> {
                 return None;
             }
 
-            // Warning: this is *very* dangerous safety wise if the user supply us with
-            // a type that have the same size as T but a different memory layout.
-            // Is there a better way? And if there isn't, shouldn't we gate this behind
-            // an "unsafe" boundary?
-            let data = *(data as *const T);
+            let data = *(data as *const [u8; N]);
 
             let register = Register::from_raw(sys::nftnl_expr_get_u32(
                 expr,
@@ -59,24 +82,45 @@ impl<T: Copy> Expression for Immediate<T> {
         }
     }
 
-    fn to_expr(&self, _rule: &Rule) -> *mut sys::nftnl_expr {
+    // call to the other implementation to generate the expression
+    fn to_expr(&self, rule: &Rule) -> *mut sys::nftnl_expr {
+        Immediate {
+            register: self.register,
+            data: self.data.as_ref(),
+        }
+        .to_expr(rule)
+    }
+}
+// As casting bytes to any type of the same size as the input would
+// be *extremely* dangerous in terms of memory safety,
+// rustables only accept to deserialize expressions with variable-size data
+// to arrays of bytes, so that the memory layout cannot be invalid.
+impl<const N: usize> Immediate<[u8; N]> {
+    pub fn from_expr(expr: *const sys::nftnl_expr) -> Option<Self> {
         unsafe {
-            let expr = try_alloc!(sys::nftnl_expr_alloc(Self::get_raw_name()));
-
-            sys::nftnl_expr_set_u32(
+            let ref_len = std::mem::size_of::<[u8; N]>() as u32;
+            let mut data_len = 0;
+            let data = sys::nftnl_expr_get(
                 expr,
-                sys::NFTNL_EXPR_IMM_DREG as u16,
-                self.register.to_raw(),
+                sys::NFTNL_EXPR_IMM_DATA as u16,
+                &mut data_len as *mut u32,
             );
 
-            sys::nftnl_expr_set(
+            if data.is_null() {
+                return None;
+            } else if data_len != ref_len {
+                debug!("Invalid size requested for deserializing an 'immediate' expression: expected {} bytes, got {}", ref_len, data_len);
+                return None;
+            }
+
+            let data = *(data as *const [u8; N]);
+
+            let register = Register::from_raw(sys::nftnl_expr_get_u32(
                 expr,
-                sys::NFTNL_EXPR_IMM_DATA as u16,
-                &self.data as *const _ as *const c_void,
-                size_of_val(&self.data) as u32,
-            );
+                sys::NFTNL_EXPR_IMM_DREG as u16,
+            ));
 
-            expr
+            register.map(|register| Immediate { data, register })
         }
     }
 }