# Stronghold design/security review and its alternatives

Here we consider Stronghold's architecture and internals in regard to cryptographic soundness.

## Unclear purpose and declared features, no multiparty computation

It's unclear the purpose of Stronghold besides a vague "secure key storage". One would expect it to support (provide API for) hardware storages or perform multi-party computation (such as multi-/threshold signature); both these features are not designed. In the end the main use-case is "long-term key storage in a snapshot -- password-encrypted file" which can be replaced by an encrypted database.

Note, previously, Stronghold had p2p feature allowing it connect to remote "vaults", this feature was removed.

## Overcomplicated architecture

### Stronghold types

Internally (in engine + runtime + client components) Stronghold implements the following types:

- `Stronghold`: contains a `Snapshot`, hashmap of `Client`s, and a `Store`;
- `Client`: contains a `KeyProvider`, a `DbView`, and a `Store`;
- `Store`: wrapper for `Cache` (byte vecs to byte vecs)
- `Cache`: hashmap keys to values with expiration (garbage collection); sensitive data must be encrypted by caller, garbage collection must be run by caller;
- `DbView`: hashmap of `Vault`s;
- `Vault`: `Key` plus hashmap of `Record`s;
- `Record`: contains `SealedTransaction`;
- Transactions -- `Transaction`, `SealedTransaction`, `DataTransaction`, `RevocationTransaction`: the purpose unclear, `SealedTransaction` contains encrypted data;
- `KeyStore`: `NCKey` plus hashmap of byte vecs;
- snapshot: encrypted
- `NonContiguousMemory`:
- other primitives: `Buffer`, `Boxed`, `BoxProvider`, `MemoryShard`, `RamMemory`.

The user code becomes overcomplicated. There's no vault abstraction/trait to allow different implementations. The architecture is completely unclear; transaction layer is unclear.

### Stronghold security mechanisms

- revocation (transaction layer): unclear purpose / usecases;
- expiration (`Cache`): added complexity, garbage collection, runtime (to manage timers, GC);
- memory protection (memlock, memprotect) (`Buffer`): lock memory pages to avoid swapping them into disk, limit read/write access to memory page;
- encryption (CryptoProtectMemory): unimplemented -- keys can be encrypted using primitives provided by OS;
- secret sharing (`NonContiguousMemoty`): secret key is split into 2 shares;
- procedures: encapsulate access to secret keys;

The only useful mechanisms are memory protection (implemented as wrapper over libsodium memory management API) and secret sharing. The rest is unnecessary, doesn't add security. Procedures layer is overcomplicated and doesn't add security.

### Stronghold public API

Public client API includes:

- `Client`, `Stronghold`, and `Store` types;
- `Provider`: implementation of `BoxProvider`;
- `Procedure`: cryptographic procedures for various operations; this is the main entry point for public API to access keys stored in Stronghold.

Public API is bloated, to use procedures users have to repack inputs into a procedure struct (instead of just calling a function with arguments) and lose type-safety of the inner keys.

### Stronghold design conclusions

In general, such complex API should support some complex use-case patterns. It's not clear what would those use-cases be and what are the benefits of such approach compared to some more simple solutions.

## No static type safety

Stronghold's implementation language is rust which provides type safety. However, Stronghold doesn't benefit from in in regard to the keys it stores. All keys and sensitive data is passed via byte arrays (slices and vecs). It's hard to add some meta information to a key or trace how a key is being processed internally (via which "stores" and "key providers" it passes through).

Also, many of the client procedures accept or produce byte vecs. This loses their initial type and requires additional conversion which potentially might be error prone.

## No runtime type safety

Stronghold also does not implement any key type checks in regard to key purpose (in which algorithm a key is allowed to be used). This leads to the following case: in order to encrypt data with a symmetric key such key must be generated as Ed25519 secret key which is misleading and potentially might lead to vulnerabilities (key leakage) if misused.

## Hardcoded key sizes

In a few places in Stronghold 32-byte key size constants are hardcoded. This leaves little room for supporting different key sizes. It could have been useful to add some metadata to a key (a few extra bytes added to a 32-byte key), or to support algorithms with a different key sizes (such as Secp256k1 ECDSA public key which is 33-byte in a compressed form, which doesn't fit a hardcoded type in public API, see `PublicKey` client's procedure).

## No zeroization

One of purposes of Stronghold is to avoid/localize potential key leakages. However, keys and sensitive data are passed around via `Vec` types which allocate memory in heap and do not protect it whatsoever, or via local array variables (eg. of type `[u8; 32]`) which are not cleared after use. This leads to keys being leaked into stack/heap/swap memory multiple times because several copies of a key can be created. This issues was partially fixed with `Zeroizing` type. But such feature should have been implemented initially.

## Weird cryptography choices, cryptography misuse

Stronghold uses questionable approach in regard to encrypting data. For example, the encrypted file format version 2 for snapshots used X25519 to generate a symmetric data encryption key, however the recipient's public key was derived from the private one. This defeats the whole purpose of X25519, misuses it and shows lack of understanding basic cryptographic principles which is unacceptable for such product.

Another case of cryptographic misuse is password-based encryption for snapshots. The recipient's private key for encrypting snapshots was derived from passwords without the possibility to keep salt used in password-based key derivation. This led to a major vulnerability where all keys were derived using low interation count (100 while recommended value should be of the order 100000) and fixed salt. The use of fixed salt leads to a possibility where an attacker having access to multiple snapshot files of different users could efficiently crack password (to at least one of snapshots) using rainbow table bruteforce algorithm. It was possible due to weak design of cryptographic primitives and tools.

## Alternative solutions

### Features

Stronghold implemented features:

- unprotected Store/Cache with expiration
- encrypted DbView/Vault
- in-memory protected KeyStore/NCKey
- password-protected snapshot

Required features:

- in-memory key protection
- easy access/operations
- snapshot encrypted storage
- cross-platform
- hardware keys

### Stronghold-based solution

Stronghold can still be a good solution if all the issues are addressed. The plan might include the following steps:

- choose security mechanisms (in-memory key protection, hardware key token support, snapshot format);
- use standard security mechanisms (for internal encryption/integrity/snapshot etc.);
- design simple architecture:
- do not target all-in-one solution;
- clear key system: master key, integrity key, snapshot key, key encryption keys, etc.; how keys are identified, created, stored, used, destroyed;
- type-safety for stored keys, support secure operations (slip10, sign, etc.);
- traits for key stores (in-memory and hardware), use standard traits as possible (eg. `signature::Signer`);
- do not include unnecessary stuff such as libp2p, transactions, revocation/expiration/GC/runtime;
- extensible by external crates/traits/impls;
- get rid of most stronghold stuff; keep only necessary (NonContiguousMemory, Buffer, etc.);
- implement the rest;
- provide migration guide.

### Existing solutions

- [Overview article](https://geekflare.com/secret-management-software/)
- [Another overview article](https://spectralops.io/blog/top-9-secret-management-tools-for-2022/)
- [Lyft's confidant library](https://github.com/lyft/confidant)
- [Infinitree encrypted DB](https://docs.rs/infinitree/latest/infinitree/)
- [HashiCorp's Vault](https://github.com/hashicorp/vault) -- tool for securely accessing secrets written in go. This is enterprise solution, looks like an overkill.
- [SecureStore](https://neosmart.net/blog/securestore-open-secrets-format/) ([rust crate](https://docs.rs/securestore/latest/securestore/)) -- cross-platform, encrypted secrets storage. It can be thought of as super-simplistic version of stronghold, it supports encrypted in-memory named key storage, encrypted persistent storage in a file; it doesn't support other memory protection techniques (such as memLock, memProtect, etc.) and hardware keys.
- [secret](https://github.com/stouset/secrets) -- wrapper for [libsodium memory management utilities](https://download.libsodium.org/doc/memory_management).
- [hard](https://github.com/tom25519/hard) -- rust safe buffers wrappers for libsodium.
- [microkv](https://docs.rs/microkv/latest/microkv/) -- persistent key-value store.
- [cocoon](https://docs.rs/cocoon/latest/cocoon/) -- protected containers to wrap sensitive data with strong encryption and format validation.

### SecureStore-based solution

SecureStore lacks a few mechanisms: in-memory protection, hardware key token support, cryptographic operations. Stronghold's in-memory protection mechanisms (NonContiguousMemory, Buffer) can be used to extend SecureStore. The rest should be implemented.

## Conclusion

Overall, Stronghold doesn't meet quality/security standards that are common in the industry and are expected by users and developers from such a software. In particular, its design is overcomplicated, it implements a lot of (useless) features (a lot of different stores, key expiration, procedures layer without key type safety), doesn't have some nice features (such as hardware key support, key type safety), it has had some serious security bugs (snapshot v2 bad key derivation that could result in practical attack and total compromise of users' seeds when snapshots are leaked, keys leaking in stack/heap memory due to lack of zeroization, misused asymmetric x25519 algorithm that was used as symmetric one).

To overcome this issue we propose to analyze and set up the list of requirements from target applications (wallet, etc.). The next step would be to search for and choose an appropriate existing well-maintained and audited solution. HashiCorp's Vault is a top enterprise solution that seems a bit overkill. A more practical and simplistic solution is SecureStore.