-
Notifications
You must be signed in to change notification settings - Fork 2.1k
cpp: Add 'cpp/mmio-unsanitized-memcpy' query #22438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Tito0015
wants to merge
7
commits into
github:main
Choose a base branch
from
Tito0015:feature/cpp-mmio-unsanitized-memcpy
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
31ca346
cpp: Add 'cpp/mmio-unsanitized-memcpy' query
Tito0015 b200dd7
Merge upstream/main into feature/cpp-mmio-unsanitized-memcpy
Tito0015 ac50b21
cpp: Add change note for mmio-unsanitized-memcpy query
Tito0015 597608a
Merge branch 'main' into feature/cpp-mmio-unsanitized-memcpy
Tito0015 799642e
Merge upstream/main into feature/cpp-mmio-unsanitized-memcpy
Tito0015 2c3434b
cpp: Move mmio-unsanitized-memcpy to experimental
Tito0015 5c92f5e
cpp: Drop experimental query id prefix and change note
Tito0015 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
34 changes: 34 additions & 0 deletions
34
cpp/ql/src/experimental/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.qhelp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| <!DOCTYPE qhelp PUBLIC | ||
| "-//Semmle//qhelp//EN" | ||
| "qhelp.dtd"> | ||
| <qhelp> | ||
| <overview> | ||
| <p> | ||
| Firmware and embedded drivers often copy data into buffers using lengths read from | ||
| allowlisted MMIO register macros such as <code>READ_REG</code> or <code>GET_MMIO</code>. | ||
| 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. | ||
| </p> | ||
| </overview> | ||
| <recommendation> | ||
| <p> | ||
| Always validate MMIO/DMA-derived lengths before passing them to <code>memcpy</code>, | ||
| <code>memmove</code>, or <code>strncpy</code>. Compare against a compile-time maximum | ||
| and reject or clamp out-of-range values before copying. | ||
| </p> | ||
| </recommendation> | ||
| <example> | ||
| <p>Bad: length from an MMIO register used directly as the copy size.</p> | ||
| <sample src="MmioUnsanitizedMemcpyBad.c" /> | ||
| <p>Good: defensive bounds check before the copy.</p> | ||
| <sample src="MmioUnsanitizedMemcpyGood.c" /> | ||
| </example> | ||
| <references> | ||
| <li> | ||
| CWE-120: Buffer Copy without Checking Size of Input | ||
| </li> | ||
| <li> | ||
| CWE-787: Out-of-bounds Write | ||
| </li> | ||
| </references> | ||
| </qhelp> |
64 changes: 64 additions & 0 deletions
64
cpp/ql/src/experimental/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.ql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| /** | ||
| * @name MMIO/DMA unsanitized memory copy | ||
| * @description Memory copy sizes derived from allowlisted MMIO/DMA register-read | ||
| * macros without bounds validation may overflow destination buffers. | ||
| * @kind path-problem | ||
| * @problem.severity error | ||
| * @precision low | ||
| * @id cpp/mmio-unsanitized-memcpy | ||
| * @tags security | ||
| * experimental | ||
| * 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 `source` reads MMIO/DMA state through an allowlisted register macro. */ | ||
| predicate isMmioSource(DataFlow::Node source) { | ||
| 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<lessThanOrEqual/3>::getABarrierNode() or | ||
| node = DataFlow::BarrierGuard<lessThanOrEqual/3>::getAnIndirectBarrierNode() | ||
| } | ||
|
|
||
| predicate observeDiffInformedIncrementalMode() { any() } | ||
| } | ||
|
|
||
| module MmioFlow = TaintTracking::Global<MmioConfig>; | ||
|
|
||
| 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" | ||
9 changes: 9 additions & 0 deletions
9
cpp/ql/src/experimental/Security/CWE/CWE-120/MmioUnsanitizedMemcpyBad.c
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
| } |
10 changes: 10 additions & 0 deletions
10
cpp/ql/src/experimental/Security/CWE/CWE-120/MmioUnsanitizedMemcpyGood.c
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
| } |
28 changes: 28 additions & 0 deletions
28
...tal/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/MmioUnsanitizedMemcpy.expected
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| #select | ||
| | test.c:26:3:26:8 | call to memcpy | test.c:25:18:25:37 | * ... | test.c:26:20:26:22 | len | Memory copy size argument is derived from $@ without sufficient bounds validation. | test.c:25:18:25:37 | * ... | an MMIO/DMA hardware register read | | ||
| | test.c:31:3:31:9 | call to memmove | test.c:30:18:30:37 | * ... | test.c:31:21:31:23 | len | Memory copy size argument is derived from $@ without sufficient bounds validation. | test.c:30:18:30:37 | * ... | an MMIO/DMA hardware register read | | ||
| | test.c:36:3:36:9 | call to strncpy | test.c:35:18:35:37 | * ... | test.c:36:21:36:23 | len | Memory copy size argument is derived from $@ without sufficient bounds validation. | test.c:35:18:35:37 | * ... | an MMIO/DMA hardware register read | | ||
| | test.c:41:3:41:8 | call to memcpy | test.c:40:18:40:37 | * ... | test.c:41:20:41:22 | len | Memory copy size argument is derived from $@ without sufficient bounds validation. | test.c:40:18:40:37 | * ... | an MMIO/DMA hardware register read | | ||
| edges | ||
| | test.c:25:18:25:37 | * ... | test.c:25:18:25:37 | * ... | provenance | | | ||
| | test.c:25:18:25:37 | * ... | test.c:26:20:26:22 | len | provenance | | | ||
| | test.c:30:18:30:37 | * ... | test.c:30:18:30:37 | * ... | provenance | | | ||
| | test.c:30:18:30:37 | * ... | test.c:31:21:31:23 | len | provenance | | | ||
| | test.c:35:18:35:37 | * ... | test.c:35:18:35:37 | * ... | provenance | | | ||
| | test.c:35:18:35:37 | * ... | test.c:36:21:36:23 | len | provenance | | | ||
| | test.c:40:18:40:37 | * ... | test.c:40:18:40:37 | * ... | provenance | | | ||
| | test.c:40:18:40:37 | * ... | test.c:41:20:41:22 | len | provenance | | | ||
| nodes | ||
| | test.c:25:18:25:37 | * ... | semmle.label | * ... | | ||
| | test.c:25:18:25:37 | * ... | semmle.label | * ... | | ||
| | test.c:26:20:26:22 | len | semmle.label | len | | ||
| | test.c:30:18:30:37 | * ... | semmle.label | * ... | | ||
| | test.c:30:18:30:37 | * ... | semmle.label | * ... | | ||
| | test.c:31:21:31:23 | len | semmle.label | len | | ||
| | test.c:35:18:35:37 | * ... | semmle.label | * ... | | ||
| | test.c:35:18:35:37 | * ... | semmle.label | * ... | | ||
| | test.c:36:21:36:23 | len | semmle.label | len | | ||
| | test.c:40:18:40:37 | * ... | semmle.label | * ... | | ||
| | test.c:40:18:40:37 | * ... | semmle.label | * ... | | ||
| | test.c:41:20:41:22 | len | semmle.label | len | | ||
| subpaths |
2 changes: 2 additions & 0 deletions
2
...mental/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/MmioUnsanitizedMemcpy.qlref
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| query: experimental/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.ql | ||
| postprocess: utils/test/InlineExpectationsTestQuery.ql |
83 changes: 83 additions & 0 deletions
83
cpp/ql/test/experimental/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/test.c
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| /* Semmle test case for MmioUnsanitizedMemcpy.ql | ||
| * Allowlisted MMIO/DMA register macros 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 GET_MMIO(addr) (*(volatile uint32_t *)(addr)) | ||
| #define REG_READ(addr) (*(volatile uint32_t *)(addr)) | ||
| #define DMA_READ(addr) (*(volatile uint32_t *)(addr)) | ||
| #define MAX_DMA_LEN 64 | ||
|
|
||
| struct VolatileField { | ||
| volatile uint32_t len; | ||
| }; | ||
|
|
||
| volatile uint32_t mmio_len_reg; | ||
| struct VolatileField vf; | ||
|
|
||
| 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_reg_read(char *dst, char *src) { | ||
| uint32_t len = REG_READ(0x51000000); // $ Source | ||
| strncpy(dst, src, len); // $ Alert | ||
| } | ||
|
|
||
| static void bad_dma_read(char *dst, char *src) { | ||
| uint32_t len = DMA_READ(0x60000000); // $ Source | ||
| memcpy(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 | ||
| } | ||
|
|
||
| static void negative_volatile_global(char *dst, char *src) { | ||
| uint32_t len = mmio_len_reg; | ||
| memcpy(dst, src, len); // GOOD | ||
| } | ||
|
|
||
| static void negative_volatile_field(char *dst, char *src) { | ||
| uint32_t len = vf.len; | ||
| memcpy(dst, src, len); // GOOD | ||
| } | ||
|
|
||
| static void negative_volatile_deref(char *dst, char *src) { | ||
| volatile uint32_t *reg = (volatile uint32_t *)0x40001000; | ||
| uint32_t len = *reg; | ||
| memcpy(dst, src, len); // GOOD | ||
| } | ||
|
|
||
| static uint32_t GET_MMIO_fn(unsigned long addr); | ||
|
|
||
| static void negative_get_mmio_function(char *dst, char *src) { | ||
| uint32_t len = GET_MMIO_fn(0x50000000); | ||
| memcpy(dst, src, len); // GOOD | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because you're not using the sink as alert location (you're using
memcpyCallinstead. CI is giving the errors below. To solve this you should either use the sink as alert location, or implementgetASelectedSinkLocation(see here)