From dd9270f248fec0a938a87ddaefabe43f3ff9524e Mon Sep 17 00:00:00 2001 From: Andrey Mnatsakanov Date: Mon, 27 Jul 2026 18:31:03 +0200 Subject: [PATCH] Close public API gaps (closes marirs/capa-rs#22) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - pub use FileFormat, Os, FileArchitecture, SecurityCheckStatus from the crate root — they appear in public fields but lived in private modules, so downstream could not name them. - FunctionCapabilities: address()/features()/capabilities() getters (fields stay private; additive). - BinarySecurityCheckOptions: builder-style no_libc(bool) — the documented option was unreachable (pub(crate) field, new() hard-coded false). - from_file/from_buffer: spawn the rules-load thread BEFORE format detection + disassembly so it actually overlaps (smda disassembles eagerly; previously join blocked immediately), propagate the real RuleSet::new error instead of misreporting DescriptionEvaluationError, and resume_unwind a loader panic instead of mislabeling it. - LibCSpec: strict FromStr (unknown versions error instead of silently degrading to LSB5); lenient From kept for compat; capa_cli uses the strict parse. New Error::InvalidLibCSpec variant. - from_file takes impl AsRef (was AsRef); existing &str/String callers keep compiling. Adds tests/public_api.rs integration tests pinning the public surface. --- CHANGELOG.md | 27 ++++++++++ examples/capa_cli.rs | 13 ++++- src/error.rs | 5 ++ src/lib.rs | 117 +++++++++++++++++++++++++++++++------------ tests/public_api.rs | 60 ++++++++++++++++++++++ 5 files changed, 189 insertions(+), 33 deletions(-) create mode 100644 tests/public_api.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index df35aa4..3222f9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,33 @@ 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 — public API gaps (closes [#22](https://github.com/marirs/capa-rs/issues/22)) + +- **Types in public fields are now nameable downstream** `pub use` + from the crate root for `FileFormat`, `Os`, `FileArchitecture` + (`Properties::format` / `os` / `arch`) and `SecurityCheckStatus` + (`FileCapabilities::security_checks`) — previously their modules + were private, so the fields were Debug-print-only. +- **`FunctionCapabilities` gained getters** `address()`, `features()`, + `capabilities()` — fields stay private (additive, non-breaking). +- **`BinarySecurityCheckOptions.no_libc` is reachable** via a new + builder-style `no_libc(bool)` method; the field is crate-private and + `new()` hard-coded `false`, so the documented option could not be + enabled before. +- **Rule loading reports its real error and actually runs in + parallel** the loader thread was spawned *after* the extractor was + built (smda disassembles eagerly, so `join` blocked immediately and + ~1000 YAML files loaded serially), and every failure was misreported + as `DescriptionEvaluationError`. The thread now spawns before format + detection/disassembly, the real `RuleSet::new` error propagates, and + a loader panic is re-thrown instead of mislabeled. +- **Strict `LibCSpec::from_str`** unknown LSB versions now error + (previously any typo silently became `LSB5`, changing fortify-check + semantics); the lenient `From` remains for compatibility, + and `capa_cli` uses the strict parse. +- **`from_file` accepts `impl AsRef`** (was `AsRef`); + existing `&str`/`String` callers keep compiling. + ## [0.5.2] — xor-zero number(0), regex /i fast path, rule pre-pruning ### Fixed — feature extraction parity diff --git a/examples/capa_cli.rs b/examples/capa_cli.rs index 93fbb63..6c270cd 100644 --- a/examples/capa_cli.rs +++ b/examples/capa_cli.rs @@ -10,7 +10,7 @@ use clap::Parser; use prettytable::{Attr, Cell, Row, Table, color, format::Alignment}; use serde_json::{Map, Value, to_value}; -use capa::{BinarySecurityCheckOptions, FileCapabilities}; +use capa::{BinarySecurityCheckOptions, FileCapabilities, LibCSpec}; #[derive(Parser)] #[clap( @@ -77,7 +77,16 @@ fn main() { let json_path = cli.output; let libc = cli.libc.map(|s| s.into()); let sysroot = cli.sysroot.map(|s| s.into()); - let libc_spec = cli.libc_spec.map(|s| s.into()); + // Strict parse (0.5.3, #22): an unknown LSB version used to fall + // back to LSB5 silently, changing fortify-check semantics. + let libc_spec = match cli.libc_spec.map(|s| s.parse::()) { + Some(Ok(spec)) => Some(spec), + Some(Err(e)) => { + eprintln!("error: {e}"); + std::process::exit(1); + } + None => None, + }; let security_check_opts = BinarySecurityCheckOptions::new(libc, sysroot, libc_spec); let start = Instant::now(); diff --git a/src/error.rs b/src/error.rs index ca3571a..0d2c074 100644 --- a/src/error.rs +++ b/src/error.rs @@ -109,4 +109,9 @@ pub enum Error { // nothing to match capabilities against. #[error("AnalyzeBuilder: .rules(path) must be called before .from_file() / .from_buffer()")] BuilderMissingRules, + + // 0.5.3 (#22): strict `LibCSpec::from_str` rejects unknown LSB + // version strings (the lenient `From` fallback stays). + #[error("invalid libc spec version: {0}")] + InvalidLibCSpec(String), } diff --git a/src/lib.rs b/src/lib.rs index a953913..e0da3e4 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -26,7 +26,6 @@ use std::{ use once_cell::sync::Lazy; use serde::{Deserialize, Serialize}; use serde_json::{Value, json}; -use smda::FileArchitecture; use yaml_rust::Yaml; // 0.4.2: regexes used in tag-string parsing — compiled once per @@ -43,16 +42,16 @@ static PARTS_ID_RE: Lazy = Lazy::new(|| { .expect("compile-time regex literal — pattern is valid") }); -use consts::FileFormat; -// 0.4.0: `Os` is referenced only by the properties-gated -// `FileCapabilities::get_os` — gating the import avoids the -// `--no-default-features` unused-import warning. -#[cfg(feature = "properties")] -use consts::Os; -use sede::{from_hex, to_hex}; - +// 0.5.3 (#22): types that appear in public fields (`Properties::format`, +// `Properties::os`, `Properties::arch`, `FileCapabilities::security_checks`) +// are re-exported from the crate root — previously downstream could not +// name them (their modules are private), making the fields unusable for +// anything but Debug-printing. +pub use crate::consts::{FileFormat, Os}; pub use crate::error::Error; -use crate::security::options::status::SecurityCheckStatus; +pub use crate::security::options::status::SecurityCheckStatus; +use sede::{from_hex, to_hex}; +pub use smda::FileArchitecture; pub(crate) mod consts; mod error; @@ -148,22 +147,37 @@ impl LibCSpec { // Used for options for binary security checks. impl From for LibCSpec { + /// Lenient conversion kept for API compatibility: unknown versions + /// fall back to the newest spec. Prefer `LibCSpec::from_str` + /// (0.5.3, #22), which rejects unknown versions. fn from(value: String) -> Self { - match value.as_str() { - "1.0.0" => LibCSpec::LSB1, - "1.1.0" => LibCSpec::LSB1dot1, - "1.2.0" => LibCSpec::LSB1dot2, - "1.3.0" => LibCSpec::LSB1dot3, - "2.0.0" => LibCSpec::LSB2, - "2.0.1" => LibCSpec::LSB2dot0dot1, - "2.1.0" => LibCSpec::LSB2dot1, - "3.0.0" => LibCSpec::LSB3, - "3.1.0" => LibCSpec::LSB3dot1, - "3.2.0" => LibCSpec::LSB3dot2, - "4.0.0" => LibCSpec::LSB4, - "4.1.0" => LibCSpec::LSB4dot1, - "5.0.0" => LibCSpec::LSB5, - _ => LibCSpec::LSB5, + value.parse().unwrap_or(LibCSpec::LSB5) + } +} + +impl std::str::FromStr for LibCSpec { + type Err = Error; + + /// Strict version parse — unknown versions are an error. + /// 0.5.3 (#22): previously the only way in was `From`, + /// which silently mapped any typo (e.g. `4.0.1`) to `LSB5` and + /// changed fortify-check semantics without a word. + fn from_str(s: &str) -> Result { + match s { + "1.0.0" => Ok(LibCSpec::LSB1), + "1.1.0" => Ok(LibCSpec::LSB1dot1), + "1.2.0" => Ok(LibCSpec::LSB1dot2), + "1.3.0" => Ok(LibCSpec::LSB1dot3), + "2.0.0" => Ok(LibCSpec::LSB2), + "2.0.1" => Ok(LibCSpec::LSB2dot0dot1), + "2.1.0" => Ok(LibCSpec::LSB2dot1), + "3.0.0" => Ok(LibCSpec::LSB3), + "3.1.0" => Ok(LibCSpec::LSB3dot1), + "3.2.0" => Ok(LibCSpec::LSB3dot2), + "4.0.0" => Ok(LibCSpec::LSB4), + "4.1.0" => Ok(LibCSpec::LSB4dot1), + "5.0.0" => Ok(LibCSpec::LSB5), + _ => Err(Error::InvalidLibCSpec(s.to_string())), } } } @@ -206,6 +220,15 @@ impl BinarySecurityCheckOptions { input_file: PathBuf::new(), } } + + /// Assume that input files do not use any C runtime libraries + /// (disables the libc-dependent ELF checks such as FORTIFY-SOURCE). + /// 0.5.3 (#22): the option existed but was unreachable — the field + /// is crate-private and `new()` hard-coded it to `false`. + pub fn no_libc(mut self, no_libc: bool) -> Self { + self.no_libc = no_libc; + self + } } impl Default for BinarySecurityCheckOptions { @@ -450,9 +473,17 @@ impl<'a> AnalyzeBuilder<'a> { /// Terminal — analyse a binary on disk. Routes through capa-rs's /// magic-byte format detection (PE → dnfile-then-smda, ELF → /// smda, Mach-O → smda) and runs the binary security checklist. - pub fn from_file(self, file_name: impl AsRef) -> Result { + /// + /// Accepts any `AsRef` (0.5.3 — was `AsRef`); non-UTF-8 + /// paths are converted with `to_string_lossy`. + pub fn from_file(self, file_name: impl AsRef) -> Result { let rule_path = self.rules.ok_or(Error::BuilderMissingRules)?; - let f = file_name.as_ref().to_string(); + let f = file_name.as_ref().to_string_lossy().into_owned(); + // Spawn the rules load FIRST so it overlaps with format + // detection and (eager) disassembly below — pre-#22 the thread + // was spawned after the extractor was built, so `join` blocked + // immediately and ~1000 rule files loaded strictly serially. + let rules_thread_handle = spawn(move || rules::RuleSet::new(&rule_path)); let (format, buffer) = get_format(&f)?; let extractor = get_file_extractors( &f, @@ -461,10 +492,14 @@ impl<'a> AnalyzeBuilder<'a> { self.high_accuracy, self.resolve_tailcalls, )?; - let rules_thread_handle = spawn(move || rules::RuleSet::new(&rule_path)); let rules = match rules_thread_handle.join() { Ok(Ok(rules)) => rules, - Ok(Err(_)) | Err(_) => return Err(Error::DescriptionEvaluationError), + // Propagate the real RuleSet error (bad YAML, missing + // dependency, …) — pre-#22 every failure was misreported + // as DescriptionEvaluationError. A loader panic is a bug; + // keep it a panic rather than mislabeling it. + Ok(Err(e)) => return Err(e), + Err(panic) => std::panic::resume_unwind(panic), }; // Security checks — defaults if caller didn't override. @@ -553,6 +588,7 @@ impl<'a> AnalyzeBuilder<'a> { /// (pass `0` if the caller has no preference). pub fn from_buffer(self, raw: &[u8], base_addr: u64, bitness: u32) -> Result { let rule_path = self.rules.ok_or(Error::BuilderMissingRules)?; + let rules_thread_handle = spawn(move || rules::RuleSet::new(&rule_path)); // Construct the extractor directly via smda's parse_buffer — // get_file_extractors routes on PE/ELF/Mach-O magic, which // a raw buffer doesn't have. @@ -565,10 +601,12 @@ impl<'a> AnalyzeBuilder<'a> { self.resolve_tailcalls, )?); - let rules_thread_handle = spawn(move || rules::RuleSet::new(&rule_path)); let rules = match rules_thread_handle.join() { Ok(Ok(rules)) => rules, - Ok(Err(_)) | Err(_) => return Err(Error::DescriptionEvaluationError), + // See `from_file`: real error propagates, a loader panic + // stays a panic (#22). + Ok(Err(e)) => return Err(e), + Err(panic) => std::panic::resume_unwind(panic), }; // 0.4.3: FLIRT setup — see `from_file` for rationale. @@ -1391,6 +1429,23 @@ pub struct FunctionCapabilities { capabilities: Vec, } +impl FunctionCapabilities { + /// Address of the analysed function. + pub fn address(&self) -> usize { + self.address + } + + /// Number of features extracted from the function. + pub fn features(&self) -> usize { + self.features + } + + /// Names of the rules that matched inside the function. + pub fn capabilities(&self) -> &[String] { + &self.capabilities + } +} + fn parse_parts_id(s: &str) -> Result<(Vec, String)> { // 0.4.2: cached at module scope (PARTS_ID_RE); was compiled per call. let re = &*PARTS_ID_RE; diff --git a/tests/public_api.rs b/tests/public_api.rs new file mode 100644 index 0000000..35b2388 --- /dev/null +++ b/tests/public_api.rs @@ -0,0 +1,60 @@ +//! #22: public API surface — downstream must be able to *name* the +//! types that appear in public fields, and the documented options must +//! be reachable. Most assertions here only need to type-check: pre-#22 +//! these types were unreachable because their modules are private. + +use capa::{BinarySecurityCheckOptions, LibCSpec}; +use std::str::FromStr; + +#[test] +fn exported_field_types_are_nameable() { + // Pre-#22 none of these paths resolved from outside the crate. + let _f: Option = None; + let _o: Option = None; + let _a: Option = None; + let _s: Option = None; +} + +#[test] +fn function_capabilities_getters_exist() { + // Fields stay private; getters are the read API (verbose feature). + let _: fn(&capa::FunctionCapabilities) -> usize = capa::FunctionCapabilities::address; + let _: fn(&capa::FunctionCapabilities) -> usize = capa::FunctionCapabilities::features; + let _: fn(&capa::FunctionCapabilities) -> &[String] = capa::FunctionCapabilities::capabilities; +} + +#[test] +fn no_libc_option_is_reachable() { + // Pre-#22 `no_libc` was pub(crate) and `new()` hard-coded false. + let _opts = BinarySecurityCheckOptions::default().no_libc(true); +} + +#[test] +fn libc_spec_from_str_is_strict() { + assert!(matches!( + LibCSpec::from_str("4.1.0"), + Ok(LibCSpec::LSB4dot1) + )); + assert!(matches!(LibCSpec::from_str("5.0.0"), Ok(LibCSpec::LSB5))); + // Unknown versions error (pre-#22 they silently became LSB5). + assert!(LibCSpec::from_str("4.0.1").is_err()); + assert!(LibCSpec::from_str("").is_err()); + // The lenient From stays for compatibility. + assert!(matches!( + LibCSpec::from("4.0.1".to_string()), + LibCSpec::LSB5 + )); +} + +#[test] +fn from_file_accepts_path_like_arguments() { + // Only checks the signature: &str, String, &Path and PathBuf must + // all compile (pre-#22 only AsRef was accepted). The calls + // fail at runtime for a missing rules dir — that's fine, we never + // execute them. + let _ = |p: &std::path::Path| { + capa::FileCapabilities::analyze() + .rules("definitely-missing-rules-dir") + .from_file(p) + }; +}