Write support for PE binaries #361

RaitoBezarius · 2023-03-11T16:51:33Z

Write support for PE binaries in a basic form

This implements reserialization for many in-memory structure with PE semantics.
It does not yet enable you to modify a PE and write it because you will need many modifications to the existing structure to avoid breaking the PE.

But you can already modify existing bytes this way.

I learned the hard way that this code may be insufficient to write the worst PE binaries out there (look the Corkami repositories on pocs and look for the PE there), I deem this to be out of scope for now as it is very hard to get all of this right.

Also, I don't think that identity rewrite should be a goal for anything else than "basic" and "simple" (read: nice) PEs, which can be accomplished in this PR with NixOS PE binaries for UEFI (and I tested a hundred of them).

Testing ways:

cargo run --example rewrite_pe /boot/EFI/Linux/some-uefi.efi /tmp/uefi.efi

As a follow-up, I will work on:

section manipulation to implement "objcopy"-like features
signature manipulate to implement "pesign"-like features

TODO:

Out of scope :

Update checksums if necessary?
Simple test with a NixOS PE binary that we can rewrite effectively the same
Canonicalization/Minimization of PEs ? (out of scope)

Relevant issues/PRs:

#277
#357

RaitoBezarius · 2023-03-13T09:31:20Z

@m4b you did say you didn't want more dependencies in goblin, are you okay with some of them in examples code? (to showcase how to use external crates for example?)

src/pe/mod.rs

RaitoBezarius · 2023-05-28T15:52:05Z

src/pe/data_directories.rs

+        let inner = bytes.gread_with(offset, scroll::LE)?;
+        Ok(DataDirectory {
+            inner,
+            offset: *offset,


Could I have your feedback on this @m4b before I go further?
I am not sure how to compute offset except by getting at parsing time.

RaitoBezarius · 2023-05-28T16:27:21Z

@m4b What do you expect from me for the testing strategy of this PR? Should I add various PE binaries or just some binaries and test rewriting them or test writing binaries from scratch for some features?

I feel like getting this feature to perfect polish will take some hoops and I am wondering if we can merge a "beta" version and stabilize it further.

src/pe/header.rs

m4b · 2023-05-29T23:20:41Z

@m4b What do you expect from me for the testing strategy of this PR? Should I add various PE binaries or just some binaries and test rewriting them or test writing binaries from scratch for some features?

I feel like getting this feature to perfect polish will take some hoops and I am wondering if we can merge a "beta" version and stabilize it further.

I'm ok with merging a beta version. I think I'd like to do a release before that, this way you can work off of master and hopefully have a full featured version ready for a crates release?

m4b · 2023-05-29T23:21:54Z

And re binaries and testing this has always been a source of tension. For now I'd recommend checking in u8 slices in test code and not binaries as we have in other places.

m4b · 2023-07-05T04:16:34Z

@RaitoBezarius friendly ping/triage wondering what work is left to take this about of WIP and getting something ready to merge? :)

RaitoBezarius · 2023-07-08T22:32:37Z

@RaitoBezarius friendly ping/triage wondering what work is left to take this about of WIP and getting something ready to merge? :)

I decided with @baloo to take this PR for a super crash test in https://github.com/RaitoBezarius/goblin-signing/tree/signed-data where we start signing binaries arbitrarily combining everything. I guess it will be a good test if I can wire this down to our integration testing with UEFI.

m4b · 2023-07-09T00:57:25Z

awesome sounds good to me and let me know how it goes; lots of good work here, don't want it to get stale :)

Lunarequest · 2023-09-02T14:35:50Z

is there any update on this? whats is left to be done(other then rebasing)

RaitoBezarius · 2023-09-02T16:56:09Z

Life has been busy and I didn't want to fuck up this by merging too early, all is left is testing two usecases "end to end":

adding new sections or new section data, usecase: replacing systemd's ukify or objcopy for creating UKIs binaries in UEFI land
signing binaries via Rust cryptography ecosystem, usecase: HSM/TPM signature of SecureBoot binaries

I feel like there should be like a final push of work, one week or something, but I am on vacations and just after I will be busy under conferences, hopefully, I aim to close this before the end of September.

m4b · 2023-09-02T18:19:10Z

Great thank you for the update !

m4b · 2023-09-02T18:20:05Z

And looking forward to seeing this merged if wasn’t clear, thanks @Lunarequest for pinging in status !

RaitoBezarius · 2023-10-02T17:51:47Z

I'm soon there, I want to rework the commit history, look for an easy basic testing strategy and I want to go for the merge then.

RaitoBezarius · 2023-10-02T17:53:28Z

Planned follow-ups:

add/remove/edit a section in a PE, this will require a superstructure called PEWriter as it is impossible to write a new section without relaying out all the PE's sections, example: "minimal objcopy" in Rust for PEs, application: replace ukify and other shenanigans.
add/remove/edit a signature in goblin-signing subcrate, example: "minimal sign this binary" in Rust using local keys, application: replace pesign and other shenanigans.

m4b

This is really amazing work @RaitoBezarius thank you so much! And thank you for committing the example too; tbh i'd really prefer not to have dev dependencies introduced, but it's fine I guess. I'll let this sit for a few hours, but otherwise I'm ready to merge, great stuff.

So as far as "feature completeness", would the PE pwrite implementation allow, e.g., writing out PE object files for a toy compiler backend, similar to what faerie did?

m4b · 2023-11-20T21:36:28Z

src/pe/header.rs

@@ -163,14 +262,24 @@ impl CoffHeader {
    }

    /// Return the COFF symbol table.
-    pub fn symbols<'a>(&self, bytes: &'a [u8]) -> error::Result<symbol::SymbolTable<'a>> {
+    pub fn symbols<'a>(&self, bytes: &'a [u8]) -> error::Result<Option<symbol::SymbolTable<'a>>> {


Hmm are you saying we should construct an empty symbol table and not an optional in this case? If yes, it might be worthwhile to not make this optional, as it might be changed back if we had the option of returning an empty symbol table / have scroll not crash on offset == 0

m4b · 2023-11-20T21:37:55Z

src/pe/header.rs

@@ -163,14 +262,24 @@ impl CoffHeader {
    }

    /// Return the COFF symbol table.
-    pub fn symbols<'a>(&self, bytes: &'a [u8]) -> error::Result<symbol::SymbolTable<'a>> {
+    pub fn symbols<'a>(&self, bytes: &'a [u8]) -> error::Result<Option<symbol::SymbolTable<'a>>> {


I guess what i'm saying does the optionality signify important semantic information to the user that the thing you are trying to get might not "be there", as opposed to working around a scroll bug. If 0 signifies it isn't "there"/it isn't required, then let's keep it optional, yes.

m4b · 2023-11-20T21:38:35Z

src/pe/mod.rs

@@ -3,7 +3,12 @@

 // TODO: panics with unwrap on None for apisetschema.dll, fhuxgraphics.dll and some others


unrelated but i wonder if this comment still true

That's a good question, I don't have those Windows binaries on my hand, but I can check them.

m4b · 2023-11-20T21:40:15Z

src/pe/mod.rs

+    /// has been deprecated.
+    pub symbols: Option<symbol::SymbolTable<'a>>,
+    /// The string table, they don't exist if COFF symbol table does not exist.
+    pub strings: Option<strtab::Strtab<'a>>,


ok I think this comment answered my question about optionality, namely the strtab and symbol table is actually optional

m4b · 2023-11-20T21:43:08Z

src/pe/section_table.rs

@@ -171,7 +208,7 @@ impl ctx::SizeWith<scroll::Endian> for SectionTable {
    }
 }

-impl ctx::TryIntoCtx<scroll::Endian> for SectionTable {
+impl ctx::TryIntoCtx<scroll::Endian> for &SectionTable {


interesting, i wasn't aware you could do this; one longstanding issue in scroll (and major 1.0 blocker) was to allow pwrite impls for reference types, but it hadn't occurred to me the user could just write the TryIntoCtx impl in this manner, i'm not sure why it never was brought up in that issue...

Yeah, I realized that there's almost no reason to give owned types to the serialization process given that you are really just reading those memory locations and discarding them, having always a reference makes them cheap to move around.

src/pe/symbol.rs

src/strtab.rs

m4b · 2023-11-21T04:34:59Z

@baloo did you want to review? Otherwise I'm going to merge in an hour or so

baloo · 2023-11-21T06:56:48Z

I've tried to help @RaitoBezarius on this (he still deserves 120% of the credit here! But we've been bouncing off ideas on matrix). The code looks good to me.

RaitoBezarius · 2023-11-21T08:48:59Z

This is really amazing work @RaitoBezarius thank you so much! And thank you for committing the example too; tbh i'd really prefer not to have dev dependencies introduced, but it's fine I guess. I'll let this sit for a few hours, but otherwise I'm ready to merge, great stuff.

Thank you so much, the next PR #381 is going to be also very good. :-)
stderrlog is not necessary per se, but I am not aware of an easier way to make the logs of log appear on stdout easily, we can remove it if you prefer.

So as far as "feature completeness", would the PE pwrite implementation allow, e.g., writing out PE object files for a toy compiler backend, similar to what faerie did?

So right now, it only focuses on PE executables, not PE object files (i.e. COFF), technically, if you had a PE object files library, you could write your COFF files and use this to assemble the final PE executable.

In the future, if someone adds COFF tables support (strings, symbols, etc.), you could probably write PE COFF files by filling the structures yourself, though, for an elegant and nice compiler, I recommend following the methodology in #381 where you have a meta structure PEBuilder or COFFBuilder and you assemble the final PE which you can write out via Scroll.

You need a lot of "layout"-ing when building such things from scratch, which can be a PITA when you want to focus on just giving a high level representation of your binary (here's my sections, this one is executable, attach some certificates here, etc.)

I am not planning to do the COFF thing in the near future because it creates a certain higher level of complexity and all I need is the PE executable stuff, but, I may come back later for this as I wonder if LLVM may someday adopt Rust and this library would probably be handy for a real compiler. :)

RaitoBezarius · 2023-11-21T08:49:25Z

And @baloo more than helped me! He stayed with my ramblings about PE and we fixed a lot of stuff, if anything, he's definitely a co-author for me :).

Required to rematerialize a whole PE, otherwise, it is a corrupted PE.

…afely It now returns a Cow as we need to transfer ownership whenever we have virtual size > size of raw data, which forces us to pad with zeroes according to the PE specification.

It can be used to zero-extend any buffer to make its length aligned on some boundary, e.g. quadwords, for attribute certificates. It is designed to be "zero cost" if you have no alignment to do, which will result in no-allocation!

It enables a lossless conversion from 64 bits to 32 bits headers except on platforms… that are strange.

This tucks `DataDirectory` with a Deref-style and expose `offset` for internal consumers (this crate) and offers write support for data directories.

All that is needed is to import the allocation `Vec`.

This is a trivial rewriter that performs the identity transformation.

To further improve the debuggability, a non-overlapping sanity check is added as a debug_assert!.

Previously, I (we?) thought that COFF symbols/strings were always there. In fact, this is not the case and they are deprecated according to https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#coff-file-header-object-and-image > The file offset of the COFF symbol table, or zero > if no COFF symbol table is present. This value should be zero for an image > because COFF debugging information is deprecated.

We had the write certificate logic in the `TryIntoCtx`, but it makes sense to have it separated to let consumers call it manually.

RaitoBezarius · 2023-11-21T08:57:51Z

src/pe/header.rs

    }

    /// Return the COFF string table.
-    pub fn strings<'a>(&self, bytes: &'a [u8]) -> error::Result<strtab::Strtab<'a>> {
+    pub fn strings<'a>(&self, bytes: &'a [u8]) -> error::Result<Option<strtab::Strtab<'a>>> {
+        // > The file offset of the COFF symbol table, or zero if no COFF symbol table is present.


You can also see it here (the optionality) @m4b
Let me know if you prefer the doc in a different way

src/pe/data_directories.rs

RaitoBezarius · 2023-11-21T10:52:20Z

Whew! What a PR, thank you @m4b for your patience. :)

m4b · 2024-01-01T01:08:47Z

released in 0.8.0, happy new year!

RaitoBezarius marked this pull request as draft March 11, 2023 16:51

RaitoBezarius force-pushed the write-certs branch from 240abc0 to 4b718e3 Compare May 1, 2023 02:34

baloo reviewed May 5, 2023

View reviewed changes

src/pe/mod.rs Outdated Show resolved Hide resolved

RaitoBezarius commented May 28, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

baloo reviewed May 28, 2023

View reviewed changes

src/pe/header.rs Outdated Show resolved Hide resolved

baloo reviewed May 28, 2023

View reviewed changes

src/pe/header.rs Outdated Show resolved Hide resolved

baloo reviewed May 28, 2023

View reviewed changes

src/pe/header.rs Outdated Show resolved Hide resolved

RaitoBezarius force-pushed the write-certs branch 4 times, most recently from ff7b1ad to aace8e5 Compare October 1, 2023 01:36

RaitoBezarius force-pushed the write-certs branch 3 times, most recently from 5ab66a6 to b76248a Compare October 2, 2023 18:39

RaitoBezarius force-pushed the write-certs branch 4 times, most recently from c445107 to 5f69b48 Compare November 20, 2023 12:30

m4b approved these changes Nov 20, 2023

View reviewed changes

RaitoBezarius added 15 commits November 21, 2023 09:56

strtab: offer len method

7f53f39

pe(header): introduce full DOS header and DOS stub

a06d03f

Required to rematerialize a whole PE, otherwise, it is a corrupted PE.

pe(section_table): add data function to retrieve the section data s…

8f05971

…afely It now returns a Cow as we need to transfer ownership whenever we have virtual size > size of raw data, which forces us to pad with zeroes according to the PE specification.

pe(utils): add a zero-extension padding helper

cd7a238

It can be used to zero-extend any buffer to make its length aligned on some boundary, e.g. quadwords, for attribute certificates. It is designed to be "zero cost" if you have no alignment to do, which will result in no-allocation!

pe: implement write support for AttributeCertificate

ac67829

pe: implement write support for DataDirectories

65ac6f2

pe: implement write support for Header

b6debe4

It enables a lossless conversion from 64 bits to 32 bits headers except on platforms… that are strange.

pe: implement write support for SymbolTable

3fdf547

pe: implement write support for data directories

e4c0b9f

This tucks `DataDirectory` with a Deref-style and expose `offset` for internal consumers (this crate) and offers write support for data directories.

pe: implement write support for SectionTable

6125c4a

All that is needed is to import the allocation `Vec`.

pe: implement write support

000d671

examples(pe): add a identity rewrite PE binary

1824672

This is a trivial rewriter that performs the identity transformation.

pe: debug assert that no overlapping sections are written

55d2580

To further improve the debuggability, a non-overlapping sanity check is added as a debug_assert!.

pe(write): factor into write_certificates

b16ed9a

We had the write certificate logic in the `TryIntoCtx`, but it makes sense to have it separated to let consumers call it manually.

RaitoBezarius force-pushed the write-certs branch from 5f69b48 to b16ed9a Compare November 21, 2023 08:56

RaitoBezarius commented Nov 21, 2023

View reviewed changes

m4b merged commit 6d664c0 into m4b:master Nov 21, 2023
6 checks passed

baloo mentioned this pull request Dec 18, 2023

pe: make sure authenticode is identical before/after signature #383

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Write support for PE binaries #361

Write support for PE binaries #361

RaitoBezarius commented Mar 11, 2023 •

edited

Loading

RaitoBezarius commented Mar 13, 2023

RaitoBezarius May 28, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

RaitoBezarius commented May 28, 2023

m4b commented May 29, 2023

m4b commented May 29, 2023

m4b commented Jul 5, 2023

RaitoBezarius commented Jul 8, 2023

m4b commented Jul 9, 2023

Lunarequest commented Sep 2, 2023

RaitoBezarius commented Sep 2, 2023

m4b commented Sep 2, 2023

m4b commented Sep 2, 2023

RaitoBezarius commented Oct 2, 2023

RaitoBezarius commented Oct 2, 2023

m4b left a comment

m4b Nov 20, 2023

m4b Nov 20, 2023

m4b Nov 20, 2023

RaitoBezarius Nov 21, 2023

m4b Nov 20, 2023

m4b Nov 20, 2023

RaitoBezarius Nov 21, 2023

m4b commented Nov 21, 2023

baloo commented Nov 21, 2023 •

edited

Loading

RaitoBezarius commented Nov 21, 2023

RaitoBezarius commented Nov 21, 2023

RaitoBezarius Nov 21, 2023

RaitoBezarius commented Nov 21, 2023

m4b commented Jan 1, 2024

		@@ -3,7 +3,12 @@

		// TODO: panics with unwrap on None for apisetschema.dll, fhuxgraphics.dll and some others

Write support for PE binaries #361

Write support for PE binaries #361

Conversation

RaitoBezarius commented Mar 11, 2023 • edited Loading

Write support for PE binaries in a basic form

RaitoBezarius commented Mar 13, 2023

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

RaitoBezarius commented May 28, 2023

m4b commented May 29, 2023

m4b commented May 29, 2023

m4b commented Jul 5, 2023

RaitoBezarius commented Jul 8, 2023

m4b commented Jul 9, 2023

Lunarequest commented Sep 2, 2023

RaitoBezarius commented Sep 2, 2023

m4b commented Sep 2, 2023

m4b commented Sep 2, 2023

RaitoBezarius commented Oct 2, 2023

RaitoBezarius commented Oct 2, 2023

m4b left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4b commented Nov 21, 2023

baloo commented Nov 21, 2023 • edited Loading

RaitoBezarius commented Nov 21, 2023

RaitoBezarius commented Nov 21, 2023

Choose a reason for hiding this comment

RaitoBezarius commented Nov 21, 2023

m4b commented Jan 1, 2024

RaitoBezarius commented Mar 11, 2023 •

edited

Loading

baloo commented Nov 21, 2023 •

edited

Loading