[safety-dance] Remove some unsound or fragile usages of unsafe

[danielhenrymantilla]

(This is currently a WIP PR, for other members of safety-dance to help audit and improve the code)

This PR aims to tackle the issues raised in rust-secure-code/safety-dance#8 :

• unsafe { ... } blocks and unsafe impls had no

  // # Safety
  //
  //   - ...

  annotations, which makes it easy to forget or miss safety requirements / invariants to uphold, be it when the code is written, or later when the code is modified; it also makes quickly auditing the code harder. That's why, imho, it is very important to have such safety comment annotations;

• The usages of unsafe corresponded to four cases:

  • .get_unchecked indexing for pe/data_directories array getters.

    • These have been factored in a macro to avoid code repetition, and within that macro compile-time assertions have been added so that the code is robust to changes;

  • unsafe fn exported to the API;

    • These have not been audited (yet): by virtue of being marked unsafe, users of the library must explicitely opt into calling these unsafe functions, and it is thus (mainly) their responsibility to do it correctly, hence making it a low-priority fix.

  • unsafe impl (for ::plain::Plain).

    • These (implicitly) relied on each field also being Plain, such as integers, but where also used with a macro (à la C++ template): hence a static assertion has been added in those cases to ensure that the "template" type parameter is indeed Plain.

    • Plain on its own offers non-unsafe transmute-based APIs to go into and from slices of bytes; the soundness responsibility of it falls down on the ::plain crate and has not been audited either. Ideally, all the ::plain usages could be replaced by ::zerocopy, since thanks to the #[derive(...)] it offers compile-time checks for the soundness of these impls.

  • unsafe { fd.read_exact(plain::as_mut_bytes(&mut /* some structure */)?); }

    • These have been the main change of the PR: with the compile-time checked #[derive(AsBytes)], we get access to a non-unsafe ::zerocopy::AsBytes::as_bytes_mut for equivalent functionality.

    • This, in turn, has showed that there were some structures that did have padding, which has been removed with the #[repr(C, packed)] annotation, and the appropriate ptr::read_unaligned-based getters.

[Shnatsel]

Does using get_unchecked on PE data directories improve performance in any measurable way? These look like they would be trivially optimized out by the compiler.

Anyway, that can be addressed in a follow-up PR.

[philipc]

Ideally, all the ::plain usages could be replaced by ::zerocopy, since thanks to the #[derive(...)] it offers compile-time checks for the soundness of these impls.

Yep, we don't want to be using both plain and zerocopy: either use plain for everything or zerocopy for everything.

[philipc]

This, in turn, has showed that there were some structures that did have padding, which has been removed with the #[repr(C, packed)] annotation, and the appropriate ptr::read_unaligned-based getters.

From what I can tell you have only added packed on unified structs. These structs don't currently implement plain, and they shouldn't either because they don't correspond to binary file formats. So please don't add packed and zerocopy support for these.

[danielhenrymantilla]

Yep, we don't want to be using both plain and zerocopy: either use plain for everything or zerocopy for everything.

Actually, after discussing with some people of the unsafety-dance, we may go back to using just plain, since using ::zerocopy increases compiles times due to it pulling in the big syn and quote dependencies.

::zerocopy main purpose was to soundly implement an unsafe marker trait (for non-unsafe .as_bytes_mut()) thanks to two compile-time checks:

• the struct needs to be #[repr(C)]

• the size of the resulting struct equals the sum of the sizes of each field (this implies there not being padding)

But it turns out that these checks can be made with just a classic macro_rules! (declarative) macro.

[philipc]

Note that goblin already uses proc macros via scroll (and from memory there's some unsafe usage in the writing parts of scroll).

[philipc]

Although, the plain structs are also supported with no_std, which doesn't use scroll.

[le-jzr]

But it turns out that these checks can be made with just a classic macro_rules! (declarative) macro.

That is possible?
Would it make sense to add such a macro to plain? I am its author.

[m4b]

Hello, thanks so much for the PR! This is super great. I’m ok with removing the pe unsafe. I’m not sure the compiler will optimize to the same tho.

In general the changes are great and I really appreciate the audit !

Now for the bad news: I just need to say that I’m pretty uncomfortable with taking on more deps in this crate. Eg static assert and zerocopy.

Unless there’s really compelling arguments I think zerocopy is a no-go; it has deps and I don’t really know if they’ll add more.

I added plain because @le-jzr was fixing with a lot of the unsafe and wrote a simple crate. I still sort of wish it was just added directly but they semi promised to not add more deps to it, it’s very lightweight so it’s ok.

If that changes I’ll just probably remove it and delete unsafe “raw” apis, and then release a new major version going forward like that.

As for scroll, I’m in control of its deps and it will never get anymore. Ideally in some ideal future it doesn’t use syn either. If I really felt like it, I could manually implement scrolls tryfromctx and goblin would have a leaf single dep, which would never get more.

This might seem a little strong but I’m literally in the process of dealing with a bunch of uncontrolled dependency bloat and this is how it starts :) if we can make do without it, we’ll make do without it, up to some “reasonable limit”, in this crate.

Anyway just wanted to be clear about that before this proceeds further or people do a bunch of work (and then get (rightfully) annoyed when I say something like what I’ve said above); again really appreciate the PR and some of the insights that’s already been gleamed here :)

If something’s unclear please let me know

[m4b]

Note that goblin already uses proc macros via scroll (and from memory there's some unsafe usage in the writing parts of scroll).

Yes scroll has I believe 10 unsafe blocks, only in this file: https://github.com/m4b/scroll/blob/eeeeb8d1cc467400479f0d88aed9ef970268bf2f/src/ctx.rs#L167

Most are copy_nonoverlapping calls, which are guarded by asserts for the src and dst, and return with an error if they're too big.

There is 1 transmute: https://github.com/m4b/scroll/blob/eeeeb8d1cc467400479f0d88aed9ef970268bf2f/src/ctx.rs#L169

I believe it's safe.

This was the only assert I was worried about: https://github.com/m4b/scroll/blob/eeeeb8d1cc467400479f0d88aed9ef970268bf2f/src/ctx.rs#L290

(e.g., it takes sizeof instead of the $size argument, could be a copy-paste bug, but probably ends up being equivalent?)

The only other remaining block I'm somewhat unsure of is the CStr stuff; https://github.com/m4b/scroll/blob/eeeeb8d1cc467400479f0d88aed9ef970268bf2f/src/ctx.rs#L545

If it's unsafe for nul terminator reasons, that is checked, and errors out otherwise; I don't think CStrs have unicode requirements?

[danielhenrymantilla]

The ::zerocopy and ::static_assertions dependencies have been removed! (no more syn and quote)

Instead, a reduced version of ::zerocopy's compile-time checks based only on macro_rules! has been added within ::goblin (@le-jzr you might be interested for plain, since your request for adding a derive that implements it when all the fields are Plain themselves will be very similar to what is currently done).

---

Yes scroll has I believe 10 unsafe blocks, only in this file: https://github.com/m4b/scroll/blob/eeeeb8d1cc467400479f0d88aed9ef970268bf2f/src/ctx.rs#L167

We will audit it too, then: rust-secure-code/safety-dance#36

I don't think CStrs have unicode requirements

Indeed they do not

[Shnatsel]

Zerocopy is still used here, and in many other places. Did you forget to git push?

Disregard this, I was confused

[danielhenrymantilla]

Yes, I have named the "one-file-inlined crate" as the original crate, so as to give credit where it is due 😅

[Shnatsel]

Still unsafe impl instead of #[macro_rules_derive(AsBytesAndFromBytes!)]. Also a forgotten git push?

[danielhenrymantilla]

Ideally the ::plain crate would implement the pattern of this PR for its Plain type, so as to derive it without requiring unsafe (thanks to a macro_rules!).

Then I will be able to update this PR to use the new ::plain and avoid requiring this semi-manually checked unsafe impl Plain (cc @le-jzr)

• (At least all these unsafe impls do have // # Safety annotations on top of it).

At which point there would not really be any unsafe left, except for the unsafe fns and the data_directories indexing (which can be studied in a future PR) 🙂

[m4b]

we can remove the data_directories indexing, i don't really care that much, unless someone else has some concerns

[m4b]

ping anyone still working on this?

[danielhenrymantilla]

Yes sorry I have been quite busy this last month. I had (and still have!) the plan to work on scroll to move some of the logic of this PR there, and then use that, so there is still a bit of work to get a polished PR 🙂

[m4b]

No worries, just checking in !

[m4b]

So there's already some merge conflicts here, and last activity was some time ago (first PR was in august). Why am i bringing that up? Well the bad news is I'm (finally) going to rustfmt this repo, which is long, long overdue.

[m4b]

closing as no activity, feel free to reopen/rebase with updates