Signing certain MSI files corrupts them

griggsrl commented 3 years ago

Hi, My team has encountered an issue while using the programmatic API to sign some MSI files. We sign many files every day including other MSIs and have no issues with the vast majority. We're not sure why these couple of MSIs are the exception. The signing doesn't fail or throw any errors at all but immediately after the MSI is signed it becomes corrupt and attempting to run the MSI displays the following error:

The code we're using to sign is as follows:

KeyStore keystore;
AuthenticodeSigner signer;
Signable file;
File certFile = null;

try {
    certFile = new File(certFilePath);
    keystore = KeyStoreUtils.load(certFile, "PKCS12", certPassphrase, null);

    signer = new AuthenticodeSigner(keystore, certKeystore, certPassphrase);

    File[] filesToSign = listFiles(regex);

    for(File targetFile : filesToSign) {        
        signer.withTimestamping(true)
            .withDigestAlgorithm(DigestAlgorithm.SHA256)
            .withSignaturesReplaced(true)
            .withTimestampingMode(TimestampingMode.RFC3161)
               .withTimestampingAuthority(timestampURL);

        file = Signable.of(targetFile);

        signer.sign(file);              
    }
} catch (Exception e) {
    // ...
}

It's pretty straightforward so I'm not sure what's going on or additional spots to look.

ebourg commented 3 years ago

Thank you for reporting this issue. What is the size of the MSI file before and after signing?

griggsrl commented 3 years ago

Oddly, all 3 MSIs that have the issue are 96KB before signing and 116KB after signing. We do have a couple of other MSI files that start at 96KB as well which don't have the issue.

ebourg commented 3 years ago

Would it be possible to send one of these MSI to ebourg@apache.org please? I'll give it a look.

griggsrl commented 3 years ago

I'll see if I can get approval to send one to you or see if I can get a sample project made to demonstrate the issue.

griggsrl commented 3 years ago

I just sent over a copy of one of the MSIs for you to take a look at. Let me know if you need anything else.

ebourg commented 3 years ago

Thank you for the file. I'm able to reproduce the error but I don't understand why it happens. It looks like a bug in Apache POI, adding an empty entry to the MSI file triggers the error but that depends on the entry name length (names between 4 and 19 characters corrupt the file).

netmackan commented 3 years ago

We also have a file that gets corrupted (according to Windows) after a signature entry is added using Apache POI. I can not share the file as it belongs to one of our customers.

But I can confirm that using a file name of 3 characters is fine (except of-course the signature is not there then). @ebourg How did you find out about the 4-19 characters range?

Previously when troubleshooting a bug in POI we discovered that the msidump tool (from msitools) segfaulted and could use that information to figure out where the problem was. Unfortunately, in this case, at least after a first look, this does not happen in this case. So we need to find some other way to figure out where the problem is.

ebourg commented 3 years ago

@netmackan I've simply opened the MSI file with POI and then added a Foo entry with a empty 4K byte array. With the name Foo the MSI was still valid, but with an entry named FooBar it became invalid (with the same 4K content). A longer entry name of 20 characters was fine (the signature entry is named \u0005DigitalSignature, so 17 characters and in the invalid range).

netmackan commented 3 years ago

I noticed now that the lengths of the existing entry names (in my file) are between 3-19 characters long. It could be a coincidence but also considering that there is a sorting algorithm (DirectoryProperty.preWrite()) that is based on the lengths of the entry names, I am getting suspicious there could be an issue with the sorting implementation if the file has entries with specific file name lengths and names...

ebourg commented 3 years ago

Thank you for the hint, I tried disabling the sorting but the modified MSI file was still invalid. Even with the sorting disabled, the entries of the modified file were in a different order (as iterated by POI) than the unmodified file. It seems that the entries have an index that doesn't match the iteration order of POI, and this index remained the same in the modified file.

I also compared with signtool, the signature entries (MsiDigitalSignatureEx and DigitalSignature) are "inserted" after the SummaryInformation entry (but the actual index start after the highest index of the original file), and the overall order isn't modified.

This order variation is likely to be the cause of the issue but it remains a mystery to me.

sstamm commented 1 year ago

Any updates on this issue? Looks like we have the same. If I check the signature afterwards with CMSSignedData data : Signable.of(targetFile).getSignatures() It looks fine. If I re-write the file but just comment fsWrite.getRoot().createOrUpdateDocument(DIGITAL_SIGNATURE_ENTRY_NAME, new ByteArrayInputStream(signatureBytes));, the order of the Properties is different but the .msi is still valid/throws no error. With this line I got the error above.