Project

General

Profile

Actions

Bug #5347

closed

nom: use of count combinator can use too much memory (6.0.x backport)

Added by Jeff Lucovsky over 2 years ago. Updated about 1 year ago.

Status:
Rejected
Priority:
Normal
Assignee:
Target version:
Affected Versions:
Effort:
Difficulty:
Label:

Description

Philippe:
Nom's count begins by allocating a Vector, which leads to arbitrary allocation due to flavors_cnt coming from network, and not even being checked against i.len()

Jason:

Actually its doing exactly what count is documented to. Run the parser count number of times and return the results in a Vec. It would be idiomatic to reserve the space in the vector up-front if you know the size.

Obviously not ideal for uses like this where we are throwing the data away.

Its less clear what we should do when we want to keep all the results:

https://github.com/OISF/suricata/blob/master/rust/src/dns/parser.rs#L202

This specific allocation would have a worse case allocation of only 786420 bytes as its u16 limited and the struct is relatively small. But obviously this could be much worse if it was a u32. Then question then becomes what are sane limits to impose, especially if not provided by the protocol spec.

Usages to look at, sorry for the raw format...

rust/src/dcerpc/parser.rs: let (i, ctxitems) = count(parse_dcerpc_bindack_result, numctxitems as usize)(i)?;
rust/src/dns/parser.rs: let (i, queries) = count(|b| dns_parse_query(b, slice), header.questions as usize)(i)?;
rust/src/dns/parser.rs: let (i, queries) = count(|b| dns_parse_query(b, input), header.questions as usize)(i)?;
rust/src/nfs/nfs4_records.rs: let (i, commands) = count(parse_request_compound_command, ops_cnt as usize)(i)?;
rust/src/nfs/nfs4_records.rs: let (i, file_handles) = count(nfs4_parse_handle, fh_handles as usize)(i)?;
rust/src/nfs/nfs4_records.rs: let (i, commands) = count(nfs4_res_compound_command, ops_cnt as usize)(i)?;
rust/src/quic/frames.rs: let (rest, tags_offset) = count(complete(parse_tag_and_offset), num_entries.into())(rest)?;
rust/src/smb/dcerpc_records.rs: let (i, ifaces) = count(parse_dcerpc_bind_iface, num_ctx_items as usize)(i)?;
rust/src/smb/dcerpc_records.rs: let (i, ifaces) = count(parse_dcerpc_bind_iface_big, num_ctx_items as usize)(i)?;
rust/src/smb/dcerpc_records.rs: let (i, results) = count(parse_dcerpc_bindack_result, num_results as usize)(i)?;
rust/src/smb/smb2_records.rs: let (i, dia_vec) = count(le_u16, dialects_count as usize)(i)?;
I think for things like DNS you can make a rule like... Our minimum datastructure size is 20 bytes (I'd have to check), our maximum message size is 65535. So even though count could be 65535 we could put a verify! in place to make sure count was less then 65535/20. I'm not sure I'd use verify as a combinator, but would return some observable error were I could create an alert.


Subtasks

Actions

Also available in: Atom PDF