search

zurmokeeper / officecrypto-tool

officecrypto-tool is a library for js that can be used to decrypt and encrypt office(excel/ppt/word) files.
https://www.npmjs.com/package/officecrypto-tool
MIT License
19 stars 4 forks source link

fix: check for password when using agile encryption #36

Closed guillenotfound closed 3 months ago

guillenotfound commented 3 months ago

Fixes: https://github.com/zurmokeeper/officecrypto-tool/issues/20

I also checked with the files from the Python library and it looks to be working, I'm not sure if the isValidZip can be improved since the Python implementation has more checks, please let me know so that I can adjust it.

After debugging the Python implementation it looks like they check wether the resulting file is a valid ZIP file to check if the decryption succeeded.

https://github.com/nolze/msoffcrypto-tool/blob/0b0bc7d82b7cccf6a39ce420b769a0a39d9fb701/msoffcrypto/format/ooxml.py#L292

I might need some help since I don't see prettier + I'm getting a bunch of errors when opening the files:

image
zurmokeeper commented 3 months ago

@guillenotfound

The validation of the isValidZip method may be useful, but it should not be the most important thing, the following one is.

https://github.com/nolze/msoffcrypto-tool/blob/0b0bc7d82b7cccf6a39ce420b769a0a39d9fb701/msoffcrypto/method/ecma376_agile.py#L443

Also, don't worry about that eslint checksum issue, it should be an OS issue, just deal with the linebreak-style in .eslintrc.js and I'll follow up with some changes

guillenotfound commented 3 months ago

@guillenotfound

The validation of the isValidZip method may be useful, but it should not be the most important thing, the following one is.

https://github.com/nolze/msoffcrypto-tool/blob/0b0bc7d82b7cccf6a39ce420b769a0a39d9fb701/msoffcrypto/method/ecma376_agile.py#L443

Also, don't worry about that eslint checksum issue, it should be an OS issue, just deal with the linebreak-style in .eslintrc.js and I'll follow up with some changes

As per my testing that's not even called, here's the code flow:

https://github.com/nolze/msoffcrypto-tool/blob/0b0bc7d82b7cccf6a39ce420b769a0a39d9fb701/msoffcrypto/__main__.py#L103 https://github.com/nolze/msoffcrypto-tool/blob/0b0bc7d82b7cccf6a39ce420b769a0a39d9fb701/msoffcrypto/format/ooxml.py#L181C9-L181C17

verify_password=false so that will prevent PWD verification and will fail here: https://github.com/nolze/msoffcrypto-tool/blob/0b0bc7d82b7cccf6a39ce420b769a0a39d9fb701/msoffcrypto/format/ooxml.py#L292

In fact the file is written to disk if you specify it but you won't be able to open it which is wrong, I've prevented that to happen with my changes, give it a try and LMK, maybe we can make that check a bit more strict just following this: https://en.wikipedia.org/wiki/List_of_file_signatures#:~:text=its%20descendants%20(rare)-,50%204B%2003%2004,-50%204B%2005

Last but not least to check if the files work you can do unzip -l filename, the command will fail if the decryption is incorrect so we need to do something kind of similar, I know the check can be a bit stricter as per the implementation in Python but it's hard to decode it for humans 😅

zurmokeeper commented 3 months ago

@guillenotfound Thank you very much, but I need some time to validate it, this PR merge might take a while!

zurmokeeper commented 3 months ago

Wouldn't it make sense to extract this variable so that it can be reused among all test files?

Now it's the same, just a separate submodule for the test file, so that the source code doesn't contain the test file when the package version is released.

zurmokeeper commented 3 months ago

@guillenotfound

The new version has been released, in the futurepnpm run testneed to install submodule dependencies, see link, hope to know, thank you!