Skip to content

Commit

Permalink
Fix undefined behavior in packet field setters
Browse files Browse the repository at this point in the history
It is currently possible to trigger undefined behavior in field setters
created by the `packet` macro, such as `MutableIpv4Packet::set_payload`,
by setting a buffer larger than the remaining space in the packet
buffer.

Fix this by replacing the unsafe `copy_nonoverlapping` call with
`copy_from_slice`. This function still compiles to `memcpy`, but
includes a range check which which will panic if the destination buffer
is too small.

Fixes #449. Fixes #411.
  • Loading branch information
tdryer committed Jul 27, 2020
1 parent 3e76647 commit cba56e8
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 8 deletions.
11 changes: 3 additions & 8 deletions pnet_macros/src/decorator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -657,14 +657,10 @@ fn handle_vec_primitive(
};

let copy_vals = if inner_ty_str == "u8" {
// Efficient copy_nonoverlapping (memcpy)
// Efficient copy_from_slice (memcpy)
format!("
// &mut and & can never overlap
unsafe {{
copy_nonoverlapping(vals[..].as_ptr(),
_self.packet[current_offset..].as_mut_ptr(),
vals.len())
}}
_self.packet[current_offset..current_offset + vals.len()]
.copy_from_slice(vals);
")
} else {
// e.g. Vec<u16> -> Vec<u8>
Expand All @@ -689,7 +685,6 @@ fn handle_vec_primitive(
#[allow(trivial_numeric_casts)]
#[cfg_attr(feature = \"clippy\", allow(used_underscore_binding))]
pub fn set_{name}(&mut self, vals: &[{inner_ty_str}]) {{
use std::ptr::copy_nonoverlapping;
let mut _self = self;
let current_offset = {co};
Expand Down
24 changes: 24 additions & 0 deletions pnet_packet/src/ipv4.rs.in
Original file line number Diff line number Diff line change
Expand Up @@ -378,3 +378,27 @@ fn ipv4_packet_option_test() {

assert_eq!(&ref_packet[..], &packet[..]);
}

#[test]
fn ipv4_packet_set_payload_test() {
use Packet;

let mut packet = [0u8; 25]; // allow 20 byte header and 5 byte payload
let mut ip_packet = MutableIpv4Packet::new(&mut packet[..]).unwrap();
ip_packet.set_total_length(25);
ip_packet.set_header_length(5);
let payload = b"stuff"; // 5 bytes
ip_packet.set_payload(&payload[..]);
assert_eq!(ip_packet.payload(), payload);
}

#[test]
#[should_panic(expected = "index 25 out of range for slice of length 24")]
fn ipv4_packet_set_payload_test_panic() {
let mut packet = [0u8; 24]; // allow 20 byte header and 4 byte payload
let mut ip_packet = MutableIpv4Packet::new(&mut packet[..]).unwrap();
ip_packet.set_total_length(25);
ip_packet.set_header_length(5);
let payload = b"stuff"; // 5 bytes
ip_packet.set_payload(&payload[..]); // panic
}

0 comments on commit cba56e8

Please sign in to comment.