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

cleaned security sensitive data after usage #1366

Conversation

dmytro-sheyko
Copy link

#1362 Crypt32Util.cryptProtectData leaves security sensitive information in memory
#1362

@dmytro-sheyko dmytro-sheyko force-pushed the bug/1362_cryptProtectData_clean branch from f0a4b7f to 6be6e67 Compare August 8, 2021 19:06
@dbwiddis
Copy link
Contributor

dbwiddis commented Aug 8, 2021

Looks good from my end. This is probably worthy of a change log entry.

@dmytro-sheyko dmytro-sheyko force-pushed the bug/1362_cryptProtectData_clean branch from 6be6e67 to 2c01ff3 Compare August 9, 2021 05:43
@dmytro-sheyko
Copy link
Author

Slightly corrected previous commit:

  1. CryptUnprotectData accepts null as pointer to description. We don't use it here, so I got rid of it.
  2. Clear result byte array when LocalFree fails and hence CryptProtectData/CryptUnprotectData throws exception

@matthiasblaesing
Copy link
Member

Also looks good to me. I agree with @dbwiddis though, this warrants an entry in the CHANGES.md file in the Bug Fixes section. For the synopsis I think something like:

Clear security sensitive data after usage in c.s.j.p.win32.Crypt32Util#cryptUnprotectData and #cryptUnprotectData.

As a rule of thumb: Changes that affect users should have an entry in the CHANGES.md- it basicly answers the question: "Why should I bother to update?".

@dmytro-sheyko
Copy link
Author

Ok. I will change the commit message and also will try to write unit test for the issue (based on the program that demonstrates the problem).
And, just make it precise, may/should I add an entry in the CHANGES.md as well?

@dbwiddis
Copy link
Contributor

dbwiddis commented Aug 9, 2021

Yes, the suggested blurb above should be in CHANGES.md in the Bug Fixes section, following the same format as the other entries.

@dmytro-sheyko dmytro-sheyko force-pushed the bug/1362_cryptProtectData_clean branch 3 times, most recently from 48fe9bc to 4ee9a93 Compare August 10, 2021 10:10
…l#cryptUnprotectData and #cryptUnprotectData
@dmytro-sheyko dmytro-sheyko force-pushed the bug/1362_cryptProtectData_clean branch from 4ee9a93 to 4b156fb Compare August 10, 2021 10:14
@matthiasblaesing
Copy link
Member

Looks good to me. Thank you.

@matthiasblaesing matthiasblaesing merged commit 80105c3 into java-native-access:master Aug 10, 2021
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.

Crypt32Util.cryptProtectData leaves security sensitive information in memory
3 participants