From: Victor Julien Date: Mon, 21 Feb 2022 19:30:45 +0000 (+0100) Subject: nfs3: improve read validation; fix partial handling X-Git-Tag: suricata-6.0.5~94 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=fb8f4ef849994036bade327d88264a0c8c982497;p=thirdparty%2Fsuricata.git nfs3: improve read validation; fix partial handling (cherry picked from commit d85b77cad064bd88c921b2f3d520fe526ad8ff82) --- diff --git a/rust/src/nfs/nfs.rs b/rust/src/nfs/nfs.rs index 0a3e038379..ad942809d1 100644 --- a/rust/src/nfs/nfs.rs +++ b/rust/src/nfs/nfs.rs @@ -895,6 +895,17 @@ impl NFSState { let chunk_offset; let nfs_version; + let mut fill_bytes = 0; + let pad = reply.count % 4; + if pad != 0 { + fill_bytes = 4 - pad; + } + + // linux defines a max of 1mb. Allow several multiples. + if reply.count == 0 || reply.count > 16777216 { + return 0; + } + match xidmapr { Some(xidmap) => { file_name = xidmap.file_name.to_vec(); @@ -916,11 +927,6 @@ impl NFSState { SCLogDebug!("chunk_offset {}", chunk_offset); let mut is_last = reply.eof; - let mut fill_bytes = 0; - let pad = reply.count % 4; - if pad != 0 { - fill_bytes = 4 - pad; - } SCLogDebug!("XID {} is_last {} fill_bytes {} reply.count {} reply.data_len {} reply.data.len() {}", r.hdr.xid, is_last, fill_bytes, reply.count, reply.data_len, reply.data.len()); @@ -998,7 +1004,8 @@ impl NFSState { if !self.is_udp { self.tc_chunk_xid = r.hdr.xid; - self.tc_chunk_left = (reply.count as u32 + fill_bytes) - reply.data.len() as u32; + debug_validate_bug_on!(reply.data.len() as u32 > reply.count); + self.tc_chunk_left = reply.count - reply.data.len() as u32; } SCLogDebug!("REPLY {} to procedure {} blob size {} / {}: chunk_left {} chunk_xid {:04X}", @@ -1224,7 +1231,7 @@ impl NFSState { // we should have enough data to parse the RPC record match parse_rpc_reply(cur_i, cur_i.len(), false) { Ok((remaining, ref hdr)) => { - match parse_nfs3_reply_read(hdr.prog_data) { + match parse_nfs3_reply_read(hdr.prog_data, false) { Ok((_, ref r)) => { // deal with the partial nfs read data self.process_partial_read_reply_record(hdr, r); diff --git a/rust/src/nfs/nfs3.rs b/rust/src/nfs/nfs3.rs index 13406675ec..4e99c00954 100644 --- a/rust/src/nfs/nfs3.rs +++ b/rust/src/nfs/nfs3.rs @@ -220,7 +220,7 @@ impl NFSState { self.set_event(NFSEvent::MalformedData); }; } else if xidmap.procedure == NFSPROC3_READ { - if let Ok((_, rd)) = parse_nfs3_reply_read(r.prog_data) { + if let Ok((_, rd)) = parse_nfs3_reply_read(r.prog_data, true) { self.process_read_record(r, &rd, Some(&xidmap)); nfs_status = rd.status; } else { diff --git a/rust/src/nfs/nfs3_records.rs b/rust/src/nfs/nfs3_records.rs index 6dabe0acb7..120ac36b0a 100644 --- a/rust/src/nfs/nfs3_records.rs +++ b/rust/src/nfs/nfs3_records.rs @@ -394,14 +394,14 @@ pub struct Nfs3RequestWrite<'a> { } /// Complete data expected -fn parse_nfs3_request_write_data_complete(i: &[u8], file_len: usize, fill_bytes: usize) -> IResult<&[u8], &[u8]> { +fn parse_nfs3_data_complete(i: &[u8], file_len: usize, fill_bytes: usize) -> IResult<&[u8], &[u8]> { let (i, file_data) = take(file_len as usize)(i)?; let (i, _) = cond(fill_bytes > 0, take(fill_bytes))(i)?; Ok((i, file_data)) } /// Partial data. We have all file_len, but need to consider fill_bytes -fn parse_nfs3_request_write_data_partial(i: &[u8], file_len: usize, fill_bytes: usize) -> IResult<&[u8], &[u8]> { +fn parse_nfs3_data_partial(i: &[u8], file_len: usize, fill_bytes: usize) -> IResult<&[u8], &[u8]> { let (i, file_data) = take(file_len as usize)(i)?; let fill_bytes = cmp::min(fill_bytes as usize, i.len()); let (i, _) = cond(fill_bytes > 0, take(fill_bytes))(i)?; @@ -421,9 +421,9 @@ pub fn parse_nfs3_request_write(i: &[u8], complete: bool) -> IResult<&[u8], Nfs3 let fill_bytes = if file_len % 4 != 0 { 4 - file_len % 4 } else { 0 }; // Handle the various file data parsing logics let (i, file_data) = if complete { - parse_nfs3_request_write_data_complete(i, file_len as usize, fill_bytes as usize)? + parse_nfs3_data_complete(i, file_len as usize, fill_bytes as usize)? } else if i.len() >= file_len as usize { - parse_nfs3_request_write_data_partial(i, file_len as usize, fill_bytes as usize)? + parse_nfs3_data_partial(i, file_len as usize, fill_bytes as usize)? } else { rest(i)? }; @@ -438,39 +438,33 @@ pub fn parse_nfs3_request_write(i: &[u8], complete: bool) -> IResult<&[u8], Nfs3 Ok((i, req)) } -/* -#[derive(Debug,PartialEq)] -pub struct Nfs3ReplyRead<'a> { - pub status: u32, - pub attr_follows: u32, - pub attr_blob: &'a[u8], - pub count: u32, - pub eof: bool, - pub data_len: u32, - pub data: &'a[u8], // likely partial +pub fn parse_nfs3_reply_read(i: &[u8], complete: bool) -> IResult<&[u8], NfsReplyRead> { + let (i, status) = be_u32(i)?; + let (i, attr_follows) = verify(be_u32, |&v| v <= 1)(i)?; + let (i, attr_blob) = take(84_usize)(i)?; // fixed size? + let (i, count) = be_u32(i)?; + let (i, eof) = verify(be_u32, |&v| v <= 1)(i)?; + let (i, data_len) = verify(be_u32, |&v| v <= count)(i)?; + let fill_bytes = if data_len % 4 != 0 { 4 - data_len % 4 } else { 0 }; + // Handle the various file data parsing logics + let (i, data) = if complete { + parse_nfs3_data_complete(i, data_len as usize, fill_bytes as usize)? + } else if i.len() >= data_len as usize { + parse_nfs3_data_partial(i, data_len as usize, fill_bytes as usize)? + } else { + rest(i)? + }; + let reply = NfsReplyRead { + status, + attr_follows, + attr_blob, + count, + eof: eof != 0, + data_len, + data, + }; + Ok((i, reply)) } -*/ -named!(pub parse_nfs3_reply_read, - do_parse!( - status: be_u32 - >> attr_follows: verify!(be_u32, |&v| v <= 1) - >> attr_blob: take!(84) // fixed size? - >> count: be_u32 - >> eof: verify!(be_u32, |&v| v <= 1) - >> data_len: be_u32 - >> data_contents: rest - >> ( - NfsReplyRead { - status:status, - attr_follows:attr_follows, - attr_blob:attr_blob, - count:count, - eof:eof != 0, - data_len:data_len, - data:data_contents, - } - )) -); #[cfg(test)] mod tests { @@ -513,4 +507,46 @@ mod tests { } } } + + #[test] + fn test_nfs3_reply_read() { + + #[rustfmt::skip] + let buf: &[u8] = &[ + 0x00, 0x00, 0x00, 0x00, /*Status: NFS3_OK (0)*/ + 0x00, 0x00, 0x00, 0x01, /*attributes_follows: (1)*/ + 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x81, 0xa4, /*attr_blob*/ + 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x0b, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x80, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x10, 0x10, 0x85, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0xb2, 0x5d, 0x38, 0x47, 0x76, 0x25, + 0x23, 0xc3, 0x46, 0x00, 0x38, 0x47, 0x71, 0xc4, + 0x21, 0xf9, 0x82, 0x80, 0x38, 0x47, 0x76, 0x25, + 0x1e, 0x65, 0xfb, 0x81, + 0x00, 0x00, 0x00, 0x0b, /*count: (11)*/ + 0x00, 0x00, 0x00, 0x01, /*EOF: (true)*/ + // [data] + 0x00, 0x00, 0x00, 0x0b, /*data_len: (11)*/ + 0x74, 0x68, 0x65, 0x20, 0x62, 0x20, 0x66, 0x69, + 0x6c, 0x65, 0x0a, /*data: ("the b file\n")*/ + 0x00, /*_data_padding*/ + ]; + + let result = parse_nfs3_reply_read(buf, true).unwrap(); + match result { + (r, reply) => { + assert_eq!(r.len(), 0); + assert_eq!(reply.status, 0); + assert_eq!(reply.attr_follows, 1); + assert_eq!(reply.attr_blob.len(), 84); + assert_eq!(reply.count, 11); + assert_eq!(reply.eof, true); + assert_eq!(reply.data_len, 11); + assert_eq!(reply.data, "the b file\n".as_bytes()); + } + } + } } diff --git a/rust/src/nfs/rpc_records.rs b/rust/src/nfs/rpc_records.rs index 04349e0681..d3988a096f 100644 --- a/rust/src/nfs/rpc_records.rs +++ b/rust/src/nfs/rpc_records.rs @@ -294,7 +294,11 @@ named_args!(pub parse_rpc_reply(start_i: usize, complete: bool) >> verifier: cond!(verifier_len > 0, take!(verifier_len as usize)) >> accept_state: be_u32 - >> rem_len: rest_len + >> frag_len: value!(hdr.frag_len) + >> rem_len: verify!(rest_len, move |&rem_len| { + let consumed = start_i - rem_len; + consumed <= frag_len as usize + 4 + }) >> prog_data_size: value!(hdr.frag_len + 4 - (start_i - rem_len) as u32) >> prog_data_f: cond!(complete, take!(prog_data_size)) >> prog_data_t: cond!(!complete, rest)