`as` cast in a real vulnerability

language design
1

FYI, there was a real bug caused by:

len < u16::max as usize

because as is an all-purpose cast, it happily converted a function pointer to an integer.

24 Likes
2

In this case len < u16::MAX.into() would have worked, and len < u16::max.into() would have caught the mistake, which is nice!

However, I'm not surprised that this code used as. I'd use as in such case myself! That's because .into() often works poorly with type inference, so I'm actually surprised that a combo of Into and < works and doesn't complain about PartialOrd being ambiguous or such.

Into also doesn't work for all integer types due to very conservative portability concerns with no opt out implemented yet, so I'm using as more than I'm comfortable with.

5 Likes
3

usize::from(u16::MAX) avoids the inference issues.

12 Likes
4

Please ignore this comment..

Wrong observation, and not all related to the fixed location. Just another place with a comparison against `MAX` to verifiy a range

I find their their fix rather strange. This is part of code that checks against conversion to i16:

            Ok(Some(Value::I64(i)))
                if i <= (std::i16::MAX as i64) && i >= (std::i16::MIN as i64) =>
            {
                Ok(Some(i as i16))
            }

Cleaner, shorter and hard to mess up (edit: forgot to return the right error):

Ok(Some(Value::I64(i))) => {
    if let Ok(val) = i16::try_from(i) {
        Ok(Some(val))
    } else{
        Err(SecurityStateError::from(
            "Unexpected type when trying to get a Value::I64",
        )),
    }
}

I wonder if diagnostics could catch or at least hint to TryFrom.

3 Likes
5

Yeah, the problem is that this one is particularly awkward because getting a function pointer as a usize is also a somewhat-reasonable thing to do :slightly_frowning_face: (For any other destination type we can lint rather more strongly about it, since things like i16::max as u16 makes no sense.)

Do we have a specific function for doing this on a function pointer? I know in lang we want to lint about things like this, but we also need ways of doing it that aren't just disabling the lint. (Come to think of it, we have https://doc.rust-lang.org/nightly/std/primitive.i32.html#method.cast_unsigned stable in nightly now, so we can start linting i32::MAX as u32 to suggest cast_unsigned instead!)

If we had something like ptr::fn_addr(u16::max) that we could recommend instead of the as, we could lint on it.

4 Likes
6

Transmute? :wink:

Okay, that doesn't work, I don't believe transmute preserves provenance unfortunately.

7

And also with different behaviour, transforming an invalid value into a missing value :slight_smile:

Without making assumptions about what the other branches of the match do you need something like this, but unfortunately it doesn't work:

Ok(Some(Value::I64(i)) if let Ok(n) = i16::try_from(i) => Ok(Some(n))
8

Oh, found it: it's https://doc.rust-lang.org/std/marker/trait.FnPtr.html#tymethod.addr, though still unstable.

If we stabilize that, then we say "no, don't use u16::min as usize, use u16::min.addr() if you want the function's address", which gives the opportunity for the user to say "wait, that's not at all what I was trying to do!" (which we can also note in the hints).

21 Likes
9

For reference, there's now a clippy lint (in nightly) that detects this footgun

4 Likes
10

In this case they're trying to encode that the length is also a valid u16, so instead of conversion we'd be looking for the type u16 ⋂ usize. While this is equivalent to u16 on all platforms, u32 ⋂ usize and signed versions i16 ⋂ usize, u32 ⋂ isize would be distinct from either of the intersections parts. Also such a type, not bound by prior history, could be made symmetrical for all cases which may be helpful in teaching its use; you don't need to remember which way to test or incantations as usize::MAX as u32 as usize for succinctly deriving the maximum bound.

I don't know. Maybe the ergonomics work out even worse and you'd still reach for as.

1 Like
11

What I'm trying to say is that there's a mental overhead in guessing whether .into() will work. Sometimes it does, sometimes it doesn't. Sometimes T::from is sufficient to make it work, sometimes it's not. This makes as the easy/lazy solution that will always work (even in too many cases as that bug shows).

as has multiple roles, some trivial, some not:

  • lossless size extension (as u128). This "type system lubrication" is often needed when working with non-usize types, and AFAIK it's harmless.

  • explicit coercions (as Box<dyn T> or even as _). These are already harmless enough to be implicit almost always.

  • integer truncation. This is unfortunately also needed frequently when using non-usize types. It may be a bug if it was meant as size extension but ends up truncating (usize as c_ulong is a footgun). There's .wrapping_add(), maybe there should be .wrapping_cast()?

  • conversion that is less literal and less unsafe than transmute, but more lossy than into. It's kida weird that the two alternatives are both functions, but as isn't.

    • There's been a good progress on eliminating as here! ptr.cast(), and now ptr.addr().
    • It'd be nice to have something better for floats too. Now that float as is saturating, the operation isn't an obvious single instruction. The cost of round() & clamp() combined with as is not clear.
10 Likes
12

My proposal for that case:

7 Likes
13

Given we have ptr::fn_addr_eq maybe we should just add a free-standing ptr::fn_addr. Then there's no risk of confusion with <*mut>::addr.

5 Likes
14

You don't want to use wrapping_cast for simple integer conversions because that silently drops the error on overflow. usize::try_from is better. There could be another conversion that panics on overflow so that you don't have to manually unwrap.

An even better solution would be to have all the indexing operations allow u16 etc so that you don't have to convert to usize at all if you want to use u16.

I think it would be best to keep type safety of indices by parametrizing slices by their index type: Slice<T, usize>, Slice<T, u16> etc, and same with arrays and Vec.

15

Is getting a function pointer as usize a reasonable enough thing to do to really worry about it? I'm hard pressed to imagine a circumstance where you need to do integer operations on function pointers—they exist somewhere, I'm sure, but they're rare enough that requiring something like as *const () as usize or as *const _ as usize (where the type of _ would be inferred to be the unique type of the function in question) to avoid a lint seems perfectly reasonable to me.

1 Like
16

If it could be as fn _ or something then sure, but forcing as fn(_, _, _) -> _ as usize is kinda annoying.

But maybe it'd be fine if we only insisted on that if the return type was something valid to as, so that say String::max as usize doesn't lint since that's far less likely to be a mistake.

(It also depends a bunch of how strongly we can lint on it. I'd like to make isize::max_value as usize a deny, but I think to be comfortable having it be deny I'd want to give people a different thing to reach for by default for the case where they want the address of the function. If it's just a warn then maybe it's ok without that.)

17

After shooting myself in the foot several times, I would go as far as require unsafe to cast a larger integer type to a smaller type, unless it can be proven at compile time to be a safe conversion, for example:

let x: u16 = ...;
... (x & 0xFF) as u8;
... unsafe { x as u8 };

It's not unsafe in the memory sense, but it is semantically. Maybe we need another, less harsh, keyword for that — trustme or something.

18

Perhaps truncme if that's the hazard.

19

There are various clippy lints to entirely deny as casts. Your case specifically is covered by cast_possible_truncation.

5 Likes
20

Indexing by other integer types would be quite convenient, and eliminate a lot of as usize boilerplate.

But that doesn't cover cases that actually need truncation. I rarely want wrapping truncation specifically, but I often need truncation without additional overhead of try_from().unwrap(). That branch can be prohibitively expensive due to preventing autovectorization and other code motion. u8::try_from(u16 >> 8) may be free, but T::try_from(sum / len) may not. Checking when it optimises out is an overhead, both cognitive and taking actual time. Having said that, having truncation with a debug-only overflow check would be ideal, similar to arithmetic overflow checks.

3 Likes