Merge pull request #498 from fkie-cad/fix/scanf-missing-parameter-registers

fix: panic in scanf/sscanf domain knowledge due to missing parameter registers

The PcodeExtractor exported empty integer parameter register lists for many
architectures (e.g. ARM, MIPS32, PPC, RISC-V), since it compared the parent
register of the storage locations with the potential input registers, which
fails for base registers like ARM's r0. Additionally, the first storage
location (which describes the return value) was not skipped. Now the base
registers are compared and exported.

The scanf/sscanf domain knowledge unwrapped the first integer parameter
registers, which panicked for these calling conventions. Calling conventions
without integer parameter registers (e.g. x86 cdecl) are now handled by
falling back to stack parameters.

Add a regression acceptance test with a new scanf_sscanf sample.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

src/cwe_checker_lib/src/ghidra_pcode/function/extern_function/domain_knowledge/scanf_sscanf.rs

@@ -1,6 +1,6 @@
 use super::prelude::*;
 
-use crate::intermediate_representation::Arg as IrArg;
+use crate::intermediate_representation::{Arg as IrArg, ByteSize, Expression as IrExpression};
 
 const APPLICABLE_SYMBOLS: [&str; 4] = ["scanf", "sscanf", "__isoc99_scanf", "__isoc99_sscanf"];
 const SSCANF_SYMBOLS: [&str; 2] = ["sscanf", "__isoc99_sscanf"];
@@ -25,26 +25,43 @@ fn apply_domain_knowledge_to(
             .calling_conventions
             .get(ir_extern_symbol.calling_convention.as_ref().unwrap())
             .unwrap();
-        let mut parameters: Vec<IrArg> = Vec::new();
+        let num_params = if SSCANF_SYMBOLS.contains(&ir_extern_symbol.name.as_str()) {
+            2
+        } else {
+            1
+        };
 
-        // TODO: Test that this is indeed on the stack for x86.
         // TODO: Insert domain knowledge about parameter type.
-        let param0 = cconv
-            .get_integer_parameter_register(0)
-            .unwrap()
-            .to_ir_arg(&ir_expr_sp);
-        parameters.push(param0);
-
-        if SSCANF_SYMBOLS.contains(&ir_extern_symbol.name.as_str()) {
-            let param1 = cconv
-                .get_integer_parameter_register(1)
-                .unwrap()
-                .to_ir_arg(&ir_expr_sp);
-            parameters.push(param1);
-        }
-
-        ir_extern_symbol.parameters = parameters;
+        ir_extern_symbol.parameters = (0..num_params)
+            .map(|idx| match cconv.get_integer_parameter_register(idx) {
+                Some(register) => register.to_ir_arg(&ir_expr_sp),
+                // Calling conventions without integer parameter registers,
+                // e.g., x86 cdecl, pass all parameters on the stack.
+                None => get_stack_param(idx, pcode_project, &ir_expr_sp),
+            })
+            .collect();
     }
 
     should_stop
 }
+
+/// Returns the `idx`th pointer-sized parameter passed on the stack.
+///
+/// On x86 the return address is pushed to the stack by the call instruction,
+/// i.e., the parameters start after it.
+fn get_stack_param(idx: usize, pcode_project: &PcodeProject, ir_expr_sp: &IrExpression) -> IrArg {
+    let pointer_size = pcode_project.stack_pointer_register.size();
+    let first_param_offset = if pcode_project.cpu_arch.starts_with("x86") {
+        pointer_size
+    } else {
+        0
+    };
+
+    IrArg::Stack {
+        address: ir_expr_sp
+            .clone()
+            .plus_const((first_param_offset + idx as u64 * pointer_size) as i64),
+        size: ByteSize::new(pointer_size),
+        data_type: None,
+    }
+}

src/ghidra/p_code_extractor/PcodeExtractor.java

@@ -195,22 +195,47 @@ public class PcodeExtractor extends GhidraScript {
 		ArrayList<Varnode> integer_parameter_register = new ArrayList<Varnode>();
 		DatatypeProperties dataTypeProperties = new DatatypeProperties(ghidraProgram);
 
+		for (Register base_register : get_integer_parameter_base_register(prototypeModel, ghidraProgram)) {
+			integer_parameter_register.add(new Varnode(context.getRegisterVarnode(base_register), context, dataTypeProperties));
+		}
+		return integer_parameter_register;
+	}
+
+	/**
+	 * Returns the base registers of the integer parameter registers in order
+	 * defined by the calling convention.
+	 */
+	private ArrayList<Register> get_integer_parameter_base_register(PrototypeModel prototypeModel,
+	        ghidra.program.model.listing.Program ghidraProgram) {
+		ArrayList<Register> integer_parameter_register = new ArrayList<Register>();
+
 		// prepare a parameter list of integers only
 		DataTypeManager dataTypeManager = ghidraProgram.getDataTypeManager();
 		IntegerDataType[] integer_parameter_list = new IntegerDataType[10];
 		for (int i = 0; i < integer_parameter_list.length; i++) {
 			integer_parameter_list[i] = new IntegerDataType(dataTypeManager);
 		}
+		// get all used registers by the prepared integer parameter list
+		// (the first element always describes the return value storage and is skipped)
+		VariableStorage[] storage_locations = prototypeModel.getStorageLocations(ghidraProgram,
+		        integer_parameter_list, false);
+		List<VariableStorage> integer_parameter_storage = Arrays.asList(storage_locations).subList(1,
+		        storage_locations.length);
 		// get all possible parameter passing registers, including floating point
 		// registers
 		for (VariableStorage potential_register : prototypeModel.getPotentialInputRegisterStorage(ghidraProgram)) {
-			// get all used registers by the prepared integer parameter list
-			for (VariableStorage integer_register : prototypeModel.getStorageLocations(ghidraProgram,
-			        integer_parameter_list, false)) {
+			if (!potential_register.isRegisterStorage()) {
+				continue;
+			}
+			Register base_register = potential_register.getRegister().getBaseRegister();
+			for (VariableStorage integer_register : integer_parameter_storage) {
 				// take only registers that are in common
+				// (compare base registers, since e.g. ARM's r0 has no parent register
+				// and PowerPC's 32-bit parameter registers are subregisters of 64-bit registers)
 				if (integer_register.isRegisterStorage()
-				        && integer_register.getRegister().getParentRegister() == potential_register.getRegister()) {
-					integer_parameter_register.add(new Varnode(potential_register.getFirstVarnode(), context, dataTypeProperties));
+				        && integer_register.getRegister().getBaseRegister() == base_register
+				        && !integer_parameter_register.contains(base_register)) {
+					integer_parameter_register.add(base_register);
 				}
 			}
 		}
@@ -239,9 +264,8 @@ public class PcodeExtractor extends GhidraScript {
 		potential_register.removeIf(reg -> reg.getRegister() == null);
 
 		// remove all integer parameter register
-		for (Varnode integer_register : get_integer_parameter_register(prototypeModel, ghidraProgram, context)) {
-			potential_register.removeIf(reg -> reg.getRegister().getName() == integer_register.getRegisterName());
-		}
+		ArrayList<Register> integer_register = get_integer_parameter_base_register(prototypeModel, ghidraProgram);
+		potential_register.removeIf(reg -> integer_register.contains(reg.getRegister().getBaseRegister()));
 
 		for (VariableStorage float_register : potential_register) {
 			float_parameter_register.add(new Varnode(float_register.getFirstVarnode(), context, dataTypeProperties));

test/artificial_samples/scanf_sscanf.c

@@ -0,0 +1,15 @@
+#include <stdio.h>
+#include <stdlib.h>
+
+// Regression test sample: calls to (s)scanf must not crash the pcode-to-IR
+// translation, e.g., on architectures where the parameter registers of the
+// calling convention are not known.
+
+int main(int argc, char **argv) {
+    char cmd[64];
+    int number;
+    scanf("%d", &number);
+    sscanf(argv[1], "%63s", cmd);
+    system(cmd);
+    return number;
+}

test/src/lib.rs

@@ -397,6 +397,35 @@ mod tests {
         run_tests!(tests, 1, "[CWE78]");
     }
 
+    /// Regression test: Calls to `scanf` and `sscanf` must not crash the
+    /// pcode-to-IR translation. The input passed through `sscanf` to `system`
+    /// should be detected as CWE-78.
+    #[test]
+    #[ignore]
+    fn scanf_sscanf() {
+        let tests = all_test_cases("scanf_sscanf", "CWE78");
+
+        // CWE-78 is not detected, but the analysis must not crash.
+        run_tests!(
+            tests,
+            1,
+            "[CWE78]",
+            ("mips64", "gcc", 0),
+            ("mips64", "clang", 0),
+            ("mips64el", "gcc", 0),
+            ("mips64el", "clang", 0),
+            ("ppc", "gcc", 0),
+            ("ppc64", "gcc", 0),
+            ("ppc64", "clang", 0),
+            ("ppc64le", "gcc", 0),
+            ("ppc64le", "clang", 0),
+            ("x86", "gcc", 0),
+            ("x86", "clang", 0),
+            ("x64", "mingw32-gcc", 0),
+            ("x86", "mingw32-gcc", 0),
+        );
+    }
+
     #[test]
     #[ignore]
     fn cwe_119() {