Skip to content

Commit

Permalink
Migrate code which modifies individual bytes of SecretData objects to…
Browse files Browse the repository at this point in the history
… use SecretBuffer.

SecretData will only get assignment operators, assigning complete chunks or SecretBuffer objects.

PiperOrigin-RevId: 720537923
Change-Id: Ib0643f5c5386496e62889ec9e52db2dc2c379950
  • Loading branch information
tholenst authored and copybara-github committed Jan 28, 2025
1 parent ce816b7 commit 27dc593
Show file tree
Hide file tree
Showing 12 changed files with 53 additions and 29 deletions.
6 changes: 6 additions & 0 deletions tink/subtle/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ cc_library(
"//tink/internal:dfsan_forwarders",
"//tink/internal:fips_utils",
"//tink/internal:safe_stringops",
"//tink/internal:secret_buffer",
"//tink/internal:ssl_unique_ptr",
"//tink/internal:util",
"//tink/util:errors",
Expand Down Expand Up @@ -340,6 +341,7 @@ cc_library(
"//tink/internal:fips_utils",
"//tink/internal:md_util",
"//tink/internal:rsa_util",
"//tink/internal:secret_buffer",
"//tink/internal:ssl_unique_ptr",
"//tink/internal:util",
"//tink/signature:rsa_ssa_pss_parameters",
Expand Down Expand Up @@ -548,6 +550,7 @@ cc_library(
"//tink/internal:call_with_core_dump_protection",
"//tink/internal:dfsan_forwarders",
"//tink/internal:fips_utils",
"//tink/internal:secret_buffer",
"//tink/internal:util",
"//tink/util:errors",
"//tink/util:secret_data",
Expand All @@ -556,6 +559,7 @@ cc_library(
"@boringssl//:crypto",
"@com_google_absl//absl/algorithm:container",
"@com_google_absl//absl/base:config",
"@com_google_absl//absl/base:nullability",
"@com_google_absl//absl/memory",
"@com_google_absl//absl/status",
"@com_google_absl//absl/strings",
Expand Down Expand Up @@ -614,6 +618,7 @@ cc_library(
visibility = ["//visibility:public"],
deps = [
":subtle_util",
"//tink/internal:secret_buffer",
"//tink/util:secret_data",
"//tink/util:status",
"@boringssl//:crypto",
Expand Down Expand Up @@ -661,6 +666,7 @@ cc_library(
"//tink/internal:call_with_core_dump_protection",
"//tink/internal:dfsan_forwarders",
"//tink/internal:fips_utils",
"//tink/internal:secret_buffer",
"//tink/util:errors",
"//tink/util:secret_data",
"//tink/util:status",
Expand Down
6 changes: 6 additions & 0 deletions tink/subtle/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ tink_cc_library(
tink::internal::dfsan_forwarders
tink::internal::fips_utils
tink::internal::safe_stringops
tink::internal::secret_buffer
tink::internal::ssl_unique_ptr
tink::internal::util
tink::util::errors
Expand Down Expand Up @@ -326,6 +327,7 @@ tink_cc_library(
tink::internal::fips_utils
tink::internal::md_util
tink::internal::rsa_util
tink::internal::secret_buffer
tink::internal::ssl_unique_ptr
tink::internal::util
tink::signature::rsa_ssa_pss_parameters
Expand Down Expand Up @@ -518,6 +520,7 @@ tink_cc_library(
tink::subtle::subtle_util
absl::algorithm_container
absl::config
absl::nullability
absl::memory
absl::status
absl::strings
Expand All @@ -528,6 +531,7 @@ tink_cc_library(
tink::internal::call_with_core_dump_protection
tink::internal::dfsan_forwarders
tink::internal::fips_utils
tink::internal::secret_buffer
tink::internal::util
tink::util::errors
tink::util::secret_data
Expand Down Expand Up @@ -587,6 +591,7 @@ tink_cc_library(
absl::strings
absl::span
crypto
tink::internal::secret_buffer
tink::util::secret_data
tink::util::status
)
Expand Down Expand Up @@ -631,6 +636,7 @@ tink_cc_library(
tink::internal::call_with_core_dump_protection
tink::internal::dfsan_forwarders
tink::internal::fips_utils
tink::internal::secret_buffer
tink::util::errors
tink::util::secret_data
tink::util::status
Expand Down
4 changes: 2 additions & 2 deletions tink/subtle/aes_cmac_boringssl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include "tink/internal/dfsan_forwarders.h"
#include "tink/internal/fips_utils.h"
#include "tink/internal/safe_stringops.h"
#include "tink/internal/secret_buffer.h"
#include "tink/internal/ssl_unique_ptr.h"
#include "tink/internal/util.h"
#include "tink/mac.h"
Expand Down Expand Up @@ -127,8 +128,7 @@ util::Status AesCmacBoringSsl::VerifyMac(absl::string_view mac,
"Incorrect tag size: expected %d, found %d", tag_size_,
mac.size());
}
util::SecretData computed_mac;
computed_mac.resize(kMaxTagSize);
internal::SecretBuffer computed_mac(kMaxTagSize);

if (!ComputeMacInternal(key_, computed_mac.data(), data)) {
return util::Status(absl::StatusCode::kInternal, "Failed to compute CMAC");
Expand Down
15 changes: 9 additions & 6 deletions tink/subtle/aes_eax_boringssl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

#include "absl/algorithm/container.h"
#include "absl/base/config.h"
#include "absl/base/nullability.h"
#include "absl/memory/memory.h"
#include "absl/status/status.h"
#include "absl/strings/string_view.h"
Expand All @@ -37,6 +38,7 @@
#include "tink/internal/call_with_core_dump_protection.h"
#include "tink/internal/dfsan_forwarders.h"
#include "tink/internal/fips_utils.h"
#include "tink/internal/secret_buffer.h"
#include "tink/internal/util.h"
#include "tink/subtle/random.h"
#include "tink/subtle/subtle_util.h"
Expand Down Expand Up @@ -138,16 +140,16 @@ bool AesEaxBoringSsl::IsValidNonceSize(size_t nonce_size_in_bytes) {
}

util::SecretData AesEaxBoringSsl::ComputeB() const {
util::SecretData block(kBlockSize, 0);
internal::SecretBuffer block(kBlockSize, 0);
EncryptBlock(&block);
MultiplyByX(block.data(), block.data());
return block;
return util::internal::AsSecretData(std::move(block));
}

util::SecretData AesEaxBoringSsl::ComputeP() const {
util::SecretData rv(kBlockSize, 0);
internal::SecretBuffer rv(kBlockSize, 0);
MultiplyByX(B_.data(), rv.data());
return rv;
return util::internal::AsSecretData(std::move(rv));
}

crypto::tink::util::StatusOr<std::unique_ptr<Aead>> AesEaxBoringSsl::New(
Expand Down Expand Up @@ -190,11 +192,12 @@ AesEaxBoringSsl::Block AesEaxBoringSsl::Pad(
return padded_block;
}

void AesEaxBoringSsl::EncryptBlock(util::SecretData* block) const {
void AesEaxBoringSsl::EncryptBlock(
absl::Nonnull<internal::SecretBuffer*> block) const {
AES_encrypt(block->data(), block->data(), aeskey_.get());
}

void AesEaxBoringSsl::EncryptBlock(Block* block) const {
void AesEaxBoringSsl::EncryptBlock(absl::Nonnull<Block*> block) const {
AES_encrypt(block->data(), block->data(), aeskey_.get());
}

Expand Down
6 changes: 4 additions & 2 deletions tink/subtle/aes_eax_boringssl.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,14 @@
#include <string>
#include <utility>

#include "absl/base/nullability.h"
#include "absl/strings/string_view.h"
#include "absl/types/span.h"
#include "openssl/aes.h"
#include "openssl/evp.h"
#include "tink/aead.h"
#include "tink/internal/fips_utils.h"
#include "tink/internal/secret_buffer.h"
#include "tink/util/secret_data.h"
#include "tink/util/status.h"
#include "tink/util/statusor.h"
Expand Down Expand Up @@ -94,8 +96,8 @@ class AesEaxBoringSsl : public Aead {
const uint8_t y[kBlockSize]);

// Encrypts a single block with AES.
void EncryptBlock(Block* block) const;
void EncryptBlock(util::SecretData* block) const;
void EncryptBlock(absl::Nonnull<Block*> block) const;
void EncryptBlock(absl::Nonnull<internal::SecretBuffer*> block) const;

// Pads a partial data block of size 0 <= len <= kBlockSize.
Block Pad(absl::Span<const uint8_t> data) const;
Expand Down
11 changes: 6 additions & 5 deletions tink/subtle/aes_siv_boringssl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#include "tink/internal/call_with_core_dump_protection.h"
#include "tink/internal/dfsan_forwarders.h"
#include "tink/internal/fips_utils.h"
#include "tink/internal/secret_buffer.h"
#include "tink/subtle/subtle_util.h"
#include "tink/util/errors.h"
#include "tink/util/secret_data.h"
Expand Down Expand Up @@ -89,18 +90,18 @@ AesSivBoringSsl::New(const util::SecretData& key) {
}

util::SecretData AesSivBoringSsl::ComputeCmacK1() const {
util::SecretData cmac_k1(kBlockSize, 0);
internal::SecretBuffer cmac_k1(kBlockSize, 0);
CallWithCoreDumpProtection([&]() {
EncryptBlock(cmac_k1.data(), cmac_k1.data());
MultiplyByX(cmac_k1.data());
});
return cmac_k1;
return util::internal::AsSecretData(std::move(cmac_k1));
}

util::SecretData AesSivBoringSsl::ComputeCmacK2() const {
util::SecretData cmac_k2(cmac_k1_);
internal::SecretBuffer cmac_k2 = util::internal::AsSecretBuffer(cmac_k1_);
CallWithCoreDumpProtection([&]() { MultiplyByX(cmac_k2.data()); });
return cmac_k2;
return util::internal::AsSecretData(cmac_k2);
}

void AesSivBoringSsl::EncryptBlock(const uint8_t in[kBlockSize],
Expand Down Expand Up @@ -277,7 +278,7 @@ util::StatusOr<std::string> AesSivBoringSsl::DecryptDeterministically(
return res;
}

util::SecretData s2v(kBlockSize);
internal::SecretBuffer s2v(kBlockSize);

// Note that we very much need to protect the calculation of the IV even when
// the plaintext may be leaked.
Expand Down
7 changes: 4 additions & 3 deletions tink/subtle/ed25519_sign_boringssl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,9 @@ util::StatusOr<std::string> Ed25519SignBoringSsl::Sign(

util::StatusOr<std::unique_ptr<PublicKeySign>> Ed25519SignBoringSsl::New(
const Ed25519PrivateKey &key) {
util::SecretData private_key =
internal::SecretBuffer private_key = util::internal::AsSecretBuffer(
key.GetPrivateKeyBytes(GetPartialKeyAccess())
.Get(InsecureSecretKeyAccess::Get());
.Get(InsecureSecretKeyAccess::Get()));
std::string public_key =
std::string(key.GetPublicKey().GetPublicKeyBytes(GetPartialKeyAccess()));
size_t private_key_size = private_key.size();
Expand All @@ -151,7 +151,8 @@ util::StatusOr<std::unique_ptr<PublicKeySign>> Ed25519SignBoringSsl::New(
public_key.size());

return New(
private_key, key.GetOutputPrefix(),
util::internal::AsSecretData(std::move(private_key)),
key.GetOutputPrefix(),
key.GetParameters().GetVariant() == Ed25519Parameters::Variant::kLegacy
? std::string(1, 0)
: "");
Expand Down
6 changes: 4 additions & 2 deletions tink/subtle/ed25519_sign_boringssl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,14 @@ util::StatusOr<Ed25519KeyPair> NewKeyPair() {
return key.status();
}

util::SecretData key_pair = (*key)->private_key;
internal::SecretBuffer key_pair =
util::internal::AsSecretBuffer((*key)->private_key);
key_pair.resize(key_pair.size() + (*key)->public_key.size());
memcpy(key_pair.data() + (*key)->private_key.size(),
(*key)->public_key.data(), (*key)->public_key.size());

return {{(*key)->public_key, key_pair}};
return {
{(*key)->public_key, util::internal::AsSecretData(std::move(key_pair))}};
}

TEST_F(Ed25519SignBoringSslTest, testBasicSign) {
Expand Down
9 changes: 5 additions & 4 deletions tink/subtle/hkdf.cc
Original file line number Diff line number Diff line change
Expand Up @@ -98,15 +98,15 @@ util::StatusOr<util::SecretData> Hkdf::ComputeHkdf(HashType hash,
return evp_md.status();
}

util::SecretData out_key(out_len);
internal::SecretBuffer out_key(out_len);
util::Status result = CallWithCoreDumpProtection([&]() {
return SslHkdf(*evp_md, util::SecretDataAsStringView(ikm), salt, info,
absl::MakeSpan(out_key.data(), out_key.size()));
});
if (!result.ok()) {
return result;
}
return out_key;
return util::internal::AsSecretData(std::move(out_key));
}

util::StatusOr<std::string> Hkdf::ComputeHkdf(HashType hash,
Expand All @@ -133,11 +133,12 @@ util::StatusOr<util::SecretData> Hkdf::ComputeEciesHkdfSymmetricKey(
HashType hash, absl::string_view kem_bytes,
const util::SecretData &shared_secret, absl::string_view salt,
absl::string_view info, size_t out_len) {
util::SecretData ikm(kem_bytes.size() + shared_secret.size());
internal::SecretBuffer ikm(kem_bytes.size() + shared_secret.size());
internal::SafeMemCopy(ikm.data(), kem_bytes.data(), kem_bytes.size());
internal::SafeMemCopy(ikm.data() + kem_bytes.size(), shared_secret.data(),
shared_secret.size());
return Hkdf::ComputeHkdf(hash, ikm, salt, info, out_len);
return Hkdf::ComputeHkdf(hash, util::internal::AsSecretData(std::move(ikm)),
salt, info, out_len);
}

} // namespace subtle
Expand Down
3 changes: 1 addition & 2 deletions tink/subtle/hmac_boringssl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,7 @@ util::Status HmacBoringSsl::VerifyMac(absl::string_view mac,
return util::Status(absl::StatusCode::kInvalidArgument,
"incorrect tag size");
}
util::SecretData buf;
buf.resize(EVP_MAX_MD_SIZE);
internal::SecretBuffer buf(EVP_MAX_MD_SIZE);
unsigned int out_len;
const uint8_t* res = internal::CallWithCoreDumpProtection([&]() {
return HMAC(md_, key_.data(), key_.size(),
Expand Down
6 changes: 4 additions & 2 deletions tink/subtle/random.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,13 @@
#include <cstddef>
#include <cstdint>
#include <string>
#include <utility>

#include "absl/status/status.h"
#include "absl/strings/str_cat.h"
#include "absl/types/span.h"
#include "openssl/rand.h"
#include "tink/internal/secret_buffer.h"
#include "tink/subtle/subtle_util.h"
#include "tink/util/secret_data.h"
#include "tink/util/status.h"
Expand Down Expand Up @@ -80,11 +82,11 @@ uint16_t Random::GetRandomUInt16() { return GetRandomUint<uint16_t>(); }
uint8_t Random::GetRandomUInt8() { return GetRandomUint<uint8_t>(); }

util::SecretData Random::GetRandomKeyBytes(size_t length) {
util::SecretData buf(length, 0);
internal::SecretBuffer buf(length, 0);
GetRandomBytes(
absl::MakeSpan(reinterpret_cast<char *>(buf.data()), buf.size()))
.IgnoreError();
return buf;
return util::internal::AsSecretData(std::move(buf));
}

} // namespace subtle
Expand Down
3 changes: 2 additions & 1 deletion tink/subtle/rsa_ssa_pss_sign_boringssl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#include "tink/internal/fips_utils.h"
#include "tink/internal/md_util.h"
#include "tink/internal/rsa_util.h"
#include "tink/internal/secret_buffer.h"
#include "tink/internal/ssl_unique_ptr.h"
#include "tink/internal/util.h"
#include "tink/partial_key_access.h"
Expand Down Expand Up @@ -86,7 +87,7 @@ util::StatusOr<std::string> SslRsaSsaPssSign(RSA* rsa_private_key,
// We store the signature temporarily in a secret data: it will depend on the
// secret key and if we store it in an unprotected buffer, dfsan will notice
// in tests.
util::SecretData temporary_buffer(kModulusSize);
internal::SecretBuffer temporary_buffer(kModulusSize);
// This will write exactly kModulusSize bytes to temporary_buffer.
if (RSA_padding_add_PKCS1_PSS_mgf1(
/*rsa=*/rsa_private_key, /*EM=*/temporary_buffer.data(),
Expand Down

0 comments on commit 27dc593

Please sign in to comment.