Traits TypedIterable and DNSIterable should be sealed or unsafe traits

[JOE1994]

Hello 🦀 ,
we (Rust group @sslab-gatech) found a memory-safety/soundness issue in this crate while scanning Rust code on crates.io for potential vulnerabilities.

Issue Description

public traits TypedIterable & DNSIterable both come with a handful of provided methods.

Provided method TypedIterable::resize_rr() which is used in other provided methods
contains an unsafe block as below.

dnssector/src/rr_iterator.rs

Lines 205 to 227 in 9dc8b2a

	packet.resize(new_packet_len, 0);
	debug_assert_eq!(
	new_packet_len,
	(offset as isize + shift) as usize + (packet_len - offset) as usize
	);
	unsafe {
	let packet_ptr = packet.as_mut_ptr();
	ptr::copy(
	packet_ptr.add(offset),
	packet_ptr.offset((offset as isize + shift) as isize),
	packet_len - offset,
	);
	}
	} else if shift < 0 {
	assert!(packet_len >= (-shift) as usize);
	unsafe {
	let packet_ptr = packet.as_mut_ptr();
	ptr::copy(
	packet_ptr.offset((offset as isize - shift) as isize),
	packet_ptr.add(offset),
	packet_len - (offset as isize - shift) as usize,
	);
	}

Because unsafe code relies on the behavioral correctness of trait implementations of TypedIterable,
we suggest making the TypedIterable trait either a sealed trait or an unsafe trait.

Even without unsafe, soundness & correctness of DNSIterable trait also relies on correct implementation of the DNSIterable trait (e.g. DNSIterable::raw(), DNSIterable::raw_mut()).
We also suggest making the DNSIterable trait either a sealed trait or an unsafe trait.

We'd like your opinion on our suggested fixes!
Thank you for checking out this issue 👍

[jedisct1]

Hi,

And thanks for your report. Unfortunately, I must confess that I don't understand the issue.

The unsafe block is here because when that code was written, Rust didn't had another way to memmove. Looks like there is copy_within to achieve that now, so the unsafe block can be replaced. I still don't understand what problem that would solve, and it will only make the code slower.

[Qwaz]

Hello, I'm Yechan from the same group. I'll try to elaborate on the issue, for what it's worth.

Under Rust's safety model, safe code should not exhibit any undefined behavior, but this was not true in the previous implementation of resize_rr(). A function should be marked unsafe if it requires additional property that cannot be checked by Rust's type system to behave safely, and the previous version of resize_rr()'s safety relied on the correctness DNSIterable::offset().

Although the three DNSIterable implementors in this crate have correct offset() implementations, DNSIterable is defined as a public safe trait and Rust's type system cannot guarantee that ALL implementations of DNSIterable::offset() is functioning correctly. Hence, we suggested to mark DNSIterable as a sealed trait which prevents the users to provide a custom implementation to this trait or mark it as a unsafe trait to indicate that the correctness of DNSIterable affects the program's safety.

I checked the updated code, and it seems that now a program panics instead of writing out of bounds. This is another valid way to fix the problem. Thank you for the quick fix!