Skip to content

Commit

Permalink
Deleted redundant SymmetricKey::GetRawKey().
Browse files Browse the repository at this point in the history
This was functionally equivalent to SymmetricKey::key(), and also
returned a always true status code. Switching to key() allowed for
the simplification of downstream functions.

R=alemate@chromium.org, alokp@chromium.org, davidben@chromium.org, rogerta@chromium.org, stanisc@chromium.org

Review-Url: https://codereview.chromium.org/2934893003 .
Cr-Commit-Position: refs/heads/master@{#480868}
  • Loading branch information
cmumford committed Jun 20, 2017
1 parent ff6656a commit 7bdfcab
Show file tree
Hide file tree
Showing 13 changed files with 37 additions and 100 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -291,17 +291,10 @@ void EasyUnlockCreateKeysOperation::CreateKeyForDeviceAtIndex(size_t index) {
return;
}

std::string raw_session_key;
session_key->GetRawKey(&raw_session_key);

challenge_creator_.reset(new ChallengeCreator(
user_key,
raw_session_key,
tpm_public_key_,
device,
user_key, session_key->key(), tpm_public_key_, device,
base::Bind(&EasyUnlockCreateKeysOperation::OnChallengeCreated,
weak_ptr_factory_.GetWeakPtr(),
index)));
weak_ptr_factory_.GetWeakPtr(), index)));
challenge_creator_->Start();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,8 @@ std::string BuildRawHMACKey() {
std::unique_ptr<crypto::SymmetricKey> key(
crypto::SymmetricKey::GenerateRandomKey(crypto::SymmetricKey::AES,
kHMACKeySizeInBits));
std::string raw_result, result;
key->GetRawKey(&raw_result);
base::Base64Encode(raw_result, &result);
std::string result;
base::Base64Encode(key->key(), &result);
return result;
}

Expand Down
4 changes: 1 addition & 3 deletions chrome/browser/signin/local_auth.cc
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,7 @@ std::string CreateSecurePasswordHash(const std::string& salt,
crypto::SymmetricKey::DeriveKeyFromPassword(
crypto::SymmetricKey::AES, password, salt, encoding.iteration_count,
encoding.hash_bits));
std::string password_hash;
const bool success = password_key->GetRawKey(&password_hash);
DCHECK(success);
std::string password_hash = password_key->key();
DCHECK_EQ(encoding.hash_bytes, password_hash.length());

UMA_HISTOGRAM_TIMES("PasswordHash.CreateTime",
Expand Down
6 changes: 1 addition & 5 deletions chromecast/media/base/decrypt_context_impl_clearkey.cc
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,7 @@ bool DecryptContextImplClearKey::DoDecrypt(CastDecoderBuffer* buffer,
output += data_offset;

// Get the key.
std::string raw_key;
if (!key_->GetRawKey(&raw_key)) {
LOG(ERROR) << "Failed to get the underlying AES key";
return false;
}
const std::string& raw_key = key_->key();
DCHECK_EQ(static_cast<int>(raw_key.length()), AES_BLOCK_SIZE);
const uint8_t* key_u8 = reinterpret_cast<const uint8_t*>(raw_key.data());
AES_KEY aes_key;
Expand Down
4 changes: 1 addition & 3 deletions chromeos/login/auth/key.cc
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,7 @@ void Key::Transform(KeyType target_key_type, const std::string& salt) {
crypto::SymmetricKey::DeriveKeyFromPassword(
crypto::SymmetricKey::AES, secret_, salt, kNumIterations,
kKeySizeInBits));
std::string raw_secret;
key->GetRawKey(&raw_secret);
base::Base64Encode(raw_secret, &secret_);
base::Base64Encode(key->key(), &secret_);
break;
}
case KEY_TYPE_SALTED_SHA256:
Expand Down
6 changes: 2 additions & 4 deletions components/sync/base/cryptographer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -348,10 +348,8 @@ std::string Cryptographer::GetDefaultNigoriKeyData() const {
if (iter == nigoris_.end())
return std::string();
sync_pb::NigoriKey key;
if (!iter->second->ExportKeys(key.mutable_user_key(),
key.mutable_encryption_key(),
key.mutable_mac_key()))
return std::string();
iter->second->ExportKeys(key.mutable_user_key(), key.mutable_encryption_key(),
key.mutable_mac_key());
return key.SerializeAsString();
}

Expand Down
36 changes: 10 additions & 26 deletions components/sync/base/nigori.cc
Original file line number Diff line number Diff line change
Expand Up @@ -75,25 +75,21 @@ bool Nigori::InitByDerivation(const std::string& hostname,
kSaltKeySizeInBits));
DCHECK(user_salt);

std::string raw_user_salt;
if (!user_salt->GetRawKey(&raw_user_salt))
return false;

// Kuser = PBKDF2(P, Suser, Nuser, 16)
user_key_ = SymmetricKey::DeriveKeyFromPassword(
SymmetricKey::AES, password, raw_user_salt, kUserIterations,
SymmetricKey::AES, password, user_salt->key(), kUserIterations,
kDerivedKeySizeInBits);
DCHECK(user_key_);

// Kenc = PBKDF2(P, Suser, Nenc, 16)
encryption_key_ = SymmetricKey::DeriveKeyFromPassword(
SymmetricKey::AES, password, raw_user_salt, kEncryptionIterations,
SymmetricKey::AES, password, user_salt->key(), kEncryptionIterations,
kDerivedKeySizeInBits);
DCHECK(encryption_key_);

// Kmac = PBKDF2(P, Suser, Nmac, 16)
mac_key_ = SymmetricKey::DeriveKeyFromPassword(
SymmetricKey::HMAC_SHA1, password, raw_user_salt, kSigningIterations,
SymmetricKey::HMAC_SHA1, password, user_salt->key(), kSigningIterations,
kDerivedKeySizeInBits);
DCHECK(mac_key_);

Expand Down Expand Up @@ -132,12 +128,8 @@ bool Nigori::Permute(Type type,
if (!encryptor.Encrypt(plaintext.str(), &ciphertext))
return false;

std::string raw_mac_key;
if (!mac_key_->GetRawKey(&raw_mac_key))
return false;

HMAC hmac(HMAC::SHA256);
if (!hmac.Init(raw_mac_key))
if (!hmac.Init(mac_key_->key()))
return false;

std::vector<unsigned char> hash(kHashSize);
Expand Down Expand Up @@ -168,12 +160,8 @@ bool Nigori::Encrypt(const std::string& value, std::string* encrypted) const {
if (!encryptor.Encrypt(value, &ciphertext))
return false;

std::string raw_mac_key;
if (!mac_key_->GetRawKey(&raw_mac_key))
return false;

HMAC hmac(HMAC::SHA256);
if (!hmac.Init(raw_mac_key))
if (!hmac.Init(mac_key_->key()))
return false;

std::vector<unsigned char> hash(kHashSize);
Expand Down Expand Up @@ -206,12 +194,8 @@ bool Nigori::Decrypt(const std::string& encrypted, std::string* value) const {
input.substr(kIvSize, input.size() - (kIvSize + kHashSize)));
std::string hash(input.substr(input.size() - kHashSize, kHashSize));

std::string raw_mac_key;
if (!mac_key_->GetRawKey(&raw_mac_key))
return false;

HMAC hmac(HMAC::SHA256);
if (!hmac.Init(raw_mac_key))
if (!hmac.Init(mac_key_->key()))
return false;

std::vector<unsigned char> expected(kHashSize);
Expand All @@ -232,20 +216,20 @@ bool Nigori::Decrypt(const std::string& encrypted, std::string* value) const {
return true;
}

bool Nigori::ExportKeys(std::string* user_key,
void Nigori::ExportKeys(std::string* user_key,
std::string* encryption_key,
std::string* mac_key) const {
DCHECK(encryption_key);
DCHECK(mac_key);
DCHECK(user_key);

if (user_key_)
user_key_->GetRawKey(user_key);
*user_key = user_key_->key();
else
user_key->clear();

return encryption_key_->GetRawKey(encryption_key) &&
mac_key_->GetRawKey(mac_key);
*encryption_key = encryption_key_->key();
*mac_key = mac_key_->key();
}

} // namespace syncer
2 changes: 1 addition & 1 deletion components/sync/base/nigori.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class Nigori {
bool Decrypt(const std::string& value, std::string* decrypted) const;

// Exports the raw derived keys.
bool ExportKeys(std::string* user_key,
void ExportKeys(std::string* user_key,
std::string* encryption_key,
std::string* mac_key) const;

Expand Down
8 changes: 4 additions & 4 deletions components/sync/base/nigori_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ TEST(SyncNigoriTest, ExportImport) {
std::string user_key;
std::string encryption_key;
std::string mac_key;
EXPECT_TRUE(nigori1.ExportKeys(&user_key, &encryption_key, &mac_key));
nigori1.ExportKeys(&user_key, &encryption_key, &mac_key);

Nigori nigori2;
EXPECT_TRUE(nigori2.InitByImport(user_key, encryption_key, mac_key));
Expand Down Expand Up @@ -159,7 +159,7 @@ TEST(SyncNigoriTest, InitByDerivationSetsUserKey) {
std::string user_key = "";
std::string encryption_key;
std::string mac_key;
EXPECT_TRUE(nigori.ExportKeys(&user_key, &encryption_key, &mac_key));
nigori.ExportKeys(&user_key, &encryption_key, &mac_key);

EXPECT_NE(user_key, "");
}
Expand All @@ -171,7 +171,7 @@ TEST(SyncNigoriTest, ToleratesEmptyUserKey) {
std::string user_key;
std::string encryption_key;
std::string mac_key;
EXPECT_TRUE(nigori1.ExportKeys(&user_key, &encryption_key, &mac_key));
nigori1.ExportKeys(&user_key, &encryption_key, &mac_key);
EXPECT_FALSE(user_key.empty());
EXPECT_FALSE(encryption_key.empty());
EXPECT_FALSE(mac_key.empty());
Expand All @@ -180,7 +180,7 @@ TEST(SyncNigoriTest, ToleratesEmptyUserKey) {
EXPECT_TRUE(nigori2.InitByImport("", encryption_key, mac_key));

user_key = "non-empty-value";
EXPECT_TRUE(nigori2.ExportKeys(&user_key, &encryption_key, &mac_key));
nigori2.ExportKeys(&user_key, &encryption_key, &mac_key);
EXPECT_TRUE(user_key.empty());
EXPECT_FALSE(encryption_key.empty());
EXPECT_FALSE(mac_key.empty());
Expand Down
7 changes: 1 addition & 6 deletions crypto/hmac.cc
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,7 @@ bool HMAC::Init(const unsigned char* key, size_t key_length) {
}

bool HMAC::Init(const SymmetricKey* key) {
std::string raw_key;
bool result = key->GetRawKey(&raw_key) && Init(raw_key);
// Zero out key copy. This might get optimized away, but one can hope.
// Using std::string to store key info at all is a larger problem.
std::fill(raw_key.begin(), raw_key.end(), 0);
return result;
return Init(key->key());
}

bool HMAC::Sign(const base::StringPiece& data,
Expand Down
5 changes: 0 additions & 5 deletions crypto/symmetric_key.cc
Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,6 @@ std::unique_ptr<SymmetricKey> SymmetricKey::Import(Algorithm algorithm,
return key;
}

bool SymmetricKey::GetRawKey(std::string* raw_key) const {
*raw_key = key_;
return true;
}

SymmetricKey::SymmetricKey() = default;

} // namespace crypto
10 changes: 3 additions & 7 deletions crypto/symmetric_key.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,18 +50,14 @@ class CRYPTO_EXPORT SymmetricKey {

// Imports an array of key bytes in |raw_key|. This key may have been
// generated by GenerateRandomKey or DeriveKeyFromPassword and exported with
// GetRawKey, or via another compatible method. The key must be of suitable
// size for use with |algorithm|. The caller owns the returned SymmetricKey.
// key(). The key must be of suitable size for use with |algorithm|.
// The caller owns the returned SymmetricKey.
static std::unique_ptr<SymmetricKey> Import(Algorithm algorithm,
const std::string& raw_key);

// Returns the raw platform specific key data.
const std::string& key() const { return key_; }

// Extracts the raw key from the platform specific data.
// Warning: |raw_key| holds the raw key as bytes and thus must be handled
// carefully.
bool GetRawKey(std::string* raw_key) const;

private:
SymmetricKey();

Expand Down
33 changes: 9 additions & 24 deletions crypto/symmetric_key_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,54 +15,40 @@ TEST(SymmetricKeyTest, GenerateRandomKey) {
std::unique_ptr<crypto::SymmetricKey> key(
crypto::SymmetricKey::GenerateRandomKey(crypto::SymmetricKey::AES, 256));
ASSERT_TRUE(key);
std::string raw_key;
EXPECT_TRUE(key->GetRawKey(&raw_key));
EXPECT_EQ(32U, raw_key.size());
EXPECT_EQ(32U, key->key().size());

// Do it again and check that the keys are different.
// (Note: this has a one-in-10^77 chance of failure!)
std::unique_ptr<crypto::SymmetricKey> key2(
crypto::SymmetricKey::GenerateRandomKey(crypto::SymmetricKey::AES, 256));
ASSERT_TRUE(key2);
std::string raw_key2;
EXPECT_TRUE(key2->GetRawKey(&raw_key2));
EXPECT_EQ(32U, raw_key2.size());
EXPECT_NE(raw_key, raw_key2);
EXPECT_EQ(32U, key2->key().size());
EXPECT_NE(key->key(), key2->key());
}

TEST(SymmetricKeyTest, ImportGeneratedKey) {
std::unique_ptr<crypto::SymmetricKey> key1(
crypto::SymmetricKey::GenerateRandomKey(crypto::SymmetricKey::AES, 256));
ASSERT_TRUE(key1);
std::string raw_key1;
EXPECT_TRUE(key1->GetRawKey(&raw_key1));

std::unique_ptr<crypto::SymmetricKey> key2(
crypto::SymmetricKey::Import(crypto::SymmetricKey::AES, raw_key1));
crypto::SymmetricKey::Import(crypto::SymmetricKey::AES, key1->key()));
ASSERT_TRUE(key2);

std::string raw_key2;
EXPECT_TRUE(key2->GetRawKey(&raw_key2));

EXPECT_EQ(raw_key1, raw_key2);
EXPECT_EQ(key1->key(), key2->key());
}

TEST(SymmetricKeyTest, ImportDerivedKey) {
std::unique_ptr<crypto::SymmetricKey> key1(
crypto::SymmetricKey::DeriveKeyFromPassword(
crypto::SymmetricKey::HMAC_SHA1, "password", "somesalt", 1024, 160));
ASSERT_TRUE(key1);
std::string raw_key1;
EXPECT_TRUE(key1->GetRawKey(&raw_key1));

std::unique_ptr<crypto::SymmetricKey> key2(
crypto::SymmetricKey::Import(crypto::SymmetricKey::HMAC_SHA1, raw_key1));
std::unique_ptr<crypto::SymmetricKey> key2(crypto::SymmetricKey::Import(
crypto::SymmetricKey::HMAC_SHA1, key1->key()));
ASSERT_TRUE(key2);

std::string raw_key2;
EXPECT_TRUE(key2->GetRawKey(&raw_key2));

EXPECT_EQ(raw_key1, raw_key2);
EXPECT_EQ(key1->key(), key2->key());
}

struct PBKDF2TestVector {
Expand All @@ -86,8 +72,7 @@ TEST_P(SymmetricKeyDeriveKeyFromPasswordTest, DeriveKeyFromPassword) {
test_data.rounds, test_data.key_size_in_bits));
ASSERT_TRUE(key);

std::string raw_key;
key->GetRawKey(&raw_key);
const std::string& raw_key = key->key();
EXPECT_EQ(test_data.key_size_in_bits / 8, raw_key.size());
EXPECT_EQ(test_data.expected,
base::ToLowerASCII(base::HexEncode(raw_key.data(),
Expand Down

0 comments on commit 7bdfcab

Please sign in to comment.