5bb280eb25
- Integrated `zupt_mac_verify_ct` in `zupt_decrypt_buffer()` to replace C XOR loop for HMAC-SHA256 - Integrated `zupt_ct_select_32` in `zupt_mlkem768_decaps()` to replace C `cmov()` for FO transformation - Added `include/zupt_jasmin.h` with extern declarations and ABI docs - Added `#ifdef ZUPT_USE_JASMIN` guards with clean C fallbacks in `zupt_crypto.c` and `zupt_mlkem.c` - Makefile now auto-detects `jasmin/*.s`, assembles and links with `-DZUPT_USE_JASMIN` Closes #3
199 lines
10 KiB
Markdown
199 lines
10 KiB
Markdown
# Zupt — Root Cause Analysis
|
||
|
||
## Summary
|
||
|
||
All compression/decompression round-trip tests pass across every file type, compression level (1–9), codec (Store, Zupt-LZ, Zupt-LZH, Zupt-LZHP), and mode (normal, solid, encrypted, encrypted-solid). No data corruption was reproduced on Linux x86-64. Address Sanitizer and Undefined Behavior Sanitizer detected zero memory errors.
|
||
|
||
The codebase does contain **16 concrete defects** in five categories: data corruption (3 critical), memory safety (2 critical), security hardening, portability, robustness, and code quality.
|
||
|
||
---
|
||
|
||
## BUG 1 — Endian-unsafe archive I/O (Portability: CRITICAL on BE targets)
|
||
|
||
**Where:** `zupt_format.c:198–202` (`w16`, `w64`, `r16`, `r64`)
|
||
|
||
**What:** These helpers use raw `fwrite(&v, N, 1, f)` which writes the host's native byte order. On little-endian (x86, ARM64, Apple Silicon) this produces LE archives. On big-endian (SPARC, s390x, MIPS-BE), archives are incompatible.
|
||
|
||
The same issue affects the central index serialization at lines 444–450 and 718–724, where `memcpy(buf, &field, 8)` writes native-endian 64-bit values.
|
||
|
||
**Root cause:** Missing explicit LE serialization layer.
|
||
|
||
**Fix:** Replace `w16`/`w64`/`r16`/`r64` with byte-level LE serialization. Replace all raw `memcpy` of multi-byte index fields with `le64_put`/`le64_get` helpers.
|
||
|
||
---
|
||
|
||
## BUG 2 — `realloc` return not checked (Robustness: CRASH on OOM)
|
||
|
||
**Where:** `zupt_format.c:130–131` (`zupt_filelist_add`)
|
||
|
||
**What:** `realloc` can return NULL if memory is exhausted. The code assigns the result directly to `fl->paths` and `fl->arc_paths`, losing the original pointer (memory leak) and then dereferencing NULL on the next access (segfault).
|
||
|
||
**Root cause:** Missing NULL check after `realloc`.
|
||
|
||
**Fix:** Assign `realloc` result to a temporary, check for NULL, return error or fall back.
|
||
|
||
---
|
||
|
||
## BUG 3 — Non-regular files not detected (File type support)
|
||
|
||
**Where:** `zupt_format.c:148–192` (`zupt_collect_files`)
|
||
|
||
**What:** The function checks `is_dir()` and treats everything else as a regular file. Symlinks, FIFOs, block/character devices, and sockets are all treated as regular files. Reading from `/dev/zero` or a FIFO could hang indefinitely; device files may produce unbounded data.
|
||
|
||
**Root cause:** Missing `S_ISREG()` / `FILE_ATTRIBUTE_NORMAL` check.
|
||
|
||
**Fix:** Add `is_regular_file()` check. Skip non-regular files with a warning to stderr.
|
||
|
||
---
|
||
|
||
## BUG 4 — Cryptographic random fallback uses `rand()` (Security: CRITICAL)
|
||
|
||
**Where:** `zupt_crypto.c:27–30` (Unix fallback) and `zupt_crypto.c:20–24` (Windows fallback)
|
||
|
||
**What:** If `/dev/urandom` fails to open (or `RtlGenRandom` fails to load), the code falls back to `srand(time(NULL)) + rand()`, which is trivially predictable. Salt and nonce generated this way would collapse all security guarantees.
|
||
|
||
**Root cause:** Defensive fallback written for "should never happen" case, but the fallback silently destroys security rather than failing loudly.
|
||
|
||
**Fix:** Remove the `rand()` fallback entirely. If the OS CSPRNG is unavailable, abort with an error message. On modern Linux, also try `getrandom(2)` before `/dev/urandom`.
|
||
|
||
---
|
||
|
||
## BUG 5 — Sensitive key material wiping may be optimized out (Security)
|
||
|
||
**Where:** `zupt_crypto.c` — multiple `memset(material, 0, N)` calls, `zupt_main.c:168` `memset(opts.password, 0, ...)`.
|
||
|
||
**What:** The C standard allows compilers to eliminate stores to memory that is never read again. `memset` of key material followed by `free()` or function return is a classic case where `-O2` can (and does) remove the wipe.
|
||
|
||
**Root cause:** No use of `explicit_bzero`, `SecureZeroMemory`, or a volatile-based wipe.
|
||
|
||
**Fix:** Add `zupt_secure_wipe()` using `explicit_bzero` (glibc), `SecureZeroMemory` (MSVC), or a volatile-pointer trick as fallback.
|
||
|
||
---
|
||
|
||
## BUG 6 — MAC comparison is not constant-time (Security: timing oracle)
|
||
|
||
**Where:** `zupt_crypto.c:155–157` (`zupt_decrypt_buffer`)
|
||
|
||
**What:** `ok &= (expected_mac[i] == stored_mac[i])` is an attempt at constant-time comparison, but the compiler may optimize it. More importantly, if `ok` becomes 0, the loop still runs (which is correct), but the compiler may short-circuit the `&=` operation.
|
||
|
||
**Root cause:** C semantics don't guarantee constant-time execution of bitwise operations.
|
||
|
||
**Fix:** Use XOR accumulation: `diff |= (expected[i] ^ stored[i])`, then check `diff == 0`. This is the standard pattern used by libsodium and OpenSSL.
|
||
|
||
---
|
||
|
||
## BUG 7 — PBKDF2 mutates salt length parameter (Correctness)
|
||
|
||
**Where:** `zupt_crypto.c:80` — `if (slen > 252) slen = 252;`
|
||
|
||
**What:** The local `slen` parameter is clamped destructively. In the current code this only runs once per derivation, so no multi-iteration issue exists. But if the function were called with `slen > 252`, the salt would be silently truncated with no warning, weakening the KDF.
|
||
|
||
**Root cause:** Safety clamp placed inside the loop body instead of documented at the API level.
|
||
|
||
**Fix:** Move the check before the loop, use a local copy, and `assert(slen <= ZUPT_SALT_SIZE)`.
|
||
|
||
---
|
||
|
||
## BUG 8 — Huffman code-length limiting uses unreliable heuristic (Compression)
|
||
|
||
**Where:** `zupt_lzh.c:228–240` (`huff_build`)
|
||
|
||
**What:** When code lengths exceed `LZH_MAX_CODELEN` (15), the code attempts a Kraft inequality fix using floating-point arithmetic and a heuristic "shorten the most frequent long code" approach. This can produce invalid Huffman codes where the Kraft sum exceeds 1.0, leading to ambiguous decoding. The floating-point precision loss compounds for large alphabets.
|
||
|
||
**Root cause:** Ad-hoc fix instead of a proper package-merge or iterative length-limiting algorithm.
|
||
|
||
**Fix:** Replace with the standard iterative bit-length limiting: count symbols per length, then redistribute excess codes from max length downward until the Kraft inequality is satisfied, using integer arithmetic only.
|
||
|
||
---
|
||
|
||
## BUG 9 — `file_hash` for multi-block files uses XOR (Integrity: weak)
|
||
|
||
**Where:** `zupt_format.c:317` — `file_hash ^= checksum;`
|
||
|
||
**What:** For multi-block files, the per-file content hash is `block_1_xxh64 XOR block_2_xxh64 XOR ...`. XOR is commutative, so reordered blocks produce the same hash. Identical blocks cancel out (two copies of the same block produce hash 0).
|
||
|
||
**Root cause:** Quick implementation that doesn't compose hashes properly.
|
||
|
||
**Fix:** Use incremental XXH64 across the entire file contents, or chain: `hash = xxh64(&prev_hash_concat_data)`.
|
||
|
||
---
|
||
|
||
## BUG 10 — Write errors silently ignored (Robustness)
|
||
|
||
**Where:** `zupt_format.c` — All `w8`/`w16`/`w64`/`fwrite` calls in compress paths ignore return values.
|
||
|
||
**What:** If the output disk is full or the filesystem encounters an error, compressed data blocks are silently truncated. The footer may still be written, producing a corrupt archive that appears valid until extraction.
|
||
|
||
**Root cause:** No error propagation from low-level write helpers.
|
||
|
||
**Fix:** Accumulate an error flag in a write context, check it before writing the footer.
|
||
|
||
---
|
||
|
||
## BUG 11 — `ftello` return not checked (Robustness)
|
||
|
||
**Where:** `zupt_format.c:293,299,432,706` etc.
|
||
|
||
**What:** `ftello` returns `-1` on error. Storing `-1` as `uint64_t` produces `0xFFFFFFFFFFFFFFFF`, which would corrupt archive offsets.
|
||
|
||
**Fix:** Check for `-1` and propagate error.
|
||
|
||
---
|
||
|
||
## BUG 12 — Version string inconsistency
|
||
|
||
**Where:** `zupt.h:33` says `ZUPT_VERSION_STRING "0.5.1"`, README says "Version 0.3.0", Makefile says "v0.4.0".
|
||
|
||
**Fix:** Synchronize all version references to 0.5.1.
|
||
|
||
---
|
||
|
||
## BUG 13 — Heap-buffer-overflow in LZH match finder quick-rejection (CRITICAL)
|
||
|
||
**Where:** `zupt_lzh.c:465` (`find_match`, quick-rejection check)
|
||
|
||
**What:** The comparison `src[ref + best] == src[ip + best]` reads past the allocated buffer when `ip + best >= slen`. ASAN reports: `heap-buffer-overflow READ of size 1` at the exact boundary. The bug is data-dependent — it triggers when `ip` is near the end of the input and a previous match set `best` to a length that extends past the buffer.
|
||
|
||
**Root cause:** Missing bounds check before the quick-rejection optimization.
|
||
|
||
**Fix:** Added `(size_t)best < slen - ip` guard before the comparison.
|
||
|
||
**Also affects:** `zupt_lz.c:55` — identical pattern in the Zupt-LZ codec's match finder. Same fix applied.
|
||
|
||
---
|
||
|
||
## BUG 14 — Heap-buffer-overflow in LZ hash function (CRITICAL)
|
||
|
||
**Where:** `zupt_lz.c:43-44` (`lz_find_match` entry guard + `lz_hash4`)
|
||
|
||
**What:** `lz_hash4` reads 4 bytes via `memcpy(&v, p, 4)`, but the entry guard only checks `ip + LZ_MIN_MATCH > src_len` where `LZ_MIN_MATCH = 3`. When exactly 3 bytes remain, the 4-byte hash read goes 1 byte past the buffer.
|
||
|
||
**Root cause:** Guard mismatch: 3-byte minimum match vs 4-byte hash function.
|
||
|
||
**Fix:** Changed guard to `ip + 4 > src_len`.
|
||
|
||
---
|
||
|
||
## BUG 15 — Huffman code-length limiting produces over-subscribed codes (CRITICAL, data corruption)
|
||
|
||
**Where:** `zupt_lzh.c:227-240` (original `huff_build` code-length limiter)
|
||
|
||
**What:** `tree_depths()` clamps depths at `LZH_MAX_CODELEN` via `dp[nd] = min(depth, 15)`. The original code tried to detect this by checking `if (dp[i] > MAX_CODELEN)` — but since `tree_depths` already clamped the values, the check never triggers. Result: the Kraft sum exceeds 2^15 (measured: 32770 vs target 32768), producing ambiguous Huffman codes. The decompressor then misinterprets symbols, causing data corruption or premature termination.
|
||
|
||
**Manifestation:** LZH decompression returns wrong size or wrong data on inputs >~100KB with skewed frequency distributions. Specifically affects: solid mode with ELF+random mixed data, prediction-transformed data, any compression level ≥ 2.
|
||
|
||
**Root cause:** The limiter detects overflow by checking code lengths, but `tree_depths()` already clamped them. The correct detection is computing the Kraft sum directly.
|
||
|
||
**Fix:** Replaced with Kraft-sum-based detection: compute `sum of 2^(MAX-len)` for all symbols, then iteratively fix by splitting shorter codes while absorbing MAX-length excess (same algorithm as zlib's `gen_bitlen`).
|
||
|
||
---
|
||
|
||
## BUG 16 — Empty file checksum failure in solid extract
|
||
|
||
**Where:** `zupt_format.c:1171-1173` (solid extract checksum verification)
|
||
|
||
**What:** Empty files (0 bytes) have `content_hash = 0` (never set during compression). On extraction, `zupt_xxh64(buf, 0, 0)` returns the XXH64 seed value (not 0), causing a checksum mismatch for every empty file in solid archives.
|
||
|
||
**Root cause:** Hash of zero-length data is not identity; the compress path sets `content_hash` only `if (sz > 0)` but the extract path unconditionally hashes.
|
||
|
||
**Fix:** Skip checksum verification for empty files in the solid extract path (matching the solid test path which already had this guard).
|