Parsers allocate based on a length field, not the number of elements that exist

[saethlin]

I've been fuzzing an internal project that uses the parsers in this crate. The fuzzer has found a number of crashes related to using a 32-bit value as the length for a nom::multi::count call. The crash occurs because nom attempts to pre-allocate for billions of elements. Even if the memory is never touched, at some size memory allocators do return a null pointer, and the Rust OOM handler aborts the program. I can provide example packets if you want/need.

I've been able to locally paper over this issue by applying a few diffs like this:

diff --git a/src/ospfv2.rs b/src/ospfv2.rs
index 7e591d1..cdc7418 100644
--- a/src/ospfv2.rs
+++ b/src/ospfv2.rs
@@ -185,6 +185,7 @@ pub struct OspfLinkStateUpdatePacket {
     #[nom(Verify = "header.packet_type == OspfPacketType::LinkStateUpdate")]
     pub header: Ospfv2PacketHeader,
     pub num_advertisements: u32,
+    #[nom(ErrorIf(num_advertisements as usize > i.len()))]
     #[nom(Count = "num_advertisements")]
     pub lsa: Vec<OspfLinkStateAdvertisement>,
 }

...but I strongly suspect I've only had to apply 3 so far because my initial corpus does not contain any actual OSPF packets, and given enough time a fuzzer will find a crash at every place this pattern occurs in the code.

This seems like it might be of interest for a security-minded parser because this a DoS vector. If it is, do you have any thoughts on a more complete solution to this? I'm not excited about the idea of sprinkling ErrorIfs all over this codebase.

[chifflier]

Hi,
Indeed, I also hit that problem in another parsing crate (also found by fuzzing). The count combinator allocates data (it calls Vec::with_capacity), which makes it faster for allocation, but fragile wrt large number of items. In that case, the parser was directly using nom, so I solved it by writing a loop pushing items.

At this point, I'm not sure of what is the best solution.
Some thoughts:

• I'd prefer to fix it in the nom-derive crate, so all using crates would be fixed at once.
• checking size: complex (if even possible), items may have variable length
• write a non-allocating count combinator and use it in nom-derive: would do the job, but hard to do without adding a new crate to do the function and add a dependency for nom-derived crates
• replace count with fold_many_m_n to do the same: possible, but fold_many_m_n requires the item to have Clone
• replace generated code with a loop: may not be easy in the current implementation of nom-derive

I need to think more about possible solutions, but I'm sure it's possible to fix it in nom-derive.