Skip to content

Commit 7d94bab

Browse files
RasmusWLRasmusWL
committed
Merge branch 'main' into django-request-handler-without-route
2 parents 828bb9a + b794fcb commit 7d94bab

File tree

211 files changed

+16708
-5260
lines changed

Some content is hidden

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

211 files changed

+16708
-5260
lines changed

‎CONTRIBUTING.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ If you have an idea for a query that you would like to share with other CodeQL u
3838

3939
- The queries and libraries must be autoformatted, for example using the "Format Document" command in [CodeQL for Visual Studio Code](https://help.semmle.com/codeql/codeql-for-vscode/procedures/about-codeql-for-vscode.html).
4040

41+
If you prefer, you can use this [pre-commit hook](misc/scripts/pre-commit) that automatically checks whether your files are correctly formatted. See the [pre-commit hook installation guide](docs/install-pre-commit-hook.md) for instructions on how to install the hook.
42+
4143
4. **Compilation**
4244

4345
- Compilation of the query and any associated libraries and tests must be resilient to future development of the [supported](docs/supported-queries.md) libraries. This means that the functionality cannot use internal libraries, cannot depend on the output of `getAQlClass`, and cannot make use of regexp matching on `toString`.

‎cpp/ql/src/Security/CWE/CWE-079/CgiXss.ql

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ class QueryString extends EnvironmentRead {
2929
}
3030

3131
class Configuration extends TaintTrackingConfiguration {
32+
override predicate isSource(Expr source) { source instanceof QueryString }
33+
3234
override predicate isSink(Element tainted) {
3335
exists(PrintStdoutCall call | call.getAnArgument() = tainted)
3436
}

‎cpp/ql/src/Security/CWE/CWE-313/CleartextSqliteDatabase.ql

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,10 @@ predicate sqlite_encryption_used() {
3434
}
3535

3636
class Configuration extends TaintTrackingConfiguration {
37+
override predicate isSource(Expr source) {
38+
super.isSource(source) and source instanceof SensitiveExpr
39+
}
40+
3741
override predicate isSink(Element taintedArg) {
3842
exists(SqliteFunctionCall sqliteCall |
3943
taintedArg = sqliteCall.getASource() and

‎cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.cpp

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
2+
image::image(int width, int height)
3+
{
4+
int x, y;
5+
6+
// allocate width * height pixels
7+
pixels = new uint32_t[width * height];
8+
9+
// fill width * height pixels
10+
for (y = 0; y < height; y++)
11+
{
12+
for (x = 0; x < width; x++)
13+
{
14+
pixels[(y * width) + height] = 0;
15+
}
16+
}
17+
18+
// ...
19+
}

‎cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.qhelp

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>The result of a multiplication is used in the size of an allocation. If the multiplication can be made to overflow, a much smaller amount of memory may be allocated than the rest of the code expects. This may lead to overflowing writes when the buffer is accessed later.</p>
8+
</overview>
9+
10+
<recommendation>
11+
<p>To fix this issue, ensure that the arithmetic used in the size of an allocation cannot overflow before memory is allocated.</p>
12+
</recommendation>
13+
14+
<example>
15+
<p>In the following example, an array of size <code>width * height</code> is allocated and stored as <code>pixels</code>. If <code>width</code> and <code>height</code> are set such that the multiplication overflows and wraps to a small value (say, 4) then the initialization code that follows the allocation will write beyond the end of the array.</p>
16+
<sample src="AllocMultiplicationOverflow.cpp"/>
17+
</example>
18+
19+
<references>
20+
<li>
21+
Cplusplus.com: <a href="http://www.cplusplus.com/articles/DE18T05o/">Integer overflow</a>.
22+
</li>
23+
</references>
24+
25+
</qhelp>

‎cpp/ql/src/experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.ql

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
/**
2+
* @name Multiplication result may overflow and be used in allocation
3+
* @description Using a multiplication result that may overflow in the size of an allocation may lead to buffer overflows when the allocated memory is used.
4+
* @kind path-problem
5+
* @problem.severity warning
6+
* @precision low
7+
* @tags security
8+
* correctness
9+
* external/cwe/cwe-190
10+
* external/cwe/cwe-128
11+
* @id cpp/multiplication-overflow-in-alloc
12+
*/
13+
14+
import cpp
15+
import semmle.code.cpp.models.interfaces.Allocation
16+
import semmle.code.cpp.dataflow.DataFlow
17+
import DataFlow::PathGraph
18+
19+
class MultToAllocConfig extends DataFlow::Configuration {
20+
MultToAllocConfig() { this = "MultToAllocConfig" }
21+
22+
override predicate isSource(DataFlow::Node node) {
23+
// a multiplication of two non-constant expressions
24+
exists(MulExpr me |
25+
me = node.asExpr() and
26+
forall(Expr e | e = me.getAnOperand() | not exists(e.getValue()))
27+
)
28+
}
29+
30+
override predicate isSink(DataFlow::Node node) {
31+
// something that affects an allocation size
32+
node.asExpr() = any(AllocationExpr ae).getSizeExpr().getAChild*()
33+
}
34+
}
35+
36+
from MultToAllocConfig config, DataFlow::PathNode source, DataFlow::PathNode sink
37+
where config.hasFlowPath(source, sink)
38+
select sink, source, sink,
39+
"Potentially overflowing value from $@ is used in the size of this allocation.", source,
40+
"multiplication"

‎cpp/ql/src/semmle/code/cpp/Function.qll

Lines changed: 34 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -391,20 +391,30 @@ class Function extends Declaration, ControlFlowNode, AccessHolder, @function {
391391
/** Holds if this function has a `noexcept` exception specification. */
392392
predicate isNoExcept() { getADeclarationEntry().isNoExcept() }
393393

394-
/** Gets a function that overloads this one. */
394+
/**
395+
* Gets a function that overloads this one.
396+
*
397+
* Note: if _overrides_ are wanted rather than _overloads_ then
398+
* `MemberFunction::getAnOverridingFunction` should be used instead.
399+
*/
395400
Function getAnOverload() {
396-
result.getName() = getName() and
397-
result.getNamespace() = getNamespace() and
398-
result != this and
399-
// If this function is declared in a class, only consider other
400-
// functions from the same class. Conversely, if this function is not
401-
// declared in a class, only consider other functions not declared in a
402-
// class.
403401
(
404-
if exists(getDeclaringType())
405-
then result.getDeclaringType() = getDeclaringType()
406-
else not exists(result.getDeclaringType())
402+
// If this function is declared in a class, only consider other
403+
// functions from the same class.
404+
exists(string name, Class declaringType |
405+
candGetAnOverloadMember(name, declaringType, this) and
406+
candGetAnOverloadMember(name, declaringType, result)
407+
)
408+
or
409+
// Conversely, if this function is not
410+
// declared in a class, only consider other functions not declared in a
411+
// class.
412+
exists(string name, Namespace namespace |
413+
candGetAnOverloadNonMember(name, namespace, this) and
414+
candGetAnOverloadNonMember(name, namespace, result)
415+
)
407416
) and
417+
result != this and
408418
// Instantiations and specializations don't participate in overload
409419
// resolution.
410420
not (
@@ -462,6 +472,19 @@ class Function extends Declaration, ControlFlowNode, AccessHolder, @function {
462472
override AccessHolder getEnclosingAccessHolder() { result = this.getDeclaringType() }
463473
}
464474

475+
pragma[noinline]
476+
private predicate candGetAnOverloadMember(string name, Class declaringType, Function f) {
477+
f.getName() = name and
478+
f.getDeclaringType() = declaringType
479+
}
480+
481+
pragma[noinline]
482+
private predicate candGetAnOverloadNonMember(string name, Namespace namespace, Function f) {
483+
f.getName() = name and
484+
f.getNamespace() = namespace and
485+
not exists(f.getDeclaringType())
486+
}
487+
465488
/**
466489
* A particular declaration or definition of a C/C++ function. For example the
467490
* declaration and definition of `MyFunction` in the following code are each a

‎cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll

Lines changed: 33 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -46,21 +46,23 @@ predicate predictableOnlyFlow(string name) {
4646

4747
private DataFlow::Node getNodeForSource(Expr source) {
4848
isUserInput(source, _) and
49-
(
50-
result = DataFlow::exprNode(source)
51-
or
52-
// Some of the sources in `isUserInput` are intended to match the value of
53-
// an expression, while others (those modeled below) are intended to match
54-
// the taint that propagates out of an argument, like the `char *` argument
55-
// to `gets`. It's impossible here to tell which is which, but the "access
56-
// to argv" source is definitely not intended to match an output argument,
57-
// and it causes false positives if we let it.
58-
//
59-
// This case goes together with the similar (but not identical) rule in
60-
// `nodeIsBarrierIn`.
61-
result = DataFlow::definitionByReferenceNodeFromArgument(source) and
62-
not argv(source.(VariableAccess).getTarget())
63-
)
49+
result = getNodeForExpr(source)
50+
}
51+
52+
private DataFlow::Node getNodeForExpr(Expr node) {
53+
result = DataFlow::exprNode(node)
54+
or
55+
// Some of the sources in `isUserInput` are intended to match the value of
56+
// an expression, while others (those modeled below) are intended to match
57+
// the taint that propagates out of an argument, like the `char *` argument
58+
// to `gets`. It's impossible here to tell which is which, but the "access
59+
// to argv" source is definitely not intended to match an output argument,
60+
// and it causes false positives if we let it.
61+
//
62+
// This case goes together with the similar (but not identical) rule in
63+
// `nodeIsBarrierIn`.
64+
result = DataFlow::definitionByReferenceNodeFromArgument(node) and
65+
not argv(node.(VariableAccess).getTarget())
6466
}
6567

6668
private class DefaultTaintTrackingCfg extends DataFlow::Configuration {
@@ -537,6 +539,9 @@ module TaintedWithPath {
537539
* a characteristic predicate.
538540
*/
539541
class TaintTrackingConfiguration extends TSingleton {
542+
/** Override this to specify which elements are sources in this configuration. */
543+
predicate isSource(Expr source) { exists(getNodeForSource(source)) }
544+
540545
/** Override this to specify which elements are sinks in this configuration. */
541546
abstract predicate isSink(Element e);
542547

@@ -553,7 +558,11 @@ module TaintedWithPath {
553558
private class AdjustedConfiguration extends DataFlow3::Configuration {
554559
AdjustedConfiguration() { this = "AdjustedConfiguration" }
555560

556-
override predicate isSource(DataFlow::Node source) { source = getNodeForSource(_) }
561+
override predicate isSource(DataFlow::Node source) {
562+
exists(TaintTrackingConfiguration cfg, Expr e |
563+
cfg.isSource(e) and source = getNodeForExpr(e)
564+
)
565+
}
557566

558567
override predicate isSink(DataFlow::Node sink) {
559568
exists(TaintTrackingConfiguration cfg | cfg.isSink(adjustedSink(sink)))
@@ -596,7 +605,8 @@ module TaintedWithPath {
596605
exists(AdjustedConfiguration cfg, DataFlow3::Node sourceNode, DataFlow3::Node sinkNode |
597606
cfg.hasFlow(sourceNode, sinkNode)
598607
|
599-
sourceNode = getNodeForSource(e)
608+
sourceNode = getNodeForExpr(e) and
609+
exists(TaintTrackingConfiguration ttCfg | ttCfg.isSource(e))
600610
or
601611
e = adjustedSink(sinkNode) and
602612
exists(TaintTrackingConfiguration ttCfg | ttCfg.isSink(e))
@@ -650,7 +660,7 @@ module TaintedWithPath {
650660

651661
/** A PathNode whose `Element` is a source. It may also be a sink. */
652662
private class InitialPathNode extends EndpointPathNode {
653-
InitialPathNode() { exists(getNodeForSource(this.inner())) }
663+
InitialPathNode() { exists(TaintTrackingConfiguration cfg | cfg.isSource(this.inner())) }
654664
}
655665

656666
/** A PathNode whose `Element` is a sink. It may also be a source. */
@@ -672,14 +682,14 @@ module TaintedWithPath {
672682
// Same for the first node
673683
exists(WrapPathNode sourceNode |
674684
DataFlow3::PathGraph::edges(sourceNode.inner(), b.(WrapPathNode).inner()) and
675-
sourceNode.inner().getNode() = getNodeForSource(a.(InitialPathNode).inner())
685+
sourceNode.inner().getNode() = getNodeForExpr(a.(InitialPathNode).inner())
676686
)
677687
or
678688
// Finally, handle the case where the path goes directly from a source to a
679689
// sink, meaning that they both need to be translated.
680690
exists(WrapPathNode sinkNode, WrapPathNode sourceNode |
681691
DataFlow3::PathGraph::edges(sourceNode.inner(), sinkNode.inner()) and
682-
sourceNode.inner().getNode() = getNodeForSource(a.(InitialPathNode).inner()) and
692+
sourceNode.inner().getNode() = getNodeForExpr(a.(InitialPathNode).inner()) and
683693
b.(FinalPathNode).inner() = adjustedSink(sinkNode.inner().getNode())
684694
)
685695
}
@@ -702,7 +712,7 @@ module TaintedWithPath {
702712
predicate taintedWithPath(Expr source, Element tainted, PathNode sourceNode, PathNode sinkNode) {
703713
exists(AdjustedConfiguration cfg, DataFlow3::Node flowSource, DataFlow3::Node flowSink |
704714
source = sourceNode.(InitialPathNode).inner() and
705-
flowSource = getNodeForSource(source) and
715+
flowSource = getNodeForExpr(source) and
706716
cfg.hasFlow(flowSource, flowSink) and
707717
tainted = adjustedSink(flowSink) and
708718
tainted = sinkNode.(FinalPathNode).inner()
@@ -724,8 +734,8 @@ module TaintedWithPath {
724734
* through a global variable.
725735
*/
726736
predicate taintedWithoutGlobals(Element tainted) {
727-
exists(PathNode sourceNode, FinalPathNode sinkNode |
728-
sourceNode.(WrapPathNode).inner().getNode() = getNodeForSource(_) and
737+
exists(AdjustedConfiguration cfg, PathNode sourceNode, FinalPathNode sinkNode |
738+
cfg.isSource(sourceNode.(WrapPathNode).inner().getNode()) and
729739
edgesWithoutGlobals+(sourceNode, sinkNode) and
730740
tainted = sinkNode.inner()
731741
)

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/AllocMultiplicationOverflow/AllocMultiplicationOverflow.expected

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
edges
2+
| test.cpp:22:17:22:21 | ... * ... | test.cpp:23:33:23:37 | size1 |
3+
nodes
4+
| test.cpp:13:33:13:37 | ... * ... | semmle.label | ... * ... |
5+
| test.cpp:15:31:15:35 | ... * ... | semmle.label | ... * ... |
6+
| test.cpp:19:34:19:38 | ... * ... | semmle.label | ... * ... |
7+
| test.cpp:22:17:22:21 | ... * ... | semmle.label | ... * ... |
8+
| test.cpp:23:33:23:37 | size1 | semmle.label | size1 |
9+
| test.cpp:30:27:30:31 | ... * ... | semmle.label | ... * ... |
10+
| test.cpp:31:27:31:31 | ... * ... | semmle.label | ... * ... |
11+
#select
12+
| test.cpp:13:33:13:37 | ... * ... | test.cpp:13:33:13:37 | ... * ... | test.cpp:13:33:13:37 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:13:33:13:37 | ... * ... | multiplication |
13+
| test.cpp:15:31:15:35 | ... * ... | test.cpp:15:31:15:35 | ... * ... | test.cpp:15:31:15:35 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:15:31:15:35 | ... * ... | multiplication |
14+
| test.cpp:19:34:19:38 | ... * ... | test.cpp:19:34:19:38 | ... * ... | test.cpp:19:34:19:38 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:19:34:19:38 | ... * ... | multiplication |
15+
| test.cpp:23:33:23:37 | size1 | test.cpp:22:17:22:21 | ... * ... | test.cpp:23:33:23:37 | size1 | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:22:17:22:21 | ... * ... | multiplication |
16+
| test.cpp:30:27:30:31 | ... * ... | test.cpp:30:27:30:31 | ... * ... | test.cpp:30:27:30:31 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:30:27:30:31 | ... * ... | multiplication |
17+
| test.cpp:31:27:31:31 | ... * ... | test.cpp:31:27:31:31 | ... * ... | test.cpp:31:27:31:31 | ... * ... | Potentially overflowing value from $@ is used in the size of this allocation. | test.cpp:31:27:31:31 | ... * ... | multiplication |

‎cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/AllocMultiplicationOverflow/AllocMultiplicationOverflow.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
experimental/Security/CWE/CWE-190/AllocMultiplicationOverflow.ql

0 commit comments

Comments
 (0)