search

rhboot / pesign

Linux tools for signed PE-COFF binaries
GNU General Public License v2.0
110 stars 51 forks source link

pesign creates bad signatures in some cases #1

Closed mlschroe closed 11 years ago

mlschroe commented 11 years ago

pesign signed our kernels with a signature that didn't match the output file. It turned out that inserting the certs calls pe_set_image_size(), which pokes into the opthdr. But the header is included in the hash calculation, so modifying it changes the hash.

In other words, pesign -h prints a different value before/after signing.

I don't see the purpose of calling pe_set_image_size at all, because it doesn't seem to even look at the size of the certs. Also, the microsoft standard says that the certs don't get loaded into memory, which indicates to me that they should not be included in image_size.

Thus my suggestion is to remove pe_set_image_size() and the pe_get_file_alignment()/pe_get_scn_alignment() functions, as calling pe_set_image_size can only break the hash and doesn't seem to have any benefit.

vathpela commented 11 years ago

Upon investigating, I think you're right about not needing to set image size per the spec. That said, I think I'd like to leave pe_get_file_alignment()/pe_get_scn_alignment() as they may be generally useful in libdpe (eventually.)

I do wonder how you're hitting this in current versions, though - we're always doing generate_digest() /after/ pe_set_image_size() in the current tree. What's the workflow here that's causing the problem?

mlschroe commented 11 years ago

We use 'pesign -E' and 'pesign -R' to sign our files in two steps.

Nevertheless, I think that generate_digest works in the inpe, but pe_set_image_size changes the outpe, which is a different file.

vathpela commented 11 years ago

Can you check and see if fad124d resolves this issue for you?

vathpela commented 11 years ago

You'll probably also want 230bda083ceaa33f99310da4e341947b01d7f54b for this use case - we weren't properly extending the file, so if it doesn't end on a 16-byte alignment, the hash generated by -h is wrong.

mlschroe commented 11 years ago

Yes, #fad124d does solve this issue for us. I'll forward your comment about #230bda0 to out pesign packager. Thanks!