Hidden unsafe due to unintentionally abusable macros and include

Slight nit, invoking the macro itself isn't/shouldn't be the unsafe thing here, it's specific to the expression passed in the argument, so to reduce the size of the unsafe block and be most explicit this would be optimal use in my opinion:

alloc_pages!(unsafe { *(0x41414141 as *mut usize) } );
1 Like

Is it the right answer to

  • color code blocks inside unsafe
  • carry over code color when substituting values inside a macro
  • not allow un-colored code inside unsafe after macro expansion

.

  • opt-in to this check via a rustc flag initially

?

This is something that bothers me as well, particularly in the context of #![forbid(unsafe_code)], which still allows crate-local unsafe code via these "escape hatches", and therefore provides a misleading sense of what it actually does.

See also unsafe attributes which work in a #![forbid(unsafe_code)] context, e.g. #[no_mangle] (and there are many more than that).

Perhaps at an edition boundary #![forbid(unsafe_code)] could be changed to disallow all of these as well.

Or perhaps things could be take a step farther: at an edition boundary, #![allow(unsafe_code)] could be required to enable them, as was proposed before here:

2 Likes

This specific example is fixed on nightly:

Which implies that updates to do the same for other such constructs would likely be accepted too.

3 Likes

I think making macro_rules! respect hygiene with respect to unsafety is a reasonable change for an upcoming edition (or at the very least macro macros, "macros 2.0").

(And I argued against the plutonium advisory, in favor of the safe! macro (which is just aliased unsafe).)

Given that it's purely safe code to rm -rf / --no-preserve-root, though, I don't think "finding potentially malicious code with rg unsafe" is a good argument for it.

7 Likes

You can't rely on the unsafe keyword to check safety of crates. There are many other "safe" ways to inject arbitrary code and evade the checks:

This is because unsafe is not a security boundary. It's a lint for double-checking programmer's own assumptions, and not a sandbox.

7 Likes

Agreed, I never claimed to rely on grepping for unsafe to be sufficient for a review. I specifically addressed that hiding vulnerabilities in safe Rust is obviously possible, but it's not relevant to this discussion.

The idea is that because Rust makes this useful distinction between safe and unsafe dialects, as a reviewer I would assume that Rust code isn't doing certain things if there is no unsafe block nearby, since the language is supposed to forbid this. For example, I would assume that a line such as x = y isn't assigning to a global variable if it's not inside an unsafe block, but using this trick it could. This concept of 'hidden unsafe' provides opportunities to create significantly harder to spot backdoors than with safe Rust.

2 Likes

But there's lots of "hidden" unsafe everywhere. vec.push(x) contains hidden unsafe. Why push!(vec, x) shouldn't be allowed to?

The key point is that we're introducing completely new unsafe code without an unsafe block, not calling existing unsafe code that has been thoroughly reviewed. The difference between my example and yours should be clear - you can't use push! to introduce new unsafe:

alloc_pages!(*(0x41414141 as *mut usize));
push!(vec, x)

To introduce the dereference here a new unsafe block would be required, which is a good thing:

push!(vec, unsafe { *(0x41414141 as *mut usize) })
6 Likes

Maybe Rust should support unsafe macro_rules! to let people write macros that aren't safe to call?

1 Like

It absolutely is different, as a function, the code in alloc_pages is self contained and can theoretically be reviewed for upholding safety for all inputs. As a macro, arbitrary new unsafe code can be inserted, which obviously won't be caught in a review of the crate since the code isn't there to be seen; in the context of reviewing the consumer of the macro, it would be very difficult for a reviewer to spot as there's no unsafe block showing that it's using the unsafe dialect.

1 Like

edit: nevermind, I misunderstood the issue

This is about safety hygiene in macros. It seems like a reasonable request, IMO. I see it as more of an exercise in intentional unsafety. Of course there are a large number of opportunities for bad things to happen. Just because I can "safely" exec rm -rf doesn't mean that unsafe is now totally worthless. unsafe has its purposes. And I think it's worth being intentional when it comes to using unsafe.

But there's been a lot of rapid back-and-forth here. I think it's worth taking a little break to slow things down so responses can be more methodical.

4 Likes

Look at the macro defined in the byteorder crate. It is not safe to call for all possible arguments. Merely, all such uses of it in the module are safe. If that macro could be defined as a function and it wasn't defined with unsafe, then we would call it unsound.

Perhaps the macro should not write the unsafe block itself and instead require the caller to do it. But this is a sub-optimal work-around.

1 Like

Disagree - the author intended for the macro to be safe to use as they didn't expect new unsafe code to be insertable - they wouldn't have marked the macro as unsafe even if that was a supported feature, because this isn't their intent for it to be used that way. If the default of the language was that a new unsafe block was required here as I'm proposing, then the macro would have been safe:

alloc_pages!(unsafe { *(0x41414141 as *mut usize) } );

Agree with the general sentiment here. I've previously raised that using unsafe in a macro is difficult to reason about because of things like overloading basic traits like arithmetic and comparisons. Generics is an interesting point to consider too.

1 Like

I'm not disagreeing with the proposal to be able to mark macros as unsafe, like we can do with functions; I think this is a good idea too.

Specifically, I think it's worth recognizing that expressions are not just traditional function inputs that can potentially tweak the function's code to be unsound, they are standalone code, and as code they should be subjected to requiring their own unsafe block if they want to use the unsafe dialect.

1 Like

I would have in byteorder. And if I didn't, I would consider it an oversight. It would be unsound and thus a bug to fix.

1 Like

Another workaround I use is to include unsafe_ in the name of the macro, so for example the byteorder macro would be called unsafe_write_num_bytes. I find this especially useful when I want to use macros in different modules.

1 Like

Let's take it back to the mmap example. The point is that this is only unsound because you can insert new unsafe code into the $length expression. To fix the macro to behave as the author intended, the trick should be fixed - not to mark the whole macro as unsafe:

#[macro_export]
macro_rules! alloc_pages {
    ($length:expr) => (
        let x = $length;
        unsafe {
            mmap(0 as *mut c_void, x, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0)
        }
    );
}
12 Likes

Oh, I've missed that detail before. I see your point now.

1 Like