Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrectly created ZIP_EM_TRAD_PKWARE-encrypted entries from sources which have no mtime #394

Closed
QrczakMK opened this issue Jul 19, 2023 · 2 comments
Labels
bug libzip doesn't behave as expected. feedback Waiting for feedback from submitter.

Comments

@QrczakMK
Copy link

A ZIP_EM_TRAD_PKWARE-encrypted entry created from a zip_source_t whose ZIP_SOURCE_STAT does not set ZIP_STAT_MTIME, with st.mtime left as -1, results in an archive which fails password verification.

This is because here: https://github.com/nih-at/libzip/blob/main/lib/zip_source_pkware_encode.c#L93 the st.mtime of -1 is converted to dostime and its higher byte is used in the random header for encryption.

Then here: https://github.com/nih-at/libzip/blob/main/lib/zip_close.c#L530 when the directory entry is written, the absent st.mtime is assumed to be the current time instead of converting the -1.

Finally here: https://github.com/nih-at/libzip/blob/main/lib/zip_source_pkware_decode.c#L111 the stored modification time is converted to dostime and its higher byte is compared with the byte stored in the header for encryption, which usually fails.

Since ZIP_SOURCE_STAT is called separately in both places, this may also produce an invalid archive when ZIP_SOURCE_STAT returns different modification times, e.g. if someone changes the modification time of a real file concurrently with archive creation. This is perhaps less important in practice but still worrying — it is understood that archiving a file concurrently with modifying it might produce weird results, but it is unexpected that it can fail password verification.

BTW, implicitly using the current time for modification times has a minor weird behavior that different entries may get slightly different times, reflecting the time passed between creating these entries. Maybe it would be nicer to get a consistent snapshot of the current time when the archive is created. The same snapshot could then be used for encryption.

@QrczakMK QrczakMK added the bug libzip doesn't behave as expected. label Jul 19, 2023
dillof added a commit that referenced this issue Aug 23, 2023
This is important, since it is used for password verification.

Adresses issue #394.
@dillof
Copy link
Member

dillof commented Aug 23, 2023

We can't reproduce the problem, but from reading the source it's clear that that case was not handled correctly.

We have committed a fix, could you please verify that it works for you?

@dillof dillof added the feedback Waiting for feedback from submitter. label Aug 23, 2023
@QrczakMK
Copy link
Author

Yes, I confirmed that my test case (which involves a lot of code which is not open sourced yet, not even committed internally yet) works with d148b0e and 25cac21 patched. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug libzip doesn't behave as expected. feedback Waiting for feedback from submitter.
Projects
None yet
Development

No branches or pull requests

2 participants