Browse Source

fix lladdr parse panic

The `RawHardwareAddress::parse()` method panics if the length of the
address is invalid. More specific, for an Ethernet address, the length
should exactly be 6 bytes, and for an IEEE 802.15.4 address, the length
should exactly be 8 bytes. Previously, we only checked if the size of
the input was at least the size of the link layer address. Since
`Ethernet::from_bytes()` does a copy_from_slice, the length of the input
should be exactly 6 bytes. A panic can be triggered when the length is
for example 7 bytes, while the `medium-ethernet` and `medium-ieee802154`
features are enabled. This commit fixes the panic by checking if the
length of the input is exactly the size of the link layer address.

This panic was discovered by people from Radically Open Security.

Signed-off-by: Thibaut Vandervelden <thvdveld@vub.be>
Thibaut Vandervelden 5 months ago
parent
commit
31fda8f7cd
1 changed files with 40 additions and 2 deletions
  1. 40 2
      src/wire/mod.rs

+ 40 - 2
src/wire/mod.rs

@@ -478,6 +478,10 @@ pub struct RawHardwareAddress {
 
 
 #[cfg(any(feature = "medium-ethernet", feature = "medium-ieee802154"))]
 #[cfg(any(feature = "medium-ethernet", feature = "medium-ieee802154"))]
 impl RawHardwareAddress {
 impl RawHardwareAddress {
+    /// Create a new `RawHardwareAddress` from a byte slice.
+    ///
+    /// # Panics
+    /// Panics if `addr.len() > MAX_HARDWARE_ADDRESS_LEN`.
     pub fn from_bytes(addr: &[u8]) -> Self {
     pub fn from_bytes(addr: &[u8]) -> Self {
         let mut data = [0u8; MAX_HARDWARE_ADDRESS_LEN];
         let mut data = [0u8; MAX_HARDWARE_ADDRESS_LEN];
         data[..addr.len()].copy_from_slice(addr);
         data[..addr.len()].copy_from_slice(addr);
@@ -504,7 +508,7 @@ impl RawHardwareAddress {
         match medium {
         match medium {
             #[cfg(feature = "medium-ethernet")]
             #[cfg(feature = "medium-ethernet")]
             Medium::Ethernet => {
             Medium::Ethernet => {
-                if self.len() < 6 {
+                if self.len() != 6 {
                     return Err(Error);
                     return Err(Error);
                 }
                 }
                 Ok(HardwareAddress::Ethernet(EthernetAddress::from_bytes(
                 Ok(HardwareAddress::Ethernet(EthernetAddress::from_bytes(
@@ -513,7 +517,7 @@ impl RawHardwareAddress {
             }
             }
             #[cfg(feature = "medium-ieee802154")]
             #[cfg(feature = "medium-ieee802154")]
             Medium::Ieee802154 => {
             Medium::Ieee802154 => {
-                if self.len() < 8 {
+                if self.len() != 8 {
                     return Err(Error);
                     return Err(Error);
                 }
                 }
                 Ok(HardwareAddress::Ieee802154(Ieee802154Address::from_bytes(
                 Ok(HardwareAddress::Ieee802154(Ieee802154Address::from_bytes(
@@ -559,3 +563,37 @@ impl From<HardwareAddress> for RawHardwareAddress {
         Self::from_bytes(addr.as_bytes())
         Self::from_bytes(addr.as_bytes())
     }
     }
 }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use rstest::rstest;
+
+    #[rstest]
+    #[cfg(feature = "medium-ethernet")]
+    #[case((Medium::Ethernet, &[0u8; 6][..]), Ok(HardwareAddress::Ethernet(EthernetAddress([0, 0, 0, 0, 0, 0]))))]
+    #[cfg(feature = "medium-ethernet")]
+    #[case((Medium::Ethernet, &[1u8; 5][..]), Err(Error))]
+    #[cfg(feature = "medium-ethernet")]
+    #[case((Medium::Ethernet, &[1u8; 7][..]), Err(Error))]
+    #[cfg(feature = "medium-ieee802154")]
+    #[case((Medium::Ieee802154, &[0u8; 8][..]), Ok(HardwareAddress::Ieee802154(Ieee802154Address::Extended([0, 0, 0, 0, 0, 0, 0, 0]))))]
+    #[cfg(feature = "medium-ieee802154")]
+    #[case((Medium::Ieee802154, &[1u8; 2][..]), Err(Error))]
+    #[cfg(feature = "medium-ieee802154")]
+    #[case((Medium::Ieee802154, &[1u8; 1][..]), Err(Error))]
+    fn parse_hardware_address(
+        #[case] input: (Medium, &[u8]),
+        #[case] expected: Result<HardwareAddress>,
+    ) {
+        let (medium, input) = input;
+
+        // NOTE: we check the length since `RawHardwareAddress::parse()` panics if the length is
+        // invalid. MAX_HARDWARE_ADDRESS_LEN is based on the medium, and depending on the feature
+        // flags, it can be different.
+        if input.len() < MAX_HARDWARE_ADDRESS_LEN {
+            let raw = RawHardwareAddress::from_bytes(input);
+            assert_eq!(raw.parse(medium), expected);
+        }
+    }
+}