From: Victor Julien Date: Fri, 18 Mar 2022 21:29:29 +0000 (-0600) Subject: nfs3: improve read validation; fix partial handling X-Git-Tag: suricata-5.0.9~55 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c6e1adb152d2956e6adfb1242598826ba5baa941;p=thirdparty%2Fsuricata.git nfs3: improve read validation; fix partial handling Note: Some parsers converted to a more functional style to faciliate additonal arguments being provided -- Jason Ish (cherry picked from commit d85b77cad064bd88c921b2f3d520fe526ad8ff82) --- diff --git a/rust/src/nfs/nfs.rs b/rust/src/nfs/nfs.rs index a4876f3e32..33f90fbde1 100644 --- a/rust/src/nfs/nfs.rs +++ b/rust/src/nfs/nfs.rs @@ -1,4 +1,4 @@ -/* Copyright (C) 2017 Open Information Security Foundation +/* Copyright (C) 2022 Open Information Security Foundation * * You can copy, redistribute or modify this Program under the terms of * the GNU General Public License version 2 as published by the Free @@ -896,6 +896,16 @@ 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(); @@ -917,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()); @@ -1001,7 +1006,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}", @@ -1308,7 +1314,7 @@ impl NFSState { // we should have enough data to parse the RPC record match parse_rpc_reply(cur_i) { Ok((remaining, ref rpc_record)) => { - match parse_nfs3_reply_read(rpc_record.prog_data) { + match parse_nfs3_reply_read(rpc_record.prog_data, false) { Ok((_, ref nfs_reply_read)) => { // deal with the partial nfs read data status |= self.process_partial_read_reply_record(rpc_record, nfs_reply_read); diff --git a/rust/src/nfs/nfs3.rs b/rust/src/nfs/nfs3.rs index 3e587c3ce4..ea0496b60f 100644 --- a/rust/src/nfs/nfs3.rs +++ b/rust/src/nfs/nfs3.rs @@ -268,7 +268,7 @@ impl NFSState { }, }; } else if xidmap.procedure == NFSPROC3_READ { - match parse_nfs3_reply_read(r.prog_data) { + match parse_nfs3_reply_read(r.prog_data, true) { Ok((_, ref reply)) => { self.process_read_record(r, reply, Some(&xidmap)); nfs_status = reply.status; diff --git a/rust/src/nfs/nfs3_records.rs b/rust/src/nfs/nfs3_records.rs index 66bba96e35..e763785a0c 100644 --- a/rust/src/nfs/nfs3_records.rs +++ b/rust/src/nfs/nfs3_records.rs @@ -391,14 +391,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!(i, file_len as usize)?; let (i, _) = cond!(i, fill_bytes > 0, take!(fill_bytes))?; 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!(i, file_len as usize)?; let fill_bytes = cmp::min(fill_bytes as usize, i.len()); let (i, _) = cond!(i, fill_bytes > 0, take!(fill_bytes))?; @@ -413,9 +413,9 @@ pub fn parse_nfs3_request_write(i: &[u8], complete: bool) -> IResult<&[u8], Nfs3 let (i, file_len) = verify!(i, be_u32, |v| v <= count)?; let fill_bytes = if file_len % 4 != 0 { 4 - file_len % 4 } else { 0 }; 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)? }; @@ -429,36 +429,30 @@ pub fn parse_nfs3_request_write(i: &[u8], complete: bool) -> IResult<&[u8], Nfs3 })) } -/* -#[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!(i, be_u32, |v| v <= 1)?; + let (i, attr_blob) = take!(i, 84_usize)?; // fixed size? + let (i, count) = be_u32(i)?; + let (i, eof) = verify!(i, be_u32, |v| v <= 1)?; + let (i, data_len) = verify!(i, be_u32, |v| v <= count)?; + 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, - } - )) -); diff --git a/rust/src/nfs/rpc_records.rs b/rust/src/nfs/rpc_records.rs index d05e1f68a7..23d574279f 100644 --- a/rust/src/nfs/rpc_records.rs +++ b/rust/src/nfs/rpc_records.rs @@ -247,35 +247,39 @@ pub fn parse_rpc(start_i: &[u8]) -> IResult<&[u8], RpcPacket> { } // to be called with data <= hdr.frag_len + 4. Sending more data is undefined. -named!(pub parse_rpc_reply, - do_parse!( - hdr: parse_rpc_packet_header +pub fn parse_rpc_reply(start_i: &[u8]) -> IResult<&[u8], RpcReplyPacket> { + let (i, hdr) = parse_rpc_packet_header(start_i)?; + let rec_size = hdr.frag_len + 4; - >> reply_state: verify!(be_u32, |v| v <= 1) + let (i, reply_state) = verify!(i, be_u32, |v| v <= 1)?; - >> verifier_flavor: be_u32 - >> verifier_len: verify!(be_u32, |size| size < RPC_MAX_VERIFIER_SIZE) - >> verifier: cond!(verifier_len > 0, take!(verifier_len as usize)) + let (i, verifier_flavor) = be_u32(i)?; + let (i, verifier_len) = verify!(i, be_u32, |size| size < RPC_MAX_VERIFIER_SIZE)?; + let (i, verifier) = cond!(i, verifier_len > 0, take!(verifier_len as usize))?; - >> accept_state: be_u32 + let (i, accept_state) = be_u32(i)?; - >> pl: rest + let consumed = start_i.len() - i.len(); + if consumed > rec_size as usize { + return Err(nom::Err::Error(error_position!(i, nom::ErrorKind::LengthValue))); + } - >> ( - RpcReplyPacket { - hdr:hdr, + let (i, prog_data) = rest(i)?; + let packet = RpcReplyPacket { + hdr, - verifier_flavor:verifier_flavor, - verifier_len:verifier_len, - verifier:verifier, + verifier_flavor, + verifier_len, + verifier, - reply_state:reply_state, - accept_state:accept_state, + reply_state, + accept_state, - prog_data:pl, - } - )) -); + //prog_data_size: data_size, + prog_data, + }; + Ok((i, packet)) +} named!(pub parse_rpc_udp_packet_header, do_parse!(