Security Audit: Multiple Findings in Key Management, Encryption, and Memory Handling

See attached detailed report below.

## Executive Summary

This audit covers the Trust Wallet wallet-core library, focusing on cryptographic implementations, key management, memory safety, input validation, and chain-specific protocol handling across C++, Rust, and interface layers.

**Scope:** Core cryptographic primitives (`PrivateKey`, `PublicKey`, `HDWallet`, `Encrypt`, `Keystore`), chain-specific signers (Ethereum, Bitcoin, Tron, Cardano), CBOR/RLP parsers, ABI decoders, WebAuthn, and the FFI boundary layer.

**Methodology:** Manual source code review with data-flow analysis, focusing on code paths that handle private keys, signatures, encrypted payloads, and user-supplied input. Each finding was verified three times: (1) identification, (2) data-flow tracing, (3) mitigation check.

**Note:** Findings from existing merged PRs (e.g., #4667 TWData zeroize, #4681 Waves cast fix, #4680 Scrypt parameter enforcement, #4577 WebAuthn bounds fix, #4565/#4571 PublicKey verify, #4592 integer overflow fixes, #4626 random buffer, #4615 mnemonic clear, #4562 private key zeroize, #4650 EIP712 cyclic deps, #4597 ABI recursion, #4610 CBOR map fix, #4663 XRP amount, #4662 ed25519 Debug removal, #4580 AES IV validation) have been excluded.

---

## Findings Summary

| ID | Severity | Title | File | Line(s) |
|----|----------|-------|------|---------|
| F-01 | Medium | HDNode not zeroed in `getMasterKey()` and `getMasterKeyExtension()` | `src/HDWallet.cpp` | 147-158 |
| F-02 | Medium | Cardano staking HDNode (`node2`) never zeroed after key extraction | `src/HDWallet.cpp` | 185-191 |
| F-03 | Medium | HDNode not zeroed in `getExtendedPrivateKeyAccount()` and related functions | `src/HDWallet.cpp` | 236-248, 251-264 |
| F-04 | Medium | Non-constant-time MAC comparison in keystore decryption | `src/Keystore/EncryptionParameters.cpp` | 134 |
| F-05 | Medium | Derived key not zeroed after use in `EncryptedPayload::decrypt()` | `src/Keystore/EncryptionParameters.cpp` | 114-168 |
| F-06 | Medium | `EncryptedPayload` destructor uses `std::fill` instead of `memzero` for sensitive data | `src/Keystore/EncryptionParameters.cpp` | 110-111 |
| F-07 | Low | Incomplete PKCS7 padding validation in `AESCBCDecrypt` | `src/Encrypt.cpp` | 79-88 |
| F-08 | Low | `getCipher()` silently defaults to AES-128-CTR for unrecognized cipher strings including `aes-128-cbc` | `src/Keystore/AESParameters.cpp` | 21-30 |
| F-09 | Low | `signAsDER()` does not validate digest length before passing to `ecdsa_sign_digest` | `src/PrivateKey.cpp` | 368-385 |
| F-10 | Low | `account + 0x80000000` can silently overflow for large account values | `src/HDWallet.cpp` | 246, 261 |
| F-11 | Informational | `TWDataReset` uses `std::fill` which compiler may optimize away | `src/interface/TWData.cpp` | 101-104 |
| F-12 | Informational | `PrivateKey::isValid()` does not check upper bound for ed25519/curve25519 keys in C++ path | `src/PrivateKey.cpp` | 72-106 |

---

## Detailed Findings

### F-01: HDNode Not Zeroed in `getMasterKey()` and `getMasterKeyExtension()`

**Severity:** Medium
**File:** `src/HDWallet.cpp`, lines 147-158

**Description:**
The `getMasterKey()` and `getMasterKeyExtension()` functions create an `HDNode` on the stack containing the master private key material, extract the key into a `Data` vector, but never call `memzero()` on the `HDNode` before it goes out of scope. The private key bytes remain on the stack and may be recoverable through memory dumps or cold-boot attacks.

In contrast, `getKeyByCurve()` (line 190, 197) correctly calls `TW::memzero(&node)` before returning.

**Vulnerable Code:**
```cpp
// src/HDWallet.cpp:147-151
template <std::size_t seedSize>
PrivateKey HDWallet<seedSize>::getMasterKey(TWCurve curve) const {
    auto node = getMasterNode(*this, curve);
    auto data = Data(node.private_key, node.private_key + PrivateKey::_size);
    return PrivateKey(data, curve);
    // node goes out of scope WITHOUT being zeroed -- private_key remains on stack
}

// src/HDWallet.cpp:153-158
template <std::size_t seedSize>
PrivateKey HDWallet<seedSize>::getMasterKeyExtension(TWCurve curve) const {
    auto node = getMasterNode(*this, curve);
    auto data = Data(node.private_key_extension, node.private_key_extension + PrivateKey::_size);
    return PrivateKey(data, curve);
    // node goes out of scope WITHOUT being zeroed
}
```

**Impact:** Master private key material persists on the stack after function return. An attacker with memory read access (e.g., via a separate vulnerability, crash dump, or memory forensics) could recover the master key.

**Recommended Fix:**
```cpp
template <std::size_t seedSize>
PrivateKey HDWallet<seedSize>::getMasterKey(TWCurve curve) const {
    auto node = getMasterNode(*this, curve);
    auto data = Data(node.private_key, node.private_key + PrivateKey::_size);
    TW::memzero(&node);
    return PrivateKey(data, curve);
}

template <std::size_t seedSize>
PrivateKey HDWallet<seedSize>::getMasterKeyExtension(TWCurve curve) const {
    auto node = getMasterNode(*this, curve);
    auto data = Data(node.private_key_extension, node.private_key_extension + PrivateKey::_size);
    TW::memzero(&node);
    return PrivateKey(data, curve);
}
```

---

### F-02: Cardano Staking HDNode (`node2`) Never Zeroed After Key Extraction

**Severity:** Medium
**File:** `src/HDWallet.cpp`, lines 185-191

**Description:**
In the `getKeyByCurve()` Cardano path, the primary `node` is correctly zeroed at line 190, but the staking path's `node2` (declared as `const auto node2` at line 185) is never zeroed. This `node2` contains the full Cardano staking private key, extension, and chain code.

**Vulnerable Code:**
```cpp
// src/HDWallet.cpp:185-191
const auto node2 = getNode(*this, curve, stakingPath);
auto pkData2 = Data(node2.private_key, node2.private_key + PrivateKey::_size);
auto extData2 = Data(node2.private_key_extension, node2.private_key_extension + PrivateKey::_size);
auto chainCode2 = Data(node2.chain_code, node2.chain_code + PrivateKey::_size);

TW::memzero(&node);  // node is zeroed
return PrivateKey(pkData, extData, chainCode, pkData2, extData2, chainCode2, curve);
// node2 is NEVER zeroed -- staking private key remains on stack
```

**Impact:** Cardano staking private key material leaks on the stack. This is particularly concerning because Cardano keys are extended (192 bytes), meaning a large amount of cryptographic material remains accessible.

**Recommended Fix:**
Declare `node2` as non-const and zero both nodes:
```cpp
auto node2 = getNode(*this, curve, stakingPath);
// ... extract key material ...
TW::memzero(&node);
TW::memzero(&node2);
return PrivateKey(pkData, extData, chainCode, pkData2, extData2, chainCode2, curve);
```

---

### F-03: HDNode Not Zeroed in `getExtendedPrivateKeyAccount()` and Related Functions

**Severity:** Medium
**File:** `src/HDWallet.cpp`, lines 236-248, 251-264

**Description:**
`getExtendedPrivateKeyAccount()` serializes an HDNode that contains the private key directly into a Base58-encoded string. The `node` is never zeroed. Similarly, `getExtendedPublicKeyAccount()` at line 259 also derives through a private key node and never zeroes it (even though it only serializes the public key, the private key is still present in the node on the stack).

`getPrivateKeyFromExtended()` at line 308 has the same issue: the deserialized `node` containing the private key is never zeroed.

**Vulnerable Code:**
```cpp
// src/HDWallet.cpp:244-247
auto node = getNode(*this, curve, derivationPath);
auto fingerprintValue = fingerprint(&node, publicKeyHasher(coin));
hdnode_private_ckd(&node, account + 0x80000000);
return serialize(&node, fingerprintValue, version, false, base58Hasher(coin));
// node with private key is NEVER zeroed
```

**Impact:** Extended private key material remains on the stack. In `getExtendedPublicKeyAccount`, even though only the public key is serialized, the full private key is present in the HDNode structure on the stack.

**Recommended Fix:**
Add `TW::memzero(&node)` before each return statement, using a local variable for the return value:
```cpp
auto result = serialize(&node, fingerprintValue, version, false, base58Hasher(coin));
TW::memzero(&node);
return result;
```

---

### F-04: Non-Constant-Time MAC Comparison in Keystore Decryption

**Severity:** Medium
**File:** `src/Keystore/EncryptionParameters.cpp`, line 134

**Description:**
The MAC verification in `EncryptedPayload::decrypt()` uses the `!=` operator on `Data` (which is `std::vector<uint8_t>`). The standard library `operator!=` for vectors performs a byte-by-byte comparison that short-circuits on the first mismatch. This creates a timing side-channel that can be exploited to recover the MAC value byte by byte, and by extension test password guesses with sub-full-computation granularity.

**Vulnerable Code:**
```cpp
// src/Keystore/EncryptionParameters.cpp:134
if (mac != _mac) {
    throw DecryptionError::invalidPassword;
}
```

**Impact:** An attacker who can measure decryption timing with sufficient precision could perform a timing attack to distinguish correct MAC prefix bytes from incorrect ones. While scrypt/PBKDF2 already impose a time floor, the MAC comparison leaks information about the derived key, potentially allowing an attacker to reduce the search space for offline attacks.

**Recommended Fix:**
Use a constant-time comparison function:
```cpp
static bool constantTimeEqual(const Data& a, const Data& b) {
    if (a.size() != b.size()) return false;
    volatile uint8_t diff = 0;
    for (size_t i = 0; i < a.size(); ++i) {
        diff |= a[i] ^ b[i];
    }
    return diff == 0;
}

// Then in decrypt():
if (!constantTimeEqual(mac, _mac)) {
    throw DecryptionError::invalidPassword;
}
```

---

### F-05: Derived Key Not Zeroed After Use in `EncryptedPayload::decrypt()`

**Severity:** Medium
**File:** `src/Keystore/EncryptionParameters.cpp`, lines 114-168

**Description:**
The `decrypt()` function derives a key from the password using scrypt or PBKDF2, uses it for MAC verification and AES decryption, but never zeroes the `derivedKey` buffer before the function returns. The derived key is the actual AES encryption key -- if recovered from memory, an attacker can decrypt the stored payload without knowing the password.

**Vulnerable Code:**
```cpp
// src/Keystore/EncryptionParameters.cpp:114-168
Data EncryptedPayload::decrypt(const Data& password) const {
    auto derivedKey = Data();
    // ... derivedKey is populated with scrypt/PBKDF2 output ...
    // ... used for MAC and decryption ...
    return decrypted;
    // derivedKey is NEVER zeroed -- contains the derived encryption key
}
```

The `AES context` (`aes_encrypt_ctx` / `aes_decrypt_ctx`) objects that hold the expanded key schedule are also not zeroed.

**Impact:** The derived encryption key and AES key schedule persist on the stack after `decrypt()` returns. This key is equivalent to the password for the purpose of decrypting the keystore payload.

**Recommended Fix:**
```cpp
Data EncryptedPayload::decrypt(const Data& password) const {
    auto derivedKey = Data();
    auto mac = Data();
    // ... existing code ...

    Data decrypted(encrypted.size());
    Data iv = params.cipherParams.iv;
    // ... decryption code ...

    // Zero sensitive material before returning
    memzero(derivedKey.data(), derivedKey.size());
    memzero(iv.data(), iv.size());

    return decrypted;
}
```

---

### F-06: `EncryptedPayload` Destructor Uses `std::fill` Instead of `memzero`

**Severity:** Medium
**File:** `src/Keystore/EncryptionParameters.cpp`, lines 109-112

**Description:**
The destructor of `EncryptedPayload` uses `std::fill` to zero out the `encrypted` and `_mac` fields. The C++ standard allows the compiler to optimize away writes to memory that are not subsequently read (dead store elimination). An optimizing compiler may remove these `std::fill` calls entirely since the object is being destroyed immediately after. The `memzero` function (from TrezorCrypto) is specifically designed to resist this optimization.

**Vulnerable Code:**
```cpp
// src/Keystore/EncryptionParameters.cpp:109-112
EncryptedPayload::~EncryptedPayload() {
    std::fill(encrypted.begin(), encrypted.end(), 0);
    std::fill(_mac.begin(), _mac.end(), 0);
}
```

**Impact:** Under compiler optimizations (e.g., `-O2`, `-O3`), the encrypted ciphertext and MAC may not be zeroed, leaving them in memory for potential recovery.

**Recommended Fix:**
```cpp
EncryptedPayload::~EncryptedPayload() {
    if (!encrypted.empty()) {
        memzero(encrypted.data(), encrypted.size());
    }
    if (!_mac.empty()) {
        memzero(_mac.data(), _mac.size());
    }
}
```

---

### F-07: Incomplete PKCS7 Padding Validation in `AESCBCDecrypt`

**Severity:** Low
**File:** `src/Encrypt.cpp`, lines 79-88

**Description:**
The PKCS7 padding removal only checks that `paddingSize <= result.size()` but does not verify that all padding bytes have the expected value. A valid PKCS7 padding of value `N` requires the last `N` bytes to all equal `N`. Without this verification, corrupted ciphertext or incorrect keys will not be detected at the padding stage, and the function may return truncated or garbage data.

Additionally, a `paddingSize` of 0 is not rejected -- if the last byte is 0x00, the unpadded size equals the full decrypted size, which is incorrect PKCS7 behavior (valid PKCS7 padding values are 1-16 for a 16-byte block).

**Vulnerable Code:**
```cpp
// src/Encrypt.cpp:79-88
if (paddingMode == TWAESPaddingModePKCS7 && result.size() > 0) {
    const byte paddingSize = result[result.size() - 1];
    if (paddingSize <= result.size()) {
        const size_t unpaddedSize = result.size() - paddingSize;
        Data resultUnpadded = TW::data(result.data(), unpaddedSize);
        return resultUnpadded;
    }
}
```

**Impact:** While this function is not directly used for keystore decryption (which uses CTR mode), it is a general-purpose utility. Improper padding validation can lead to padding oracle attacks if the function is used in an interactive decryption context, and can mask decryption failures.

**Recommended Fix:**
```cpp
if (paddingMode == TWAESPaddingModePKCS7 && result.size() > 0) {
    const byte paddingSize = result[result.size() - 1];
    if (paddingSize == 0 || paddingSize > blockSize || paddingSize > result.size()) {
        throw std::invalid_argument("Invalid PKCS7 padding");
    }
    // Verify all padding bytes
    for (size_t i = result.size() - paddingSize; i < result.size(); ++i) {
        if (result[i] != paddingSize) {
            throw std::invalid_argument("Invalid PKCS7 padding");
        }
    }
    const size_t unpaddedSize = result.size() - paddingSize;
    Data resultUnpadded = TW::data(result.data(), unpaddedSize);
    return resultUnpadded;
}
```

---

### F-08: `getCipher()` Silently Defaults to AES-128-CTR for Unrecognized Cipher Strings

**Severity:** Low
**File:** `src/Keystore/AESParameters.cpp`, lines 21-30

**Description:**
The `getCipher()` function defaults to `TWStoredKeyEncryptionAes128Ctr` for any unrecognized cipher string. Notably, `aes-128-cbc` is not handled in this function (it only checks for CTR variants), so when deserializing a keystore that uses `aes-128-cbc`, the cipher is misidentified as `aes-128-ctr`.

Note: There is an open PR #4694 that addresses AES parameter deserialization, but it is not yet merged. This finding documents the current state on `master`.

**Vulnerable Code:**
```cpp
// src/Keystore/AESParameters.cpp:21-30
static TWStoredKeyEncryption getCipher(const std::string& cipher) {
    if (cipher == Keystore::gAes128Ctr) {
        return TWStoredKeyEncryption::TWStoredKeyEncryptionAes128Ctr;
    } else if (cipher == Keystore::gAes192Ctr) {
        return TWStoredKeyEncryption::TWStoredKeyEncryptionAes192Ctr;
    } else if (cipher == Keystore::gAes256Ctr) {
        return TWStoredKeyEncryption::TWStoredKeyEncryptionAes256Ctr;
    }
    return TWStoredKeyEncryptionAes128Ctr; // Silent default
}
```

**Impact:** A keystore file encrypted with `aes-128-cbc` would be decrypted using the wrong mode (CTR), producing incorrect plaintext without an error.

**Recommended Fix:**
```cpp
static TWStoredKeyEncryption getCipher(const std::string& cipher) {
    if (cipher == Keystore::gAes128Ctr) {
        return TWStoredKeyEncryptionAes128Ctr;
    } else if (cipher == Keystore::gAes128Cbc) {
        return TWStoredKeyEncryptionAes128Cbc;
    } else if (cipher == Keystore::gAes192Ctr) {
        return TWStoredKeyEncryptionAes192Ctr;
    } else if (cipher == Keystore::gAes256Ctr) {
        return TWStoredKeyEncryptionAes256Ctr;
    }
    throw std::invalid_argument("Unsupported cipher: " + cipher);
}
```

---

### F-09: `signAsDER()` Does Not Validate Digest Length

**Severity:** Low
**File:** `src/PrivateKey.cpp`, lines 368-385

**Description:**
The `signAsDER()` method passes the digest directly to `ecdsa_sign_digest()` without validating that the digest is at least 32 bytes. While the `sign()` method uses `ecdsa_sign_digest_checked()` which has a length check (line 258-261), `signAsDER()` bypasses this wrapper and calls `ecdsa_sign_digest()` directly. If a short digest is provided, the TrezorCrypto library will read beyond the input buffer.

**Vulnerable Code:**
```cpp
// src/PrivateKey.cpp:368-385
Data PrivateKey::signAsDER(const Data& digest) const {
    // ...
    Data sig(64);
    bool success =
        ecdsa_sign_digest(&secp256k1, key().data(), digest.data(), sig.data(), nullptr, nullptr) == 0;
    // No check on digest.size() >= 32
```

**Impact:** Calling `signAsDER()` with a digest shorter than 32 bytes causes a heap buffer over-read in `ecdsa_sign_digest()`. While unlikely in normal wallet operations, it could be triggered by malformed input.

**Recommended Fix:**
```cpp
Data PrivateKey::signAsDER(const Data& digest) const {
    if (_curve.has_value() && _curve.value() != TWCurveSECP256k1) {
        throw std::invalid_argument("DER signature is only supported for SECP256k1");
    }
    if (digest.size() < 32) {
        return {};
    }
    // ... rest of function
```

---

### F-10: `account + 0x80000000` Can Silently Overflow

**Severity:** Low
**File:** `src/HDWallet.cpp`, lines 246, 261

**Description:**
The `getExtendedPrivateKeyAccount()` and `getExtendedPublicKeyAccount()` functions compute `account + 0x80000000` to derive a hardened child key. The `account` parameter is `uint32_t`, so if `account >= 0x80000000` (2147483648), the addition wraps around silently, producing an incorrect derivation path.

**Vulnerable Code:**
```cpp
// src/HDWallet.cpp:246
hdnode_private_ckd(&node, account + 0x80000000);
```

**Impact:** If an external caller passes an `account` value >= 2147483648, the derivation path silently wraps, potentially deriving keys for an unintended path.

**Recommended Fix:**
```cpp
if (account >= 0x80000000) {
    return "";
}
hdnode_private_ckd(&node, account + 0x80000000);
```

---

### F-11: `TWDataReset` Uses `std::fill` Which Compiler May Optimize Away

**Severity:** Informational
**File:** `src/interface/TWData.cpp`, lines 101-104

**Description:**
`TWDataReset()` uses `std::fill` to zero a `TWData` buffer. Unlike `TWDataDelete()` (which correctly uses `memzero`), `TWDataReset()` uses `std::fill` which the compiler may optimize away as a dead store.

**Recommended Fix:**
```cpp
void TWDataReset(TWData *_Nonnull data) {
    auto* v = const_cast<Data*>(reinterpret_cast<const Data*>(data));
    if (!v->empty()) {
        memzero(v->data(), v->size());
    }
}
```

---

### F-12: `PrivateKey::isValid()` Does Not Check Curve Order for ed25519/Curve25519

**Severity:** Informational
**File:** `src/PrivateKey.cpp`, lines 72-106

**Description:**
The C++ `PrivateKey::isValid(data, curve)` function only performs curve order validation for SECP256k1 and NIST256p1 curves. For ed25519 variants and curve25519, it falls through to the default case which returns `true` without curve-specific validation. This is inconsistent with the Rust implementation which performs curve-specific validation for all key types.

**Impact:** Minimal in practice, as ed25519 signing applies clamping. However, the inconsistency between C++ and Rust validation could lead to edge-case behavioral differences at the FFI boundary.

---

## Testing Methodology

1. **Manual Code Review:** Line-by-line analysis of all files handling private keys, encryption, and sensitive data
2. **Data Flow Analysis:** Traced private key bytes from creation through derivation, signing, and destruction to identify points where sensitive data is not properly cleared
3. **Differential Analysis:** Compared C++ and Rust implementations for consistency in validation and error handling
4. **Pattern Matching:** Searched for known anti-patterns: `std::fill` on sensitive data, `operator!=` for MAC comparison, unchecked casts, missing bounds checks
5. **Existing Fix Review:** Cross-referenced all findings against merged PRs and open issues to eliminate duplicates

## Positive Observations

The codebase shows significant security maturity:

- The Rust `PrivateKey` uses `#[derive(ZeroizeOnDrop)]` correctly
- `TWDataDelete` properly uses `memzero` (per PR #4667)
- `HDWallet` destructor correctly zeroes seed, mnemonic, and passphrase
- `PrivateKey` destructor calls `cleanup()` which uses `memzero`
- ScryptParameters validation is thorough with overflow checks (reinforced by PR #4680)
- Random buffer generation was recently hardened (PR #4626)
- WebAuthn parser bounds checking was fixed (PR #4577)
- PublicKey verify functions now validate signature lengths (PR #4565, #4571, #4693)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Security Audit: Multiple Findings in Key Management, Encryption, and Memory Handling #4706

Executive Summary

Findings Summary

Detailed Findings

F-01: HDNode Not Zeroed in `getMasterKey()` and `getMasterKeyExtension()`

F-02: Cardano Staking HDNode (`node2`) Never Zeroed After Key Extraction

F-03: HDNode Not Zeroed in `getExtendedPrivateKeyAccount()` and Related Functions

F-04: Non-Constant-Time MAC Comparison in Keystore Decryption

F-05: Derived Key Not Zeroed After Use in `EncryptedPayload::decrypt()`

F-06: `EncryptedPayload` Destructor Uses `std::fill` Instead of `memzero`

F-07: Incomplete PKCS7 Padding Validation in `AESCBCDecrypt`

F-08: `getCipher()` Silently Defaults to AES-128-CTR for Unrecognized Cipher Strings

F-09: `signAsDER()` Does Not Validate Digest Length

F-10: `account + 0x80000000` Can Silently Overflow

F-11: `TWDataReset` Uses `std::fill` Which Compiler May Optimize Away

F-12: `PrivateKey::isValid()` Does Not Check Curve Order for ed25519/Curve25519

Testing Methodology

Positive Observations

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

ID	Severity	Title	File	Line(s)
F-01	Medium	HDNode not zeroed in `getMasterKey()` and `getMasterKeyExtension()`	`src/HDWallet.cpp`	147-158
F-02	Medium	Cardano staking HDNode (`node2`) never zeroed after key extraction	`src/HDWallet.cpp`	185-191
F-03	Medium	HDNode not zeroed in `getExtendedPrivateKeyAccount()` and related functions	`src/HDWallet.cpp`	236-248, 251-264
F-04	Medium	Non-constant-time MAC comparison in keystore decryption	`src/Keystore/EncryptionParameters.cpp`	134
F-05	Medium	Derived key not zeroed after use in `EncryptedPayload::decrypt()`	`src/Keystore/EncryptionParameters.cpp`	114-168
F-06	Medium	`EncryptedPayload` destructor uses `std::fill` instead of `memzero` for sensitive data	`src/Keystore/EncryptionParameters.cpp`	110-111
F-07	Low	Incomplete PKCS7 padding validation in `AESCBCDecrypt`	`src/Encrypt.cpp`	79-88
F-08	Low	`getCipher()` silently defaults to AES-128-CTR for unrecognized cipher strings including `aes-128-cbc`	`src/Keystore/AESParameters.cpp`	21-30
F-09	Low	`signAsDER()` does not validate digest length before passing to `ecdsa_sign_digest`	`src/PrivateKey.cpp`	368-385
F-10	Low	`account + 0x80000000` can silently overflow for large account values	`src/HDWallet.cpp`	246, 261
F-11	Informational	`TWDataReset` uses `std::fill` which compiler may optimize away	`src/interface/TWData.cpp`	101-104
F-12	Informational	`PrivateKey::isValid()` does not check upper bound for ed25519/curve25519 keys in C++ path	`src/PrivateKey.cpp`	72-106

Security Audit: Multiple Findings in Key Management, Encryption, and Memory Handling #4706

Description

Executive Summary

Findings Summary

Detailed Findings

F-01: HDNode Not Zeroed in getMasterKey() and getMasterKeyExtension()

F-02: Cardano Staking HDNode (node2) Never Zeroed After Key Extraction

F-03: HDNode Not Zeroed in getExtendedPrivateKeyAccount() and Related Functions

F-04: Non-Constant-Time MAC Comparison in Keystore Decryption

F-05: Derived Key Not Zeroed After Use in EncryptedPayload::decrypt()

F-06: EncryptedPayload Destructor Uses std::fill Instead of memzero

F-07: Incomplete PKCS7 Padding Validation in AESCBCDecrypt

F-08: getCipher() Silently Defaults to AES-128-CTR for Unrecognized Cipher Strings

F-09: signAsDER() Does Not Validate Digest Length

F-10: account + 0x80000000 Can Silently Overflow

F-11: TWDataReset Uses std::fill Which Compiler May Optimize Away

F-12: PrivateKey::isValid() Does Not Check Curve Order for ed25519/Curve25519

Testing Methodology

Positive Observations

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

F-01: HDNode Not Zeroed in `getMasterKey()` and `getMasterKeyExtension()`

F-02: Cardano Staking HDNode (`node2`) Never Zeroed After Key Extraction

F-03: HDNode Not Zeroed in `getExtendedPrivateKeyAccount()` and Related Functions

F-05: Derived Key Not Zeroed After Use in `EncryptedPayload::decrypt()`

F-06: `EncryptedPayload` Destructor Uses `std::fill` Instead of `memzero`

F-07: Incomplete PKCS7 Padding Validation in `AESCBCDecrypt`

F-08: `getCipher()` Silently Defaults to AES-128-CTR for Unrecognized Cipher Strings

F-09: `signAsDER()` Does Not Validate Digest Length

F-10: `account + 0x80000000` Can Silently Overflow

F-11: `TWDataReset` Uses `std::fill` Which Compiler May Optimize Away

F-12: `PrivateKey::isValid()` Does Not Check Curve Order for ed25519/Curve25519