PE: preparations for a writer outside of Goblin #389

RaitoBezarius · 2024-01-23T01:42:46Z

Hi there @m4b, as we discussed it in #381. Here's are commits, more in the mind of:

Preparations for 1.0 and breaking changes: this PR should be good to go for a patch release I'd say.
Enable to write an external writer given enough data

For now, I will focus on making it possible to write final executables, not object files. I believe this may require more efforts from someone who is really interested into writing a PE compiler or PE object files with this ecosystem.

This is a bunch of QoL improvements for the writer that I'm preparing in another repo, if it's good, and I get myself to 0.x something, I will try to see if it makes sense to send it to faerie or something that would be the good place for such a thing.

In the time being, to unblock on the reviews and merging, here's a bunch of easy things.

I make some stuff pub on purpose because I don't believe it's a big deal to let the user observe it, please correct me if you disagree, and I can try to find a way to make it better.

As far as I can tell, this PR has no breaking changes. I have more in stock which I am fleshing out to make this easy for you.

src/pe/data_directories.rs

src/pe/utils.rs

This is a generic function to perform various alignments, this avoids reinventing locally the magic alignment dance and reusable for consumers who may themselves align things.

src/pe/certificate_table.rs

src/pe/section_table.rs

src/pe/utils.rs

RaitoBezarius · 2024-01-23T02:11:24Z

https://github.com/RaitoBezarius/ifrit will be my model for the meta writer PE, if it graduates, I will send it your way in faerie if it makes sense. :)

m4b · 2024-02-04T23:56:03Z

lol i love the name :) so just catching up here again, iiuc, this has no breaking changes, but you'd like this patch merged right?

m4b

mostly questions about pub

src/pe/certificate_table.rs

src/pe/mod.rs

src/pe/utils.rs

RaitoBezarius · 2024-02-11T20:43:22Z

Addressed your comments @m4b, please take a look again.

Right now, it's mostly working, except I'm chasing an alignment issue somewhere... which appeared since the split.

m4b · 2024-02-12T00:46:32Z

Cool let me know when you found it ! otherwise this is looking pretty good :)

This is a simple function to assemble a template section table with zeros in the unpredictable fields, it's up to the caller to carry on the fill procedure, e.g. a writer structure which can do the layout. It can automatically set the virtual address and size based on another binary to append it, only the on-disk offsets are left to the caller.

Previously, we were handling the offset ourselves, but we can just use `gwrite_with` to let the offset be moved. We don't need to use the "read" offset, this is only a distraction and can get wrong results sometimes. We only need to write in the order and assume linearity.

It is now easy to peek into the data of a data directory and list all directories with their originally parsed offsets.

> 216/232 8 Reserved, must be zero According to the table mentioned in https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#optional-header-data-directories-image-only In the past, we returned errors in case they were present.

Relative virtual address to disk offset is a classical operation to perform for various things, it is likely this operation won't change and if it changes, we can keep it around and deprecate it quite easily in the next release. An example for external consumers: compute debug directory data offsets from RVAs to write a debug directory.

- derives additionally `Debug` on it for logging - derives additionally `Pwrite`, `SizeWith` for good developer experience with scroll on it - provides a natural type alias in accordance with `WIN_CERTIFICATE` name

We forgot those but they come handy in the UEFI ecosystem when you are reading UEFI signature lists.

There's no reason to render it as private, this enable a user to peek inside the raw structure and perform more complicated reading without holding the original bytes around. Nonetheless, we expose it via a function to make it easier to deprecate it or modify as we need.

It can be hard to debug why your writer is not working as intended, here are some `debug!` traces to help in that endeavor.

…e certificates It's possible that a user may pass an improperly created attribute certificate and the write will cause all sorts of failure. We sprinkle some `debug_assert!` to avoid this.

RaitoBezarius · 2024-02-12T23:30:26Z

@m4b I believe we are ready to go, I tracked the last bug which was not a bug in Goblin but my tests being overzealous and wanting equality between unaligned certificates and aligned certificates, so I forked out to this sort of things: https://github.com/nix-community/goblin-signing/pull/6/files#diff-85ad85dc9b5387ec57ec5c58ab8880b11f7897b4e4cf3bb78f02907e630891d3R26-R53

JohnScience · 2024-03-06T00:09:54Z

Hi there @m4b, as we discussed it in #381. Here's are commits, more in the mind of:

Preparations for 1.0 and breaking changes: this PR should be good to go for a patch release I'd say.

Enable to write an external writer given enough data

For now, I will focus on making it possible to write final executables, not object files. I believe this may require more efforts from someone who is really interested into writing a PE compiler or PE object files with this ecosystem.

This is a bunch of QoL improvements for the writer that I'm preparing in another repo, if it's good, and I get myself to 0.x something, I will try to see if it makes sense to send it to faerie or something that would be the good place for such a thing.

In the time being, to unblock on the reviews and merging, here's a bunch of easy things.

I make some stuff pub on purpose because I don't believe it's a big deal to let the user observe it, please correct me if you disagree, and I can try to find a way to make it better.

As far as I can tell, this PR has no breaking changes. I have more in stock which I am fleshing out to make this easy for you.

Since you mention breaking changes, there are many that should be made for goblin::pe, in my opinion. Right now, I'm documenting everything there and pointing out stuff that in my opinion is a bad idea. There are many such things.

m4b

I think this is ok, other than the optional thing and the unwrap being infallible (which can be fixed backwards compatibly); I will wait till tomorrow, if no response, will merge; thanks for your patience @RaitoBezarius :)

I will note this does make a lot more functions pub, which is more maintenance burden, but seems good overall so i'm ok with it.

m4b · 2024-03-10T07:16:24Z

src/pe/data_directories.rs

+    /// Given a view of the PE binary, represented by `bytes` and the on-disk offset `disk_offset`
+    /// this will return a view of the data directory's contents.
+    /// If the range are out of bands, this will fail with a [`error::Error::Malformed`] error.
+    pub fn data<'a>(&self, bytes: &'a [u8], disk_offset: usize) -> error::Result<&'a [u8]> {


I wonder if this should have just returned an Option ? sort of like get() for arrays; e.g., is the error message actually useful? there's only one parameter passed in, so similar to an index, it means it's the wrong value basically (bad offset). anyway it's already here so it's probably fine, other than it having to allocate for the format.

m4b · 2024-03-10T07:18:41Z

src/pe/section_table.rs

+                .last()
+                .map(|last_section| last_section.virtual_address + last_section.virtual_size)
+                .ok_or(0u32)
+                .unwrap();


is this unwrap infallible?

m4b · 2024-03-10T07:32:04Z

Hi there @m4b, as we discussed it in #381. Here's are commits, more in the mind of:

Preparations for 1.0 and breaking changes: this PR should be good to go for a patch release I'd say.

Enable to write an external writer given enough data

For now, I will focus on making it possible to write final executables, not object files. I believe this may require more efforts from someone who is really interested into writing a PE compiler or PE object files with this ecosystem.
This is a bunch of QoL improvements for the writer that I'm preparing in another repo, if it's good, and I get myself to 0.x something, I will try to see if it makes sense to send it to faerie or something that would be the good place for such a thing.
In the time being, to unblock on the reviews and merging, here's a bunch of easy things.
I make some stuff pub on purpose because I don't believe it's a big deal to let the user observe it, please correct me if you disagree, and I can try to find a way to make it better.
As far as I can tell, this PR has no breaking changes. I have more in stock which I am fleshing out to make this easy for you.

Since you mention breaking changes, there are many that should be made for goblin::pe, in my opinion. Right now, I'm documenting everything there and pointing out stuff that in my opinion is a bad idea. There are many such things.

@JohnScience if you have some suggestions here feel free to open up a tracker issue/wishlist/things you think are wrong and we can investigate :) especially if you think there are crucial things that need fixing and will break backwards compat, now is the time; if they are large overarching changes to the architecture of goblin's PE though, there is realistically not a good chance it will be merged, but hard to say :)

RaitoBezarius force-pushed the preps-for-ifrit-1 branch 2 times, most recently from e744736 to 97d7bda Compare January 23, 2024 01:43