rusticata / pcap-parser

PCAP/PCAPNG file format parser written in pure Rust. Fast, zero-copy, safe.
Other
104 stars 24 forks source link

Infinite loop if capacity < frame size #29

Closed mhils closed 9 months ago

mhils commented 1 year ago

First off, thank you for the fantastic work here! šŸ˜ƒ :cake:

We've been running into a small footgun where reader.next() would return Incomplete, but calling refill() does not help because the buffer capacity is too small. Here's a repro based on the example code README, which gets stuck in an infinite loop:

#!/usr/bin/env -S cargo +nightly -Zscript

//! ```cargo
//! [dependencies]
//! pcap-parser = "0.14.1"
//! ```

use pcap_parser::*;
use pcap_parser::traits::PcapReaderIterator;
use std::fs::File;

fn main() {
    let file = File::open("repro.pcapng").unwrap();
    let mut num_blocks = 0;
    // smaller capacity for demonstration
    let mut reader = PcapNGReader::new(1500, file).expect("PcapNGReader");
    loop {
        match reader.next() {
            Ok((offset, _block)) => {
                println!("got new block");
                num_blocks += 1;
                reader.consume(offset);
            },
            Err(PcapError::Eof) => break,
            Err(PcapError::Incomplete) => {
                reader.refill().unwrap();
            },
            Err(e) => panic!("error while reading: {:?}", e),
        }
    }
    println!("num_blocks: {}", num_blocks);
}

repro.pcapng.zip

This seems to affect both PCAP and PCAPNG files. Not sure what the right fix is here, I would guess .refill() should maybe error?

iczero commented 1 year ago

Hi, I have a PR that adds the additional bytes needed field to PcapError::Incomplete: https://github.com/rusticata/pcap-parser/pull/28. You can check this against the current buffer size and grow if needed. Please feel free to use the fork.

chifflier commented 9 months ago

Hi, thank you for the report and the test file. I can confirm the problem and will commit a fix soon.

chifflier commented 9 months ago

I committed a fix for this problem, which adds a new error type BufferTooSmall. This error will be raised only if buffer capacity is not enough to store the data, not if there are only missing bytes.

iczero commented 9 months ago

@chifflier Do you reckon it would be useful to also include the new required size of the buffer in the BufferTooSmall variant (a bit similar to what Incomplete has)? I believe doing that could allow the application to resize the buffer to a reasonable value. If it's too large then maybe the pcap is malformed and the app could decide to error out.