-
-
Notifications
You must be signed in to change notification settings - Fork 203
Description
CryptoVec currently mlocks all allocated memory to prevent swapping sensitive data to disk. While secure, this has significant performance overhead for high-throughput use cases like SFTP file transfers.
Profiling shows mlock/munlock syscalls using a lot of CPU time in one of my projects (25%+) when most of the data flowing through buffers isn't actually cryptographic secrets - it's packet payloads, channel data (stdin/stdout), etc.
OpenSSH's approach
OpenSSH's sshbuf uses non-mlocked buffers by default:
sshbuf_new()at packet.c:245-248 creates
buffers forincoming_packet,outgoing_packet, etc. without mlock- Channel data at channels.c:3479 uses regular
buffers - Sensitive data (private keys, shared secrets) gets explicit secure handling via
freezero()
(misc.c)
Proposed changes
1. Consistent zeroization
The resize() truncation path uses memset at cryptovec.rs:216 which could be optimized away by the compiler. However, reallocation uses zeroize at line 230 and Drop uses zeroize at line 367, both with the optimization barrier at line 383.
Should use zeroize() consistently in all paths, matching:
- OpenSSH's explicit_bzero pattern
- RustCrypto's optimization_barrier approach (see also issue
#1269)
2. Conditional mlock with categorized buffers
Add a secure: bool field to CryptoVec and categorize buffers:
Should be mlocked (secrets)
- Shared secrets (K) - passed to
kex_derive_keys(), cleared withexplicit_bzeroafter use
(kex.c:1123-1145) - Derived session keys -
newkeys->enc.key,newkeys->mac.keycleared withexplicit_bzero, struct freed withfreezero(kex.c:690-711) - Exchange hash working buffer - contains K during derivation via
ssh_digest_update_buffer(hashctx, shared_secret)
(kex.c:1078-1097) - Agent lock password -
lock_pwhashcleared withexplicit_bzeroafter use
(ssh-agent.c:1474-1485)
Should NOT be mlocked (not secrets)
- Packet I/O buffers -
incoming_packet,outgoing_packetcreated withsshbuf_new(), no mlock
(packet.c:245-248) - Channel data -
c->input,c->output,c->extendedcreated withsshbuf_new(), no mlock
(channels.c:540-542) - Decompression buffer -
compression_buffercreated withsshbuf_new(), no mlock
(packet.c:800-801) - Public keys - regular
sshbufallocation,sshbuf_freeusesfreezerofor zeroization but no mlock
(sshbuf.c:191-192)
Note: OpenSSH's sshbuf_new() (sshbuf.c:93) does not
mlock, but sshbuf_free() (sshbuf.c:191) uses
freezero() to zero memory. The model is: zero everything on free, but only mlock actual secrets.
Question: What should the API default be?
Option A: Secure by default
CryptoVec::new() // mlocked
CryptoVec::new_unlocked() // not mlocked, for non-secrets
CryptoVec::from_slice_unlocked() // not mlocked
CryptoVec::from_vec_unlocked() // not mlocked- Safer for library consumers who may not think about security
- External code using CryptoVec would need to opt-out for performance
Option B: Performance by default (matches OpenSSH)
CryptoVec::new() // not mlocked (like sshbuf_new())
CryptoVec::new_secure() // mlocked, for secrets
CryptoVec::from_slice_secure()
CryptoVec::from_vec_secure()- Matches OpenSSH's model
- External code would need to opt-in for security
Which approach do you prefer? I'm happy to implement either way.