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

Fix use after context release #368

Merged
merged 3 commits into from
Mar 6, 2019

Conversation

tmtmazum
Copy link

@tmtmazum tmtmazum commented Mar 4, 2019

Fixes a couple of bugs on windows caught by AppVerifier

  • in function mz_crypt_pbkdf2(..), CryptDeleteHash was being called after the associated Crypt context was released
    • hmac1 and hmac2 are created using mz_crypt_hmac_init, so they each get their own context (provider) from the CryptAcquireContext(..) function
    • hmac3 is created using mz_crypt_hmac_copy, which creates a duplicate of hmac2's hash, sharing the same context
    • During destruction, hmac1 and hmac2 were being deleted first, followed by hmac3. This meant that the crypt contexts were released first by CryptReleaseContext. The subsequent call to CryptDestroyHash(..) on hmac3's hash would lead to UB as the associated context has already been released by that point
    • Fix: call mz_crypt_hmac_delete(..) on hmac3 first, so that the contexts are cleared at the end after the hashes are destroyed.
  • in function mz_crypt_hmac_copy, if target->hash already contained a hash prior to the function being called, that hash would be overwritten, without CryptDestroyHash being called on it.
    • Fix: check if target->hash is non-empty in mz_crypt_hmac_copy and call CryptDestroyHash on it

@nmoinvaz nmoinvaz changed the base branch from master to dev March 6, 2019 02:03
@nmoinvaz nmoinvaz merged commit 066157e into zlib-ng:dev Mar 6, 2019
@nmoinvaz
Copy link
Member

nmoinvaz commented Mar 6, 2019

Looks good. Thanks for the pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants