Safe memory storage #1997

Harrm · 2024-03-01T10:42:35Z

Referenced issues

resolves https://github.com/qdrvm/KAGOME-audit/issues/29

Description of the Change

Storage for private keys and seeds is allocated in secure OpenSSL heap.

Benefits

Secure heap won't be swapped on disk or appear in core dumps, it is also protected from reads due to out-of-bounds UB
Private key memory is properly cleaned up when no longer needed

Possible Drawbacks

Data leaks to insecure memory due to interaction with dependencies, primarily libp2p
Secure store is limited in size and in the current implementation its size is fixed in compile time

Usage Examples or Tests

Alternate Designs

…e/safe-memory

turuslan · 2024-03-06T07:18:42Z

core/crypto/common.cpp

+/**
+ * Copyright Quadrivium LLC
+ * All Rights Reserved
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#include "crypto/common.hpp"
+
+namespace kagome::crypto {}


Remove empty .cpp file

core/crypto/common.hpp

turuslan · 2024-03-06T08:02:33Z

core/crypto/bip39/bip39_types.hpp


 namespace kagome::crypto::bip39 {
  namespace constants {
    constexpr size_t BIP39_SEED_LEN_512 = 64u;
  }  // namespace constants

-  using Bip39Seed = common::Buffer;
+  struct Bip39Tag;
+  using Bip39Seed = PrivateKey<constants::BIP39_SEED_LEN_512, Bip39Tag>;


Revert, Bip39Seed is variable size.
https://github.com/paritytech/polkadot-sdk/blob/8f8297e9debe79139e3a41be8b54a5fcd603f783/substrate/primitives/core/src/crypto.rs#L966-L991
from_string_with_seed() { if let Some(hex) { Self::Seed::fromHex(hex) } }

core/crypto/common.hpp

turuslan · 2024-03-06T08:55:25Z

core/crypto/crypto_store/crypto_suites.hpp

@@ -71,7 +70,7 @@ namespace kagome::crypto {
     * Create a seed from its bytes
     */
    virtual outcome::result<Seed> toSeed(
-        common::BufferView bytes) const noexcept = 0;
+        SecureCleanGuard<uint8_t> bytes) const noexcept = 0;


Revert to (span) (~~or (const SecureCleanGuard &)~~)?

Why? I want to make the user to clean up the memory used to iniitalize the seed.

core/crypto/pbkdf2/pbkdf2_provider.hpp

core/crypto/common.hpp

turuslan · 2024-03-07T08:19:04Z

core/common/hexutil.hpp

+  template <std::output_iterator<uint8_t> Iter>
+  outcome::result<void> unhex_to(std::string_view hex, Iter out) {


out is vector or array or span

Suggested change

template <std::output_iterator<uint8_t> Iter>

outcome::result<void> unhex_to(std::string_view hex, Iter out) {

template <typename T>

outcome::result<void> unhex_to(std::string_view hex, T &out) {

if constexpr (requires { out.resize(); }) {

out.resize(hex.size() / 2);

} else if (hex.size() != out.size() * 2) {

return ERROR;

}

core/common/hexutil.hpp

turuslan · 2024-03-07T08:24:04Z

core/authority_discovery/publisher/address_publisher.cpp

+          .secret_key = crypto::Ed25519PrivateKey::from(
+              crypto::SecureCleanGuard(private_key_bytes)),


Suggested change

.secret_key = crypto::Ed25519PrivateKey::from(

crypto::SecureCleanGuard(private_key_bytes)),

.secret_key = fromSpan<crypto::Ed25519PrivateKey>(libp2p_key.privateKey.data).value(),

turuslan · 2024-03-08T03:40:06Z

core/common/buffer.hpp

+    template <size_t Offset, size_t Length>
+    BufferView view() const {
+      return std::span(*this).template subspan<Offset, Length>();
+    }
+


Don't add Buffer member function.

Use std::span{}.subspan<_, _>() directly.

Must check size.

May make free function for any span.

Suggested change

template <size_t Offset, size_t Length>

BufferView view() const {

return std::span(*this).template subspan<Offset, Length>();

}

turuslan · 2024-03-08T03:49:14Z

core/crypto/bip39/mnemonic.cpp

+      if (seed.size() != HEX_SEED_STR_LENGTH) {
+        return bip39::MnemonicError::INVALID_SEED_LENGTH;
+      }
+      SecureBuffer<> bytes(HEX_SEED_BIT_LENGTH, 0);
+      OUTCOME_TRY(common::unhexWith0x(seed, bytes.begin()));
+      OUTCOME_TRY(seed, Bip39Seed::from(std::move(bytes)));


If Mnemonic.seed is Bip39Seed, and Bip39Seed is not vector but byte[64]

Suggested change

if (seed.size() != HEX_SEED_STR_LENGTH) {

return bip39::MnemonicError::INVALID_SEED_LENGTH;

}

SecureBuffer<> bytes(HEX_SEED_BIT_LENGTH, 0);

OUTCOME_TRY(common::unhexWith0x(seed, bytes.begin()));

OUTCOME_TRY(seed, Bip39Seed::from(std::move(bytes)));

Bip39Seed seed;

// ed25519/sr25519/ecdsa/bandersnatch seeds are 32 bytes

OUTCOME_TRY(common::unhexWith0x(seed, std::span{seed}.first(32));

mnemonic.seed = std::move(seed);

turuslan · 2024-03-08T03:51:55Z

core/crypto/bip39/mnemonic.hpp

@@ -29,12 +30,12 @@ namespace kagome::crypto::bip39 {
  struct Mnemonic {
    using Words = std::vector<std::string>;

-    boost::variant<common::Buffer, Words> seed;
+    std::variant<Words, Bip39Seed> seed;


It's not Bip39Seed (byte[64]).
May rename to entropy.
Must be variant<Words, vector> or variant<Words, byte[32]> (because current crypto types use 32 bytes seed).

core/crypto/crypto_store/crypto_store_impl.cpp

core/crypto/pbkdf2/pbkdf2_provider.hpp

…mory

Harrm added 11 commits February 28, 2024 15:28

Working on safe memory heap integration

d8940c7

Safe memory storage

cceeb73

Comment

5fc4177

Comment

5890611

Remove free function for cleanup

c3fde9e

Make seeds protected too

a51b29b

add logging

915f966

Fix common.cpp

717154b

Merge branch 'master' into feature/safe-memory

dae1767

Minor fix

2f3d7d1

Merge remote-tracking branch 'origin/feature/safe-memory' into featur…

e4db62c

…e/safe-memory

Harrm requested review from turuslan and iceseer March 5, 2024 09:39

Harrm marked this pull request as ready for review March 5, 2024 09:39

turuslan reviewed Mar 6, 2024

View reviewed changes

turuslan reviewed Mar 7, 2024

View reviewed changes

Fixed some tests

d85e5f9

turuslan reviewed Mar 8, 2024

View reviewed changes

Harrm added 3 commits March 11, 2024 11:47

Fix bip39 seed initialization

e0668a1

Fixes from review

3e8c623

Merge branch 'master' of github.com:qdrvm/kagome into feature/safe-me…

d68fec4

…mory

iceseer approved these changes Mar 12, 2024

View reviewed changes

turuslan approved these changes Mar 13, 2024

View reviewed changes

Merge branch 'master' into feature/safe-memory

5797a07

Harrm merged commit 3848227 into master Mar 21, 2024
9 of 10 checks passed

Harrm deleted the feature/safe-memory branch March 21, 2024 11:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Safe memory storage #1997

Safe memory storage #1997

Harrm commented Mar 1, 2024 •

edited

Loading

turuslan Mar 6, 2024

turuslan Mar 6, 2024

turuslan Mar 6, 2024 •

edited

Loading

Harrm Mar 11, 2024

turuslan Mar 7, 2024

turuslan Mar 7, 2024

turuslan Mar 8, 2024

Harrm Mar 11, 2024

turuslan Mar 8, 2024

turuslan Mar 8, 2024

		template <std::output_iterator<uint8_t> Iter>
		outcome::result<void> unhex_to(std::string_view hex, Iter out) {

-  template <std::output_iterator<uint8_t> Iter>
-  outcome::result<void> unhex_to(std::string_view hex, Iter out) {
+  template <typename T>
+  outcome::result<void> unhex_to(std::string_view hex, T &out) {
+    if constexpr (requires { out.resize(); }) {
+      out.resize(hex.size() / 2);
+    } else if (hex.size() != out.size() * 2) {
+      return ERROR;
+    }

		.secret_key = crypto::Ed25519PrivateKey::from(
		crypto::SecureCleanGuard(private_key_bytes)),

	.secret_key = crypto::Ed25519PrivateKey::from(
	crypto::SecureCleanGuard(private_key_bytes)),
	.secret_key = fromSpan<crypto::Ed25519PrivateKey>(libp2p_key.privateKey.data).value(),

Safe memory storage #1997

Safe memory storage #1997

Conversation

Harrm commented Mar 1, 2024 • edited Loading

Referenced issues

Description of the Change

Benefits

Possible Drawbacks

Usage Examples or Tests

Alternate Designs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turuslan Mar 6, 2024 • 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

Choose a reason for hiding this comment

Harrm commented Mar 1, 2024 •

edited

Loading

turuslan Mar 6, 2024 •

edited

Loading