hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-05-02 04:05:14 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge remote-tracking branch 'origin/main' into jorgectf/python/ldapinsecureauth

Browse Source
This commit is contained in:
jorgectf
2021-09-12 20:36:25 +02:00
parent 18b05bc56e 0e9d36b922
commit 3cf28ad6ce
7 changed files with 141 additions and 104 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
cpp/change-notes/2021-07-20-toctou-race-condition.md Normal file
View File

@@ -0,0 +1,2 @@
lgtm,codescanning
* Improvements have been made to the `cpp/toctou-race-condition` query, both to find more correct results and fewer false positive results.

86
cpp/ql/src/Security/CWE/CWE-367/TOCTOUFilesystemRace.ql
View File

@@ -26,28 +26,29 @@ import semmle.code.cpp.controlflow.Guards
*/
FunctionCall filenameOperation(Expr path) {
exists(string name | name = result.getTarget().getName() |
(
name = "remove" or
name = "unlink" or
name = "rmdir" or
name = "rename" or
name = "chmod" or
name = "chown" or
name = "fopen" or
name = "open" or
name = "freopen" or
name = "_open" or
name = "_wopen" or
name = "_wfopen"
) and
name =
[
"remove", "unlink", "rmdir", "rename", "fopen", "open", "freopen", "_open", "_wopen",
"_wfopen", "_fsopen", "_wfsopen"
] and
result.getArgument(0) = path
or
(
name = "fopen_s" or
name = "wfopen_s"
) and
name = ["fopen_s", "wfopen_s", "rename"] and
result.getArgument(1) = path
)
or
result = sensitiveFilenameOperation(path)
}
/**
* An operation on a filename that is likely to modify the security properties
* of the corresponding file and may return an indication of success.
*/
FunctionCall sensitiveFilenameOperation(Expr path) {
exists(string name | name = result.getTarget().getName() |
name = ["chmod", "chown"] and
result.getArgument(0) = path
)
}
/**
@@ -56,11 +57,7 @@ FunctionCall filenameOperation(Expr path) {
*/
FunctionCall accessCheck(Expr path) {
exists(string name | name = result.getTarget().getName() |
name = "access" or
name = "_access" or
name = "_waccess" or
name = "_access_s" or
name = "_waccess_s"
name = ["access", "_access", "_waccess", "_access_s", "_waccess_s"]
) and
path = result.getArgument(0)
}
@@ -72,9 +69,7 @@ FunctionCall accessCheck(Expr path) {
*/
FunctionCall stat(Expr path, Expr buf) {
exists(string name | name = result.getTarget().getName() |
name = "stat" or
name = "lstat" or
name = "fstat" or
name = ["stat", "lstat", "fstat"] or
name.matches("\\_stat%") or
name.matches("\\_wstat%")
) and
@@ -98,29 +93,36 @@ from Expr check, Expr checkPath, FunctionCall use, Expr usePath
where
// `check` looks like a check on a filename
(
// either:
// an access check
check = accessCheck(checkPath)
or
// a stat
check = stat(checkPath, _)
(
// either:
// an access check
check = accessCheck(checkPath)
or
// a stat
check = stat(checkPath, _)
or
// access to a member variable on the stat buf
// (morally, this should be a use-use pair, but it seems unlikely
// that this variable will get reused in practice)
exists(Expr call, Expr e, Variable v |
call = stat(checkPath, e) and
e.getAChild*().(VariableAccess).getTarget() = v and
check.(VariableAccess).getTarget() = v and
not e.getAChild*() = check // the call that writes to the pointer is not where the pointer is checked.
)
) and
// `op` looks like an operation on a filename
use = filenameOperation(usePath)
or
// another filename operation (null pointers can indicate errors)
check = filenameOperation(checkPath)
or
// access to a member variable on the stat buf
// (morally, this should be a use-use pair, but it seems unlikely
// that this variable will get reused in practice)
exists(Variable buf | exists(stat(checkPath, buf.getAnAccess())) |
check.(VariableAccess).getQualifier() = buf.getAnAccess()
)
check = filenameOperation(checkPath) and
// `op` looks like a sensitive operation on a filename
use = sensitiveFilenameOperation(usePath)
) and
// `checkPath` and `usePath` refer to the same SSA variable
exists(SsaDefinition def, StackVariable v |
def.getAUse(v) = checkPath and def.getAUse(v) = usePath
) and
// `op` looks like an operation on a filename
use = filenameOperation(usePath) and
// the return value of `check` is used (possibly with one step of
// variable indirection) in a guard which controls `use`
exists(GuardCondition guard | referenceTo(check, guard.getAChild*()) |

13
cpp/ql/test/query-tests/Security/CWE/CWE-367/semmle/TOCTOUFilesystemRace.expected
View File

@@ -1,14 +1,13 @@
| test2.cpp:39:7:39:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:39:13:39:16 | path | filename | test2.cpp:34:6:34:10 | call to fopen | checked |
| test2.cpp:52:7:52:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:52:13:52:16 | path | filename | test2.cpp:52:7:52:11 | call to fopen | checked |
| test2.cpp:69:7:69:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:69:13:69:16 | path | filename | test2.cpp:67:6:67:9 | call to stat | checked |
| test2.cpp:98:7:98:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:98:13:98:16 | path | filename | test2.cpp:96:15:96:17 | foo | checked |
| test2.cpp:83:7:83:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:83:13:83:16 | path | filename | test2.cpp:81:6:81:8 | buf | checked |
| test2.cpp:98:7:98:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:98:13:98:16 | path | filename | test2.cpp:96:6:96:12 | buf_ptr | checked |
| test2.cpp:115:7:115:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:115:13:115:16 | path | filename | test2.cpp:113:22:113:24 | buf | checked |
| test2.cpp:130:7:130:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:130:13:130:16 | path | filename | test2.cpp:128:21:128:27 | buf_ptr | checked |
| test2.cpp:157:7:157:10 | call to open | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:157:12:157:15 | path | filename | test2.cpp:155:6:155:9 | call to stat | checked |
| test2.cpp:170:7:170:10 | call to open | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:170:12:170:15 | path | filename | test2.cpp:168:6:168:10 | call to lstat | checked |
| test2.cpp:245:3:245:7 | call to chmod | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:245:9:245:12 | path | filename | test2.cpp:238:6:238:10 | call to fopen | checked |
| test2.cpp:255:3:255:8 | call to remove | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:255:10:255:14 | path1 | filename | test2.cpp:253:6:253:11 | call to rename | checked |
| test2.cpp:277:7:277:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:277:13:277:16 | path | filename | test2.cpp:275:6:275:11 | call to access | checked |
| test2.cpp:303:7:303:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:303:13:303:16 | path | filename | test2.cpp:301:7:301:12 | call to access | checked |
| test2.cpp:317:7:317:11 | call to fopen | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:317:13:317:16 | path | filename | test2.cpp:313:6:313:11 | call to access | checked |
| test.cpp:21:3:21:8 | call to remove | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test.cpp:21:10:21:14 | file1 | filename | test.cpp:19:7:19:12 | call to rename | checked |
| test.cpp:35:3:35:8 | call to remove | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test.cpp:35:10:35:14 | file1 | filename | test.cpp:32:7:32:12 | call to rename | checked |
| test.cpp:49:3:49:8 | call to remove | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test.cpp:49:10:49:14 | file1 | filename | test.cpp:47:7:47:12 | call to rename | checked |
| test2.cpp:348:3:348:7 | call to chmod | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:348:9:348:12 | path | filename | test2.cpp:341:6:341:10 | call to fopen | checked |
| test2.cpp:356:3:356:7 | call to chmod | The $@ being operated upon was previously $@, but the underlying file may have been changed since then. | test2.cpp:356:9:356:13 | path2 | filename | test2.cpp:354:7:354:12 | call to rename | checked |

6
cpp/ql/test/query-tests/Security/CWE/CWE-367/semmle/test.cpp
View File

@@ -18,7 +18,7 @@ void test1()
create(file1);
if (!rename(file1, file2))
{
remove(file1); // DUBIOUS (bad but perhaps not exploitable) [REPORTED]
remove(file1); // DUBIOUS (bad but perhaps not exploitable)
}
}
@@ -32,7 +32,7 @@ void test2()
if (!rename(file1, file2))
{
file1.set("d.txt");
remove(file1); // GOOD [FALSE POSITIVE]
remove(file1); // GOOD
}
}
@@ -46,6 +46,6 @@ void test3()
create(file1);
if (!rename(file1, file2))
{
remove(file1); // DUBIOUS (bad but perhaps not exploitable) [REPORTED]
remove(file1); // DUBIOUS (bad but perhaps not exploitable)
}
}

39
cpp/ql/test/query-tests/Security/CWE/CWE-367/semmle/test2.cpp
View File

@@ -36,7 +36,7 @@ void test1_1(const char *path)
if (f == NULL)
{
// retry
f = fopen(path, "r"); // GOOD (this is just trying again) [FALSE POSITIVE]
f = fopen(path, "r"); // GOOD (this is just trying again)
}
// ...
@@ -49,7 +49,7 @@ void test1_2(const char *path)
// try until we succeed
while (f == NULL)
{
f = fopen(path, "r"); // GOOD (this is just trying again) [FALSE POSITIVE]
f = fopen(path, "r"); // GOOD (this is just trying again)
// ...
}
@@ -80,7 +80,7 @@ void test2_2(const char *path)
stat(path, &buf);
if (buf.foo > 0)
{
f = fopen(path, "r"); // BAD [NOT DETECTED]
f = fopen(path, "r"); // BAD
}
// ...
@@ -112,7 +112,7 @@ void test2_4(const char *path)
stat(path, &buf);
if (stat_condition(&buf))
{
f = fopen(path, "r"); // BAD [NOT DETECTED]
f = fopen(path, "r"); // BAD
}
// ...
@@ -127,7 +127,7 @@ void test2_5(const char *path)
stat(path, buf_ptr);
if (stat_condition(buf_ptr))
{
f = fopen(path, "r"); // BAD [NOT DETECTED]
f = fopen(path, "r"); // BAD
}
// ...
@@ -242,7 +242,7 @@ void test4_1(const char *path)
fclose(f);
chmod(path, 0); // DUBIOUS (bad but perhaps not exploitable) [REPORTED]
chmod(path, 0); // BAD
}
}
@@ -252,7 +252,7 @@ void test5_1(const char *path1, const char *path2)
{
if (rename(path1, path2))
{
remove(path1); // DUBIOUS (bad but perhaps not exploitable) [REPORTED]
remove(path1); // DUBIOUS (bad but perhaps not exploitable)
}
}
@@ -331,3 +331,28 @@ void test6_5(const char *path1, const char *path2)
// ...
}
}
// --- open / rename -> chmod ---
void test7_1(const char *path)
{
FILE *f;
f = fopen(path, "wt");
if (f != 0)
{
// ...
fclose(f);
chmod(path, 1234); // BAD
}
}
void test7_1(const char *path1, const char *path2)
{
if (!rename(path1, path2))
{
chmod(path2, 1234); // BAD
}
}

3
python/change-notes/2021-07-12-add-typetrackingnode.md
View File

@@ -1,3 +1,2 @@
lgtm,codescanning
* The `track` and `backtrack` methods on `LocalSourceNode` have been deprecated. When writing
type trackers, the corresponding methods on `TypeTrackingNode` should be used instead.
* The `track` and `backtrack` methods on `LocalSourceNode` are in the process of being deprecated. When using type trackers, the corresponding methods on `TypeTrackingNode` should be used instead.

96
python/ql/src/semmle/python/dataflow/new/internal/LocalSources.qll
View File

@@ -104,26 +104,20 @@ class LocalSourceNode extends Node {
}
/**
* DEPRECATED. Use `TypeTrackingNode::track` instead.
*
* Gets a node that this node may flow to using one heap and/or interprocedural step.
*
* See `TypeTracker` for more details about how to use this.
*/
pragma[inline]
deprecated LocalSourceNode track(TypeTracker t2, TypeTracker t) { t = t2.step(this, result) }
LocalSourceNode track(TypeTracker t2, TypeTracker t) { t = t2.step(this, result) }
/**
* DEPRECATED. Use `TypeTrackingNode::backtrack` instead.
*
* Gets a node that may flow into this one using one heap and/or interprocedural step.
*
* See `TypeBackTracker` for more details about how to use this.
*/
pragma[inline]
deprecated LocalSourceNode backtrack(TypeBackTracker t2, TypeBackTracker t) {
t2 = t.step(result, this)
}
LocalSourceNode backtrack(TypeBackTracker t2, TypeBackTracker t) { t2 = t.step(result, this) }
}
/**
@@ -131,40 +125,46 @@ class LocalSourceNode extends Node {
*
* All steps made during type tracking should be between instances of this class.
*/
class TypeTrackingNode extends Node {
TypeTrackingNode() {
this instanceof LocalSourceNode
or
this instanceof ModuleVariableNode
class TypeTrackingNode = LocalSourceNode;
/** Temporary holding ground for the `TypeTrackingNode` class. */
private module FutureWork {
class FutureTypeTrackingNode extends Node {
FutureTypeTrackingNode() {
this instanceof LocalSourceNode
or
this instanceof ModuleVariableNode
}
/**
* Holds if this node can flow to `nodeTo` in one or more local flow steps.
*
* For `ModuleVariableNode`s, the only "local" step is to the node itself.
* For `LocalSourceNode`s, this is the usual notion of local flow.
*/
pragma[inline]
predicate flowsTo(Node node) {
this instanceof ModuleVariableNode and this = node
or
this.(LocalSourceNode).flowsTo(node)
}
/**
* Gets a node that this node may flow to using one heap and/or interprocedural step.
*
* See `TypeTracker` for more details about how to use this.
*/
pragma[inline]
TypeTrackingNode track(TypeTracker t2, TypeTracker t) { t = t2.step(this, result) }
/**
* Gets a node that may flow into this one using one heap and/or interprocedural step.
*
* See `TypeBackTracker` for more details about how to use this.
*/
pragma[inline]
TypeTrackingNode backtrack(TypeBackTracker t2, TypeBackTracker t) { t2 = t.step(result, this) }
}
/**
* Holds if this node can flow to `nodeTo` in one or more local flow steps.
*
* For `ModuleVariableNode`s, the only "local" step is to the node itself.
* For `LocalSourceNode`s, this is the usual notion of local flow.
*/
predicate flowsTo(Node node) {
this instanceof ModuleVariableNode and this = node
or
this.(LocalSourceNode).flowsTo(node)
}
/**
* Gets a node that this node may flow to using one heap and/or interprocedural step.
*
* See `TypeTracker` for more details about how to use this.
*/
pragma[inline]
TypeTrackingNode track(TypeTracker t2, TypeTracker t) { t = t2.step(this, result) }
/**
* Gets a node that may flow into this one using one heap and/or interprocedural step.
*
* See `TypeBackTracker` for more details about how to use this.
*/
pragma[inline]
TypeTrackingNode backtrack(TypeBackTracker t2, TypeBackTracker t) { t2 = t.step(result, this) }
}
cached
@@ -179,11 +179,21 @@ private module Cached {
source = sink
or
exists(Node second |
simpleLocalFlowStep(source, second) and
simpleLocalFlowStep*(second, sink)
localSourceFlowStep(source, second) and
localSourceFlowStep*(second, sink)
)
}
/**
* Helper predicate for `hasLocalSource`. Removes any steps go to module variable reads, as these
* are already local source nodes in their own right.
*/
cached
private predicate localSourceFlowStep(Node nodeFrom, Node nodeTo) {
simpleLocalFlowStep(nodeFrom, nodeTo) and
not nodeTo = any(ModuleVariableNode v).getARead()
}
/**
* Holds if `base` flows to the base of `ref` and `ref` has attribute name `attr`.
*/