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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
14419ea
Securely erase memory & reduce public API
nbougalis Sep 6, 2017
4a201a8
[FOLD] Address review feedback
nbougalis Sep 6, 2017
b278267
[FOLD] A couple of more
nbougalis Sep 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions src/ripple/crypto/impl/GenerateDeterministicKey.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

#include <BeastConfig.h>
#include <ripple/basics/contract.h>
#include <ripple/beast/crypto/secure_erase.h>
#include <ripple/crypto/GenerateDeterministicKey.h>
#include <ripple/crypto/impl/ec_key.h>
#include <ripple/crypto/impl/openssl.h>
Expand Down Expand Up @@ -74,8 +75,6 @@ copy_uint32 (FwdIt out, std::uint32_t v)
*out = v & 0xff;
}

// #define EC_DEBUG

// Functions to add support for deterministic EC keys

// --> seed
Expand All @@ -94,12 +93,12 @@ static bignum generateRootDeterministicKey (uint128 const& seed)
std::copy(seed.begin(), seed.end(), buf.begin());
copy_uint32 (buf.begin() + 16, seq++);
auto root = sha512Half(buf);
std::fill (buf.begin(), buf.end(), 0); // security erase
beast::secure_erase(buf.data(), buf.size());
privKey.assign (root.data(), root.size());
root.zero(); // security erase
beast::secure_erase(root.data(), root.size());
}
while (privKey.is_zero() || privKey >= secp256k1curve.order);

beast::secure_erase(&seq, sizeof(seq));
return privKey;
}

Expand Down Expand Up @@ -153,8 +152,9 @@ static bignum makeHash (Blob const& pubGen, int seq, bignum const& order)
copy_uint32 (buf.begin() + 33, seq);
copy_uint32 (buf.begin() + 37, subSeq++);
auto root = sha512Half_s(buf);
std::fill(buf.begin(), buf.end(), 0); // security erase
beast::secure_erase(buf.data(), buf.size());
result.assign (root.data(), root.size());
beast::secure_erase(root.data(), root.size());
}
while (result.is_zero() || result >= order);

Expand Down
24 changes: 0 additions & 24 deletions src/ripple/protocol/SecretKey.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,30 +88,6 @@ operator!= (SecretKey const& lhs,

//------------------------------------------------------------------------------

/** Produces a sequence of secp256k1 key pairs. */
class Generator
{
private:
Blob gen_; // VFALCO compile time size?

public:
explicit
Generator (Seed const& seed);

/** Generate the nth key pair.

The seed is required to produce the private key.
*/
std::pair<PublicKey, SecretKey>
operator()(Seed const& seed, std::size_t ordinal) const;

/** Generate the nth public key. */
PublicKey
operator()(std::size_t ordinal) const;
};

//------------------------------------------------------------------------------

/** Parse a secret key */
template <>
boost::optional<SecretKey>
Expand Down
5 changes: 3 additions & 2 deletions src/ripple/protocol/Serializer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include <ripple/basics/contract.h>
#include <ripple/basics/Buffer.h>
#include <ripple/basics/Slice.h>
#include <ripple/beast/crypto/secure_erase.h>
#include <cassert>
#include <cstdint>
#include <iomanip>
Expand Down Expand Up @@ -198,8 +199,8 @@ class Serializer
}
void secureErase ()
{
memset (& (mData.front ()), 0, mData.size ());
erase ();
beast::secure_erase(mData.data(), mData.size());
mData.clear ();
}
void erase ()
{
Expand Down
75 changes: 46 additions & 29 deletions src/ripple/protocol/impl/SecretKey.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,35 +56,46 @@ SecretKey::to_string() const
}

//------------------------------------------------------------------------------

Generator::Generator (Seed const& seed)
/** Produces a sequence of secp256k1 key pairs. */
class Generator
{
uint128 ui;
std::memcpy(ui.data(),
seed.data(), seed.size());
gen_ = generateRootDeterministicPublicKey(ui);
}
private:
Blob gen_; // VFALCO compile time size?

std::pair<PublicKey, SecretKey>
Generator::operator()(Seed const& seed, std::size_t ordinal) const
{
uint128 ui;
std::memcpy(ui.data(), seed.data(), seed.size());
auto gsk = generatePrivateDeterministicKey(gen_, ui, ordinal);
auto gpk = generatePublicDeterministicKey(gen_, ordinal);
SecretKey const sk(Slice{ gsk.data(), gsk.size() });
PublicKey const pk(Slice{ gpk.data(), gpk.size() });
beast::secure_erase(ui.data(), ui.size());
beast::secure_erase(gsk.data(), gsk.size());
return { pk, sk };
}
public:
explicit
Generator (Seed const& seed)
{
// FIXME: Avoid copying the seed into a uint128 key only to have
// generateRootDeterministicPublicKey copy out of it.
uint128 ui;
std::memcpy(ui.data(),
seed.data(), seed.size());
gen_ = generateRootDeterministicPublicKey(ui);
}

PublicKey
Generator::operator()(std::size_t ordinal) const
{
auto gpk = generatePublicDeterministicKey(gen_, ordinal);
return PublicKey(Slice{ gpk.data(), gpk.size() });
}
/** Generate the nth key pair.

The seed is required to produce the private key.
*/
std::pair<PublicKey, SecretKey>
operator()(Seed const& seed, std::size_t ordinal) const
{
// FIXME: Avoid copying the seed into a uint128 key only to have
// generatePrivateDeterministicKey copy out of it.
uint128 ui;
std::memcpy(ui.data(), seed.data(), seed.size());
auto gsk = generatePrivateDeterministicKey(gen_, ui, ordinal);
auto gpk = generatePublicDeterministicKey(gen_, ordinal);
SecretKey const sk(Slice
{ gsk.data(), gsk.size() });
PublicKey const pk(Slice
{ gpk.data(), gpk.size() });
beast::secure_erase(ui.data(), ui.size());
beast::secure_erase(gsk.data(), gsk.size());
return {pk, sk};
}
};

//------------------------------------------------------------------------------

Expand Down Expand Up @@ -192,19 +203,25 @@ generateSecretKey (KeyType type, Seed const& seed)
{
if (type == KeyType::ed25519)
{
auto const key = sha512Half_s(Slice(
auto key = sha512Half_s(Slice(
seed.data(), seed.size()));
return SecretKey(Slice{ key.data(), key.size() });
SecretKey sk = Slice{ key.data(), key.size() };
beast::secure_erase(key.data(), key.size());
return sk;
}

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.

uint128 ps;
std::memcpy(ps.data(),
seed.data(), seed.size());
auto const upk =
generateRootDeterministicPrivateKey(ps);
return SecretKey(Slice{ upk.data(), upk.size() });
SecretKey sk = Slice{ upk.data(), upk.size() };
beast::secure_erase(ps.data(), ps.size());
return sk;
}

LogicError ("generateSecretKey: unknown key type");
Expand Down