From bc3b51acd4a58ea5175ad22776875db275329a11 Mon Sep 17 00:00:00 2001 From: Andrey Mnatsakanov Date: Tue, 28 Jul 2026 15:41:15 +0200 Subject: [PATCH] Fix silent feature corruption in smda extractor (closes marirs/capa-rs#24) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - parse_operand_to_number: require a leading digit for h-suffixed and bare hex literals — register names ah/bh/ch/dh parsed as 0xA-0xD and hex-looking labels (beef, face) parsed as numbers. - mask negative immediates at the function's bitness (was always u32, truncating x64 values like mov rax, -1 to 0xFFFFFFFF). - emit stack string characteristic once per basic block (was pushed per instruction past the threshold with no break). - drop the duplicate plain-ASCII pass from extract_unicode_strings — extract_file_strings already runs extract_ascii_strings alongside, so every ASCII string was emitted twice. Adds 3 regression tests (16 total, all passing). --- CHANGELOG.md | 19 +++++++ src/extractor/smda.rs | 120 +++++++++++++++++++++++++++++++++++------- 2 files changed, 119 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index df35aa4..5fc39dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,25 @@ This project follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html). `u16::from_be_bytes` received the chunk bytes in reverse order, turning every UTF-16BE string into non-ASCII garbage that was then dropped. +### Fixed — silent feature corruption in the smda extractor (closes [#24](https://github.com/marirs/capa-rs/issues/24)) + +- **Registers and labels no longer parse as numbers** the `h`-suffix + case of `parse_operand_to_number` stripped the suffix and parsed the + rest as hex, so `mov al, ah` emitted `Number(0xA)` (same for + `bh`/`ch`/`dh`); the bare-hex fallback also accepted hex-looking + labels (`beef`, `face`). Both paths now require a leading digit, per + the Intel convention for hex literals. +- **Negative immediates mask at the function's bitness** previously + always masked to u32, so `mov rax, -1` on x64 emitted + `Number(0xFFFFFFFF)` instead of `Number(0xFFFFFFFFFFFFFFFF)`. +- **`stack string` emitted once per basic block** the push sat inside + the instruction loop with no `break`, adding a duplicate + characteristic for every instruction past the threshold. +- **ASCII strings no longer emitted twice** `extract_unicode_strings` + ran a plain-ASCII pass with the same printable class as + `extract_ascii_strings`, and `extract_file_strings` calls both; the + UTF-16 extractor is now UTF-16-only. + ## [0.5.2] — xor-zero number(0), regex /i fast path, rule pre-pruning ### Fixed — feature extraction parity diff --git a/src/extractor/smda.rs b/src/extractor/smda.rs index 8edc668..1a9593d 100644 --- a/src/extractor/smda.rs +++ b/src/extractor/smda.rs @@ -310,15 +310,18 @@ impl<'data> super::Extractor for Extractor<'data> { if instr.is_mov_imm_to_stack()? { count += instr.get_printable_len()?; } - if count > 8 { - //MIN_STACKSTRING_LEN - res.push(( - crate::rules::features::Feature::Characteristic( - crate::rules::features::CharacteristicFeature::new("stack string", "")?, - ), - *bb.0, - )); - } + } + if count > 8 { + //MIN_STACKSTRING_LEN + // Emitted once per basic block — previously the push sat + // inside the loop with no `break`, so every instruction + // past the threshold added a duplicate (#24). + res.push(( + crate::rules::features::Feature::Characteristic( + crate::rules::features::CharacteristicFeature::new("stack string", "")?, + ), + *bb.0, + )); } Ok(res) } @@ -1263,7 +1266,16 @@ impl<'data> Extractor<'data> { // case 2: if operand is like 1234h if let Some(stripped_operand) = operand.strip_suffix('h') { - return i128::from_str_radix(stripped_operand, 16).ok(); + // Intel convention: an h-suffixed hex literal must start + // with a digit (0ABh). Without this check the register + // names ah/bh/ch/dh parsed as 0xA/0xB/0xC/0xD (#24). + if stripped_operand + .chars() + .next() + .is_some_and(|c| c.is_ascii_digit()) + { + return i128::from_str_radix(stripped_operand, 16).ok(); + } } // case 3: if operand is like +0x1234 @@ -1288,8 +1300,13 @@ impl<'data> Extractor<'data> { return Some(val); } - // case 5: if operand is like 0x1234 - i128::from_str_radix(operand, 16).ok() + // case 5: bare hex without 0x/h, e.g. 0dead. Must start with a + // digit — otherwise hex-looking labels (beef, face, add) parse + // as numbers (#24). + if operand.chars().next().is_some_and(|c| c.is_ascii_digit()) { + return i128::from_str_radix(operand, 16).ok(); + } + None } pub fn extract_insn_number_features( @@ -1317,7 +1334,12 @@ impl<'data> Extractor<'data> { insn.offset, )); } else { - let masked_value = (s as u32) as i128; // Convierte a u32 y de vuelta a i128 + // Negative immediates are emitted as their + // unsigned interpretation at the function's + // bitness — masking to u32 truncated x64 values + // (mov rax, -1 → 0xFFFFFFFF instead of + // 0xFFFFFFFFFFFFFFFF) (#24). + let masked_value = mask_to_bitness(s, f.bitness); res.push(( crate::rules::features::Feature::Number( crate::rules::features::NumberFeature::new( @@ -1570,6 +1592,16 @@ pub fn generate_symbols(dll: &Option, symbol: &Option) -> Result Ok(res) } +/// Unsigned interpretation of a negative immediate at the given bitness +/// (#24): `-1` is `0xFFFFFFFF` on 32-bit and `0xFFFFFFFFFFFFFFFF` on +/// 64-bit. Previously the mask was always u32, truncating x64 values. +fn mask_to_bitness(value: i128, bitness: u32) -> i128 { + match bitness { + 64 => (value as u64) as i128, + _ => (value as u32) as i128, + } +} + pub fn derefs(report: &DisassemblyReport<'_>, p: &u64) -> Result> { let mut res = vec![]; let mut depth = 0; @@ -1784,7 +1816,6 @@ pub fn extract_unicode_strings(data: &[u8], min_length: usize) -> Result Result