search

ebourg / jsign

Java implementation of Microsoft Authenticode for signing Windows executables, installers & scripts
https://ebourg.github.io/jsign
Apache License 2.0
259 stars 108 forks source link

Fix attaching signatures for PE files #126

Closed markt-asf closed 2 years ago

markt-asf commented 2 years ago

I spotted this testing repeatable builds with Apache Tomcat. This might not be the right way to fix the issue generally but it does fix the issue I was seeing. In short, when re-attaching the signature the PE file needs to be padded to a multiple of 8 bytes first - just like it is when signing the PE file directly.

coveralls commented 2 years ago

Coverage Status

Coverage increased (+0.0001%) to 83.278% when pulling 325df255119ec8adb14c4b9930e2994223c7d6ed on markt-asf:fix-signature-attach into 3e7335468ae318e87739ff1962f2fc2785820a26 on ebourg:master.

ebourg commented 2 years ago

The non padded file was the NSIS installer?

markt-asf commented 2 years ago

It was definitely the NSIS uninstaller. It may have been the installer as well but an issue with the Tomcat build script meant that the re-attached signature for the installer was always going to be invalid.

markt-asf commented 2 years ago

Hmm. My proposed patch may have triggered the file locking issue again.

ebourg commented 2 years ago

I'm not even sure the checksum is properly computed when the file isn't padded, the tests don't cover this case.

I'd expected the code to:

  1. compute the checksum as if the file was padded
  2. pad the file only when the signature table is added, i.e. in PEFile#setSignature(), currently this is performed by AuthenticodeSigner#sign() and it doesn't feel at the right place, the signer code should be format agnostic.

I'll investigate further, thank you for reporting this.

markt-asf commented 2 years ago

Ack. I found the other file locking issue and updated the PR to include the hack I am currently using to work around it. I did wonder if refactoring signer.sign() and attach() so they both accepted a Signable instance would help. Regardless, I'm sure my hacks are just that and aren't the best long term solution but hopefully they do at least serve to demonstrate the issue and provide a hint to the right solution.

ebourg commented 2 years ago

I've made a couple of changes that should cover the case of a non padded file signed with a detached signature, could you give it a try? I haven't looked at the file locking issue yet.

markt-asf commented 2 years ago

LGTM. I re-ran my signing the uninstaller test and adding the detached signature results in a valid signature (as far as Windows is concerned) whereas before your change it would result in an invalid signature.

ebourg commented 2 years ago

Thank you for checking, I've also merged the locking fix. Time to release now.

ebourg commented 2 years ago

I did wonder if refactoring signer.sign() and attach() so they both accepted a Signable instance would help.

I've implemented your suggestion. AuthenticodeSigner.sign() no longer closes the file, Signable is now Closeable, attach/detach work directly on the signable, and a single try-with-resource encompasses the whole sign/attach/detach operation. That's much cleaner!