mdsteele / rust-cfb

Rust library for reading/writing Compound File Binary (structured storage) files
MIT License
46 stars 20 forks source link

Remove root stream length check #6

Closed Jake-Shadle closed 3 years ago

Jake-Shadle commented 3 years ago

When trying to read MSI files that are part of the Window SDK install (eg. https://download.visualstudio.microsoft.com/download/pr/100107594/d176ecb4240a304d1a2af2391e8965d4/Windows%20SDK%20Desktop%20Headers%20x86-x86_en-us.msi), basically all of them fail this check. When the check is removed, rust-msi is able to give the same results as msiinfo, so it feels like it is just too strict.

mdsteele commented 3 years ago

Thanks for catching this. A little more detail for future reference:

When trying to open https://download.visualstudio.microsoft.com/download/pr/100107594/d176ecb4240a304d1a2af2391e8965d4/Windows%20SDK%20Desktop%20Headers%20x86-x86_en-us.msi with rust-cfb, I get the error:

Malformed directory (root stream len is 9728, but should be 5888)

So, the root stream (that is, the "mini stream", as mentioned in MS-CFB section 2.6.2) is longer than necessary. My guess is that it contains (unused) data for unallocated mini sectors beyond the last allocated mini sector in the MiniFAT (each mini sector is 64 bytes long; 9728 = 152 64 and 5888 = 92 64). I can't find anything in the MS-CFB spec that says that's not allowed, so I don't know why I originally made this check so strict.

Rather than eliminating the check entirely, though, I'm wondering if we should change it to:

if root_entry.stream_len < expected_root_stream_len {
    ...
}

So that the mini stream is allowed to be too long, but still not allowed to be too short. Does that work for the other MSI files you tried in the Windows SDK install?

Jake-Shadle commented 3 years ago

Oh that is a good point, better to to catch too small streams to avoid out of bound reads later on, I'll check tomorrow if that check works against the MSIs that I am reading from the sdk.

Jake-Shadle commented 3 years ago

Confirmed that just loosening the check to just ensure the root stream is not less than the expected length works on all of the MSI files that I am reading from the SDK.

mdsteele commented 3 years ago

Thanks!