af_alg_iv::as_slice makes a slice of the struct data, but the fields are public.

[Lokathor]

af_alg_iv::as_slice makes a slice, but the fields are public and it's a public type, so you could construct a value, set the length to be some invalid value, and then call as_slice to make a slice with an improper length, pointing into invalid memory.

[Shnatsel]

This is a read/write buffer overflow, which is most likely an exploitable security vulnerability. Please prioritize fixing this.

[semarie]

@gnzlbg the as_slice code is present in both src/unix/linux_like/android/mod.rs and src/unix/linux_like/linux/mod.rs.

Removing it, means the type af_alg_iv will lost PartialEq, Eq, Debug, and Hash traits, and I doubt it will be possible to implement them without trusting ivlen for knowing iv array size.

But removing it is also a breaking change. It isn't used internally by libc but do it exist ways to know which (public) crates are using it ?

[Shnatsel]

Alternatively you could make the fields private and create unsafe fn accessors to mutate them.

[semarie]

@Shnatsel libc crate intent is to export the exact binding of struct definition "as it".

I think it would be against RFC1291 to make fields private and functions to modify them.

[gnzlbg]

@semarie this type is borked.

It should be a dynamically-sized type (e.g. using [c_uchar] as its last field), but instead its a sized type (using a zero-sized array).

Then its fields are public, but should be private, because how to actually emulate dynamically sized types is an implementation detail (e.g. ideally we would just make this a type alias to [c_uchar] if usize == u32).

So I'd be fine with:

• adding a Deref impl of &af_alg_iv to &[c_uchar], and a DerefMut impl of &mut af_alg_iv to &mut [c_uchar]
• deprecating the fields of these type, telling people to use the deref to slice instead
• one release later, making the types private, and changing the zero-sized array to actually be a [c_uchar]

This is the PR that added it #1261 and this PR added it to nix nix-rust/nix#1031 (cc @glebpom , cc @asomers ). What do you think ? Do you know of any users / crates depending on this type that we should try to coordinate a fix with ?

Everything is recent enough that we might be able to land a fix without much ecosystem disruption beyond maybe releasing a patch version of nix releases since ~april.

This is a read/write buffer overflow, which is most likely an exploitable security vulnerability. Please prioritize fixing this.

While this isn't good, to actually trigger this one has to:

• construct one of these types: the APIs that work on these only pass them by pointer, because they are unsized, so it doesn't make much sense for Rust code to store anything but a pointer to these anywhere - it can happen, but doing so is useless, since the payload would always be zero. libc does not expose any of these APIs AFAICT. Nix does not do this.

• enable the unstable cargo feature extra_traits which is disabled by default. Most direct dependencies of libc do not enable this feature, although nix does, it does not use any of the impls of this type.

• it then requires users to actively modify the len fields, which can happen, but is something that makes no sense when using these APIs. Nix does not do this, nor does it allow doing this

If you know a crate with a vulnerability related to this, it would be good to know, but it feels like a long shot. Any API dealing with this type is going to give you a *const af_alg_iv so if you wanted to mutate the fields you'd need to cast that to *mut af_alg_iv and then perform an unsafe pointer dereference.

[glebpom]

af_alg_iv is used only as a "header" in a cmsg message, and doesn't hold the actual IV data (take a look here). The only reason to actually have a zero sized iv field there, is to easily access the memory in a cmsg messages right after the length field. Considering that, it seems like there is no reason to have as_slice and other dependent traits implemented, this type is required specifically for constructing cmsg.

[eduardosm]

This issue should receive more attention.

Enabling the extra_traits makes it possible to produce a segmentation fault with a single-line of non-unsafe code:

println!("{:?}", Box::new(libc::af_alg_iv { ivlen: 10000000, iv: [] }));

[tgross35]

Let's remove this for 1.0, we are looking to get rid of the extra traits anyway.