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

Zeroise private random data on the stack before returning #762

Merged
merged 6 commits into from
Jan 19, 2023

Conversation

torben-hansen
Copy link
Contributor

@torben-hansen torben-hansen commented Jan 18, 2023

CryptoAlg-1524

Description of changes:

Zeroise private random data immediately after use. This is just good secret value hygiene.

Most is stack-allocated. So, good chances it would be overwritten fairly quickly anyway. Looked in both /crypto and /ssl, grepping my way through.

Testing:

CI

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and
the ISC license.

@torben-hansen torben-hansen force-pushed the zeroise_seed_after_use branch from 3aceaa2 to a3be553 Compare January 18, 2023 19:18
@torben-hansen torben-hansen marked this pull request as ready for review January 18, 2023 19:18
justsmth
justsmth previously approved these changes Jan 18, 2023
@torben-hansen torben-hansen changed the title Zeroise the random seed used to generate the ED25519 keys Zeroise private random data on the stack before returning Jan 18, 2023
Copy link
Contributor

@andrewhop andrewhop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you consider using sizeof in the calls to OPENSSL_cleanse to ensure all the data is cleansed as expected? If a future change made one of these arrays smaller it would trigger an address sanitizer error as cleanse tries to cleanse past the end, but if the array gets bigger the cleanse would only cleanse the first part.

Also this change introduces an inconsistencies: some places use a new constant (ED25519_SEED_LEN) while others continue to use magic numbers.

crypto/curve25519/curve25519.c Outdated Show resolved Hide resolved
crypto/curve25519/curve25519.c Outdated Show resolved Hide resolved
crypto/fipsmodule/rand/rand.c Show resolved Hide resolved
@torben-hansen
Copy link
Contributor Author

Did you consider using sizeof in the calls to OPENSSL_cleanse to ensure all the data is cleansed as expected?

There is an equal issue here if the array is made a pointer that is dynamically allocated. Then sizeof would be bad. So, I think just sticking to current implementation. Let me know if you have other concerns.

@torben-hansen torben-hansen merged commit 51d17f7 into aws:main Jan 19, 2023
@torben-hansen torben-hansen deleted the zeroise_seed_after_use branch January 19, 2023 23:39
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.

3 participants