pe: make sure authenticode is identical before/after signature

[baloo]

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!

[baloo]

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.

[m4b]

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]

@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]

@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 https://github.com/m4b/goblin/pull/361)

[Foxboron]

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]

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]

@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).

[Foxboron]

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]

released in 0.8.0, happy new years!

[baloo]

Thanks! and happy new year to you too!