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

Securely erase memory & reduce public API #2224

Closed
wants to merge 3 commits into from
Closed

Securely erase memory & reduce public API #2224

wants to merge 3 commits into from

Conversation

nbougalis
Copy link
Contributor

No description provided.

@HowardHinnant
Copy link
Contributor

Here's one more spot that needs treatment: https://github.com/ripple/rippled/blob/develop/src/ripple/protocol/Serializer.h#L201

Copy link
Contributor

@HowardHinnant HowardHinnant left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Contributor

@mellery451 mellery451 left a comment

Choose a reason for hiding this comment

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

👍

@@ -199,6 +210,8 @@ generateSecretKey (KeyType type, Seed const& seed)

if (type == KeyType::secp256k1)
{
// FIXME: Avoid copying the seed into a uint128 key only to have
// generateRootDeterministicPrivateKey copy out of it.
Copy link
Contributor

Choose a reason for hiding this comment

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

I probably don't understand all the nuances of the data - why don't we need to secure_erase the upk below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'd need to massage things a bit to do it, but it's possible. Perhaps in the interest of thoroughness we should, even if it's a bit of extra work.

As I've marked elsewhere in the file, ideally we should change generateRootDeterministicPrivateKey to just take a Slice and not a uint128, and have it return a SecretKey directly.

@codecov-io
Copy link

Codecov Report

Merging #2224 into develop will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2224      +/-   ##
===========================================
- Coverage    70.07%   70.06%   -0.01%     
===========================================
  Files          689      689              
  Lines        50730    50734       +4     
===========================================
- Hits         35550    35549       -1     
- Misses       15180    15185       +5
Impacted Files Coverage Δ
src/ripple/protocol/Serializer.h 85.32% <ø> (ø) ⬆️
src/ripple/protocol/SecretKey.h 100% <ø> (ø) ⬆️
...rc/ripple/crypto/impl/GenerateDeterministicKey.cpp 98.63% <100%> (+0.03%) ⬆️
src/ripple/protocol/impl/SecretKey.cpp 83.33% <100%> (+3.33%) ⬆️
src/ripple/basics/DecayingSample.h 77.77% <0%> (-8.34%) ⬇️
src/ripple/core/Stoppable.h 93.33% <0%> (-6.67%) ⬇️
src/ripple/app/ledger/PendingSaves.h 93.54% <0%> (-6.46%) ⬇️
src/ripple/core/impl/JobQueue.cpp 86.85% <0%> (-0.47%) ⬇️
src/ripple/app/ledger/impl/LedgerMaster.cpp 46.41% <0%> (-0.23%) ⬇️
src/ripple/app/main/Application.cpp 60.37% <0%> (-0.12%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc64050...b278267. Read the comment docs.

@HowardHinnant
Copy link
Contributor

👍

@nbougalis nbougalis mentioned this pull request Sep 23, 2017
@nbougalis
Copy link
Contributor Author

Merged as 39f9135

@nbougalis nbougalis closed this Sep 25, 2017
@nbougalis nbougalis added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Sep 25, 2017
@nbougalis nbougalis deleted the ncc branch March 22, 2018 03:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants