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 #2

Updated by Jeff Lucovsky over 2 years ago

  • Subject changed from nom: use of count combinator can use too much memory to Backport 6.0lx: nom: use of count combinator can use too much memory
  • Parent task set to #5279
Actions #3

Updated by Jeff Lucovsky over 2 years ago

  • Subject changed from Backport 6.0lx: nom: use of count combinator can use too much memory to Backport 6.0.x: nom: use of count combinator can use too much memory
Actions #4

Updated by Jeff Lucovsky over 2 years ago

  • Subject changed from Backport 6.0.x: nom: use of count combinator can use too much memory to nom: use of count combinator can use too much memory (6.0.x backport)
Actions #5

Updated by Victor Julien over 2 years ago

  • Target version changed from 6.0.6 to 6.0.7
Actions #6

Updated by Victor Julien over 2 years ago

  • Target version changed from 6.0.7 to 6.0.8
Actions #7

Updated by Victor Julien about 2 years ago

  • Target version changed from 6.0.8 to 6.0.9
Actions #8

Updated by Victor Julien about 2 years ago

  • Target version changed from 6.0.9 to 6.0.10
Actions #9

Updated by Victor Julien almost 2 years ago

  • Assignee changed from Shivani Bhardwaj to Jason Ish
Actions #10

Updated by Jason Ish almost 2 years ago

Uses of count in master-6.0.x:

1.  src/dcerpc/parser.rs:        >> ctxitems: count!(parse_dcerpc_bindack_result, numctxitems as usize)
2.  src/dns/parser.rs:            >> queries: count!(
3.  src/dns/parser.rs:        queries: count!(call!(dns_parse_query, input),
4.  src/nfs/nfs4_records.rs:        >>  commands: count!(parse_request_compound_command, ops_cnt as usize)
5.  src/nfs/nfs4_records.rs:        >>  commands: count!(nfs4_res_compound_command, ops_cnt as usize)
6.  src/smb/dcerpc_records.rs:        >>  ifaces: count!(parse_dcerpc_bind_iface, num_ctx_items as usize)
7.  src/smb/dcerpc_records.rs:        >>  ifaces: count!(parse_dcerpc_bind_iface_big, num_ctx_items as usize)
8.  src/smb/dcerpc_records.rs:        >>  results: count!(parse_dcerpc_bindack_result, num_results as usize)
9.  src/smb/smb2_records.rs:        >>  dia_vec: count!(le_u16, dialects_count as usize)

1) Count is from a u8, limited to an array of size 256.
2, 3) Array size could be 65535. Could limit to 9363.
4, 5) Limited to array size of 64. Fixed by commit b8875d4a223762db5efd9976104fa7761b679214.
6, 7, 8) Maximum array size is from a u8, so 256.
9. Maximum array size is 65535.

Notes for DNS:
- Maximum size DNS message is 65535 bytes.
- The minimum size request/response is 4 bytes for rrclass/rrtype plus 3 bytes for the minimum size
- This means we could reject record counts greater than 9363.

Actions #11

Updated by Victor Julien almost 2 years ago

  • Target version changed from 6.0.10 to 6.0.11
Actions #12

Updated by Jason Ish over 1 year ago

  • Target version changed from 6.0.11 to 6.0.12

Pushing forward as I don't see this as critical for now.

Actions #13

Updated by Victor Julien over 1 year ago

  • Target version changed from 6.0.12 to 6.0.13
Actions #14

Updated by Jason Ish over 1 year ago

I believe we are good here and should consider closing this out instead of carrying it forward.

Actions #15

Updated by Victor Julien over 1 year ago

  • Target version changed from 6.0.13 to 6.0.14
Actions #16

Updated by Philippe Antoine over 1 year ago

I agree it should be closed...

Could we have a specific linter that checks that nom's count is not called with something u32 from the network ?

Actions #17

Updated by Jason Ish over 1 year ago

Philippe Antoine wrote in #note-16:

I agree it should be closed...

Could we have a specific linter that checks that nom's count is not called with something u32 from the network ?

How would a linter know if the value came from the network?

Actions #18

Updated by Jason Ish over 1 year ago

  • Status changed from Assigned to Rejected

Closing, the impact isn't dangerous enough to fix.

Note: Mark as rejected as the uses of count in 6.0.x were considered OK.

Actions #19

Updated by Victor Julien about 1 year ago

  • Private changed from Yes to No
Actions

Also available in: Atom PDF