WI-0420: Eliminate redundant crypto work, I/O waste & memory bloat#43
WI-0420: Eliminate redundant crypto work, I/O waste & memory bloat#43mrvigneshvt wants to merge 12 commits into
Conversation
… blob unlock_container no longer reads entire encrypted blob into memory. Uses streaming SHA-256 that only buffers the header portion (salt + metadata_len + nonce + metadata_ciphertext) for KDF + session building. Full blob is still hashed for integrity verification, but not retained. V1 legacy path unchanged (full read). Added crypto::stream_verify_header() helper. Co-authored-by: forked.online <vixyz@forked.online>
get_file_data_v2 verified plaintext integrity with SHA-256 after GCM-authenticated decrypt already proved it. Deleted 4 lines. GCM auth failure bubbles as CryptoError::Decryption — the canonical integrity proof. Co-authored-by: forked.online <vixyz@forked.online>
save_edits_v2 no longer reads entire encrypted blob. Opens file once, seeks to each retained file offset, reads only that ciphertext. I/O proportional to kept data, not full container size. Co-authored-by: forked.online <vixyz@forked.online>
export: new export::write_ctnr() writes header + stream-copies blob file via io::copy (no full-Vec buffer). import: reads 10-byte header prefix, validates, then stream-copies blob body with inline SHA-256 hash (no full .ctnr read into memory). Co-authored-by: forked.online <vixyz@forked.online>
download_files now collects all file metadata in a single Mutex lock at loop start. Eliminates 3 lock acquisitions per file (two for file name lookup, one for total_bytes). Loop body uses pre-fetched Vec directly. Co-authored-by: forked.online <vixyz@forked.online>
…2, and tests encrypt_file_chunked: removed Sha256 hasher (kept return type, returns empty string). convert_v1_to_v2: replaced sha256_hex call with empty string. Updated vault.rs + test assertions. GCM auth is the canonical integrity proof — SHA-256 was a redundant full pass on every encrypt. Also: removed unused sha2 import from vault.rs. Co-authored-by: forked.online <vixyz@forked.online>
🤖 forked.online code reviewTL;DR This PR optimizes the Cryptainer backend's resource usage by eliminating full-file memory buffering, removing redundant SHA-256 integrity checks after GCM decryption, streaming container exports and imports, and reducing Mutex lock contention during file downloads. Walkthrough
Findings🟠 major: 1 Findings outside the diff
View the full review on forked.online → One-way review — to re-review, push a new commit or use the dashboard. Reviews run on your own provider key (BYOP). |
![forked-online-personal-9829[bot]](https://avatars.githubusercontent.com/u/71164943?s=60&v=4){kind=small}
| src_file.read_exact(&mut header_prefix)?; | ||
|
|
||
| // Validate magic + parse header length | ||
| if &header_prefix[0..4] != export::MAGIC { | ||
| return Err(CryptoError::InvalidFormat("Not a .ctnr file".into())); | ||
| } | ||
| if header_prefix[4] != export::NULL { | ||
| return Err(CryptoError::InvalidFormat("Invalid null byte".into())); | ||
| } | ||
| if header_prefix[5] != export::VERSION { | ||
| return Err(CryptoError::InvalidFormat(format!( | ||
| "Unsupported version: {}. Expected {}", header_prefix[5], export::VERSION | ||
| ))); | ||
| } |
There was a problem hiding this comment.
[critical · security] Arbitrary file creation/overwrite via path traversal in import_container
In import_container, the container header is parsed from the imported .ctnr file. The header.id field is a user-controlled string and is used directly to construct the path of the imported blob file on the filesystem via let blob_path = blobs_dir.join(format!("{}.enc", header.id));. Since Path::join resolves relative paths, an attacker can craft a malicious .ctnr file containing path traversal sequences (e.g., ../../../../something) in the id field to write or overwrite arbitrary files outside the intended directory.
Evidence: src-tauri/src/commands.rs:1111
| src_file.read_exact(&mut header_prefix)?; | |
| // Validate magic + parse header length | |
| if &header_prefix[0..4] != export::MAGIC { | |
| return Err(CryptoError::InvalidFormat("Not a .ctnr file".into())); | |
| } | |
| if header_prefix[4] != export::NULL { | |
| return Err(CryptoError::InvalidFormat("Invalid null byte".into())); | |
| } | |
| if header_prefix[5] != export::VERSION { | |
| return Err(CryptoError::InvalidFormat(format!( | |
| "Unsupported version: {}. Expected {}", header_prefix[5], export::VERSION | |
| ))); | |
| } | |
| if uuid::Uuid::parse_str(&header.id).is_err() { | |
| return Err(CryptoError::InvalidFormat( | |
| "Invalid container ID format".into(), | |
| )); | |
| } |
| // Phase 1: read fixed-size prefix: salt(16) + meta_len(4) + nonce(12) = 32 bytes | ||
| let prefix_len = SALT_LEN + 4 + NONCE_LEN; | ||
| let mut prefix = vec![0u8; prefix_len]; | ||
| file.read_exact(&mut prefix)?; |
There was a problem hiding this comment.
[major · security] Denial of service (OOM crash) via unchecked metadata length allocation
In stream_verify_header, the metadata ciphertext length (meta_len) is read directly from the untrusted file header as a u32 and immediately used to allocate a Vec buffer via let mut meta_ciphertext = vec![0u8; meta_len]; without checking if it exceeds the actual file size. If a corrupted or malicious file specifies a massive length, the default allocator will panic and abort the process, crashing the entire Tauri application.
Evidence: src-tauri/src/crypto.rs:211
| file.read_exact(&mut prefix)?; | |
| let file_len = file.metadata()?.len() as usize; | |
| if meta_len > file_len || meta_len > 50 * 1024 * 1024 { | |
| return Err(CryptoError::InvalidFormat("Metadata size is invalid or too large".into())); | |
| } |
| return Err(CryptoError::InvalidFormat(format!( | ||
| "Unsupported version: {}. Expected {}", header_prefix[5], export::VERSION | ||
| ))); |
There was a problem hiding this comment.
[major · security] Denial of service (OOM crash) via unchecked header length allocation
Similar to stream_verify_header, import_container reads header_len from the untrusted import file prefix and immediately attempts to allocate vec![0u8; header_len] without validating it against the source file's metadata size, exposing the application to an immediate Out-of-Memory panic and crash.
Evidence: src-tauri/src/commands.rs:1093
| return Err(CryptoError::InvalidFormat(format!( | |
| "Unsupported version: {}. Expected {}", header_prefix[5], export::VERSION | |
| ))); | |
| let src_len = src_file.metadata()?.len() as usize; | |
| if header_len > src_len || header_len > 50 * 1024 * 1024 { | |
| return Err(CryptoError::InvalidFormat("Header size is invalid or too large".into())); | |
| } |
| message: "Writing imported container\u{2026}".into(), | ||
| }); | ||
| std::fs::write(&blob_path, &blob)?; | ||
|
|
||
| // Stream-copy blob body with inline SHA-256 | ||
| let mut blob_file = std::fs::File::create(&blob_path)?; | ||
| let mut hasher = sha2::Sha256::new(); | ||
| let mut buf = vec![0u8; 65536]; | ||
| loop { | ||
| let n = src_file.read(&mut buf)?; | ||
| if n == 0 { break; } | ||
| hasher.update(&buf[..n]); |
There was a problem hiding this comment.
[minor · bug] Orphaned blob file left on disk on stream-copy I/O errors
In import_container, the streaming write loop copies data to blob_path. If an I/O error occurs (e.g. disk full or read failure) during the streaming loop, the ? operator triggers an early return. This bypasses the cleanup handler that would normally delete the orphaned blob file on integrity or DB insertion failures.
Evidence: src-tauri/src/commands.rs:1128
| message: "Writing imported container\u{2026}".into(), | |
| }); | |
| std::fs::write(&blob_path, &blob)?; | |
| // Stream-copy blob body with inline SHA-256 | |
| let mut blob_file = std::fs::File::create(&blob_path)?; | |
| let mut hasher = sha2::Sha256::new(); | |
| let mut buf = vec![0u8; 65536]; | |
| loop { | |
| let n = src_file.read(&mut buf)?; | |
| if n == 0 { break; } | |
| hasher.update(&buf[..n]); | |
| let copy_result = (|| -> std::result::Result<(), CryptoError> { | |
| loop { | |
| let n = src_file.read(&mut buf)?; | |
| if n == 0 { break; } | |
| hasher.update(&buf[..n]); | |
| blob_file.write_all(&buf[..n])?; | |
| } | |
| Ok(()) | |
| })(); | |
| if let Err(e) = copy_result { | |
| let _ = std::fs::remove_file(&blob_path); | |
| return Err(e); | |
| } |
- 🔴 path traversal: validate header.id is UUID before blob path - 🟠 OOM meta_len: cap at file_len or 50MB before allocation - 🟠 OOM header_len: cap at src_len or 50MB before allocation - 🟡 orphaned blob: wrap streaming copy in closure, cleanup on error Co-authored-by: forked.online <vixyz@forked.online>
| // Guard against OOM from malicious/corrupted metadata size | ||
| let file_len = file.metadata()?.len() as usize; | ||
| if meta_len > file_len || meta_len > 50 * 1024 * 1024 { | ||
| return Err(CryptoError::InvalidFormat("Metadata size is invalid or too large".into())); | ||
| } |
There was a problem hiding this comment.
[major · bug] Integer truncation of file size on 32-bit targets blocks unlocking large containers
In stream_verify_header, file.metadata()?.len() (a u64) is cast to usize with as usize. On 32-bit target architectures (such as 32-bit Android, iOS, or older x86/ARM devices, which are supported by Tauri), usize is 32-bit. For any container file larger than 4GB, the file size will be truncated modulo 2^32. Consequently, the guard check if meta_len > file_len will unexpectedly evaluate to true when the truncated file_len is smaller than meta_len, preventing users on 32-bit platforms from unlocking any container larger than 4GB.
Evidence: src-tauri/src/crypto.rs:212
| // Guard against OOM from malicious/corrupted metadata size | |
| let file_len = file.metadata()?.len() as usize; | |
| if meta_len > file_len || meta_len > 50 * 1024 * 1024 { | |
| return Err(CryptoError::InvalidFormat("Metadata size is invalid or too large".into())); | |
| } | |
| // Guard against OOM from malicious/corrupted metadata size | |
| let file_len = file.metadata()?.len(); | |
| if meta_len as u64 > file_len || meta_len > 50 * 1024 * 1024 { | |
| return Err(CryptoError::InvalidFormat("Metadata size is invalid or too large".into())); | |
| } |
| // Guard against OOM from malicious/corrupted header size | ||
| let src_len = src_file.metadata()?.len() as usize; | ||
| if header_len > src_len || header_len > 50 * 1024 * 1024 { | ||
| return Err(CryptoError::InvalidFormat("Header size is invalid or too large".into())); | ||
| } |
There was a problem hiding this comment.
[major · bug] Integer truncation of file size on 32-bit targets blocks importing large containers
In import_container, src_file.metadata()?.len() (a u64) is cast to usize with as usize. On 32-bit platforms, this truncates the 64-bit file size. If a user imports a container larger than 4GB, the truncated file size can be smaller than header_len, triggering the OOM guard block header_len > src_len and rejecting a perfectly valid container import as invalid/too large.
Evidence: src-tauri/src/commands.rs:1098
| // Guard against OOM from malicious/corrupted header size | |
| let src_len = src_file.metadata()?.len() as usize; | |
| if header_len > src_len || header_len > 50 * 1024 * 1024 { | |
| return Err(CryptoError::InvalidFormat("Header size is invalid or too large".into())); | |
| } | |
| // Guard against OOM from malicious/corrupted header size | |
| let src_len = src_file.metadata()?.len(); | |
| if header_len as u64 > src_len || header_len > 50 * 1024 * 1024 { | |
| return Err(CryptoError::InvalidFormat("Header size is invalid or too large".into())); | |
| } |
| let mut blob_file = std::fs::File::create(&blob_path)?; | ||
| let mut hasher = sha2::Sha256::new(); | ||
| let mut buf = vec![0u8; 65536]; | ||
| let copy_result = (|| -> std::result::Result<(), CryptoError> { | ||
| loop { | ||
| let n = src_file.read(&mut buf)?; | ||
| if n == 0 { break; } | ||
| hasher.update(&buf[..n]); | ||
| blob_file.write_all(&buf[..n])?; | ||
| } | ||
| Ok(()) | ||
| })(); | ||
| if let Err(e) = copy_result { | ||
| let _ = std::fs::remove_file(&blob_path); | ||
| return Err(e); | ||
| } | ||
|
|
||
| // Verify blob integrity | ||
| let actual_sha256 = hex::encode(hasher.finalize()); |
There was a problem hiding this comment.
[major · bug] Open file handle prevents deletion of orphaned blobs on Windows
In import_container, blob_file is created and remains in scope for the rest of the function. On Windows, trying to delete an open file via std::fs::remove_file fails with an "Access is Denied" or sharing violation error because the file handle is still active. Consequently, if the stream-copy fails, the integrity SHA-256 check fails, or the SQLite database insertion fails, the cleanup remove_file calls will fail to delete the file, leaving orphaned encrypted blobs on disk.
Evidence: src-tauri/src/commands.rs:1139
| let mut blob_file = std::fs::File::create(&blob_path)?; | |
| let mut hasher = sha2::Sha256::new(); | |
| let mut buf = vec![0u8; 65536]; | |
| let copy_result = (|| -> std::result::Result<(), CryptoError> { | |
| loop { | |
| let n = src_file.read(&mut buf)?; | |
| if n == 0 { break; } | |
| hasher.update(&buf[..n]); | |
| blob_file.write_all(&buf[..n])?; | |
| } | |
| Ok(()) | |
| })(); | |
| if let Err(e) = copy_result { | |
| let _ = std::fs::remove_file(&blob_path); | |
| return Err(e); | |
| } | |
| // Verify blob integrity | |
| let actual_sha256 = hex::encode(hasher.finalize()); | |
| let mut blob_file = std::fs::File::create(&blob_path)?; | |
| let mut hasher = sha2::Sha256::new(); | |
| let mut buf = vec![0u8; 65536]; | |
| let copy_result = (|| -> std::result::Result<(), CryptoError> { | |
| loop { | |
| let n = src_file.read(&mut buf)?; | |
| if n == 0 { break; } | |
| hasher.update(&buf[..n]); | |
| blob_file.write_all(&buf[..n])?; | |
| } | |
| Ok(()) | |
| })(); | |
| drop(blob_file); | |
| if let Err(e) = copy_result { | |
| let _ = std::fs::remove_file(&blob_path); | |
| return Err(e); | |
| } |
|
@forked-online-personal-9829 review again |
I have re-reviewed the PR at head commit Resolved & Verified
Outstanding Items to AddressThere are two critical OS/architecture edge cases still active in the current head: 1. Windows File Lock Blocks Cleanup (Finding [13])In let mut blob_file = std::fs::File::create(&blob_path)?;
// ...
if let Err(e) = copy_result {
let _ = std::fs::remove_file(&blob_path); // Fails on Windows
return Err(e);
}On Windows, trying to delete a file while there is an active write handle held open will raise a sharing/permission violation, leaving orphaned blobs on disk. Fix: Explicitly 2. Integer Truncation on 32-Bit Targets (Findings [11] & [12])In both let file_len = file.metadata()?.len() as usize; // Truncates to 32-bit on 32-bit systems
if meta_len > file_len || meta_len > 50 * 1024 * 1024 {If a user is running a 32-bit build and unlocks/imports a container larger than 4GB, Fix: Perform the checks using let file_len = file.metadata()?.len();
if meta_len as u64 > file_len || meta_len > 50 * 1024 * 1024 { |
…32-bit truncation - Windows compat: drop(blob_file) before remove_file calls - 32-bit safety: compare file sizes as u64, not usize (prevents truncation on 32-bit targets for containers >4GB) Co-authored-by: forked.online <vixyz@forked.online>
|
@forked-online-personal-9829 review |
|
Re-review started — the summary comment will update when it finishes. |
1 participant
Summary
Audit found 10 places where Cryptainer's backend did unnecessary full-file reads, redundant SHA-256 after GCM-authenticated decrypt, or 2x memory buffering. This epic eliminates all of them.
Changes
Streaming unlock (WI-0421)
crypto::stream_verify_header()— stream-reads blob through SHA-256, only buffers header portion (salt + metadata_len + nonce + metadata_ciphertext)unlock_containerv2 path uses streaming; v1 path unchangedRemove redundant SHA-256 after GCM (WI-0422)
get_file_data_v2: deleted SHA-256 hash+comparison — GCM tag is canonical integrity proofRemove SHA-256 producers (WI-0423)
encrypt_file_chunked: removed Sha256 hasher (returns empty string)convert_v1_to_v2: replacedsha256_hexcall with empty stringsha2import fromvault.rssave_edits seek + read instead of full blob (WI-0424)
std::fs::readwithFile::open+ seek/read_exact per retained file offsetStream export/import (WI-0425)
export::write_ctnr()writes .ctnr header thenio::copy's blob filedownload_files lock reduction (WI-0426)
Progress bar fix
save_edits_v2:bytes_totalnow computed as sum of retained + new plaintext sizes (was hardcoded 0)Files changed
src-tauri/src/crypto.rs— newstream_verify_header()src-tauri/src/commands.rs— unlock, decrypt, save_edits, download_files, export, importsrc-tauri/src/vault.rs— encrypt_file_chunked, unused import cleanupsrc-tauri/src/export.rs— newwrite_ctnr()src-tauri/tests/v2_integration.rs— test assertions for SHA-256 removalVerification
cargo test— 41/41 passv2_integration— 11/11 passGenerated with forked.online