Skip to content

Fix 10 correctness/regression issues found in review of #1372 before merging QUANTUMSTRAND into master #1375

Description

@mr-tz

Reviewed the full diff of #1372 (quantumstrandmaster, closes #943). CI is green across the board, but a source-level review turned up 10 issues, several of which silently produce wrong output or regress existing behavior. Filing these to track fixes before merge; plan is to address all of them and rebase the branch on top of master afterward.

High priority — silently wrong output / behavior contradicts docs

  • XOR-decoded PE/ELF layouts fail the structured-layout check (floss/enrich.py:34)
    is_structured_layout() does an exact match on "pe"/"elf", but an XOR-obfuscated header gets " (XOR decoded with key: 0x...)" appended to layout.name (floss/layout/pe.py:342, floss/layout/elf.py:260). The check then fails and compute_layout() silently falls back to classic static strings — losing section names, code/reloc/XOR tags, and structure annotations for exactly the obfuscated-malware case FLOSS is built to handle.

  • -y/--yes doesn't skip deobfuscation for Go/Rust binaries (floss/pipeline.py:396)
    The flag's help text says deobfuscation "defaults to not running it," but that only happens inside the interactive-prompt branch, which -y bypasses entirely. Only .NET gets enable_stack/tight/decoded_strings force-disabled elsewhere (floss/pipeline.py:387-389); Go/Rust binaries keep the Analysis() default of True. A non-interactive/CI run with -y on a Go or Rust sample runs full deobfuscation anyway.

  • Static-strings path now reads the whole file into memory, bypassing the large-file guard (floss/pipeline.py:309)
    sample.read_bytes() runs unconditionally before the MAX_FILE_SIZE/--large-file check (floss/pipeline.py:504), which now only guards the deobfuscation stage. master's equivalent path used mmap (floss/utils.py: get_static_strings) specifically to avoid this. Regression risk: OOM or a large perf hit on big samples even for --string-type static-only runs.

Medium priority — regressions / silent misbehavior

  • --disable-progress CLI flag silently removed (floss/cli.py)
    Present in master's parser, dropped from the rewritten floss/cli.py. floss/pipeline.py (Options.disable_progress, gated in 5 places) still fully implements it as dead code that's permanently False. Existing scripts using --disable-progress now hit an argparse "unrecognized arguments" error, with no replacement short of -q/--quiet (which also silences other output).

  • Cache key ignores --format, can serve stale/wrong results (floss/cache.py:216)
    The cache key (sha256 + version) and covers() never account for Options.format. Analyzing the same shellcode bytes first with --format sc32 then --format sc64 returns the stale sc32-interpreted stack/tight/decoded strings — vivisect never re-runs under the new interpretation.

  • PE/ELF root-level strings never get structure annotations (floss/layout/base.py:334)
    PELayout.mark_structures / ELFLayout.mark_structures override the base method but only recurse into children — neither applies structures_by_address to the node's own self.strings. MachOLayout does this correctly by calling super().mark_structures(). Effect: strings in the ELF header/program-header/section-header gap (attached to the root layout node, since elf.py doesn't tile a header child the way pe.py does) render with an empty .structure, so --structure elf-header never matches them even though the structure was recorded at that offset.

  • --summary + --analyze-functions silently disables every string type (floss/main.py:227)
    --summary auto-disables stack/tight/decoded when no --string-type flags are given; --analyze-functions then sees an empty enabled_string_types and additionally disables static. Result: nothing enabled, no error raised — just an empty/meaningless summary.

  • Viewer tag filter uses AND instead of OR semantics (viewer/src/App.tsx:556)
    The tag-filter checkbox list uses .every() (a string shows only if all its tags are checked) instead of .some(). Unchecking one tag hides every string that carries that tag, even if another of its tags is still checked — inverted from the OR semantics a multi-select checkbox filter (and the per-tag counts shown next to it) implies.

Lower priority

  • Typo makes --pes mode unreachable in extract_strings.py (scripts/tags/extract_strings.py:265)
    if args.libs: max_len = MAX_LEN_PES elif args.libs: max_len = MAX_LEN_LIBS — the second condition should be args.pes. The script's own documented usage (extract_strings.py -d --pes C:\Windows cwinpes) raises ValueError("unknown extraction type") when run without an explicit --max-len.

  • Git-LFS-pointer detection missing from 3 of 4 tag database loaders (floss/tags/oss.py, winapi.py, expert.py)
    Only floss/tags/gp.py:140 (StringHashDatabase.from_file) checks for an unpulled LFS pointer and raises "Git LFS pointer detected ...; please run \git lfs pull`". The other loaders don't, so a fresh clone without git lfs pullgets a confusing rawgzip.BadGzipFile: Not a gzipped fileor amsgspec` decode error instead of an actionable message.

Also noted, not blocking

  • --only/--no were renamed to --string-type/--no-string-type with no backward-compat alias, and floss/version.py is still 3.1.1 despite this being a major feature release — worth a version bump alongside the fixes above.

Plan

Address all ten items above, then rebase quantumstrand onto current master before merging.

Metadata

Metadata

Assignees

Labels

QSQUANTUMSTRAND

Type

No type

Projects

Status
Backlog

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions