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

baloo · 2023-12-08T07:19:42Z

When adding the signature, the last section of the file will be padded to 8-bytes align. We need to make sure the payload we feed to a signer is always padded to 8-bytes.

This fixes signature breakage.

cc @RaitoBezarius
Thanks to @Foxboron for the heads up!

When adding the signature, the last section of the file will be padded to 8-bytes align. We need to make sure the payload we feed to a signer is always padded to 8-bytes. This fixes signature breakage.

baloo · 2023-12-09T07:45:48Z

I ended up re implementing the authenticode parser according to the specification. I think it's better that way, although a little bit more complex.

src/pe/authenticode.rs

Foxboron · 2023-12-09T11:04:47Z

src/pe/authenticode.rs

+                        if file_size > sum_of_bytes_hashed {
+                            let extra_data_start = sum_of_bytes_hashed;
+                            let len =
+                                file_size - sections.certificate_table_size - sum_of_bytes_hashed;


Suggested change

file_size - sections.certificate_table_size - sum_of_bytes_hashed;

file_size - sections.certificate_table_size + sum_of_bytes_hashed;

The spec calls for:

(File Size) - ((Size of AttributeCertificateTable) + SUM_OF_BYTES_HASHED) ^ ^

I can go with file_size - (sections.certificate_table_size + sum_of_bytes_hashed), but your suggestion is wrong I think.

It's probably wrong :) I thought it was more important to point out the error.

m4b

only question about allocation; otherwise, looks good to me, thankfully authenticode::ExcludedSections is not pub so not a breaking change

m4b · 2023-12-18T07:03:00Z

src/pe/authenticode.rs

        }
    }
 }

 pub struct ExcludedSectionsIter<'s> {
    pe: &'s PE<'s>,
    state: IterState,
+    sections: VecDeque<SectionTable>,


doesn't this mean the iterator allocates every time it's created? this is a little unusual for iterator implementations (most people expect the allocation to occur at their own choosing, e.g., from collect::<Vec<_>>() or whatever

Yes it does. But parsing PE already requires you to allocate (for the section table specifically).
I don't really have a better option here. The spec requires me to sort the section tables, and there is a non-fixed amount of them.

m4b · 2023-12-18T07:04:52Z

src/pe/authenticode.rs

+                        //    image. The NumberOfSections field of COFF File Header indicates how big
+                        //    the table should be. Do not include any section headers in the table whose
+                        //    SizeOfRawData field is zero.
+                        let mut sections: VecDeque<SectionTable> = self


i'd not expect allocations in the iterator itself; (same comment as above); is there anyway to avoid allocating? (if there's not, or it's too difficult, that's fine, just something to maybe document?)

m4b · 2023-12-18T07:10:41Z

after this change I believe we'll do a release, which will have the breaking changes we've accumulated, thanks for everyone's patience

Foxboron · 2023-12-18T12:49:13Z

@baloo Just to be sure I understand the change. Are you aligning the file size itself or are you just making sure the buffer you are feeding the singer is aligned?

The spec demands the file size itself to be aligned. The buffer you feed the signed is missing 12 bytes which is going to affect the padding you are applying.

baloo · 2023-12-18T22:58:29Z

@Foxboron The file itself might be read-only (stored in usr/lib like /usr/lib/shim/shimx64.efi). I can't make changes to the file in a parser.
What I'm trying to do here is to make sure the data I feed to the signer is padded like it would be once we append a signature to the original file (and not break its checksum).

The spec demands the file size itself to be aligned. The buffer you feed the signed is missing 12 bytes which is going to affect the padding you are applying.
Where is this 12 bytes alignment requirement? I'm only taking care of the 8 bytes alignment here.

The 8 bytes alignment problem is already handled when we actually write to the file. (this was taken care of in #361)

Foxboron · 2023-12-18T23:07:54Z

What I'm trying to do here is to make sure the data I feed to the signer is padded like it would be once we append a signature to the original file (and not break its checksum).

Sure, but you need to check the actual file size, not the buffer you are feeding the signer. I don't know Rust but

                            debug!("hashing {extra_data_start:#x} {len:#x}",);
                            let buf = &bytes[extra_data_start..extra_data_start + len];

                            self.state = IterState::Padding(buf.len());

Tells me you are not passing the file size itself, just a slice of the bytes you are hashing. The actual file size is defined as let file_size = bytes.len(); and not passed to the padding function.

Foxboron · 2023-12-18T23:20:01Z

I would probably not hide the padding between file_size > sum_of_bytes_hashed either. I don't think there is any case where this isn't true (we are always omitting at least 12 bytes I think?), but the padding should be checked unconditionally.

baloo · 2023-12-19T05:15:48Z

@Foxboron I see your point.

Padding was done by virtue of hash_size holding the data that was sent to the hasher / signer. I think the code was working because the sum_of_bytes_hashed and the certificate_table_size were 8-bytes aligned. So it will still end up being equal and I ended up with a derivative value that was equally 8-bytes aligned as file_size was.

I rewrote the logic in the last commit to be closer to the spec, and what you expect (? I think?).

Just to confirm, you meant 8-bytes aligned right? I can't find any requirement to align stuff to 12-bytes in the spec, nor any other implementation (not even in yours).

baloo · 2023-12-19T05:17:11Z

src/pe/authenticode.rs

+                            // NOTE (safety): pad size will be at most 7, and PADDING has a size of 7
+                            //                pad_size is computed ~10 lines above.
+                            debug_assert!(pad_size <= 7);
+                            debug_assert_eq!(PADDING.len(), 7);


@m4b are those debug_assert fine? I thought I'd might as well back up my claims?

If they’re some internal check that should only be false if someone refactors this code and doesn’t maintain some internal invariant afterwards then yea. If they’re like things that probably should be true but maybe aren’t if the binary is malformed, then no, should be an error

no, they are only meant to protect against a refactor.

Foxboron · 2023-12-19T07:56:05Z

Just to confirm, you meant 8-bytes aligned right? I can't find any requirement to align stuff to 12-bytes in the spec, nor any other implementation (not even in yours).

Yes, when I mention the 12 bytes I'm thinking of the bytes we are omitting from the hash buffer (checksum + optional datadir 4).

m4b

let's go!

m4b · 2024-01-01T01:18:22Z

released in 0.8.0, happy new years!

baloo · 2024-01-01T03:48:55Z

Thanks! and happy new year to you too!

pe: make sure authenticode is identical before/after signature

621bce2

When adding the signature, the last section of the file will be padded to 8-bytes align. We need to make sure the payload we feed to a signer is always padded to 8-bytes. This fixes signature breakage.

baloo force-pushed the baloo/fixup-authenticode branch from 3f6b748 to 621bce2 Compare December 8, 2023 07:27

authenticode: rework the parser according to the specification

1f1885a

baloo commented Dec 9, 2023

View reviewed changes

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

Foxboron reviewed Dec 9, 2023

View reviewed changes

authenticode: avoid mem::replace and the pending state.

d70d582

baloo force-pushed the baloo/fixup-authenticode branch from 0d8ac79 to d70d582 Compare December 10, 2023 06:31

baloo mentioned this pull request Dec 11, 2023

Guide about authenticode RustCrypto/formats#1250

Closed

m4b reviewed Dec 18, 2023

View reviewed changes

baloo added 2 commits December 18, 2023 19:48

authenticode: document allocations

a00bc43

authenticode: fixup padding

66ef2ae

baloo force-pushed the baloo/fixup-authenticode branch from 10b2f78 to 66ef2ae Compare December 19, 2023 05:11

baloo commented Dec 19, 2023

View reviewed changes

m4b approved these changes Jan 1, 2024

View reviewed changes

m4b merged commit 8b4b1b4 into m4b:master Jan 1, 2024
6 checks passed

baloo deleted the baloo/fixup-authenticode branch January 1, 2024 03:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

baloo commented Dec 8, 2023 •

edited

Loading

baloo commented Dec 9, 2023

Foxboron Dec 9, 2023

baloo Dec 9, 2023 •

edited

Loading

Foxboron Dec 11, 2023

m4b left a comment

m4b Dec 18, 2023

baloo Dec 18, 2023

m4b Dec 18, 2023

m4b commented Dec 18, 2023

Foxboron commented Dec 18, 2023

baloo commented Dec 18, 2023

Foxboron commented Dec 18, 2023

Foxboron commented Dec 18, 2023

baloo commented Dec 19, 2023

baloo Dec 19, 2023

m4b Dec 19, 2023

baloo Dec 19, 2023

Foxboron commented Dec 19, 2023

m4b left a comment

m4b commented Jan 1, 2024

baloo commented Jan 1, 2024

	file_size - sections.certificate_table_size - sum_of_bytes_hashed;
	file_size - sections.certificate_table_size + sum_of_bytes_hashed;

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

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

Conversation

baloo commented Dec 8, 2023 • edited Loading

baloo commented Dec 9, 2023

Choose a reason for hiding this comment

baloo Dec 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

m4b commented Dec 18, 2023

Foxboron commented Dec 18, 2023

baloo commented Dec 18, 2023

Foxboron commented Dec 18, 2023

Foxboron commented Dec 18, 2023

baloo commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Foxboron commented Dec 19, 2023

m4b left a comment

Choose a reason for hiding this comment

m4b commented Jan 1, 2024

baloo commented Jan 1, 2024

baloo commented Dec 8, 2023 •

edited

Loading

baloo Dec 9, 2023 •

edited

Loading