Skip to content

Commit 141c5af

Browse files
Rasmus Lerchedahl PetersenRasmus Lerchedahl Petersen
Rasmus Lerchedahl Petersen
authored and
Rasmus Lerchedahl Petersen
committed
Merge branch 'main' of https://github.com/github/codeql into python/captured-variables-for-typetracking
2 parents a25c7f7 + b35637e commit 141c5af

File tree

37 files changed

+895
-367
lines changed

37 files changed

+895
-367
lines changed

‎cpp/ql/lib/semmle/code/cpp/dataflow/internal/DataFlow.qll

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -361,3 +361,52 @@ module MergePathGraph<
361361
}
362362
}
363363
}
364+
365+
/**
366+
* Constructs a `PathGraph` from three `PathGraph`s by disjoint union.
367+
*/
368+
module MergePathGraph3<
369+
PathNodeSig PathNode1, PathNodeSig PathNode2, PathNodeSig PathNode3,
370+
PathGraphSig<PathNode1> Graph1, PathGraphSig<PathNode2> Graph2, PathGraphSig<PathNode3> Graph3>
371+
{
372+
private module MergedInner = MergePathGraph<PathNode1, PathNode2, Graph1, Graph2>;
373+
374+
private module Merged =
375+
MergePathGraph<MergedInner::PathNode, PathNode3, MergedInner::PathGraph, Graph3>;
376+
377+
/** A node in a graph of path explanations that is formed by disjoint union of the three given graphs. */
378+
class PathNode instanceof Merged::PathNode {
379+
/** Gets this as a projection on the first given `PathGraph`. */
380+
PathNode1 asPathNode1() { result = super.asPathNode1().asPathNode1() }
381+
382+
/** Gets this as a projection on the second given `PathGraph`. */
383+
PathNode2 asPathNode2() { result = super.asPathNode1().asPathNode2() }
384+
385+
/** Gets this as a projection on the third given `PathGraph`. */
386+
PathNode3 asPathNode3() { result = super.asPathNode2() }
387+
388+
/** Gets a textual representation of this element. */
389+
string toString() { result = super.toString() }
390+
391+
/**
392+
* Holds if this element is at the specified location.
393+
* The location spans column `startcolumn` of line `startline` to
394+
* column `endcolumn` of line `endline` in file `filepath`.
395+
* For more information, see
396+
* [Locations](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
397+
*/
398+
predicate hasLocationInfo(
399+
string filepath, int startline, int startcolumn, int endline, int endcolumn
400+
) {
401+
super.hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
402+
}
403+
404+
/** Gets the underlying `Node`. */
405+
Node getNode() { result = super.getNode() }
406+
}
407+
408+
/**
409+
* Provides the query predicates needed to include a graph in a path-problem query.
410+
*/
411+
module PathGraph = Merged::PathGraph;
412+
}

‎cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlow.qll

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -361,3 +361,52 @@ module MergePathGraph<
361361
}
362362
}
363363
}
364+
365+
/**
366+
* Constructs a `PathGraph` from three `PathGraph`s by disjoint union.
367+
*/
368+
module MergePathGraph3<
369+
PathNodeSig PathNode1, PathNodeSig PathNode2, PathNodeSig PathNode3,
370+
PathGraphSig<PathNode1> Graph1, PathGraphSig<PathNode2> Graph2, PathGraphSig<PathNode3> Graph3>
371+
{
372+
private module MergedInner = MergePathGraph<PathNode1, PathNode2, Graph1, Graph2>;
373+
374+
private module Merged =
375+
MergePathGraph<MergedInner::PathNode, PathNode3, MergedInner::PathGraph, Graph3>;
376+
377+
/** A node in a graph of path explanations that is formed by disjoint union of the three given graphs. */
378+
class PathNode instanceof Merged::PathNode {
379+
/** Gets this as a projection on the first given `PathGraph`. */
380+
PathNode1 asPathNode1() { result = super.asPathNode1().asPathNode1() }
381+
382+
/** Gets this as a projection on the second given `PathGraph`. */
383+
PathNode2 asPathNode2() { result = super.asPathNode1().asPathNode2() }
384+
385+
/** Gets this as a projection on the third given `PathGraph`. */
386+
PathNode3 asPathNode3() { result = super.asPathNode2() }
387+
388+
/** Gets a textual representation of this element. */
389+
string toString() { result = super.toString() }
390+
391+
/**
392+
* Holds if this element is at the specified location.
393+
* The location spans column `startcolumn` of line `startline` to
394+
* column `endcolumn` of line `endline` in file `filepath`.
395+
* For more information, see
396+
* [Locations](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
397+
*/
398+
predicate hasLocationInfo(
399+
string filepath, int startline, int startcolumn, int endline, int endcolumn
400+
) {
401+
super.hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
402+
}
403+
404+
/** Gets the underlying `Node`. */
405+
Node getNode() { result = super.getNode() }
406+
}
407+
408+
/**
409+
* Provides the query predicates needed to include a graph in a path-problem query.
410+
*/
411+
module PathGraph = Merged::PathGraph;
412+
}

‎csharp/ql/lib/ext/System.IO.model.yml

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,23 @@ extensions:
44
extensible: sourceModel
55
data:
66
- ["System.IO", "FileStream", False, "FileStream", "", "", "Argument[this]", "file", "manual"]
7+
- ["System.IO", "FileStream", False, "FileStream", "", "", "Argument[this]", "file-write", "manual"]
8+
- ["System.IO", "StreamWriter", False, "StreamWriter", "(System.String)", "", "Argument[this]", "file-write", "manual"]
9+
- ["System.IO", "StreamWriter", False, "StreamWriter", "(System.String,System.Boolean)", "", "Argument[this]", "file-write", "manual"]
10+
- ["System.IO", "StreamWriter", False, "StreamWriter", "(System.String,System.Boolean,System.Text.Encoding)", "", "Argument[this]", "file-write", "manual"]
11+
- ["System.IO", "StreamWriter", False, "StreamWriter", "(System.String,System.Boolean,System.Text.Encoding,System.Int32)", "", "Argument[this]", "file-write", "manual"]
12+
- ["System.IO", "StreamWriter", False, "StreamWriter", "(System.String,System.Text.Encoding,System.IO.FileStreamOptions)", "", "Argument[this]", "file-write", "manual"]
13+
- ["System.IO", "StreamWriter", False, "StreamWriter", "(System.String,System.IO.FileStreamOptions)", "", "Argument[this]", "file-write", "manual"]
14+
- ["System.IO", "File", False, "Open", "", "", "ReturnValue", "file-write", "manual"]
15+
- ["System.IO", "File", False, "OpenWrite", "", "", "ReturnValue", "file-write", "manual"]
16+
- ["System.IO", "File", False, "Create", "", "", "ReturnValue", "file-write", "manual"]
17+
- ["System.IO", "File", False, "CreateText", "", "", "ReturnValue", "file-write", "manual"]
18+
- ["System.IO", "File", False, "AppendText", "", "", "ReturnValue", "file-write", "manual"]
19+
- ["System.IO", "FileInfo", False, "Open", "", "", "ReturnValue", "file-write", "manual"]
20+
- ["System.IO", "FileInfo", False, "OpenWrite", "", "", "ReturnValue", "file-write", "manual"]
21+
- ["System.IO", "FileInfo", False, "Create", "", "", "ReturnValue", "file-write", "manual"]
22+
- ["System.IO", "FileInfo", False, "CreateText", "", "", "ReturnValue", "file-write", "manual"]
23+
- ["System.IO", "FileInfo", False, "AppendText", "", "", "ReturnValue", "file-write", "manual"]
724
- addsTo:
825
pack: codeql/csharp-all
926
extensible: summaryModel

‎csharp/ql/lib/semmle/code/csharp/dataflow/ExternalFlow.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,7 @@ module ModelValidation {
215215
)
216216
or
217217
exists(string kind | sourceModel(_, _, _, _, _, _, _, kind, _) |
218-
not kind = ["local", "remote", "file"] and
218+
not kind = ["local", "remote", "file", "file-write"] and
219219
result = "Invalid kind \"" + kind + "\" in source model."
220220
)
221221
}

‎csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlow.qll

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -361,3 +361,52 @@ module MergePathGraph<
361361
}
362362
}
363363
}
364+
365+
/**
366+
* Constructs a `PathGraph` from three `PathGraph`s by disjoint union.
367+
*/
368+
module MergePathGraph3<
369+
PathNodeSig PathNode1, PathNodeSig PathNode2, PathNodeSig PathNode3,
370+
PathGraphSig<PathNode1> Graph1, PathGraphSig<PathNode2> Graph2, PathGraphSig<PathNode3> Graph3>
371+
{
372+
private module MergedInner = MergePathGraph<PathNode1, PathNode2, Graph1, Graph2>;
373+
374+
private module Merged =
375+
MergePathGraph<MergedInner::PathNode, PathNode3, MergedInner::PathGraph, Graph3>;
376+
377+
/** A node in a graph of path explanations that is formed by disjoint union of the three given graphs. */
378+
class PathNode instanceof Merged::PathNode {
379+
/** Gets this as a projection on the first given `PathGraph`. */
380+
PathNode1 asPathNode1() { result = super.asPathNode1().asPathNode1() }
381+
382+
/** Gets this as a projection on the second given `PathGraph`. */
383+
PathNode2 asPathNode2() { result = super.asPathNode1().asPathNode2() }
384+
385+
/** Gets this as a projection on the third given `PathGraph`. */
386+
PathNode3 asPathNode3() { result = super.asPathNode2() }
387+
388+
/** Gets a textual representation of this element. */
389+
string toString() { result = super.toString() }
390+
391+
/**
392+
* Holds if this element is at the specified location.
393+
* The location spans column `startcolumn` of line `startline` to
394+
* column `endcolumn` of line `endline` in file `filepath`.
395+
* For more information, see
396+
* [Locations](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
397+
*/
398+
predicate hasLocationInfo(
399+
string filepath, int startline, int startcolumn, int endline, int endcolumn
400+
) {
401+
super.hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
402+
}
403+
404+
/** Gets the underlying `Node`. */
405+
Node getNode() { result = super.getNode() }
406+
}
407+
408+
/**
409+
* Provides the query predicates needed to include a graph in a path-problem query.
410+
*/
411+
module PathGraph = Merged::PathGraph;
412+
}

‎csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsinks/ExternalLocationSink.qll

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import csharp
66
private import Remote
77
private import semmle.code.csharp.commons.Loggers
88
private import semmle.code.csharp.frameworks.system.Web
9+
private import semmle.code.csharp.frameworks.system.IO
910
private import semmle.code.csharp.dataflow.ExternalFlow
1011

1112
/**
@@ -63,3 +64,56 @@ class CookieStorageSink extends ExternalLocationSink, RemoteFlowSink {
6364
)
6465
}
6566
}
67+
68+
private predicate isFileWriteCall(Expr stream, Expr data) {
69+
exists(MethodCall mc, Method m | mc.getTarget() = m.getAnOverrider*() |
70+
m.hasQualifiedName("System.IO", "Stream", ["Write", "WriteAsync"]) and
71+
stream = mc.getQualifier() and
72+
data = mc.getArgument(0)
73+
or
74+
m.hasQualifiedName("System.IO", "TextWriter",
75+
["Write", "WriteAsync", "WriteLine", "WriteLineAsync"]) and
76+
stream = mc.getQualifier() and
77+
data = mc.getArgument(0)
78+
or
79+
m.hasQualifiedName("System.Xml.Linq", "XDocument", ["Save", "SaveAsync"]) and
80+
data = mc.getQualifier() and
81+
stream = mc.getArgument(0)
82+
)
83+
}
84+
85+
/** A configuration for tracking flow from calls that open a file in write mode to methods that write to that file, excluding encrypted streams. */
86+
private module LocalFileOutputStreamConfig implements DataFlow::ConfigSig {
87+
predicate isSource(DataFlow::Node src) { sourceNode(src, "file-write") }
88+
89+
predicate isSink(DataFlow::Node sink) { isFileWriteCall(sink.asExpr(), _) }
90+
91+
predicate isBarrier(DataFlow::Node node) {
92+
node.asExpr()
93+
.(ObjectCreation)
94+
.getObjectType()
95+
.hasQualifiedName("System.Security.Cryptography", "CryptoStream")
96+
}
97+
98+
predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
99+
exists(ObjectCreation oc |
100+
node2.asExpr() = oc and
101+
node1.asExpr() = oc.getArgument(0) and
102+
oc.getObjectType() instanceof SystemIOStreamWriterClass
103+
)
104+
}
105+
}
106+
107+
private module LocalFileOutputStreamFlow = DataFlow::Global<LocalFileOutputStreamConfig>;
108+
109+
/**
110+
* A write to the local filesystem.
111+
*/
112+
class LocalFileOutputSink extends ExternalLocationSink {
113+
LocalFileOutputSink() {
114+
exists(DataFlow::Node streamSink |
115+
LocalFileOutputStreamFlow::flowTo(streamSink) and
116+
isFileWriteCall(streamSink.asExpr(), this.asExpr())
117+
)
118+
}
119+
}

‎csharp/ql/src/change-notes/2023-04-05-external-location-sinks.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Additional sinks modelling writes to unencrypted local files have been added to `ExternalLocationSink`, used by the `cs/cleartext-storage` and `cs/exposure-of-sensitive-information` queries.

‎csharp/ql/test/query-tests/Security Features/CWE-312/CleartextStorage.cs

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22
using System.Web;
33
using System.Web.Security;
44
using System.Windows.Forms;
5+
using System.IO;
6+
using System.Security.Cryptography;
57

68
public class ClearTextStorageHandler : IHttpHandler
79
{
@@ -24,13 +26,33 @@ public void ProcessRequest(HttpContext ctx)
2426
logger.Warn(GetPassword());
2527
// GOOD: Logging encrypted sensitive data
2628
logger.Warn(Encode(GetPassword(), "Password"));
29+
30+
// BAD: Storing sensitive data in local file
31+
using (var writeStream = File.Open("passwords.txt", FileMode.Create))
32+
{
33+
var writer = new StreamWriter(writeStream);
34+
writer.Write(GetPassword());
35+
writer.Close();
36+
}
37+
38+
// GOOD: Storing encrypted sensitive data
39+
using (var writeStream = File.Open("passwords.txt", FileMode.Create))
40+
{
41+
var writer = new StreamWriter(new CryptoStream(writeStream, GetEncryptor(), CryptoStreamMode.Write));
42+
writer.Write(GetPassword());
43+
writer.Close();
44+
}
2745
}
2846

2947
public string Encode(string value, string type)
3048
{
3149
return Encoding.UTF8.GetString(MachineKey.Protect(Encoding.UTF8.GetBytes(value), type));
3250
}
3351

52+
public ICryptoTransform GetEncryptor(){
53+
return null;
54+
}
55+
3456
public string GetPassword()
3557
{
3658
return "password";

‎csharp/ql/test/query-tests/Security Features/CWE-312/CleartextStorage.expected

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,22 @@
11
edges
22
nodes
3-
| CleartextStorage.cs:13:50:13:59 | access to field accountKey | semmle.label | access to field accountKey |
4-
| CleartextStorage.cs:14:62:14:74 | call to method GetPassword | semmle.label | call to method GetPassword |
5-
| CleartextStorage.cs:15:69:15:81 | call to method GetPassword | semmle.label | call to method GetPassword |
6-
| CleartextStorage.cs:16:50:16:63 | call to method GetAccountID | semmle.label | call to method GetAccountID |
7-
| CleartextStorage.cs:24:21:24:33 | call to method GetPassword | semmle.label | call to method GetPassword |
8-
| CleartextStorage.cs:72:21:72:33 | access to property Text | semmle.label | access to property Text |
9-
| CleartextStorage.cs:73:21:73:29 | access to property Text | semmle.label | access to property Text |
10-
| CleartextStorage.cs:74:21:74:29 | access to property Text | semmle.label | access to property Text |
3+
| CleartextStorage.cs:15:50:15:59 | access to field accountKey | semmle.label | access to field accountKey |
4+
| CleartextStorage.cs:16:62:16:74 | call to method GetPassword | semmle.label | call to method GetPassword |
5+
| CleartextStorage.cs:17:69:17:81 | call to method GetPassword | semmle.label | call to method GetPassword |
6+
| CleartextStorage.cs:18:50:18:63 | call to method GetAccountID | semmle.label | call to method GetAccountID |
7+
| CleartextStorage.cs:26:21:26:33 | call to method GetPassword | semmle.label | call to method GetPassword |
8+
| CleartextStorage.cs:34:26:34:38 | call to method GetPassword | semmle.label | call to method GetPassword |
9+
| CleartextStorage.cs:94:21:94:33 | access to property Text | semmle.label | access to property Text |
10+
| CleartextStorage.cs:95:21:95:29 | access to property Text | semmle.label | access to property Text |
11+
| CleartextStorage.cs:96:21:96:29 | access to property Text | semmle.label | access to property Text |
1112
subpaths
1213
#select
13-
| CleartextStorage.cs:13:50:13:59 | access to field accountKey | CleartextStorage.cs:13:50:13:59 | access to field accountKey | CleartextStorage.cs:13:50:13:59 | access to field accountKey | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:13:50:13:59 | access to field accountKey | access to field accountKey |
14-
| CleartextStorage.cs:14:62:14:74 | call to method GetPassword | CleartextStorage.cs:14:62:14:74 | call to method GetPassword | CleartextStorage.cs:14:62:14:74 | call to method GetPassword | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:14:62:14:74 | call to method GetPassword | call to method GetPassword |
15-
| CleartextStorage.cs:15:69:15:81 | call to method GetPassword | CleartextStorage.cs:15:69:15:81 | call to method GetPassword | CleartextStorage.cs:15:69:15:81 | call to method GetPassword | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:15:69:15:81 | call to method GetPassword | call to method GetPassword |
16-
| CleartextStorage.cs:16:50:16:63 | call to method GetAccountID | CleartextStorage.cs:16:50:16:63 | call to method GetAccountID | CleartextStorage.cs:16:50:16:63 | call to method GetAccountID | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:16:50:16:63 | call to method GetAccountID | call to method GetAccountID |
17-
| CleartextStorage.cs:24:21:24:33 | call to method GetPassword | CleartextStorage.cs:24:21:24:33 | call to method GetPassword | CleartextStorage.cs:24:21:24:33 | call to method GetPassword | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:24:21:24:33 | call to method GetPassword | call to method GetPassword |
18-
| CleartextStorage.cs:72:21:72:33 | access to property Text | CleartextStorage.cs:72:21:72:33 | access to property Text | CleartextStorage.cs:72:21:72:33 | access to property Text | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:72:21:72:33 | access to property Text | access to property Text |
19-
| CleartextStorage.cs:73:21:73:29 | access to property Text | CleartextStorage.cs:73:21:73:29 | access to property Text | CleartextStorage.cs:73:21:73:29 | access to property Text | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:73:21:73:29 | access to property Text | access to property Text |
20-
| CleartextStorage.cs:74:21:74:29 | access to property Text | CleartextStorage.cs:74:21:74:29 | access to property Text | CleartextStorage.cs:74:21:74:29 | access to property Text | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:74:21:74:29 | access to property Text | access to property Text |
14+
| CleartextStorage.cs:15:50:15:59 | access to field accountKey | CleartextStorage.cs:15:50:15:59 | access to field accountKey | CleartextStorage.cs:15:50:15:59 | access to field accountKey | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:15:50:15:59 | access to field accountKey | access to field accountKey |
15+
| CleartextStorage.cs:16:62:16:74 | call to method GetPassword | CleartextStorage.cs:16:62:16:74 | call to method GetPassword | CleartextStorage.cs:16:62:16:74 | call to method GetPassword | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:16:62:16:74 | call to method GetPassword | call to method GetPassword |
16+
| CleartextStorage.cs:17:69:17:81 | call to method GetPassword | CleartextStorage.cs:17:69:17:81 | call to method GetPassword | CleartextStorage.cs:17:69:17:81 | call to method GetPassword | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:17:69:17:81 | call to method GetPassword | call to method GetPassword |
17+
| CleartextStorage.cs:18:50:18:63 | call to method GetAccountID | CleartextStorage.cs:18:50:18:63 | call to method GetAccountID | CleartextStorage.cs:18:50:18:63 | call to method GetAccountID | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:18:50:18:63 | call to method GetAccountID | call to method GetAccountID |
18+
| CleartextStorage.cs:26:21:26:33 | call to method GetPassword | CleartextStorage.cs:26:21:26:33 | call to method GetPassword | CleartextStorage.cs:26:21:26:33 | call to method GetPassword | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:26:21:26:33 | call to method GetPassword | call to method GetPassword |
19+
| CleartextStorage.cs:34:26:34:38 | call to method GetPassword | CleartextStorage.cs:34:26:34:38 | call to method GetPassword | CleartextStorage.cs:34:26:34:38 | call to method GetPassword | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:34:26:34:38 | call to method GetPassword | call to method GetPassword |
20+
| CleartextStorage.cs:94:21:94:33 | access to property Text | CleartextStorage.cs:94:21:94:33 | access to property Text | CleartextStorage.cs:94:21:94:33 | access to property Text | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:94:21:94:33 | access to property Text | access to property Text |
21+
| CleartextStorage.cs:95:21:95:29 | access to property Text | CleartextStorage.cs:95:21:95:29 | access to property Text | CleartextStorage.cs:95:21:95:29 | access to property Text | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:95:21:95:29 | access to property Text | access to property Text |
22+
| CleartextStorage.cs:96:21:96:29 | access to property Text | CleartextStorage.cs:96:21:96:29 | access to property Text | CleartextStorage.cs:96:21:96:29 | access to property Text | This stores sensitive data returned by $@ as clear text. | CleartextStorage.cs:96:21:96:29 | access to property Text | access to property Text |

0 commit comments

Comments
 (0)