From e7d13ed7d18aaf39d13a3200904816bc7a5327be Mon Sep 17 00:00:00 2001 From: Eric Kidd Date: Mon, 1 Jan 2018 15:23:06 -0500 Subject: [PATCH] Don't allow deserializing names which shouldn't exist This addresses the potential security issue discussed in the last patch by implementing custom versions of `deserialize` that check for objects corresponding to each name. --- src/elements/index_map.rs | 96 ++++++++++++++++++------- src/elements/name_section.rs | 136 ++++++++++++++++++++--------------- 2 files changed, 147 insertions(+), 85 deletions(-) diff --git a/src/elements/index_map.rs b/src/elements/index_map.rs index a4bbebd..dda36db 100644 --- a/src/elements/index_map.rs +++ b/src/elements/index_map.rs @@ -128,6 +128,49 @@ impl IndexMap { // Note that this does the right thing because we use `&self`. self.into_iter() } + + /// Custom deserialization routine. + /// + /// We will allocate an underlying array no larger than `max_entry_space` to + /// hold the data, so the maximum index must be less than `max_entry_space`. + /// This prevents mallicious *.wasm files from having a single entry with + /// the index `u32::MAX`, which would consume far too much memory. + /// + /// The `deserialize_value` function will be passed the index of the value + /// being deserialized, and must deserialize the value. + pub fn deserialize_with( + max_entry_space: usize, + deserialize_value: &F, + rdr: &mut R, + ) -> Result, Error> + where + R: Read, + F: Fn(u32, &mut R) -> Result, + { + let len: u32 = VarUint32::deserialize(rdr)?.into(); + let mut map = IndexMap::with_capacity(len as usize); + let mut prev_idx = None; + for _ in 0..len { + let idx: u32 = VarUint32::deserialize(rdr)?.into(); + if idx as usize >= max_entry_space { + return Err(Error::Other("index is larger than expected")); + } + match prev_idx { + Some(prev) if prev >= idx => { + // Supposedly these names must be "sorted by index", so + // let's try enforcing that and seeing what happens. + return Err(Error::Other("indices are out of order")); + } + _ => { + prev_idx = Some(idx); + } + } + let val = deserialize_value(idx, rdr)?; + map.insert(idx, val); + } + Ok(map) + } + } impl Clone for IndexMap { @@ -286,34 +329,22 @@ where } } -impl Deserialize for IndexMap +impl IndexMap where T: Deserialize, Error: From<::Error>, { - type Error = Error; - - fn deserialize(rdr: &mut R) -> Result { - // SECURITY: Implemented wasted space checking. - let len: u32 = VarUint32::deserialize(rdr)?.into(); - let mut map = IndexMap::with_capacity(len as usize); - let mut prev_idx = None; - for _ in 0..len { - let idx: u32 = VarUint32::deserialize(rdr)?.into(); - match prev_idx { - Some(prev) if prev >= idx => { - // Supposedly these names must be "sorted by index", so - // let's try enforcing that and seeing what happens. - return Err(Error::Other("indices are out of order")); - } - _ => { - prev_idx = Some(idx); - } - } - let val = T::deserialize(rdr)?; - map.insert(idx, val); - } - Ok(map) + /// Deserialize a map containing simple values that support `Deserialize`. + /// We will allocate an underlying array no larger than `max_entry_space` to + /// hold the data, so the maximum index must be less than `max_entry_space`. + pub fn deserialize( + max_entry_space: usize, + rdr: &mut R, + ) -> Result { + let deserialize_value: fn(u32, &mut R) -> Result = |_idx, rdr| { + T::deserialize(rdr).map_err(Error::from) + }; + Self::deserialize_with(max_entry_space, &deserialize_value, rdr) } } @@ -513,7 +544,7 @@ mod tests { .expect("serialize failed"); let mut input = io::Cursor::new(&output); - let deserialized = IndexMap::deserialize(&mut input).expect("deserialize failed"); + let deserialized = IndexMap::deserialize(2, &mut input).expect("deserialize failed"); assert_eq!(deserialized, map); } @@ -527,7 +558,7 @@ mod tests { "val 0".to_string().serialize(&mut valid).unwrap(); VarUint32::from(1u32).serialize(&mut valid).unwrap(); "val 1".to_string().serialize(&mut valid).unwrap(); - let map = IndexMap::::deserialize(&mut io::Cursor::new(valid)) + let map = IndexMap::::deserialize(2, &mut io::Cursor::new(valid)) .expect("unexpected error deserializing"); assert_eq!(map.len(), 2); @@ -538,7 +569,18 @@ mod tests { "val 1".to_string().serialize(&mut invalid).unwrap(); VarUint32::from(0u32).serialize(&mut invalid).unwrap(); "val 0".to_string().serialize(&mut invalid).unwrap(); - let res = IndexMap::::deserialize(&mut io::Cursor::new(invalid)); + let res = IndexMap::::deserialize(2, &mut io::Cursor::new(invalid)); + assert!(res.is_err()); + } + + #[test] + fn deserialize_enforces_max_idx() { + // Build an example with an out-of-bounds index by hand. + let mut invalid = vec![]; + VarUint32::from(1u32).serialize(&mut invalid).unwrap(); + VarUint32::from(5u32).serialize(&mut invalid).unwrap(); + "val 5".to_string().serialize(&mut invalid).unwrap(); + let res = IndexMap::::deserialize(1, &mut io::Cursor::new(invalid)); assert!(res.is_err()); } } diff --git a/src/elements/name_section.rs b/src/elements/name_section.rs index d1e5b34..1ee2fe9 100644 --- a/src/elements/name_section.rs +++ b/src/elements/name_section.rs @@ -1,6 +1,6 @@ use std::io::{Read, Write}; -use super::{Deserialize, Error, Serialize, VarUint32, VarUint7}; +use super::{Deserialize, Error, Module, Serialize, VarUint32, VarUint7}; use super::index_map::IndexMap; const NAME_TYPE_MODULE: u8 = 0; @@ -28,6 +28,31 @@ pub enum NameSection { }, } +impl NameSection { + /// Deserialize a name section. + pub fn deserialize( + module: &Module, + rdr: &mut R, + ) -> Result { + let name_type: u8 = VarUint7::deserialize(rdr)?.into(); + let name_payload_len: u32 = VarUint32::deserialize(rdr)?.into(); + let name_section = match name_type { + NAME_TYPE_MODULE => NameSection::Module(ModuleNameSection::deserialize(rdr)?), + NAME_TYPE_FUNCTION => NameSection::Function(FunctionNameSection::deserialize(module, rdr)?), + NAME_TYPE_LOCAL => NameSection::Local(LocalNameSection::deserialize(module, rdr)?), + _ => { + let mut name_payload = vec![0u8; name_payload_len as usize]; + rdr.read_exact(&mut name_payload)?; + NameSection::Unparsed { + name_type, + name_payload, + } + } + }; + Ok(name_section) + } +} + impl Serialize for NameSection { type Error = Error; @@ -60,29 +85,6 @@ impl Serialize for NameSection { } } -impl Deserialize for NameSection { - type Error = Error; - - fn deserialize(rdr: &mut R) -> Result { - let name_type: u8 = VarUint7::deserialize(rdr)?.into(); - let name_payload_len: u32 = VarUint32::deserialize(rdr)?.into(); - let name_section = match name_type { - NAME_TYPE_MODULE => NameSection::Module(ModuleNameSection::deserialize(rdr)?), - NAME_TYPE_FUNCTION => NameSection::Function(FunctionNameSection::deserialize(rdr)?), - NAME_TYPE_LOCAL => NameSection::Local(LocalNameSection::deserialize(rdr)?), - _ => { - let mut name_payload = vec![0u8; name_payload_len as usize]; - rdr.read_exact(&mut name_payload)?; - NameSection::Unparsed { - name_type, - name_payload, - } - } - }; - Ok(name_section) - } -} - /// The name of this module. #[derive(Clone, Debug, PartialEq)] pub struct ModuleNameSection { @@ -139,6 +141,19 @@ impl FunctionNameSection { pub fn names_mut(&mut self) -> &mut NameMap { &mut self.names } + + /// Deserialize names, making sure that all names correspond to functions. + pub fn deserialize( + module: &Module, + rdr: &mut R, + ) -> Result { + let funcs = module.function_section().ok_or_else(|| { + Error::Other("cannot deserialize names without a function section") + })?; + let max_entry_space = funcs.entries().len(); + let names = IndexMap::deserialize(max_entry_space, rdr)?; + Ok(FunctionNameSection { names }) + } } impl Serialize for FunctionNameSection { @@ -149,15 +164,6 @@ impl Serialize for FunctionNameSection { } } -impl Deserialize for FunctionNameSection { - type Error = Error; - - fn deserialize(rdr: &mut R) -> Result { - let names = IndexMap::deserialize(rdr)?; - Ok(FunctionNameSection { names }) - } -} - /// The names of the local variables in this module's functions. #[derive(Clone, Debug, Default, PartialEq)] pub struct LocalNameSection { @@ -175,7 +181,33 @@ impl LocalNameSection { pub fn local_names_mut(&mut self) -> &mut IndexMap { &mut self.local_names } -} + + /// Deserialize names, making sure that all names correspond to local + /// variables. + pub fn deserialize( + module: &Module, + rdr: &mut R, + ) -> Result { + let funcs = module.function_section().ok_or_else(|| { + Error::Other("cannot deserialize local names without a function section") + })?; + let max_entry_space = funcs.entries().len(); + + let code = module.code_section().ok_or_else(|| { + Error::Other("cannot deserialize local names without a code section") + })?; + let deserialize_locals = |idx: u32, rdr: &mut R| { + let max_local_entry_space = code.bodies()[idx as usize].locals().len(); + IndexMap::deserialize(max_local_entry_space, rdr) + }; + + let local_names = IndexMap::deserialize_with( + max_entry_space, + &deserialize_locals, + rdr, + )?; + Ok(LocalNameSection { local_names }) + }} impl Serialize for LocalNameSection { type Error = Error; @@ -185,15 +217,6 @@ impl Serialize for LocalNameSection { } } -impl Deserialize for LocalNameSection { - type Error = Error; - - fn deserialize(rdr: &mut R) -> Result { - let local_names = IndexMap::deserialize(rdr)?; - Ok(LocalNameSection { local_names }) - } -} - /// A map from indices to names. pub type NameMap = IndexMap; @@ -205,48 +228,45 @@ mod tests { // A helper funtion for the tests. Serialize a section, deserialize it, // and make sure it matches the original. - fn serialize_and_deserialize(original: NameSection) { + fn serialize_test(original: NameSection) -> Vec { let mut buffer = vec![]; original - .clone() .serialize(&mut buffer) .expect("serialize error"); - let mut input = Cursor::new(buffer); - let deserialized = NameSection::deserialize(&mut input).expect("deserialize error"); - assert_eq!(deserialized, original); + buffer } #[test] - fn serialize_and_deserialize_module_name() { - let sect = ModuleNameSection::new("my_mod"); - serialize_and_deserialize(NameSection::Module(sect)); + fn serialize_module_name() { + let original = NameSection::Module(ModuleNameSection::new("my_mod")); + serialize_test(original.clone()); } #[test] - fn serialize_and_deserialize_function_names() { + fn serialize_function_names() { let mut sect = FunctionNameSection::default(); sect.names_mut().insert(0, "hello_world".to_string()); - serialize_and_deserialize(NameSection::Function(sect)); + serialize_test(NameSection::Function(sect)); } #[test] - fn serialize_and_deserialize_local_names() { + fn serialize_local_names() { let mut sect = LocalNameSection::default(); let mut locals = NameMap::default(); locals.insert(0, "msg".to_string()); sect.local_names_mut().insert(0, locals); - serialize_and_deserialize(NameSection::Local(sect)); + serialize_test(NameSection::Local(sect)); } #[test] fn serialize_and_deserialize_unparsed() { - let sect = NameSection::Unparsed { + let original = NameSection::Unparsed { // A made-up name section type which is unlikely to be allocated // soon, in order to allow us to test `Unparsed`. name_type: 120, name_payload: vec![0u8, 1, 2], }; - serialize_and_deserialize(sect); + serialize_test(original.clone()); } #[test] @@ -261,7 +281,7 @@ mod tests { match *s { Section::Custom(ref cs) if cs.name() == "name" => { let mut cursor = Cursor::new(cs.payload()); - let section = NameSection::deserialize(&mut cursor) + let section = NameSection::deserialize(&module, &mut cursor) .expect("could not parse name section"); if let NameSection::Function(function_names) = section { function_names_opt = Some(function_names);