From e08474a28364621e9d4117b846bb9c03e175457f Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Tue, 20 Oct 2026 16:51:38 +0000 Subject: [PATCH] nfs: check a v3 WRITE and READ claim against the record The args of a WRITE request and of a READ reply were trusted to fit the record that carries them. Where the claim fell below the extent the parse covers, the file data ran past the record boundary into the next RPC, which was then read as a record. A max-tx of 0 was accepted and evicted every transaction on insert. Compare the claim against that extent: for a WRITE the handle, offset, count, stable flag and its data, for a READ the status, the optional attributes, the count, the eof flag and its data. A record whose claim is below the extent gets a transaction with the malformed data event and a skip to the record boundary, as an oversized claim already does. Only the v3 records arrive in pieces, so process_read_record now learns which reply it is given and the check is v3's. Reject a max-tx of 0 at config load. Ticket: #9263. --- rust/src/nfs/nfs.rs | 96 +++++++++++++++++++++++++++++++++++++++----- rust/src/nfs/nfs2.rs | 2 +- rust/src/nfs/nfs3.rs | 2 +- rust/src/nfs/nfs4.rs | 2 +- 4 files changed, 90 insertions(+), 12 deletions(-) diff --git a/rust/src/nfs/nfs.rs b/rust/src/nfs/nfs.rs index e7ed7fd018..c859eee0da 100644 --- a/rust/src/nfs/nfs.rs +++ b/rust/src/nfs/nfs.rs @@ -1188,6 +1188,12 @@ impl NFSState { pub fn process_write_record<'b>( &mut self, flow: *mut Flow, r: &RpcPacket<'b>, w: &Nfs3RequestWrite<'b>, ) -> u32 { + // the parsed extent within prog_data: fh len + fh + offset + count + // + stable + file_len + data; the XDR fill follows it when unaligned + let extent = 24u32 + .saturating_add(w.handle.len) + .saturating_add(w.file_len); + // for now assume that stable FILE_SYNC flags means a single chunk let is_last = w.stable == 2; // a partial record's file tx must stay open (found by handle) until @@ -1202,8 +1208,42 @@ impl NFSState { Vec::new() }; + // The record must cover the parsed extent: a claim shorter than the + // args would run the file data past the record boundary into the next + // RPC. A claim larger than the extent is not an error, it is the + // record tail (the XDR pad, or bytes the sender put in the record + // after the args): the chunk promise covers it so it is never parsed. + // No alignment is assumed, some senders do not pad the handle. // unbuffered record tail (file data + XDR fill) left in the stream let tail = record_tail(r.prog_data_size, r.prog_data.len()); + if !self.is_udp && extent > r.prog_data_size { + SCLogDebug!( + "WRITE claim below extent (claim {} extent {}): skipping {} stream bytes", + r.prog_data_size, + extent, + tail + ); + // rejected tx + event, same pattern as the too-large arm + let tx = self.new_file_tx(&file_handle, &file_name, Direction::ToServer); + tx.procedure = NFSPROC3_WRITE; + tx.xid = r.hdr.xid; + tx.is_first = true; + tx.nfs_version = r.progver as u16; + tx.request_done = true; + tx.response_done = true; + tx.is_file_closed = true; + tx.tx_data.set_event(NFSEvent::MalformedData as u8); + tx.tx_data.0.updated_ts = true; + tx.tx_data.0.updated_tc = true; + tx.tx_data.0.files_opened = 0; + self.set_skip(Direction::ToServer, tail); + self.ts_chunk_xid = 0; + self.ts_chunk_left = 0; + self.ts_chunk_tail = 0; + self.ts_chunk_fh.clear(); + return 0; + } + if w.count == 0 { // No file data, but the rest of a partial record must not be // parsed as the next RPC record: skip to the record boundary. @@ -1894,13 +1934,14 @@ impl NFSState { /// have a complete record. Otherwise we do a lookup ourselves. pub fn process_read_record<'b>( &mut self, flow: *mut Flow, r: &RpcReplyPacket<'b>, reply: &NfsReplyRead<'b>, - xidmapr: Option<&NFSRequestXidMap>, + xidmapr: Option<&NFSRequestXidMap>, v3: bool, ) -> u32 { - let file_name; - let file_handle; - let chunk_offset; - let nfs_version; - + // The record must cover the parsed extent (status + attr_follows + + // [post_op_attr] + count + eof + data_len + data); a larger claim is + // the record tail, which the chunk promise consumes + let extent = 20u32 + .saturating_add(if reply.attr_follows != 0 { 84 } else { 0 }) + .saturating_add(reply.data_len); // unbuffered record tail (reply data + XDR fill) left in the stream let tail = record_tail(r.prog_data_size, r.prog_data.len()); @@ -1918,6 +1959,38 @@ impl NFSState { return 0; } + if v3 && !self.is_udp && extent > r.prog_data_size { + SCLogDebug!( + "READ claim below extent (claim {} extent {}): skipping {} stream bytes", + r.prog_data_size, + extent, + tail + ); + // rejected tx + event, same pattern as the v4 malformed-response arm + let mut tx = self.new_tx(); + tx.procedure = NFSPROC3_READ; + tx.xid = r.hdr.xid; + tx.nfs_version = self.nfs_version; + tx.request_done = true; + tx.response_done = true; + tx.is_file_closed = true; + tx.tx_data.set_event(NFSEvent::MalformedData as u8); + tx.tx_data.0.updated_ts = true; + tx.tx_data.0.updated_tc = true; + self.transactions.push(tx); + self.set_skip(Direction::ToClient, tail); + self.tc_chunk_xid = 0; + self.tc_chunk_left = 0; + self.tc_chunk_tail = 0; + self.tc_chunk_fh.clear(); + return 0; + } + + let file_name; + let file_handle; + let chunk_offset; + let nfs_version; + match xidmapr { Some(xidmap) => { file_name = xidmap.file_name.to_vec(); @@ -2217,7 +2290,7 @@ impl NFSState { reply.count ); - return self.process_read_record(flow, r, reply, None); + return self.process_read_record(flow, r, reply, None, true); } fn peek_reply_record(&mut self, r: &RpcPacketHeader) -> u32 { @@ -2701,7 +2774,7 @@ impl NFSState { data_len: 0, data: &[], }; - self.process_read_record(flow, hdr, &reply, None); + self.process_read_record(flow, hdr, &reply, None, false); return AppLayerResult::ok(); } // Definitive structural error: skip the record (bounded @@ -3613,7 +3686,12 @@ pub unsafe extern "C" fn SCRegisterNfsUdpParser() { } if let Some(val) = conf_get("app-layer.protocols.nfs.max-tx") { if let Ok(v) = val.parse::() { - NFS_MAX_TX = v; + // 0 would evict every transaction on insert + if v == 0 { + SCLogError!("Invalid value for nfs.max-tx"); + } else { + NFS_MAX_TX = v; + } } else { SCLogError!("Invalid value for nfs.max-tx"); } diff --git a/rust/src/nfs/nfs2.rs b/rust/src/nfs/nfs2.rs index 788ee05c15..0cc5d2f6fa 100644 --- a/rust/src/nfs/nfs2.rs +++ b/rust/src/nfs/nfs2.rs @@ -113,7 +113,7 @@ impl NFSState { match parse_nfs2_reply_read(r.prog_data) { Ok((_, ref reply)) => { SCLogDebug!("NFSv2: READ reply record"); - self.process_read_record(flow, r, reply, Some(xidmap)); + self.process_read_record(flow, r, reply, Some(xidmap), false); nfs_status = reply.status; } _ => { diff --git a/rust/src/nfs/nfs3.rs b/rust/src/nfs/nfs3.rs index e842567ec3..5c1d655538 100644 --- a/rust/src/nfs/nfs3.rs +++ b/rust/src/nfs/nfs3.rs @@ -237,7 +237,7 @@ impl NFSState { }; } else if xidmap.procedure == NFSPROC3_READ { if let Ok((_, rd)) = parse_nfs3_reply_read(r.prog_data, true) { - self.process_read_record(flow, r, &rd, Some(xidmap)); + self.process_read_record(flow, r, &rd, Some(xidmap), true); nfs_status = rd.status; } else { self.set_event(NFSEvent::MalformedData); diff --git a/rust/src/nfs/nfs4.rs b/rust/src/nfs/nfs4.rs index aa2c488fd5..824dac55d1 100644 --- a/rust/src/nfs/nfs4.rs +++ b/rust/src/nfs/nfs4.rs @@ -438,7 +438,7 @@ impl NFSState { data_len: rd.data.len() as u32, data: rd.data, }; - self.process_read_record(flow, r, &reply, Some(xidmap)); + self.process_read_record(flow, r, &reply, Some(xidmap), false); } Nfs4ResponseContent::Open(_s, Some(ref _rd)) => { SCLogDebug!("OPENv4: status {} opendata {:?}", _s, _rd);