Implementing From<&mut AsciiStr> for &mut str and &mut [u8] is unsound

[tormol]

They allow writing non-ASCII values to an AsciiStr which when read out as an AsciiChar will produce values outside the valid niche.

These impls were added by me in 4fbd050, so 0.9, 0.8 and 0.7 are affected.

Here's an example using these impls to create out-of-bounds array indexing in safe code (when compiled in release mode):

let mut buf = [0u8; 1];
let ascii = buf.as_mut_ascii_str().unwrap();
let byte_view = <&mut[u8] as From<&mut AsciiStr>>::from(ascii);
let arr = [0b11011101u8; 128];
byte_view[0] = 180;
assert_ne!(arr[ascii[0] as u8 as usize], 0b11011101);

I don't see any good way to tell users of the crate to stop using these impls:
Deprecation notices on trait impls are ignored (by both Rust 1.38 and Rust 1.9).
Changing the impls to panic or return an empty slice could break working code (that never writes non-ASCII values) at run-time.

The only fix we could make appears to be to remove the impls, telling users of them to do the unsafe pointer casting explicitly. On one hand this will make any accidental users of it aware of the problem when they update Cargo.lock, but it will also break any use that happened to be OK with a minor release, and any reverse dependencies of these uses.
On the other hand doing nothing and hoping nobody accidentally uses these impls feels irresponsible. What do you think @tomprogrammer?
In any case I don't think we need to fix 0.7 and 0.8, as Rust didn't backport the security fix in 1.29.1 to previous affected versions.

[Shnatsel]

I don't see any good way to tell users of the crate to stop using these impls

You can file a security advisory at https://github.com/RustSec/advisory-db
This will alert anyone running cargo-audit that they're using a vulnerable version and need to upgrade. You can even include the specific functions that need to be called to be affected by the issue.

Out-of-bounds indexing in safe code is usually an exploitable vulnerability, so I would strongly recommend filing a security advisory.

[tomprogrammer]

I think Shnatsels idea is good, we should file an advisory. I've yanked the affected versions already.

[KisaragiEffective]

I've filed advisory, so this can be closed now :)

[shinmao]

Out-of-bounds indexing in safe code is usually an exploitable vulnerability, so I would strongly recommend filing a security advisory.

@tomprogrammer hi, I am confused about the purpose of PoC provided here.

arr[ascii[0] as u8 as usize]

I understand that here ascii[0] should not have value larger than 127. However, in your PoC, the length of arr is user-defined. You can also define arr with an arbitrary length less than 127. In this case, even though ascii[0] can only ascii value, you still can trigger OOB above.

Therefore, I can't understand why your PoC can prove the unsoundness. Could you give some more explanation, thanks?

[tormol]

@shinmao:
If the length of the array that is indexed into is less than 128, then also ascii values can be out of bounds and the compiler has to insert a check that the index is within bounds. That check also catches the invalid values above 127.
(And for arrays longer than 255, all byte values are valid indexes, so the invalid ascii values are also within bounds.)

@KisaragiEffective:
Thanks.

[shinmao]

@tomprogrammer thanks for the response. The reason why I feel confused is that this OOB case could never be checked by compiler statically. For example,

let arr = [0u8, 1, 2, 3, 4, 5];
println!("{}", arr[ arr[4] as u8 as usize ] );

This code can be compiled and have a runtime panic because compiler couldn't understand the value of arr[4] statically. and we won't say that arr has UB because it allows OOB indexing in safe code here.

However, maybe this is the only way to show unsoundness here?

[tormol]

In your example there is no OOB indexing or UB because the inserted check will panic and prevent the OOB indexing from occurring.
But in the ascii example, the check is optimized out which lets OOB indexing actually happen.
Why the compiler is allowed to optimize out the check:

1. AsciiChar is an enum whose variants all have discriminants /values below 128.
2. When the value from ascii[0] is cast to u8 and then usize, the compiler remembers that it is coming from an Asciichar and therefore will never be more than 127.
   (While values up to 255 are physically possible, those occurring would already be UB and can therefore be assumed never to happen.)
3. The length of arr is greater than 127, so all valid AsciiChar values are valid indexes.

In my example the unsoundness happens in the From impl on line 3, the UB happens on line 5, and line 6 just shows that the UB can be used to make bad stuff happen. Putting the value into an Option<AsciiChar> as Some but getting None back might also show it, but would depend on how the compiler implements the variant checking.

I hope this explains it.
(And by the way, I'm @tormol, not tomprogrammer)

[shinmao]

@tormol sorry I ping the wrong person hahaha

Thanks for your clear answer. It really helps solve my question!