Skip to content

Commit c14d404

Browse files
geoffw0geoffw0
authored
Merge pull request github#14748 from geoffw0/pathinjectionsinks
Swift: Add more path injection sinks
2 parents 8be6aed + c327f0f commit c14d404

File tree

7 files changed

+284
-96
lines changed

7 files changed

+284
-96
lines changed

‎swift/ql/lib/codeql/swift/frameworks/StandardLibrary/NsString.qll

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,8 +103,8 @@ private class NsStringSummaries extends SummaryModelCsv {
103103
";NSString;true;data(using:);;;Argument[-1];ReturnValue;taint",
104104
";NSString;true;data(using:allowLossyConversion:);;;Argument[-1];ReturnValue;taint",
105105
";NSString;true;path(withComponents:);;;Argument[0].CollectionElement;ReturnValue;taint",
106-
";NSString;true;completePath(into:caseSensitive:matchesInto:filterTypes:);;;Argument[-1];Argument[0];taint",
107-
";NSString;true;completePath(into:caseSensitive:matchesInto:filterTypes:);;;Argument[-1];Argument[2];taint",
106+
";NSString;true;completePath(into:caseSensitive:matchesInto:filterTypes:);;;Argument[-1];Argument[0].CollectionElement;taint",
107+
";NSString;true;completePath(into:caseSensitive:matchesInto:filterTypes:);;;Argument[-1];Argument[2].CollectionElement.CollectionElement;taint",
108108
";NSString;true;getFileSystemRepresentation(_:maxLength:);;;Argument[-1];Argument[0];taint",
109109
";NSString;true;appendingPathComponent(_:);;;Argument[-1..0];ReturnValue;taint",
110110
";NSString;true;appendingPathComponent(_:conformingTo:);;;Argument[-1..0];ReturnValue;taint",

‎swift/ql/lib/codeql/swift/frameworks/StandardLibrary/NsUrl.qll

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,34 @@
33
*/
44

55
import swift
6+
private import codeql.swift.dataflow.DataFlow
67
private import codeql.swift.dataflow.ExternalFlow
8+
private import codeql.swift.dataflow.FlowSteps
9+
10+
/**
11+
* A content implying that, if an `NSURL` is tainted, then all its fields are tainted.
12+
*/
13+
private class NSUrlFieldsInheritTaint extends TaintInheritingContent,
14+
DataFlow::Content::FieldContent
15+
{
16+
NSUrlFieldsInheritTaint() {
17+
this.getField().getEnclosingDecl().asNominalTypeDecl().getFullName() = "NSURL"
18+
}
19+
}
720

821
/**
922
* A model for `NSURL` members that permit taint flow.
1023
*/
1124
private class NsUrlSummaries extends SummaryModelCsv {
1225
override predicate row(string row) {
13-
row = ";NSURL;true;init(string:);(String);;Argument[0];ReturnValue.OptionalSome;taint"
26+
row =
27+
[
28+
";NSURL;true;init(string:);(String);;Argument[0];ReturnValue.OptionalSome;taint",
29+
";NSURL;true;appendingPathComponent(_:);;;Argument[-1..0];ReturnValue;taint",
30+
";NSURL;true;appendingPathComponent(_:isDirectory:);;;Argument[-1..0];ReturnValue;taint",
31+
";NSURL;true;appendingPathComponent(_:conformingTo:);;;Argument[-1..0];ReturnValue;taint",
32+
";NSURL;true;appendingPathExtension(_:);;;Argument[-1..0];ReturnValue;taint",
33+
";NSURL;true;appendingPathExtension(for:);;;Argument[-1];ReturnValue;taint",
34+
]
1435
}
1536
}

‎swift/ql/lib/codeql/swift/frameworks/StandardLibrary/String.qll

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,8 @@ private class StringSummaries extends SummaryModelCsv {
4545
";StringProtocol;true;applyingTransform(_:reverse:);;;Argument[-1];ReturnValue;taint",
4646
";StringProtocol;true;cString(using:);;;Argument[-1];ReturnValue;taint",
4747
";StringProtocol;true;capitalized(with:);;;Argument[-1];ReturnValue;taint",
48-
";StringProtocol;true;completePath(into:caseSensitive:matchesInto:filterTypes:);;;Argument[-1];Argument[0].OptionalSome.CollectionElement;taint",
49-
";StringProtocol;true;completePath(into:caseSensitive:matchesInto:filterTypes:);;;Argument[-1];Argument[2].OptionalSome.CollectionElement.CollectionElement;taint",
48+
";StringProtocol;true;completePath(into:caseSensitive:matchesInto:filterTypes:);;;Argument[-1];Argument[0].CollectionElement;taint",
49+
";StringProtocol;true;completePath(into:caseSensitive:matchesInto:filterTypes:);;;Argument[-1];Argument[2].CollectionElement.CollectionElement;taint",
5050
";StringProtocol;true;components(separatedBy:);;;Argument[-1];ReturnValue;taint",
5151
";StringProtocol;true;data(using:allowLossyConversion:);;;Argument[-1];ReturnValue;taint",
5252
";StringProtocol;true;folding(options:locale:);;;Argument[-1];ReturnValue;taint",

‎swift/ql/lib/codeql/swift/frameworks/StandardLibrary/Url.qll

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,8 @@ class UrlDecl extends StructDecl {
1515
/**
1616
* A content implying that, if a `URL` is tainted, then all its fields are tainted.
1717
*/
18-
private class UriFieldsInheritTaint extends TaintInheritingContent, DataFlow::Content::FieldContent {
19-
UriFieldsInheritTaint() {
18+
private class UrlFieldsInheritTaint extends TaintInheritingContent, DataFlow::Content::FieldContent {
19+
UrlFieldsInheritTaint() {
2020
this.getField().getEnclosingDecl().asNominalTypeDecl() instanceof UrlDecl
2121
}
2222
}
@@ -106,6 +106,8 @@ private class UrlSummaries extends SummaryModelCsv {
106106
";URL;true;init(dataRepresentation:relativeTo:isAbsolute:);;;Argument[0];ReturnValue;taint",
107107
";URL;true;init(dataRepresentation:relativeTo:isAbsolute:);;;Argument[1].OptionalSome;ReturnValue;taint",
108108
";URL;true;init(_:strategy:);;;Argument[0];ReturnValue;taint",
109+
";URL;true;init(filePath:);;;Argument[0];ReturnValue.OptionalSome;taint",
110+
";URL;true;init(filePath:isDirectory:);;;Argument[0];ReturnValue.OptionalSome;taint",
109111
";URL;true;init(filePath:directoryHint:);;;Argument[0];ReturnValue.OptionalSome;taint",
110112
";URL;true;init(filePath:directoryHint:relativeTo:);;;Argument[0];ReturnValue;taint",
111113
";URL;true;init(filePath:directoryHint:relativeTo:);;;Argument[2].OptionalSome;ReturnValue;taint",
@@ -126,6 +128,7 @@ private class UrlSummaries extends SummaryModelCsv {
126128
";URL;true;appendingPathComponent(_:conformingTo:);;;Argument[-1..0];ReturnValue;taint",
127129
";URL;true;appendPathExtension(_:);;;Argument[-1..0];Argument[-1];taint",
128130
";URL;true;appendingPathExtension(_:);;;Argument[-1..0];ReturnValue;taint",
131+
";URL;true;appendingPathExtension(for:);;;Argument[-1];ReturnValue;taint",
129132
";URL;true;deletingLastPathComponent();;;Argument[-1];ReturnValue;taint",
130133
";URL;true;deletingPathExtension();;;Argument[-1];ReturnValue;taint",
131134
";URL;true;bookmarkData(options:includingResourceValuesForKeys:relativeTo:);;;Argument[-1];ReturnValue;taint",

‎swift/ql/lib/codeql/swift/security/PathInjectionExtensions.qll

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,41 @@ private class EnumConstructorPathInjectionSink extends PathInjectionSink {
6161
}
6262
}
6363

64+
/**
65+
* A string that might be a label for a path argument.
66+
*/
67+
pragma[inline]
68+
private predicate pathLikeHeuristic(string label) {
69+
label =
70+
[
71+
"atFile", "atPath", "atDirectory", "toFile", "toPath", "toDirectory", "inFile", "inPath",
72+
"inDirectory", "contentsOfFile", "contentsOfPath", "contentsOfDirectory", "filePath",
73+
"directory", "directoryPath"
74+
]
75+
}
76+
77+
/**
78+
* A path injection sink that is determined by imprecise methods.
79+
*/
80+
private class HeuristicPathInjectionSink extends PathInjectionSink {
81+
HeuristicPathInjectionSink() {
82+
// by parameter name
83+
exists(CallExpr ce, int ix, ParamDecl pd |
84+
pathLikeHeuristic(pragma[only_bind_into](pd.getName())) and
85+
pd.getType().getUnderlyingType().getName() = ["String", "NSString"] and
86+
pd = ce.getStaticTarget().getParam(ix) and
87+
this.asExpr() = ce.getArgument(ix).getExpr()
88+
)
89+
or
90+
// by argument name
91+
exists(Argument a |
92+
pathLikeHeuristic(pragma[only_bind_into](a.getLabel())) and
93+
a.getExpr().getType().getUnderlyingType().getName() = ["String", "NSString"] and
94+
this.asExpr() = a.getExpr()
95+
)
96+
}
97+
}
98+
6499
private class DefaultPathInjectionBarrier extends PathInjectionBarrier {
65100
DefaultPathInjectionBarrier() {
66101
// This is a simplified implementation.
@@ -87,7 +122,14 @@ private class PathInjectionSinks extends SinkModelCsv {
87122
override predicate row(string row) {
88123
row =
89124
[
125+
";Data;true;init(contentsOf:options:);;;Argument[0];path-injection",
90126
";Data;true;write(to:options:);;;Argument[0];path-injection",
127+
";NSData;true;init(contentsOfFile:);;;Argument[0];path-injection",
128+
";NSData;true;init(contentsOfFile:options:);;;Argument[0];path-injection",
129+
";NSData;true;init(contentsOf:);;;Argument[0];path-injection",
130+
";NSData;true;init(contentsOf:options:);;;Argument[0];path-injection",
131+
";NSData;true;init(contentsOfMappedFile:);;;Argument[0];path-injection",
132+
";NSData;true;dataWithContentsOfMappedFile(_:);;;Argument[0];path-injection",
91133
";NSData;true;write(to:atomically:);;;Argument[0];path-injection",
92134
";NSData;true;write(to:options:);;;Argument[0];path-injection",
93135
";NSData;true;write(toFile:atomically:);;;Argument[0];path-injection",
@@ -118,12 +160,14 @@ private class PathInjectionSinks extends SinkModelCsv {
118160
";FileManager;true;fileExists(atPath:);;;Argument[0];path-injection",
119161
";FileManager;true;fileExists(atPath:isDirectory:);;;Argument[0];path-injection",
120162
";FileManager;true;setAttributes(_:ofItemAtPath:);;;Argument[1];path-injection",
163+
";FileManager;true;attributesOfItem(atPath:);;;Argument[0];path-injection",
121164
";FileManager;true;contents(atPath:);;;Argument[0];path-injection",
122165
";FileManager;true;contentsEqual(atPath:andPath:);;;Argument[0..1];path-injection",
123166
";FileManager;true;changeCurrentDirectoryPath(_:);;;Argument[0];path-injection",
124167
";FileManager;true;unmountVolume(at:options:completionHandler:);;;Argument[0];path-injection",
125168
// Deprecated FileManager methods:
126169
";FileManager;true;changeFileAttributes(_:atPath:);;;Argument[1];path-injection",
170+
";FileManager;true;fileAttributes(atPath:traverseLink:);;;Argument[0];path-injection",
127171
";FileManager;true;directoryContents(atPath:);;;Argument[0];path-injection",
128172
";FileManager;true;createDirectory(atPath:attributes:);;;Argument[0];path-injection",
129173
";FileManager;true;createSymbolicLink(atPath:pathContent:);;;Argument[0..1];path-injection",
@@ -146,6 +190,7 @@ private class PathInjectionSinks extends SinkModelCsv {
146190
";ArchiveByteStream;true;withFileStream(path:mode:options:permissions:_:);;;Argument[0];path-injection",
147191
";Bundle;true;init(url:);;;Argument[0];path-injection",
148192
";Bundle;true;init(path:);;;Argument[0];path-injection",
193+
";NSURL;writeBookmarkData(_:to:options:);;;Argument[1];path-injection",
149194
// GRDB
150195
";Database;true;init(path:description:configuration:);;;Argument[0];path-injection",
151196
";DatabasePool;true;init(path:configuration:);;;Argument[0];path-injection",

‎swift/ql/src/change-notes/2023-11-10-path-injection.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
5+
* Added additional sinks for the "Uncontrolled data used in path expression" (`swift/path-injection`) query. Some of these sinks are heuristic (imprecise) in nature.

0 commit comments

Comments
 (0)