Skip to content

Commit 7a7ea7c

Browse files
redsun82redsun82
authored
Merge branch 'main' into redsun82-distrusted-actions-owner-override
2 parents 8a67fab + 39547c7 commit 7a7ea7c

155 files changed

Lines changed: 2453 additions & 2113 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎cpp/ql/integration-tests/query-suite/not_included_in_qls.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,7 @@ ql/cpp/ql/src/experimental/Security/CWE/CWE-078/WordexpTainted.ql
242242
ql/cpp/ql/src/experimental/Security/CWE/CWE-1041/FindWrapperFunctions.ql
243243
ql/cpp/ql/src/experimental/Security/CWE/CWE-1126/DeclarationOfVariableWithUnnecessarilyWideScope.ql
244244
ql/cpp/ql/src/experimental/Security/CWE/CWE-120/MemoryUnsafeFunctionScan.ql
245+
ql/cpp/ql/src/experimental/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.ql
245246
ql/cpp/ql/src/experimental/Security/CWE/CWE-1240/CustomCryptographicPrimitive.ql
246247
ql/cpp/ql/src/experimental/Security/CWE/CWE-125/DangerousWorksWithMultibyteOrWideCharacters.ql
247248
ql/cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.ql

‎cpp/ql/src/experimental/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.qhelp‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
Firmware and embedded drivers often copy data into buffers using lengths read from
8+
allowlisted MMIO register macros such as <code>READ_REG</code> or <code>GET_MMIO</code>.
9+
When those lengths are not validated against the destination buffer size, an attacker who
10+
can influence hardware registers or DMA metadata can trigger buffer overflows.
11+
</p>
12+
</overview>
13+
<recommendation>
14+
<p>
15+
Always validate MMIO/DMA-derived lengths before passing them to <code>memcpy</code>,
16+
<code>memmove</code>, or <code>strncpy</code>. Compare against a compile-time maximum
17+
and reject or clamp out-of-range values before copying.
18+
</p>
19+
</recommendation>
20+
<example>
21+
<p>Bad: length from an MMIO register used directly as the copy size.</p>
22+
<sample src="MmioUnsanitizedMemcpyBad.c" />
23+
<p>Good: defensive bounds check before the copy.</p>
24+
<sample src="MmioUnsanitizedMemcpyGood.c" />
25+
</example>
26+
<references>
27+
</references>
28+
</qhelp>

‎cpp/ql/src/experimental/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.ql‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
/**
2+
* @name MMIO/DMA unsanitized memory copy
3+
* @description Memory copy sizes derived from allowlisted MMIO/DMA register-read
4+
* macros without bounds validation may overflow destination buffers.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @precision low
8+
* @id cpp/mmio-unsanitized-memcpy
9+
* @tags security
10+
* experimental
11+
* external/cwe/cwe-120
12+
* external/cwe/cwe-787
13+
*/
14+
15+
import cpp
16+
import semmle.code.cpp.dataflow.new.TaintTracking
17+
import semmle.code.cpp.controlflow.IRGuards
18+
import MmioFlow::PathGraph
19+
20+
/** Holds if `source` reads MMIO/DMA state through an allowlisted register macro. */
21+
predicate isMmioSource(DataFlow::Node source) {
22+
exists(MacroInvocation mi |
23+
mi.getMacro().hasName(["READ_REG", "GET_MMIO", "REG_READ", "DMA_READ"]) and
24+
source.asExpr() = mi.getExpr()
25+
)
26+
}
27+
28+
predicate isMemcpySizeSink(DataFlow::Node sink, FunctionCall fc) {
29+
fc.getTarget().hasName(["memcpy", "memmove", "strncpy", "wmemcpy", "wmemmove"]) and
30+
sink.asExpr() = fc.getArgument(2)
31+
}
32+
33+
/** Recognizes relational comparison bounds checks using public IRGuards API. */
34+
predicate lessThanOrEqual(IRGuardCondition g, Expr e, boolean branch) {
35+
exists(Operand left |
36+
g.comparesLt(left, _, _, true, branch) or
37+
g.comparesEq(left, _, _, true, branch)
38+
|
39+
left.getDef().getConvertedResultExpression() = e
40+
)
41+
}
42+
43+
module MmioConfig implements DataFlow::ConfigSig {
44+
predicate isSource(DataFlow::Node source) { isMmioSource(source) }
45+
46+
predicate isSink(DataFlow::Node sink) { isMemcpySizeSink(sink, _) }
47+
48+
predicate isBarrier(DataFlow::Node node) {
49+
node = DataFlow::BarrierGuard<lessThanOrEqual/3>::getABarrierNode() or
50+
node = DataFlow::BarrierGuard<lessThanOrEqual/3>::getAnIndirectBarrierNode()
51+
}
52+
53+
predicate observeDiffInformedIncrementalMode() { any() }
54+
}
55+
56+
module MmioFlow = TaintTracking::Global<MmioConfig>;
57+
58+
from FunctionCall memcpyCall, MmioFlow::PathNode source, MmioFlow::PathNode sink
59+
where
60+
MmioFlow::flowPath(source, sink) and
61+
isMemcpySizeSink(sink.getNode(), memcpyCall)
62+
select sink.getNode(), source, sink,
63+
"Memory copy size argument is derived from $@ without sufficient bounds validation.",
64+
source.getNode(), "an MMIO/DMA hardware register read"

‎cpp/ql/src/experimental/Security/CWE/CWE-120/MmioUnsanitizedMemcpyBad.c‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
#define READ_REG(addr) (*(volatile unsigned int *)(addr))
2+
#define MAX_DMA_LEN 64
3+
4+
void *memcpy(void *dest, const void *src, unsigned long n);
5+
6+
void bad_mmio_memcpy(char *dst, char *src) {
7+
unsigned int len = READ_REG(0x40001000);
8+
memcpy(dst, src, len);
9+
}

‎cpp/ql/src/experimental/Security/CWE/CWE-120/MmioUnsanitizedMemcpyGood.c‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
#define READ_REG(addr) (*(volatile unsigned int *)(addr))
2+
#define MAX_DMA_LEN 64
3+
4+
void *memcpy(void *dest, const void *src, unsigned long n);
5+
6+
void good_mmio_memcpy(char *dst, char *src) {
7+
unsigned int len = READ_REG(0x40001000);
8+
if (len <= MAX_DMA_LEN)
9+
memcpy(dst, src, len);
10+
}

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/MmioUnsanitizedMemcpy.expected‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
#select
2+
| test.c:26:20:26:22 | len | 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 |
3+
| test.c:31:21:31:23 | len | 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 |
4+
| test.c:36:21:36:23 | len | 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 |
5+
| test.c:41:20:41:22 | len | 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 |
6+
edges
7+
| test.c:25:18:25:37 | * ... | test.c:25:18:25:37 | * ... | provenance | |
8+
| test.c:25:18:25:37 | * ... | test.c:26:20:26:22 | len | provenance | |
9+
| test.c:30:18:30:37 | * ... | test.c:30:18:30:37 | * ... | provenance | |
10+
| test.c:30:18:30:37 | * ... | test.c:31:21:31:23 | len | provenance | |
11+
| test.c:35:18:35:37 | * ... | test.c:35:18:35:37 | * ... | provenance | |
12+
| test.c:35:18:35:37 | * ... | test.c:36:21:36:23 | len | provenance | |
13+
| test.c:40:18:40:37 | * ... | test.c:40:18:40:37 | * ... | provenance | |
14+
| test.c:40:18:40:37 | * ... | test.c:41:20:41:22 | len | provenance | |
15+
nodes
16+
| test.c:25:18:25:37 | * ... | semmle.label | * ... |
17+
| test.c:25:18:25:37 | * ... | semmle.label | * ... |
18+
| test.c:26:20:26:22 | len | semmle.label | len |
19+
| test.c:30:18:30:37 | * ... | semmle.label | * ... |
20+
| test.c:30:18:30:37 | * ... | semmle.label | * ... |
21+
| test.c:31:21:31:23 | len | semmle.label | len |
22+
| test.c:35:18:35:37 | * ... | semmle.label | * ... |
23+
| test.c:35:18:35:37 | * ... | semmle.label | * ... |
24+
| test.c:36:21:36:23 | len | semmle.label | len |
25+
| test.c:40:18:40:37 | * ... | semmle.label | * ... |
26+
| test.c:40:18:40:37 | * ... | semmle.label | * ... |
27+
| test.c:41:20:41:22 | len | semmle.label | len |
28+
subpaths

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/MmioUnsanitizedMemcpy.qlref‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
query: experimental/Security/CWE/CWE-120/MmioUnsanitizedMemcpy.ql
2+
postprocess: utils/test/InlineExpectationsTestQuery.ql

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-120/MmioUnsanitizedMemcpy/test.c‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
/* Test case for MmioUnsanitizedMemcpy.ql
2+
* Allowlisted MMIO/DMA register macros flowing into memcpy/memmove/strncpy size parameters.
3+
*/
4+
5+
typedef unsigned int uint32_t;
6+
7+
void *memcpy(void *dest, const void *src, unsigned long n);
8+
void *memmove(void *dest, const void *src, unsigned long n);
9+
char *strncpy(char *dest, const char *src, unsigned long n);
10+
11+
#define READ_REG(addr) (*(volatile uint32_t *)(addr))
12+
#define GET_MMIO(addr) (*(volatile uint32_t *)(addr))
13+
#define REG_READ(addr) (*(volatile uint32_t *)(addr))
14+
#define DMA_READ(addr) (*(volatile uint32_t *)(addr))
15+
#define MAX_DMA_LEN 64
16+
17+
struct VolatileField {
18+
volatile uint32_t len;
19+
};
20+
21+
volatile uint32_t mmio_len_reg;
22+
struct VolatileField vf;
23+
24+
static void bad_read_reg(char *dst, char *src) {
25+
uint32_t len = READ_REG(0x40001000); // $ Source
26+
memcpy(dst, src, len); // $ Alert
27+
}
28+
29+
static void bad_get_mmio(char *dst, char *src) {
30+
uint32_t len = GET_MMIO(0x50000000); // $ Source
31+
memmove(dst, src, len); // $ Alert
32+
}
33+
34+
static void bad_reg_read(char *dst, char *src) {
35+
uint32_t len = REG_READ(0x51000000); // $ Source
36+
strncpy(dst, src, len); // $ Alert
37+
}
38+
39+
static void bad_dma_read(char *dst, char *src) {
40+
uint32_t len = DMA_READ(0x60000000); // $ Source
41+
memcpy(dst, src, len); // $ Alert
42+
}
43+
44+
static void good_bounded(char *dst, char *src) {
45+
uint32_t len = READ_REG(0x40001000);
46+
if (len <= MAX_DMA_LEN)
47+
memcpy(dst, src, len); // GOOD
48+
}
49+
50+
static void good_early_return(char *dst, char *src) {
51+
uint32_t len = DMA_READ(0x60000000);
52+
if (len > MAX_DMA_LEN)
53+
return;
54+
memcpy(dst, src, len); // GOOD
55+
}
56+
57+
static void good_constant_size(char *dst, char *src) {
58+
uint32_t len = READ_REG(0x40001000);
59+
memcpy(dst, src, 32); // GOOD
60+
}
61+
62+
static void negative_volatile_global(char *dst, char *src) {
63+
uint32_t len = mmio_len_reg;
64+
memcpy(dst, src, len); // GOOD
65+
}
66+
67+
static void negative_volatile_field(char *dst, char *src) {
68+
uint32_t len = vf.len;
69+
memcpy(dst, src, len); // GOOD
70+
}
71+
72+
static void negative_volatile_deref(char *dst, char *src) {
73+
volatile uint32_t *reg = (volatile uint32_t *)0x40001000;
74+
uint32_t len = *reg;
75+
memcpy(dst, src, len); // GOOD
76+
}
77+
78+
static uint32_t GET_MMIO_fn(unsigned long addr);
79+
80+
static void negative_get_mmio_function(char *dst, char *src) {
81+
uint32_t len = GET_MMIO_fn(0x50000000);
82+
memcpy(dst, src, len); // GOOD
83+
}

‎shared/yeast-macros/src/lib.rs‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -131,8 +131,6 @@ pub fn trees(input: TokenStream) -> TokenStream {
131131
/// (output_template)
132132
/// )
133133
///
134-
/// // Shorthand: captures become fields on the output node
135-
/// rule!((query ...) => output_kind)
136134
/// ```
137135
///
138136
/// Captures become Rust variables automatically:

‎shared/yeast-macros/src/parse.rs‎

Lines changed: 3 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -768,8 +768,7 @@ fn extract_captures_inner(
768768
/// ```
769769
///
770770
/// Template bodies (`=> (kind …)`) never carry an annotation — the
771-
/// output kind is the template root. The shorthand `=> kind` (no
772-
/// body) also carries no annotation. See `parse_rule_top` for dispatch.
771+
/// output kind is the template root.
773772
#[derive(Clone, Debug)]
774773
struct ReturnAnnotation {
775774
kind: Ident,
@@ -793,7 +792,6 @@ enum AnnotationMultiplicity {
793792
/// `kind {` → annotation (single)
794793
/// `kind? {` → annotation (optional)
795794
/// `kind* {` → annotation (repeated)
796-
/// `kind` → shorthand form (no `{` follows) — NOT an annotation
797795
/// anything else → template or bare block — NOT an annotation
798796
fn try_consume_return_annotation(tokens: &mut Tokens) -> Result<Option<ReturnAnnotation>> {
799797
// Must start with an identifier (the kind name).
@@ -881,15 +879,13 @@ pub fn parse_rule_top(input: TokenStream) -> Result<TokenStream> {
881879
let raw_bindings = capture_bindings(raw_captures.into_iter());
882880
let translated_bindings = capture_bindings(translated_captures.into_iter());
883881

884-
// Parse transform: the token(s) after `=>` fall into one of three
882+
// Parse transform: the token(s) after `=>` fall into one of two
885883
// shapes, dispatched in order:
886884
//
887885
// 1. `kind [? | *] { rust_body }` — annotated Rust body (NEW).
888886
// Static-analysis-ready: the annotation declares the output
889887
// kind and multiplicity in the schema's own vocabulary.
890-
// 2. `kind` alone — shorthand: emit `(kind field: {@cap})…` from
891-
// the query's captures.
892-
// 3. anything else — full template form (`(kind …)` or bare
888+
// 2. anything else — full template form (`(kind …)` or bare
893889
// `{ … }` splice via `parse_direct_list`).
894890
let annotation = try_consume_return_annotation(&mut tokens)?;
895891

@@ -926,65 +922,6 @@ pub fn parse_rule_top(input: TokenStream) -> Result<TokenStream> {
926922
yeast::IntoFieldIds::extend_into(__value, &mut __ids);
927923
__ids
928924
}
929-
} else if peek_is_field(&mut tokens) && {
930-
// Shorthand form: bare identifier = output node kind.
931-
// Auto-generate template from captures.
932-
let mut lookahead = tokens.clone();
933-
lookahead.next(); // skip ident
934-
lookahead.peek().is_none() // nothing after = shorthand
935-
} {
936-
let output_kind = expect_ident(&mut tokens, "expected output node kind")?;
937-
let output_kind_str = output_kind.to_string();
938-
939-
// Generate field assignments from captures
940-
let field_stmts: Vec<TokenStream> = captures
941-
.iter()
942-
.map(|cap| {
943-
let name = Ident::new(&cap.name, Span::call_site());
944-
let name_str = &cap.name;
945-
match cap.multiplicity {
946-
CaptureMultiplicity::Repeated => quote! {
947-
let __field_id = #ctx_ident.ast.field_id_for_name(#name_str)
948-
.unwrap_or_else(|| panic!("field '{}' not found", #name_str));
949-
__fields.insert(
950-
__field_id,
951-
#name.into_iter()
952-
.map(::std::convert::Into::<yeast::Id>::into)
953-
.collect(),
954-
);
955-
},
956-
CaptureMultiplicity::Optional => quote! {
957-
let __field_id = #ctx_ident.ast.field_id_for_name(#name_str)
958-
.unwrap_or_else(|| panic!("field '{}' not found", #name_str));
959-
if let Some(__id) = #name {
960-
__fields.entry(__field_id).or_insert_with(Vec::new)
961-
.push(::std::convert::Into::<yeast::Id>::into(__id));
962-
}
963-
},
964-
CaptureMultiplicity::Single => quote! {
965-
let __field_id = #ctx_ident.ast.field_id_for_name(#name_str)
966-
.unwrap_or_else(|| panic!("field '{}' not found", #name_str));
967-
__fields.entry(__field_id).or_insert_with(Vec::new)
968-
.push(::std::convert::Into::<yeast::Id>::into(#name));
969-
},
970-
}
971-
})
972-
.collect();
973-
974-
quote! {
975-
let __kind = #ctx_ident.ast.id_for_node_kind(#output_kind_str)
976-
.unwrap_or_else(|| panic!("node kind '{}' not found", #output_kind_str));
977-
let mut __fields = std::collections::BTreeMap::new();
978-
#(#field_stmts)*
979-
let __id = #ctx_ident.ast.create_node_with_range(
980-
__kind,
981-
yeast::NodeContent::DynamicString(String::new()),
982-
__fields,
983-
true,
984-
__source_range,
985-
);
986-
vec![__id]
987-
}
988925
} else {
989926
// Reject bare `{ ... }` transforms — they used to be accepted
990927
// as either a Rust body producing a `Vec<Id>` or a template
@@ -1482,8 +1419,5 @@ mod rules_tests {
14821419
// Match expressions inside a block: `=>` is inside braces.
14831420
let toks = quote! { { match x { 1 => 2, _ => 3 } } };
14841421
assert!(!has_top_level_arrow(&toks));
1485-
// Bare shorthand form: top-level `=>` followed by a bare ident.
1486-
let toks = quote! { (a) => kind };
1487-
assert!(has_top_level_arrow(&toks));
14881422
}
14891423
}

0 commit comments

Comments
 (0)