Unsound implementation created misaligned pointer

[shinmao]

The source of unsoundness

Hi, we found that <observers::map::HitcountsMapObserver<M> as observers::Observer<S>>::post_exec could create a misaligned pointer:

LibAFL/libafl/src/observers/map.rs

Line 1246 in 3625e88

let map16 = unsafe { core::slice::from_raw_parts_mut(map.as_mut_ptr() as *mut u16, cnt) };

At this line, the u8 raw pointer was cast to u16 pointer and passed to core::slice::from_raw_parts_mut which requires the pointer to be aligned. Otherwise, it could lead to undefined behavior.

[domenukk]

Great find, thanks! Can you please take a look at #1530 if this would be an okay fix?

[shinmao]

@domenukk thanks for the response. I just saw the code and considered it should be fixed!

[shinmao]

Another question:

LibAFL/libafl_bolts/src/staterestore.rs

Lines 215 to 220 in 0e149af

	fn content_mut(&mut self) -> &mut StateShMemContent {
	let ptr = self.shmem.as_slice().as_ptr();
	#[allow(clippy::cast_ptr_alignment)] // Beginning of the page will always be aligned
	unsafe {
	&mut *(ptr as *mut StateShMemContent)
	}

The safety of this function depends on the usage of internal library. The comments mentioned that "Beginning of the page will always be aligned": Is that guaranteed because the start address is 0x0 and could be multiple of any alignments?

[domenukk]

For all shmem implementations I know the start of the page will be aligned, but I'll happily replace this with a read_unaligned

[domenukk]

Ah no, it's not that easy

[shinmao]

I think it's fine if the address is 0x0.

[domenukk]

I'll add a debug_assert

[shinmao]

I have another issue about bolts::anymap::unpack_type_id. Personally, I would like to open another issue to avoid misunderstanding. Would you want me to describe it here?

[domenukk]

Yes please open another issue :)

[shinmao]

Would you mind help me report it to RUSTSEC so that we could remind users to update to the latest version?

[tokatoka]

Yes 👍
How can we help?

[shinmao]

@tokatoka I just send the PR to the advisory-db (please see the link: rustsec/advisory-db@03f3cea). What I can do now is that update the information of patched version. Is it patched and release in a new version, and which version?

[tokatoka]

no there's no new version released yet

[domenukk]

Maybe we should do a release then, cc @andreafioraldi

[shinmao]

Hi @domenukk , do you agree to create a security policy under the repo? Then I can send the advisory that all users can check the dependencies on Github.

[andreafioraldi]

Hi @domenukk , do you agree to create a security policy under the repo? Then I can send the advisory that all users can check the dependencies on Github.

Hi @shinmao , thanks for the report!
I don't think that there is a threat model here and thus we cannot consider a bug in the code processing the coverage map as a security issue.
I don't mind too much about definitions, feel free to push things to advisory-db once we release the next version, but I would not bother by email any of our users for this bug.

[shinmao]

@andreafioraldi thanks for the response.
I have sent PR to advisory-db (link) and wondering whether they have contacted you.

[andreafioraldi]

Not yet, but if they want to email users with things like dependabot i will not let them proceed, this is not a security vulnerability and users must not receive ambiguos alerts on their mailboxes.

[shinmao]

Understood. It is just something like undefined behavior from the perspective of Rust's language model.