GH-31603: [C++][Python] Wrap encryption keys in secure string #46017

EnricoMi · 2025-04-03T09:50:21Z

Rationale for this change

Arrow deals with secrets like encryption / decryption keys which must be kept private. One way of leaking such secrets is through memory allocation where another process allocates memory that previously hold the secret, because that memory was not cleared before being freed.

What changes are included in this PR?

Uses various implementations of securely clearing memory, notably

SecureZeroMemory(Windows)
memset_s(STDC)
OPENSSL_cleanse (OpenSSL >= 3)
explicit_bzero(glibc 2.25+)
volatile memset (fallback).

Are these changes tested?

Unit tests.

Are there any user-facing changes?

APIs that hand over secrets to Arrow require the secret to be encapsulated in a SecureString.

This PR includes breaking changes to public APIs.

TODO:

collect list of affected APIs
provide instructions for migration
deprecate string-based methods
make SecureString available to Python API
make sure all paths are tested (test all compile directives)
move to arrow/util
Supersedes EXP: SecureZero helper to securely clear memory #12890.

GitHub Issue: [C++][Parquet] WipeOutDecryptionKeys doesn't securely wipe out keys #31603

github-actions · 2025-04-03T09:50:50Z

⚠️ GitHub issue #31603 has been automatically assigned in GitHub to PR creator.

github-actions · 2025-04-03T09:52:19Z

⚠️ GitHub issue #31603 has been automatically assigned in GitHub to PR creator.

github-actions · 2025-04-03T10:00:11Z

⚠️ GitHub issue #31603 has been automatically assigned in GitHub to PR creator.

pitrou · 2025-04-03T10:11:18Z

@EnricoMi Do you want a preliminary review on this?

pitrou · 2025-04-03T10:23:12Z

For the record, OpenSSL allows allocating memory from a secure heap, but that requires calling process-wide initialization functions. I've asked about this.

EnricoMi · 2025-04-03T13:25:03Z

@EnricoMi Do you want a preliminary review on this?

Yes, please! This is in a complete PoC state (Linux build and test is green).

Is this the right direction before I roll this out to Python?
Can I break the API or should I deprecate exiting methods and provide both, std::string and secure string methods?
Do you have some hints how I can fix the link issues for Windows? I presume I need to add the secure_string.cc file somewhere so Windows links to the object file.
Shouldn't it be sufficient to only use OPENSSL_cleanse and count on OpenSSL providing OS-dependent implementations?

EnricoMi · 2025-04-03T13:29:20Z

I agree, that OPENSSL_secure_malloc seems problematic. What if the application crashes (or receives kill -9) and CRYPTO_secure_malloc_done cannot be called. Are secrets then not protected? If secrets are protected without CRYPTO_secure_malloc_done, and multiple calls to OPENSSL_secure_malloc are idempotent, then this would be a good way to go.

pitrou · 2025-04-03T14:11:16Z

Is this the right direction before I roll this out to Python?

Yes!

Can I break the API or should I deprecate exiting methods and provide both, std::string and secure string methods?

I would rather deprecate the existing methods.

Do you have some hints how I can fix the link issues for Windows?

You probably want to add a PARQUET_EXPORT annotation to the SecureString class?

Shouldn't it be sufficient to only use OPENSSL_cleanse and count on OpenSSL providing OS-dependent implementations?

Well, currently the OpenSSL function is very crude :)
https://github.com/openssl/openssl/blob/c66e00398c9feabc02ff6e678089a3dc95f985d2/crypto/mem_clr.c#L18-L25

If secrets are protected without CRYPTO_secure_malloc_done, and multiple calls to OPENSSL_secure_malloc are idempotent, then this would be a good way to go.

What I'm worried about is different libraries in the same process all trying to use the CRYPTO_secure_malloc APIs, or perhaps even one library calling CRYPTO_secure_malloc_done while another library is still using those API calls.

cpp/src/parquet/encryption/secure_string.h

pitrou · 2025-04-03T15:37:13Z

cpp/src/parquet/encryption/secure_string_test.cc

+  {
+    std::string tiny("abc");
+    SecureString::secure_clear(tiny);
+    assert_securely_cleared(tiny);
+  }


For more stringent testing, I think we also want to examine the string area before it is cleared (or moved, etc.).

Something like (untested):

std::string_view StringArea(const std::string& string) { return std::string_view(string.data(), string.capacity()); } void AssertSecurelyCleared(std::string_view area) { // the entire area is filled with zeros std::string zeros(area.size(), '\0'); ASSERT_EQ(area, std::string_view(zeros)); } void AssertSecurelyCleared(const std::string& string) { AssertSecurelyCleared(StringArea(string)); } TEST(TestSecureString, SecureClearCheck) { // short string { std::string tiny("abc"); auto old_area = StringArea(tiny); SecureString::SecureClear(tiny); AssertSecurelyCleared(tiny); AssertSecurelyCleared(old_area); } // etc.

But tiny.data() and old_area.data() point to the same address, what is the point testing old_area? Just in case pointers change? If they would change, then old_area would point to a freed memory area, right?

pitrou · 2025-04-03T15:49:15Z

cpp/src/parquet/encryption/secure_string_test.cc

+  ASSERT_FALSE(secret_from_secret.empty());
+  ASSERT_EQ(secret_from_secret, secret_from_move_secret);
+}
+


Probably also want a test that a SecureString was cleared on destruction ?

How would I do that, the memory where SecureString.as_span points to will be freed on deconstruction. How can I access that memory securely?

The standardese "true" way would be to template SecureString on an allocator so that you could intercept destruction of the bytes it stores. That's entirely too much here; we could just add a debug assertion inside ~SecureString (maybe only enabled by an env var or #if defined(ARROW_ASSERT_SECURE_STRING_ZEROED))

Oh, alternatively: since every STL I'm aware of implements std::string with an inline short string case, we could use:

// Most std::string impls store their contents inline, so we can check // the object representation for a crib to ensure that destruction does // wipe all sensitive bytes. constexpr auto kCrib = "01CRIB01"; alignas(SecureString) char object_repr[sizeof(SecureString)]; std::string_view object_repr_view{object_repr, sizeof(object_repr)}; SecureString* cribbed = new (&object_repr) SecureString(std::string(kCrib)); bool crib_found_before = object_repr_view.find(kCrib) != object_repr_view.npos; cribbed->~SecureString(); // NOTE: do not allow this call to be skipped! bool crib_found_after = object_repr_view.find(kCrib) != object_repr_view.npos; if (crib_found_before) { ASSERT_FALSE(crib_found_after); }

I think that's well-defined: https://godbolt.org/z/31cY59EM3

cpp/src/parquet/encryption/encryption.h

pitrou · 2025-04-03T16:05:12Z

cpp/src/parquet/encryption/encryption.h

  std::string aad_prefix_;
  std::shared_ptr<AADPrefixVerifier> aad_prefix_verifier_;

-  const std::string empty_string_ = "";
+  const encryption::SecureString empty_string_ = encryption::SecureString("");


Is empty_string_ useful at all? It looks rather dubious.

It is, method FileDecryptionProperties::column_key returns an empty string as the encryption key when the key id is not found. It should rather throw a meaningful key not found exception. I may fix that while I am touching this code anyway.

This looks like a holdover from the first draft of encryption; IIRC column_key() returned references to strings at that time (so return ""; would fail to compile)

Still needed, improved name.

cpp/src/parquet/encryption/encryption.h

bkietz · 2025-04-04T16:57:56Z

cpp/src/parquet/encryption/encryption.h

  std::string aad_prefix_;
  std::shared_ptr<AADPrefixVerifier> aad_prefix_verifier_;

-  const std::string empty_string_ = "";
+  const encryption::SecureString empty_string_ = encryption::SecureString("");


This looks like a holdover from the first draft of encryption; IIRC column_key() returned references to strings at that time (so return ""; would fail to compile)

cpp/src/parquet/encryption/file_key_wrapper.cc

bkietz · 2025-04-04T17:03:04Z

cpp/src/parquet/encryption/internal_file_decryptor.cc

@@ -46,7 +46,7 @@ int32_t Decryptor::CiphertextLength(int32_t plaintext_len) const {

 int32_t Decryptor::Decrypt(::arrow::util::span<const uint8_t> ciphertext,
                           ::arrow::util::span<uint8_t> plaintext) {
-  return aes_decryptor_->Decrypt(ciphertext, str2span(key_), str2span(aad_), plaintext);
+  return aes_decryptor_->Decrypt(ciphertext, key_.as_span(), str2span(aad_), plaintext);


Since str2span is no longer used, we could delete it

It is still being used to convert aad and encrypted_key strings to spans.

bkietz · 2025-04-04T17:06:10Z

cpp/src/parquet/encryption/read_configurations_test.cc

-  std::string kFooterEncryptionKey_ = std::string(kFooterEncryptionKey);
-  std::string kColumnEncryptionKey1_ = std::string(kColumnEncryptionKey1);
-  std::string kColumnEncryptionKey2_ = std::string(kColumnEncryptionKey2);
+  SecureString kFooterEncryptionKey_ = kFooterEncryptionKey;


const seems warranted here

Suggested change

SecureString kFooterEncryptionKey_ = kFooterEncryptionKey;

const SecureString kFooterEncryptionKey_ = kFooterEncryptionKey;

Doesn't that hold for all k... members in this class (TestDecryptionConfiguration) and TestEncryptionConfiguration as well? Is it worth it given this are tests?

bkietz · 2025-04-04T17:10:47Z

cpp/src/parquet/encryption/secure_string.cc

+  // by the compiler.
+  static const volatile auto memset_v = &memset;
+  memset_v(data, 0, size);
+  __asm__ __volatile__("" ::"r"(data) : "memory");


It seems questionable to me that the ultimate fallback should not be a simple loop

Suggested change

__asm__ __volatile__("" ::"r"(data) : "memory");

for (size_t i = 0; i < size; ++i) {

data[i] = 0;

}

I don't know how cross platform this __asm__ block is.

Also, since this code is borrowed from another project it needs to be mentioned in LICENSE.txt, for example

arrow/LICENSE.txt

Lines 1541 to 1543 in 7df396e

A function gettimeofday in utilities.cc is based on

http://www.google.com/codesearch/p?hl=en#dR3YEbitojA/COPYING&q=GetSystemTimeAsFileTime%20license:bsd

What about

volatile char *p = data; while (size--) *p++ = 0;

https://cplusplus.com/articles/ETqpX9L8/

Given https://github.com/BLAKE2/libb2/blob/master/src/blake2-impl.h is CC0 Public Domain Dedication, and our code deviates a lot, you could say it is as much inspired by that implementation than it is by all those variations available on stackoverflow.

I don't think this qualifies as a copy or derived work.

bkietz · 2025-04-04T17:11:57Z

cpp/src/parquet/encryption/secure_string.h

+  SecureString() noexcept = default;
+  SecureString(SecureString&&) noexcept;
+  SecureString(const SecureString&) noexcept = default;
+  explicit SecureString(std::string&&) noexcept;


Suggested change

explicit SecureString(std::string&&) noexcept;

explicit SecureString(std::string&&) noexcept;

explicit SecureString(size_t size) : SecureString{std::string(size, '\0')} {}

I have added SecureString(size_t, char) to mimic basic_string<char> constructors.

cpp/src/parquet/encryption/secure_string.h

bkietz · 2025-04-04T17:17:37Z

cpp/src/parquet/encryption/secure_string_test.cc

+  ASSERT_FALSE(secret_from_secret.empty());
+  ASSERT_EQ(secret_from_secret, secret_from_move_secret);
+}
+


The standardese "true" way would be to template SecureString on an allocator so that you could intercept destruction of the bytes it stores. That's entirely too much here; we could just add a debug assertion inside ~SecureString (maybe only enabled by an env var or #if defined(ARROW_ASSERT_SECURE_STRING_ZEROED))

bkietz · 2025-04-07T14:53:02Z

cpp/src/parquet/encryption/secure_string.h

+#include "arrow/util/span.h"
+#include "parquet/platform.h"
+
+namespace parquet::encryption {


Would you please move this to arrow/util/? I think it will be useful outside parquet

EnricoMi · 2025-04-11T11:05:01Z

cpp/src/parquet/encryption/encryption.cc

  if (key.empty()) return this;

-  DCHECK(!key.empty());
-  key_ = key;
+  DCHECK(key_.empty());


This looked suspicious, all other setter methods check if the member is unset.
Here, the check is always true as it checks the input.

This is a breaking change if user code calls this setter twice with non-empty keys.

EnricoMi · 2025-04-11T11:09:07Z

cpp/src/parquet/encryption/encryption.cc

+                                                       std::string column_path,
+                                                       encryption::SecureString key,
+                                                       std::string key_metadata)
+    : column_path_(std::move(column_path)),


This moves member initialization from the body, which is the more standard way. However, the body could accidentally check input arguments, which after std::move are "empty".

While this is correct:

encrypted_with_footer_key_(encrypted && key.empty()), key_(std::move(key)),

if members are shuffled around, then this becomes incorrect:

key_(std::move(key)), encrypted_with_footer_key_(encrypted && key.empty()),

as encrypted_with_footer_key_ is initialized on the "empty" key.

Do you consider this pattern too risky for future bugs? Happy to revert this initialization.

EnricoMi · 2025-04-11T11:14:38Z

cpp/src/parquet/encryption/encryption.cc

  if (column_decryption_properties_.find(column_path) !=
      column_decryption_properties_.end()) {
    auto column_prop = column_decryption_properties_.at(column_path);
    if (column_prop != nullptr) {
      return column_prop->key();
    }
  }
-  return empty_string_;
+  return no_key_;


we are still returning a reference to a SecureString. I tried to return some std::optional, to replace empty SecureString notation for "no key given" with std::nullopt, but this opened a can of worms. Not going down this rabbit whole in this PR.

At least the naming is improved.

github-actions · 2025-04-11T11:26:25Z

⚠️ GitHub issue #31603 has been automatically assigned in GitHub to PR creator.

github-actions · 2025-04-11T13:05:21Z

⚠️ GitHub issue #31603 has been automatically assigned in GitHub to PR creator.

github-actions · 2025-04-11T13:10:40Z

⚠️ GitHub issue #31603 has been automatically assigned in GitHub to PR creator.

github-actions bot added Component: Parquet Component: C++ awaiting review Awaiting review labels Apr 3, 2025

EnricoMi mentioned this pull request Apr 3, 2025

EXP: SecureZero helper to securely clear memory #12890

Draft

pitrou reviewed Apr 3, 2025

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Apr 3, 2025

EnricoMi force-pushed the secure-string branch 2 times, most recently from 9848539 to b303afa Compare April 4, 2025 16:29

bkietz requested changes Apr 4, 2025

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Apr 4, 2025

bkietz mentioned this pull request Apr 7, 2025

GH-40343: [C++] Move S3FileSystem to the registry #41559

Merged

bkietz requested changes Apr 7, 2025

View reviewed changes

EnricoMi force-pushed the secure-string branch from b303afa to 4590921 Compare April 11, 2025 10:52

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 11, 2025

EnricoMi commented Apr 11, 2025

View reviewed changes

EnricoMi force-pushed the secure-string branch from 4590921 to 88d4bb0 Compare April 11, 2025 11:25

Wrap encryption keys in SecureString

5f73a1f

EnricoMi added 11 commits April 11, 2025 13:25

Rename non-trivial SecureString methods

0456be4

Add mutable as_span method, add as_view

689bc8d

More tests

42d7319

Change SecureClear(std::string&) to SecureClear(std::string*)

01ab190

Decrypt key directly intoSecureString

f3f4087

Remove const from KeyWithMasterId members

f90f196

Replace call-by-ref with call-by-value when copied

0e74be5

Use const for constants and construct SecureString from consts in tests

29bc5e9

Inline IntegerKeyIdRetriever::GetKey(std::string) implementation

75c1f0a

Add comment to no_key

d3670d4

Revert InternalFileDecryptor::RetrieveColumnKeyIfEmpty

adbbfc8

EnricoMi force-pushed the secure-string branch from 88d4bb0 to adbbfc8 Compare April 11, 2025 11:25

Remove noexcept

fe79da0

EnricoMi force-pushed the secure-string branch from 82d4425 to fe79da0 Compare April 11, 2025 17:04

	SecureString kFooterEncryptionKey_ = kFooterEncryptionKey;
	const SecureString kFooterEncryptionKey_ = kFooterEncryptionKey;

-  __asm__ __volatile__("" ::"r"(data) : "memory");
+  for (size_t i = 0; i < size; ++i) {
+    data[i] = 0;
+  }

	A function gettimeofday in utilities.cc is based on

	http://www.google.com/codesearch/p?hl=en#dR3YEbitojA/COPYING&q=GetSystemTimeAsFileTime%20license:bsd

	explicit SecureString(std::string&&) noexcept;
	explicit SecureString(std::string&&) noexcept;
	explicit SecureString(size_t size) : SecureString{std::string(size, '\0')} {}

GH-31603: [C++][Python] Wrap encryption keys in secure string #46017

Are you sure you want to change the base?

GH-31603: [C++][Python] Wrap encryption keys in secure string #46017

Conversation

EnricoMi commented Apr 3, 2025 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Apr 3, 2025

github-actions bot commented Apr 3, 2025

github-actions bot commented Apr 3, 2025

pitrou commented Apr 3, 2025

pitrou commented Apr 3, 2025

EnricoMi commented Apr 3, 2025

EnricoMi commented Apr 3, 2025

pitrou commented Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkietz Apr 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnricoMi Apr 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EnricoMi Apr 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkietz Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

EnricoMi Apr 11, 2025 • edited Loading

Choose a reason for hiding this comment

EnricoMi Apr 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 11, 2025

github-actions bot commented Apr 11, 2025

github-actions bot commented Apr 11, 2025

EnricoMi commented Apr 3, 2025 •

edited

Loading

pitrou commented Apr 3, 2025 •

edited

Loading

bkietz Apr 4, 2025 •

edited

Loading

EnricoMi Apr 10, 2025 •

edited

Loading

EnricoMi Apr 11, 2025 •

edited

Loading

bkietz Apr 7, 2025 •

edited

Loading

EnricoMi Apr 11, 2025 •

edited

Loading

EnricoMi Apr 11, 2025 •

edited

Loading