diff --git a/cpp/ql/src/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.qhelp b/cpp/ql/src/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.qhelp new file mode 100644 index 000000000000..0b431d77cf53 --- /dev/null +++ b/cpp/ql/src/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.qhelp @@ -0,0 +1,35 @@ + + + +

+Firmware and embedded drivers often copy data into buffers using lengths read from +memory-mapped I/O (MMIO) registers or DMA descriptor fields. When those lengths are not +validated against the destination buffer size, an attacker who can influence hardware +registers or DMA metadata can trigger buffer overflows and potentially achieve remote +code execution on microcontrollers, WiFi stacks, and cellular basebands. +

+
+ +

+Always validate MMIO/DMA-derived lengths before passing them to memcpy, +memmove, or strncpy. Compare against a compile-time maximum +and reject or clamp out-of-range values before copying. +

+
+ +

Bad: length from an MMIO register used directly as the copy size.

+ +

Good: defensive bounds check before the copy.

+ +
+ +
  • +CWE-120: Buffer Copy without Checking Size of Input +
  • +
  • +CWE-787: Out-of-bounds Write +
  • +
    +
    diff --git a/cpp/ql/src/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.ql b/cpp/ql/src/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.ql new file mode 100644 index 000000000000..8d47e2fc939f --- /dev/null +++ b/cpp/ql/src/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.ql @@ -0,0 +1,84 @@ +/** + * @name MMIO/DMA unsanitized memory copy + * @description Memory copy sizes derived from memory-mapped I/O or DMA + * descriptor fields without bounds validation may overflow + * destination buffers. + * @kind path-problem + * @problem.severity error + * @security-severity 8.6 + * @precision medium + * @id cpp/mmio-unsanitized-memcpy + * @tags security + * external/cwe/cwe-120 + * external/cwe/cwe-787 + */ + +import cpp +import semmle.code.cpp.dataflow.new.TaintTracking +import semmle.code.cpp.controlflow.IRGuards +import MmioFlow::PathGraph + +/** Holds if `e` is an expression that reads MMIO/DMA hardware state. */ +predicate isMmioExpr(Expr e) { + exists(VariableAccess va | va = e and va.getTarget().isVolatile()) + or + exists(FieldAccess fa | fa = e and fa.getTarget().getType().isVolatile()) + or + exists(FunctionCall call | + call = e and + call.getTarget().hasName(["READ_REG", "GET_MMIO", "REG_READ", "DMA_READ"]) + ) + or + exists(PointerDereferenceExpr deref | + deref = e and + deref.getOperand().getUnspecifiedType() instanceof PointerType and + deref.getOperand().getUnspecifiedType().(PointerType).getBaseType().isVolatile() + ) +} + +predicate isMmioSource(DataFlow::Node source) { + isMmioExpr(source.asExpr()) + or + exists(MacroInvocation mi | + mi.getMacro().hasName(["READ_REG", "GET_MMIO", "REG_READ", "DMA_READ"]) and + source.asExpr() = mi.getExpr() + ) +} + +predicate isMemcpySizeSink(DataFlow::Node sink, FunctionCall fc) { + fc.getTarget().hasName(["memcpy", "memmove", "strncpy", "wmemcpy", "wmemmove"]) and + sink.asExpr() = fc.getArgument(2) +} + +/** Recognizes relational comparison bounds checks using public IRGuards API. */ +predicate lessThanOrEqual(IRGuardCondition g, Expr e, boolean branch) { + exists(Operand left | + g.comparesLt(left, _, _, true, branch) or + g.comparesEq(left, _, _, true, branch) + | + left.getDef().getConvertedResultExpression() = e + ) +} + +module MmioConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node source) { isMmioSource(source) } + + predicate isSink(DataFlow::Node sink) { isMemcpySizeSink(sink, _) } + + predicate isBarrier(DataFlow::Node node) { + node = DataFlow::BarrierGuard::getABarrierNode() or + node = DataFlow::BarrierGuard::getAnIndirectBarrierNode() + } + + predicate observeDiffInformedIncrementalMode() { any() } +} + +module MmioFlow = TaintTracking::Global; + +from FunctionCall memcpyCall, MmioFlow::PathNode source, MmioFlow::PathNode sink +where + MmioFlow::flowPath(source, sink) and + isMemcpySizeSink(sink.getNode(), memcpyCall) +select memcpyCall, source, sink, + "Memory copy size argument is derived from $@ without sufficient bounds validation.", + source.getNode(), "an MMIO/DMA hardware register read" diff --git a/cpp/ql/src/Security/CWE/CWE-120/MmioUnsanitizedMemcpyBad.c b/cpp/ql/src/Security/CWE/CWE-120/MmioUnsanitizedMemcpyBad.c new file mode 100644 index 000000000000..13e68fc7561d --- /dev/null +++ b/cpp/ql/src/Security/CWE/CWE-120/MmioUnsanitizedMemcpyBad.c @@ -0,0 +1,9 @@ +#define READ_REG(addr) (*(volatile unsigned int *)(addr)) +#define MAX_DMA_LEN 64 + +void *memcpy(void *dest, const void *src, unsigned long n); + +void bad_mmio_memcpy(char *dst, char *src) { + unsigned int len = READ_REG(0x40001000); + memcpy(dst, src, len); +} diff --git a/cpp/ql/src/Security/CWE/CWE-120/MmioUnsanitizedMemcpyGood.c b/cpp/ql/src/Security/CWE/CWE-120/MmioUnsanitizedMemcpyGood.c new file mode 100644 index 000000000000..38c37a3f3c64 --- /dev/null +++ b/cpp/ql/src/Security/CWE/CWE-120/MmioUnsanitizedMemcpyGood.c @@ -0,0 +1,10 @@ +#define READ_REG(addr) (*(volatile unsigned int *)(addr)) +#define MAX_DMA_LEN 64 + +void *memcpy(void *dest, const void *src, unsigned long n); + +void good_mmio_memcpy(char *dst, char *src) { + unsigned int len = READ_REG(0x40001000); + if (len <= MAX_DMA_LEN) + memcpy(dst, src, len); +} diff --git a/cpp/ql/src/codeql-suites/cpp-security-extended.qls b/cpp/ql/src/codeql-suites/cpp-security-extended.qls index 69c014c4c6f8..6c992b3da313 100644 --- a/cpp/ql/src/codeql-suites/cpp-security-extended.qls +++ b/cpp/ql/src/codeql-suites/cpp-security-extended.qls @@ -3,3 +3,6 @@ - apply: security-extended-selectors.yml from: codeql/suite-helpers - apply: codeql-suites/exclude-slow-queries.yml +# CWE-120: MMIO/DMA unsanitized memcpy (also selected by metadata; explicit for review) +- include: + id: cpp/mmio-unsanitized-memcpy diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/MmioUnsanitizedMemcpy.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/MmioUnsanitizedMemcpy.expected new file mode 100644 index 000000000000..78511215ff39 --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/MmioUnsanitizedMemcpy.expected @@ -0,0 +1,22 @@ +#select +| test.c:21:3:21:8 | call to memcpy | test.c:20:18:20:37 | * ... | test.c:21:20:21:22 | len | Memory copy size argument is derived from $@ without sufficient bounds validation. | test.c:20:18:20:37 | * ... | an MMIO/DMA hardware register read | +| test.c:26:3:26:9 | call to memmove | test.c:25:18:25:25 | call to GET_MMIO | test.c:26:21:26:23 | len | Memory copy size argument is derived from $@ without sufficient bounds validation. | test.c:25:18:25:25 | call to GET_MMIO | an MMIO/DMA hardware register read | +| test.c:31:3:31:9 | call to strncpy | test.c:30:18:30:29 | mmio_len_reg | test.c:31:21:31:23 | len | Memory copy size argument is derived from $@ without sufficient bounds validation. | test.c:30:18:30:29 | mmio_len_reg | an MMIO/DMA hardware register read | +edges +| test.c:20:18:20:37 | * ... | test.c:20:18:20:37 | * ... | provenance | | +| test.c:20:18:20:37 | * ... | test.c:21:20:21:22 | len | provenance | | +| test.c:25:18:25:25 | call to GET_MMIO | test.c:25:18:25:25 | call to GET_MMIO | provenance | | +| test.c:25:18:25:25 | call to GET_MMIO | test.c:26:21:26:23 | len | provenance | | +| test.c:30:18:30:29 | mmio_len_reg | test.c:30:18:30:29 | mmio_len_reg | provenance | | +| test.c:30:18:30:29 | mmio_len_reg | test.c:31:21:31:23 | len | provenance | | +nodes +| test.c:20:18:20:37 | * ... | semmle.label | * ... | +| test.c:20:18:20:37 | * ... | semmle.label | * ... | +| test.c:21:20:21:22 | len | semmle.label | len | +| test.c:25:18:25:25 | call to GET_MMIO | semmle.label | call to GET_MMIO | +| test.c:25:18:25:25 | call to GET_MMIO | semmle.label | call to GET_MMIO | +| test.c:26:21:26:23 | len | semmle.label | len | +| test.c:30:18:30:29 | mmio_len_reg | semmle.label | mmio_len_reg | +| test.c:30:18:30:29 | mmio_len_reg | semmle.label | mmio_len_reg | +| test.c:31:21:31:23 | len | semmle.label | len | +subpaths diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/MmioUnsanitizedMemcpy.qlref b/cpp/ql/test/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/MmioUnsanitizedMemcpy.qlref new file mode 100644 index 000000000000..e82093d33480 --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/MmioUnsanitizedMemcpy.qlref @@ -0,0 +1,2 @@ +query: Security/CWE/CWE-120/MmioUnsanitizedMemcpy.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/test.c b/cpp/ql/test/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/test.c new file mode 100644 index 000000000000..01286d2a14b4 --- /dev/null +++ b/cpp/ql/test/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/test.c @@ -0,0 +1,50 @@ +/* Semmle test case for MmioUnsanitizedMemcpy.ql + * MMIO/DMA register reads flowing into memcpy/memmove/strncpy size parameters. + */ + +typedef unsigned int uint32_t; + +void *memcpy(void *dest, const void *src, unsigned long n); +void *memmove(void *dest, const void *src, unsigned long n); +char *strncpy(char *dest, const char *src, unsigned long n); + +#define READ_REG(addr) (*(volatile uint32_t *)(addr)) +#define MAX_DMA_LEN 64 + +uint32_t GET_MMIO(unsigned long addr); +uint32_t DMA_READ(unsigned long addr); + +volatile uint32_t mmio_len_reg; + +static void bad_read_reg(char *dst, char *src) { + uint32_t len = READ_REG(0x40001000); // $ Source + memcpy(dst, src, len); // $ Alert +} + +static void bad_get_mmio(char *dst, char *src) { + uint32_t len = GET_MMIO(0x50000000); // $ Source + memmove(dst, src, len); // $ Alert +} + +static void bad_volatile_global(char *dst, char *src) { + uint32_t len = mmio_len_reg; // $ Source + strncpy(dst, src, len); // $ Alert +} + +static void good_bounded(char *dst, char *src) { + uint32_t len = READ_REG(0x40001000); + if (len <= MAX_DMA_LEN) + memcpy(dst, src, len); // GOOD +} + +static void good_early_return(char *dst, char *src) { + uint32_t len = DMA_READ(0x60000000); + if (len > MAX_DMA_LEN) + return; + memcpy(dst, src, len); // GOOD +} + +static void good_constant_size(char *dst, char *src) { + uint32_t len = READ_REG(0x40001000); + memcpy(dst, src, 32); // GOOD — constant size, not tainted sink +}