Merge pull request #154 from NLnetLabs/check-roa-address-bounds

Check prefix, address, and max lengths while decoding.

src/repository/resources/ipres.rs

@@ -92,13 +92,13 @@ impl IpResources {
                         if v4.is_some() {
                             xerr!(return Err(decode::Malformed.into()));
                         }
-                        v4 = Some(Self::take_from(cons)?);
+                        v4 = Some(Self::take_from(cons, AddressFamily::Ipv4)?);
                     }
                     AddressFamily::Ipv6 => {
                         if v6.is_some() {
                             xerr!(return Err(decode::Malformed.into()));
                         }
-                        v6 = Some(Self::take_from(cons)?);
+                        v6 = Some(Self::take_from(cons, AddressFamily::Ipv6)?);
                     }
                 }
                 Ok(())
@@ -112,15 +112,16 @@ impl IpResources {
 
     /// Takes a single set of  IP resources from a constructed value.
     pub fn take_from<S: decode::Source>(
-        cons: &mut decode::Constructed<S>
+        cons: &mut decode::Constructed<S>,
+        family: AddressFamily,
     ) -> Result<Self, S::Err> {
         cons.take_value(|tag, content| {
             if tag == Tag::NULL {
                 content.to_null()?;
                 Ok(ResourcesChoice::Inherit)
             }
             else if tag == Tag::SEQUENCE {
-                IpBlocks::parse_content(content)
+                IpBlocks::parse_content(content, family)
                     .map(ResourcesChoice::Blocks)
             }
             else {
@@ -448,24 +449,39 @@ impl IpBlocks {
     pub fn take_from<S: decode::Source>(
         cons: &mut decode::Constructed<S>
     ) -> Result<Self, S::Err> {
-        cons.take_sequence(Self::parse_cons_content)
+        cons.take_sequence(|cons| {
+            // The family is here only for checking the lengths of
+            // bitstrings. Since we don’t care, IPv6 will work.
+            Self::parse_cons_content(cons, AddressFamily::Ipv6)
+        })
+    }
+
+    pub fn take_from_with_family<S: decode::Source>(
+        cons: &mut decode::Constructed<S>,
+        family: AddressFamily
+    ) -> Result<Self, S::Err> {
+        cons.take_sequence(|cons| {
+            Self::parse_cons_content(cons, family)
+        })
     }
 
     /// Parses the content of a AS ID blocks sequence.
     fn parse_content<S: decode::Source>(
-        content: &mut decode::Content<S>
+        content: &mut decode::Content<S>,
+        family: AddressFamily,
     ) -> Result<Self, S::Err> {
         let cons = content.as_constructed()?;
-        Self::parse_cons_content(cons)
+        Self::parse_cons_content(cons, family)
     }
 
     fn parse_cons_content<S: decode::Source>(
-        cons: &mut decode::Constructed<S>
+        cons: &mut decode::Constructed<S>,
+        family: AddressFamily,
     ) -> Result<Self, S::Err> {
         let mut err = None;
 
         let res = iter::repeat_with(||
-            IpBlock::take_opt_from(cons)
+            IpBlock::take_opt_from_with_family(cons, family)
         ).map(|item| {
             match item {
                 Ok(Some(val)) => Some(val),
@@ -706,6 +722,27 @@ impl IpBlock {
         })
     }
 
+    pub fn take_opt_from_with_family<S: decode::Source>(
+        cons: &mut decode::Constructed<S>,
+        family: AddressFamily,
+    ) -> Result<Option<Self>, S::Err> {
+        cons.take_opt_value(|tag, content| {
+            if tag == Tag::BIT_STRING {
+                Prefix::parse_content_with_family(
+                    content, family
+                ).map(IpBlock::Prefix)
+            }
+            else if tag == Tag::SEQUENCE {
+                AddressRange::parse_content_with_family(
+                    content, family
+                ).map(IpBlock::Range)
+            }
+            else {
+                xerr!(Err(decode::Malformed.into()))
+            }
+        })
+    }
+
     /// Returns an encoder for the range.
     ///
     /// This encoder will produce a `IPAddressOrRange` value.
@@ -995,6 +1032,24 @@ impl AddressRange {
         })
     }
 
+    fn parse_content_with_family<S: decode::Source>(
+        content: &mut decode::Content<S>,
+        family: AddressFamily,
+    ) -> Result<Self, S::Err> {
+        let mut cons = content.as_constructed()?;
+        let min = Prefix::take_from(&mut cons)?;
+        let max = Prefix::take_from(&mut cons)?;
+        if min.addr_len() > family.max_addr_len()
+            || max.addr_len() > family.max_addr_len()
+        {
+            return Err(decode::Malformed.into())
+        }
+        Ok(AddressRange {
+            min: min.min(),
+            max: max.max(),
+        })
+    }
+
     /*
     /// Skips over the address range at the beginning of a value.
     fn skip_opt_in<S: decode::Source>(
@@ -1244,6 +1299,19 @@ impl Prefix {
             &BitString::from_content(content)?
         )?)
     }
+
+    pub fn parse_content_with_family<S: decode::Source>(
+        content: &mut decode::Content<S>,
+        family: AddressFamily,
+    ) -> Result<Self, S::Err> {
+        let res = Self::from_bit_string(
+            &BitString::from_content(content)?
+        )?;
+        if res.addr_len() > family.max_addr_len() {
+            return Err(decode::Malformed.into())
+        }
+        Ok(res)
+    }
 }
 
 
@@ -1512,6 +1580,14 @@ impl AddressFamily {
             }
         )
     }
+
+    /// Returns the maximum prefix length for this family.
+    pub fn max_addr_len(self) -> u8 {
+        match self {
+            AddressFamily::Ipv4 => 32,
+            AddressFamily::Ipv6 => 128
+        }
+    }
 }
 
 

src/repository/roa.rs

@@ -145,13 +145,17 @@ impl RouteOriginAttestation {
                             if v4.is_some() {
                                 xerr!(return Err(decode::Malformed.into()));
                             }
-                            v4 = Some(RoaIpAddresses::take_from(cons)?);
+                            v4 = Some(RoaIpAddresses::take_from(
+                                cons, AddressFamily::Ipv4
+                            )?);
                         }
                         AddressFamily::Ipv6 => {
                             if v6.is_some() {
                                 xerr!(return Err(decode::Malformed.into()));
                             }
-                            v6 = Some(RoaIpAddresses::take_from(cons)?);
+                            v6 = Some(RoaIpAddresses::take_from(
+                                cons, AddressFamily::Ipv6
+                            )?);
                         }
                     }
                     Ok(())
@@ -222,11 +226,12 @@ pub struct RoaIpAddresses(Captured);
 
 impl RoaIpAddresses {
     fn take_from<S: decode::Source>(
-        cons: &mut decode::Constructed<S>
+        cons: &mut decode::Constructed<S>,
+        family: AddressFamily,
     ) -> Result<Self, S::Err> {
         cons.take_sequence(|cons| {
             cons.capture(|cons| {
-                while let Some(()) = RoaIpAddress::skip_opt_in(cons)? { }
+                while RoaIpAddress::skip_opt_in(cons, family)?.is_some() { }
                 Ok(())
             })
         }).map(RoaIpAddresses)
@@ -272,7 +277,7 @@ impl<'a> Iterator for RoaIpAddressIter<'a> {
         }
         else {
             Mode::Der.decode(&mut self.0, |cons| {
-                RoaIpAddress::take_opt_from(cons)
+                RoaIpAddress::take_opt_from_unchecked(cons)
             }).unwrap()
         }
     }
@@ -319,7 +324,7 @@ impl RoaIpAddress {
     // The address is the same as in section 2.1.1 of RFC 3779, that is, it
     // is a bit string with all the bits of the prefix.
 
-    fn take_opt_from<S: decode::Source>(
+    fn take_opt_from_unchecked<S: decode::Source>(
         cons: &mut decode::Constructed<S>
     ) -> Result<Option<Self>, S::Err> {
         cons.take_opt_sequence(|cons| {
@@ -330,10 +335,34 @@ impl RoaIpAddress {
         })
     }
 
+    /// Skips one address in a source.
+    ///
+    /// In order to check that the address is correctly formatted, this
+    /// function needs to know the address family of the address.
     fn skip_opt_in<S: decode::Source>(
-        cons: &mut decode::Constructed<S>
+        cons: &mut decode::Constructed<S>,
+        family: AddressFamily,
     ) -> Result<Option<()>, S::Err> {
-        Self::take_opt_from(cons).map(|res| res.map(|_| ()))
+        let addr = match Self::take_opt_from_unchecked(cons)? {
+            Some(addr) => addr,
+            None => return Ok(None)
+        };
+
+        // Check that the prefix length fits the address family.
+        if addr.prefix.addr_len() > family.max_addr_len() {
+            return Err(decode::Malformed.into())
+        }
+
+        // Check that a max length fits both family and prefix length.
+        if let Some(max_length) = addr.max_length {
+            if max_length > family.max_addr_len() 
+                || max_length < addr.prefix.addr_len()
+            {
+                return Err(decode::Malformed.into())
+            }
+        }
+
+        Ok(Some(()))
     }
 
     fn encode(&self) -> impl encode::Values {
@@ -590,6 +619,30 @@ mod test {
             ).is_ok()
         )
     }
+
+    #[test]
+    fn decode_illegal_roas() {
+        assert!(
+            Roa::decode(
+                include_bytes!(
+                    "../../test-data/prefix-len-overflow.roa"
+                ).as_ref(),
+                false
+            ).is_err()
+        );
+        assert!(
+            Roa::decode(
+                include_bytes!("../../test-data/maxlen-overflow.roa").as_ref(),
+                false
+            ).is_err()
+        );
+        assert!(
+            Roa::decode(
+                include_bytes!("../../test-data/maxlen-underflow.roa").as_ref(),
+                false
+            ).is_err()
+        );
+    }
 }
 
 #[cfg(all(test, feature="softkeys"))]

src/repository/rta.rs

@@ -183,13 +183,17 @@ impl ResourceTaggedAttestation {
                                 if v4.is_some() {
                                     xerr!(return Err(decode::Malformed.into()));
                                 }
-                                v4 = Some(IpBlocks::take_from(cons)?);
+                                v4 = Some(IpBlocks::take_from_with_family(
+                                    cons, AddressFamily::Ipv4
+                                )?);
                             }
                             AddressFamily::Ipv6 => {
                                 if v6.is_some() {
                                     xerr!(return Err(decode::Malformed.into()));
                                 }
-                                v6 = Some(IpBlocks::take_from(cons)?);
+                                v6 = Some(IpBlocks::take_from_with_family(
+                                    cons, AddressFamily::Ipv6
+                                )?);
                             }
                         }
                         Ok(())