Repository navigation
feat(utility): read Go and Rust strings at their real length - #382
Conversation
Take the length from the load paired with the pointer (next argument register or stack word) or from a (pointer, length) header, and decode the exact bytes as UTF-8. Adds mode="rust"; with no mode the report language selects it.
Use Go's register order and separate SysV/Win64 rules for Rust so a nearby argument cannot be taken as the string length. Stop pairing after writes to the pointer, length or stack slot, and keep the scan inside one basic block. Follow full-width pointer copies with lengths loaded on either side of the copy, but do not treat a pointer value as a length. Add decoded x86-64 and AArch64 regressions for these cases.
Blocks also split where a later jump lands. The pointer keeps its value on the path falling into such a join, so stopping the forward scan at the block end lost pairs like lea rdi; lea rdx | mov esi, imm; call. Backward scans still stop at the pointer's own block start.
danielplohmann
left a comment
There was a problem hiding this comment.
Thanks. On Go this is a clear step up: the old stack-string check needed ref; mov; mov imm and missed the regabi lea rax; mov ebx, N; call shape completely. Rust needs more work before it can go in, though.
1. Rust mode drops and truncates C strings
extract_strings now picks sized mode from _language_mode, and Rust scores 0.6 as soon as the raw bytes contain /rustc/, RUST_BACKTRACE or RUST_MIN_STACK. In sized mode the NUL-terminated path never runs, so a C string survives only if it happens to pair with a length.
rust_pe_gnufrom the test fixtures: all 7 MinGW CRT strings found on master are gone ("Mingw-w64 runtime failure:\n", "VirtualProtect failed with code 0x%x", …).mirai_x64with 7 bytes/rustc/appended: 162 strings → 81, and 33 of those are cut short because an unrelated argument register is read as the length."/dev/watchdog"comes out as"/d", and"ZOJFKRA"as"ZOJFK".
Mixed C/Rust binaries are normal (CRT, vendored C libraries, FFI), and for malware the second case means a sample can switch off most string extraction with seven bytes. Please keep running the NUL-terminated path in Go/Rust mode (at least for references that don't pair), and only pick Rust mode on stronger evidence than a substring.
2. data_addr now points at the header for Rust
When a string is found through a (ptr, len) header, the ref records the header's address. On master all 2657 Rust stringrefs on rust_pe_gnu had the string bytes at data_addr. On this branch 826 of 1892 point at the panic Location struct (e.g. 0x14009e6c8 → "/rustc/…"). That changes what data_addr means for MCRIT and other consumers, and BinarySynthesizer._iterStringRefs writes the string plus a NUL at data_addr, overwriting the header. Could read_go_string return the resolved pointer, and the ref use that?
3. Cost of sized reads
The length cap (MAX_SIZED_STRING_LEN = 0x10000) works, but read_sized_string has no cache (unlike read_string), and the printable check loops over up to 64 KiB in Python. With 2000 synthetic lea rax,[run]; mov ebx,0xffff refs pointing at a 64 KiB printable run: 2.68 ms per ref on this branch against 0.04 ms on master's generic path. Please cache on (offset, length) and do the printable check at C speed (a compiled regex, or isprintable() plus a translate for whitespace).
4. Smaller points
- The old
reraise_non_operational_exceptionguard is gone, and the pairing callsinsn.getDetailed()on about 12 instructions around every ref. If capstone ever fails to re-decode one, theValueErrorturns the whole report into an error report. I couldn't trigger it, but atryaround the per-ref pairing is cheap. - Strings containing any non-printable character (ANSI escapes, NBSP, a BOM) are now dropped entirely where the old Go check kept the printable prefix, and UTF-16 detection on that path is gone. Is that intended?
_looks_like_pointerrejects a literal whose first 4 bytes read as an in-image address, which happens for 32-bit images based at0x6X000000.- Register pairing still reads the wrong length now and then (
lea rcx,str; lea r8,loc; mov edx,0x1d; callgives "reentrant init/rustc/8bab26f4"). Master over-read these as well, so it is not a regression, but it is the same cause as the truncation in point 1.
- Rust mode needs a /rustc/<commit>/ path, not just the language score - unpaired Rust refs keep the NUL-terminated read for C strings - header refs record the string address as data_addr - cache sized reads and check printability with a regex - keep ESC, NBSP and BOM; reject other controls - pointer-lookalike check only for lengths shorter than a pointer - a failed re-decode costs one reference, not the report - pair a &str returned in rax:rdx
danielplohmann
left a comment
There was a problem hiding this comment.
Thanks for 58ec157. Re-measured on the bundled fixtures, master ddffed4 against 58ec157 (disassembleUnmappedBuffer, WITH_STRINGS=True):
| review point | status |
|---|---|
| 1. Rust mode drops C strings | partly: 6 of 7 MinGW CRT strings on rust_pe_gnu are back, "Mingw-w64 runtime failure:\n" still comes out as "M" |
2. data_addr on headers |
resolved: all 2173 refs point at the string bytes |
| 3. cost of sized reads | resolved: 0.19 ms/ref uncached on the synthetic 64 KiB case (was 2.68), rust_pe_gnu string extraction 4.9 s vs 6.5 s on master |
4. try around pairing, _looks_like_pointer |
resolved |
| 4. ESC/NBSP/BOM | resolved for those three; any other control character still drops the whole string instead of keeping the printable prefix |
| 4. UTF-16 on the Go path | not answered; it's gone entirely now. Defensible for Go, but please say so |
A note on the mirai_x64 + /rustc/ check: bytes appended past the end of the ELF never reach the mapped buffer, so "identical to master" holds for any appended marker and doesn't prove anything. Writing the full 50-byte /rustc/<40 hex>/ path into zero padding inside mirai's .rodata (file offset 0xe560) does switch it to Rust mode: 162 → 150 strings, 28 truncated (/dev/watchdog → /d, ZOJFKRA → ZOJFK), 17 lost, 5 junk. mov esi, 2; mov edi, "/dev/watchdog"; call open pairs 2 as the length. A C program linking a Rust staticlib carries that path without anyone planting it.
New problems:
1. Go build ID alone switches a C binary to Go mode, and Go mode has no NUL-terminated path (StringExtractor.py:494-501, :587-611).
With mode=None, _language_mode picks Go once getGoScore() reaches 0.6, which takes only the Go build ID substring. On master, Go mode was used only when a pclntab was found. Writing Go build ID: "abcdefabcdef" into mirai_x64's .rodata padding takes it from 162 strings to 2, and both are truncated. That is the 7-byte switch from my first review again with 11 bytes, and it also hits Go dumps where pclntab detection fails. Please use Go mode only when a pclntab was found, and give unpaired refs the C-string path in Go mode as in Rust mode. (Minor: on a tie, max() picks Go, while LanguageAnalyzer._deriveGuess prefers Rust.)
2. About 700 genuine Rust strings now produce nothing. On rust_pe_gnu, 739 master refs (728 of 8+ characters) sit on instructions where the PR emits no string, including "cannot modify the panic hook from a panicking thread", "fatal runtime error: an irrecoverable error occurred while synchronizing threads" and "assertion failed: self.is_code_point_boundary(new_len)". A wrongly paired length breaks the UTF-8 decode (lea rdx, str; lea rcx, [rbp-8]; mov r8d, 0xb7), or nothing pairs at all, and then _starts_string_run (:218-236) refuses the C fallback because the preceding byte is printable. When a sized read fails, please fall back to a read without length, or at least to the printable run as master did. The CHANGELOG presents 2657 → 2173 as over-reads removed; this loss needs to be in it.
3. Synthesis writes a NUL into the next packed literal (BinarySynthesizer.py:186-194). string + b"\x00" at data_addr was harmless while strings over-read each other. With exact lengths, 319 of 887 distinct strings on rust_pe_gnu have their terminator land on the first byte of another string, and 120 end up with an embedded NUL after planting (master: 0). Skip the NUL for sized strings, or write all terminators first.
4. Smaller:
_starts_string_rundrops tail-merged C strings in Rust mode ('\r\n','/','12345678'on mirai with the planted path).- CHANGELOG: string output changes on every Go/Rust binary, so it needs a
Compatibilitybullet; also add the measured tree's<sha>, whether the figures were reproduced, the losses from 2 and from an in-image/rustc/path, and(#382). _length_registerssniffs the binary format once per function rather than once per report;_string_cachecan now hold 10k entries of up to 64 KiB.- The new tests are stub-based; a fixture regression for the MinGW strings and for 1 would keep these from coming back.
Downstream is fine: nothing in mcrit, mcrit-plugin or capa reads the stringref type or data_addr, so the new utf8 type is harmless.
I'm keeping this out of 4.10.0 so the release doesn't wait on it. Go mode is a real improvement, so let's get it into the next one.
…modes - Go mode only when a pclntab was found; a Go build ID string no longer switches C code to it. - References without a usable length take the NUL-terminated read in both modes, stopped at the next referenced address so packed literals are not read into each other. Referenced addresses are code data refs plus aligned words holding an image address (not in zero-based images). - A paired length that ends inside a NUL-terminated string at the start of a run, where nothing else is referenced, falls back to the C string (open(path, flags) pairs the flags as a length). - Synthesis writes all terminators before the strings, so a terminator no longer lands on the next packed literal. - Length register pairs are derived once per report; the string cache is bounded by characters as well as entries.
|
Thanks. Pushed
1. Go build ID. Go mode now only comes from a found pclntab, and 2. Lost Rust strings. A missing or failed length now falls back to the NUL-terminated read. So that this doesn't bring the over-reads back, the read stops at the next referenced address. Those are code data refs plus aligned words in the image that hold an image address, which covers literals only reached through a 3. Synthesis. All terminators are written first, then the strings. 4. Smaller:
What's left with the in-image String extraction on |
|
Let's release this on 4.10.0. |
danielplohmann
left a comment
There was a problem hiding this comment.
Review 3 — #382 on 2bd3204 (24d3bdf + 43ba1db + master merges up to af37541)
Thanks, Rony. This round closes everything I raised on 58ec157, and your table reproduces exactly here.
4.10.0 has shipped already, so this goes into the next release. Before merge I want one small code change (cgo
C strings in Go mode), the slow marker, and a CHANGELOG trim. Details below.
Reproduction (disassembleUnmappedBuffer, WITH_STRINGS=True, master = af37541)
| case | master | PR | diff vs master |
|---|---|---|---|
rust_pe_gnu |
2657 refs, 820 strings > 200 chars | 2909, 1 | 1195 same, 1443 shortened, 271 gained, 19 lost |
mirai_x64 |
162 | 162 | identical |
mirai_x64 + Go build ID: "abcdefabcdef" @ 0xe560 |
162 | 162 | identical |
mirai_x64 + /rustc/<40 hex>/ @ 0xe560 |
162 | 166 | 145 same, 16 shortened, 1 lost, 5 gained |
rust_pe_gnu: 16 of the 19 "lost" are the old'8'reads at aLocationheader; the PR now yields the
/rustc/…path at that instruction, withdata_addron the string. The other 3 are'0q'pointer bytes, which
matches your "3 lost, all pointer bytes". I sampled the 1443 shortened ones and they end at packed-literal
boundaries ("even"|"odd","mid > len"|"calledResult::unwrap()…"). Of the 271 gained, 115 are 1-3 chars,
but they are real fmt literals (0x×46,{,[,::,µs, U+FFFD). 4 strings are typedutf8.
1513 distinctdata_addrs, and no address reads as two different strings.mirai_x64+/rustc/: the 16 shortened ones are your 11 + 5 ("\r\n"→"\r","url="→"url", the
abcdefgh…table read at 8,"VGAJ"→"V"). The 5 gained are junk (CFO,I,x,J)./dev/watchdog
stays whole now.- String extraction time on
rust_pe_gnu: 0.66 s → 1.12 s, and the whole run 2.52 s → 2.99 s. Your +0.5 s
holds. The "(4.5 s)" in the entry is ambiguous: say what it is the time of, and on which box.
C/C++ fixtures, master vs PR: bit-identical refs on cxx_pe_gnu (37), elf_cxx_landing_pads_x64 (16),
elf_cet_landing_pads_x64 (6), komplex (85), cutwail (29), bashlite (144), mirai_i386 (159),
arm64_pe_probe (39). msvc_cxx_pe, msvc_cxx_pdb_pe, aarch64_static, elf_cxx_landing_pads_arm64,
elf_ehframe_hdr_x64 and rust_pe_msvc_i686 have 0 refs on both (the i686 Rust one gets 4 functions and a Rust
score of 0, so this PR does not reach it). There is no Go fixture in tests/, and no Go toolchain here, so I
could not reproduce your go1.24.7 figures (164 → 1776, 1705 → 1803).
Real-world check: Ubuntu's /usr/bin/rg scores rust 0.6, but it is built with --remap-path-prefix and has no
/rustc/<hash>/ path, so it stays in C mode (31398 refs, same path as master). Distro Rust builds keep the master
behaviour. That is fine, but it is a recall limit of the gate and worth one sentence in the PR.
Status against review 2
| # | point | status |
|---|---|---|
| 1 | Go build ID alone switched C to Go mode |
fixed: Go mode only from the pclntab, and planted mirai is identical |
| 2 | ~739 Rust strings lost when pairing went wrong | fixed: unsized fallback, 2657 → 2909, > 200 chars 820 → 1 |
| 3 | synthesis wrote NUL into the next packed literal | fixed: terminators first, 1513 strings intact (test passes) |
| 4a | _starts_string_run dropped tail-merged C strings |
traded: now kept, but cut where a suffix is referenced ("\r\n" → "\r"). Documented cost, accepted |
| 4b | CHANGELOG: Compatibility, sha, losses, (#382) | done, with trims below |
| 4c | _length_registers per function |
fixed: once per report |
| 4d | cache size | fixed: cleared at 10000 reads / 4M chars |
| 4e | stub-only tests | fixed: testStringExtractorFixtures.py uses rust_pe_gnu and mirai_x64. Needs slow, see below |
| — | /rustc/ planted in mirai |
fixed: 162 → 166, 16 shortened (was 150, 28 shortened) |
Remaining
1. Go mode still cuts C strings in cgo code (please change). _cuts_c_string runs only for
mode == "rust", so in Go mode open(path, O_RDWR) still pairs rdi/rsi and reads "/d". Go mode needs only an
8-byte header (f1 ff ff ff 00 00 01 08) that findPcLntabHeaderOffsets accepts. With that header planted into
padding (mirai_x64 0xe560, mirai_i386 0xd4b8 with ptrsize 4, bashlite 0x82c0), compared with the same tree
unplanted:
| fixture | refs | lost | shortened |
|---|---|---|---|
mirai_x64 |
162 → 115 | 47 (GET, url=, \r\n, …) |
7 (/dev/watchdog → /d, /dev/misc/watchdog → /d, Cookie: → Cookie:) |
mirai_i386 |
159 → 139 | 21 | 4 (same watchdog pair) |
bashlite |
144 → 35 | 109 (mostly pointer bytes, but also URG ACK RST FIN SYN PSH x86 ALL) |
4 |
This is not a regression against master. On the same input, master raises OverflowError: Python int too large to convert to C ssize_t in detect_ascii_len (via read_go_string → read_string(…, huge length)), so it loses the
whole report. MAX_SIZED_STRING_LEN fixes that, and the fix deserves a ### Fixed line of its own. The pclntab gate
is also older than this PR. But cgo binaries are common. Your own reason for keeping
_cuts_c_string Rust-only is the C code that gets linked in, and that applies to cgo too. Please drop the mode
condition, rerun your go1.24.7 pair, and put the delta in the entry. The 4-char minimum for unsized reads in Go mode
also drops 3-char C strings (SYN, GET). I am fine keeping it, but name it in the cost.
2. Pointer-word scan: fine in practice, but keep it from outliving extraction. _stored_pointers on real Go
images: /usr/bin/gh (35.5 MB mapped) 50k targets, 0.20 s, 6 MB peak; /usr/bin/docker (63 MB) 164k targets,
0.29 s, 21 MB. Synthetic worst case, a 64 MB image of distinct in-image words: 8.4M targets, 1.3 s, ~1 GB peak
(64-bit); 16.8M targets, 2.1 s, ~2 GB (32-bit). At MAX_IMAGE_SIZE = 100 MB that comes to about 1.6 / 3.2 GB, and
an attacker can reach it, because either mode can be forced (point 1, /rustc/ plant). Please:
- Clear
_referenced_targets(and_string_cache) in_addStringsToReport'sfinally. Today the sorted list
stays on the report for its whole lifetime, andfromDict/mcrit workers keep reports around. - Optional: hold the targets in an
array("Q"), sorted, instead of alistof ints. That cuts the resident list
by about 4x.
Also: zero-based images skip the scan (good, documented). On 32-bit images based at 0x400000, aligned text such as
?? ?? 4x 00 decodes as an in-image address, so string bytes themselves can produce stop targets. I did not see
this on a fixture. Low risk, so no change asked, but a sentence in the docstring would not hurt.
3. Interior pointers split a literal (observation, no change asked). rust_pe_gnu 0x14002909a:
"cmd.exe /e:ON /v:OFF /d /c \"" is referenced at its start and at +1 (0x140029084). The PR yields "c" and
"md.exe /e:ON /v:OFF /d /c \"", where master yielded both run into the next literal. Neither is right. Stopping at
the next referenced address has no way to tell an iter + 1 from a literal boundary.
4. Tests. testStringExtractorFixtures.py disassembles rust_pe_gnu in setUpClass (3.0 s setup) and
mirai_x64 three times. AGENTS.md: a test that disassembles a real fixture is slow, so add
pytestmark = pytest.mark.slow. Please also add a cgo-style Go-mode case for point 1 (a fake pclntab header in
mirai padding keeps /dev/watchdog). Run here: testStringExtractor.py + testStringExtractorFixtures.py 85
passed, testSynthesis.py + testDalvikStringRefsWithStrings.py 72 passed, testCommonModels.py +
testIntegration.py 60 passed. ruff check and ruff format --check are clean.
5. Code nits.
- AGENTS.md says no comments unless needed. Many of the new
#lines repeat the docstring next to them (for
example the threeSmdaReportattribute comments and the_SIZED_STRING_REJECT_REand_LENGTH_SCAN_WINDOW
comments). Keep the docstrings and the two or three comments that carry a why (C code passingopen(path, flags), terminators-first in synthesis). Drop the rest. smda.utility.StringExtractornow importssmda.synthesis.sniffBinaryFormatonly to choose Win64 or SysV
pairs.report.xheader/report.abiare already on the report. Either move the sniffer tosmda.commonor
decide from the ABI, so that utility does not depend on synthesis.- Most of the ~480 new lines are a small dataflow engine over capstone operand strings (
_MEMORY_SLOT_RE, alias
tables,_written_locations). I accept it here because it is isolated and only runs in Go/Rust mode, but it
must not spread to other callers.
CHANGELOG (against "How an entry is written")
- Prefix
(utility)matches the PR scope, and the corpus, the tree, the cost and(#382)are all there. Good. - Change not yet reproduced to Reproduced by reviewer on bundled fixtures for the
rust_pe_gnuandmirai_x64
figures. If the go1.24.7 figures go in, they need not reproduced because they are not bundled. - The Added bullet is 10 lines; the guide says about six. Keep the mechanism, the
rust_pe_gnuline and the
/rustc/cost. Move the 11 + 5 split and the timing detail to the PR body. - Add a
### Fixedbullet: Go mode no longer loses the whole report toOverflowErrorwhen a header length word
is out of range (repro: point 1). - Fix the "(4.5 s)" wording (see Reproduction above).
Verdict: REQUEST_CHANGES. The change is small: _cuts_c_string in Go mode, release _referenced_targets after
extraction, slow marker, CHANGELOG trim plus the Fixed line. The core of the PR is right and reproduces. I expect
to approve the next round without another measurement pass beyond point 1.
- Apply the cut-C-string fallback in Go mode too, so cgo code keeps
open(path, flags) strings whole. Go unsized reads keep 2+ characters.
- Clear the reference targets and string cache when extraction ends; hold
the targets in an array("Q").
- Move sniffBinaryFormat to smda.common.BinaryFormat; synthesis re-exports it.
- Mark the fixture tests slow, add a pclntab-planted mirai case, drop
comments that repeat docstrings.
…elplohmann#394, danielplohmann#395 and danielplohmann#396 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Thanks. Pushed 1. cgo C strings.
I also lowered the Go unsized minimum from 4 to 2. On the go1.24.7 pair it made no difference except 4 two-character reads on 386 ( 2. 3. Agreed, no change. 4. 5. Dropped the comments that repeated docstrings. I kept the rustc-path, pointer-low-bytes, NUL-in-length, C-fallback and terminators-first ones. CHANGELOG: "reproduced by reviewer" for the fixtures, Added bullet trimmed to 7 lines, timing given as your 0.66 s -> 1.12 s, and a Fixed bullet for the On distro builds: Rust binaries built with ruff, ruff format and ty are clean, and the full suite passes. |
danielplohmann
left a comment
There was a problem hiding this comment.
Approving on c4cd51a: your 0575a23 + d20681f, our merge of master after #389, #394, #395 and #396 (10fca38; the two CHANGELOG conflicts are resolved with master's bullets first, then yours), and your 3.11 fix on top. The quoted annotation is the better fix. I had written Optional[array] locally, but you pushed first, and your version keeps the int.
All points from review 3 are addressed. Checked on the merged tree, under Python 3.11 too:
testStringExtractor,testStringExtractorFixtures,testSynthesis,testDisassemblyResultRefs: 161 passed.mirai_x64: 162 strings clean. With the planted pclntab header, 125 strings, and/dev/watchdogandGETare kept now (master:OverflowError, so no report).rust_pe_gnu: 2909 refs, the same as before the merge.
Non-blocking, for the record:
- Under a planted pclntab header
url=is still lost onmirai_x64. That is the documented cost of the unsized minimum in Go mode. - The go1.24.7 figures are still not reproduced here (no Go fixture, no Go toolchain), and the CHANGELOG says so.
Thanks for staying with this one through four rounds.
StringExtractor imports CS_GRP_BRANCH_RELATIVE since #382, which capstone 4.x lacks; with only capstone<6 declared, pip resolved 4.0.2 beside dumpulator and import smda.Disassembler raised ImportError. 5.0.0.post1 and 5.0.1 import distutils and fail on Python 3.12+, hence 5.0.2. Full suite including the ARM backend passes on capstone 5.0.2.
Go and Rust pack string literals next to each other without a NUL between them. That meant a reference to
truecould turn intotruefalsenil. This changes the extractor to read the number of bytes the reference actually passes.The length comes from an x86 or AArch64 register pair, an x86 stack store, a 32-bit
push len; push ptrpair, or a(pointer, length)header in data. This also replaces the old Go stack-string check, which missed the register calling convention used by newer Go builds.The register checks use Go's own order and separate SysV and Win64 rules for Rust. When a headerless Rust report leaves two possible register pairs, the extractor skips that pair rather than choosing whichever immediate happens to be nearby.
The scan stays inside a basic block and stops when the pointer or length is overwritten. It checks partial and implicit register writes, overlapping stack writes, and changes to the stack base. A full-width pointer copy can still be followed after the original register is reused, whether its length was loaded before or after the copy. A register already holding a pointer cannot double as the length.
The string bytes are decoded as UTF-8. Non-ASCII strings get type
utf8, and synthesis writes them back as UTF-8 instead of dropping their non-ASCII bytes. Slice headers, pointer tables and targets outside the report's mapped sections are rejected.This adds
mode="rust". With no mode supplied, the report's Go or Rust language score selects the path when it is above 0.5. Other languages keep the existing path.The tests cover register, stack and push pairs, 32-bit and 64-bit headers, and decoded x86-64 and AArch64 instructions. They also cover stale lengths, register copies, partial writes, implicit writes, stack-slot overlap, block boundaries, UTF-8, and the existing default path. Checked with
python -m pytest tests/ -q --cov=smda --cov-report=xml: 2,272 passed, 1 skipped, with 89.33% overall coverage and 100% changed-line coverage across this PR.make lint,ruff format --check .andmake typecheckalso pass. The fixture files are unchanged.