Skip to content

Commit 4df25f4

Browse files
rdmarsh2rdmarsh2
authored
Merge pull request #14797 from geoffw0/sqlsinks
Swift: Heuristic sinks for swift/sql-injection
2 parents e79ad3b + 0b82f8a commit 4df25f4

File tree

4 files changed

+120
-0
lines changed

4 files changed

+120
-0
lines changed

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,36 @@ private class GrdbDefaultSqlInjectionSink extends SqlInjectionSink {
147147
}
148148
}
149149

150+
/**
151+
* Holds if `f`, `ix` describe `pd` and `pd` is a parameter that might be
152+
* executed as SQL.
153+
*/
154+
pragma[noinline]
155+
predicate sqlLikeHeuristic(Callable f, int ix, ParamDecl pd) {
156+
pd.getName() = "sql" and
157+
pd = f.getParam(ix)
158+
}
159+
160+
/**
161+
* An SQL injection sink that is determined by imprecise methods.
162+
*/
163+
private class HeuristicSqlInjectionSink extends SqlInjectionSink {
164+
HeuristicSqlInjectionSink() {
165+
// by parameter name
166+
exists(CallExpr ce, Callable f, int ix |
167+
sqlLikeHeuristic(f, ix, _) and
168+
f = ce.getStaticTarget() and
169+
this.asExpr() = ce.getArgument(ix).getExpr()
170+
)
171+
or
172+
// by argument name
173+
exists(Argument a |
174+
a.getLabel() = "sql" and
175+
this.asExpr() = a.getExpr()
176+
)
177+
}
178+
}
179+
150180
/**
151181
* A sink defined in a CSV model.
152182
*/

‎swift/ql/src/change-notes/2023-11-14-sql-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 heuristic (imprecise) sinks for the "Database query built from user-controlled sources" (`swift/sql-injection`) query.

‎swift/ql/test/query-tests/Security/CWE-089/SqlInjection.expected

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,13 @@ edges
9797
| SQLite.swift:62:25:62:79 | call to String.init(contentsOf:) | SQLite.swift:117:16:117:16 | unsafeQuery1 |
9898
| SQLite.swift:62:25:62:79 | call to String.init(contentsOf:) | SQLite.swift:119:16:119:16 | unsafeQuery1 |
9999
| SQLite.swift:62:25:62:79 | call to String.init(contentsOf:) | SQLite.swift:132:20:132:20 | remoteString |
100+
| other.swift:46:25:46:79 | call to String.init(contentsOf:) | other.swift:50:22:50:22 | remoteString |
101+
| other.swift:46:25:46:79 | call to String.init(contentsOf:) | other.swift:52:14:52:14 | remoteString |
102+
| other.swift:46:25:46:79 | call to String.init(contentsOf:) | other.swift:53:14:53:14 | remoteString |
103+
| other.swift:46:25:46:79 | call to String.init(contentsOf:) | other.swift:54:31:54:31 | remoteString |
104+
| other.swift:46:25:46:79 | call to String.init(contentsOf:) | other.swift:55:14:55:14 | remoteString |
105+
| other.swift:46:25:46:79 | call to String.init(contentsOf:) | other.swift:57:16:57:16 | remoteString |
106+
| other.swift:54:31:54:31 | remoteString | other.swift:54:14:54:43 | call to NSString.init(string:) |
100107
| sqlite3_c_api.swift:122:26:122:80 | call to String.init(contentsOf:) | sqlite3_c_api.swift:133:33:133:33 | unsafeQuery1 |
101108
| sqlite3_c_api.swift:122:26:122:80 | call to String.init(contentsOf:) | sqlite3_c_api.swift:134:33:134:33 | unsafeQuery2 |
102109
| sqlite3_c_api.swift:122:26:122:80 | call to String.init(contentsOf:) | sqlite3_c_api.swift:135:33:135:33 | unsafeQuery3 |
@@ -222,6 +229,14 @@ nodes
222229
| SQLite.swift:117:16:117:16 | unsafeQuery1 | semmle.label | unsafeQuery1 |
223230
| SQLite.swift:119:16:119:16 | unsafeQuery1 | semmle.label | unsafeQuery1 |
224231
| SQLite.swift:132:20:132:20 | remoteString | semmle.label | remoteString |
232+
| other.swift:46:25:46:79 | call to String.init(contentsOf:) | semmle.label | call to String.init(contentsOf:) |
233+
| other.swift:50:22:50:22 | remoteString | semmle.label | remoteString |
234+
| other.swift:52:14:52:14 | remoteString | semmle.label | remoteString |
235+
| other.swift:53:14:53:14 | remoteString | semmle.label | remoteString |
236+
| other.swift:54:14:54:43 | call to NSString.init(string:) | semmle.label | call to NSString.init(string:) |
237+
| other.swift:54:31:54:31 | remoteString | semmle.label | remoteString |
238+
| other.swift:55:14:55:14 | remoteString | semmle.label | remoteString |
239+
| other.swift:57:16:57:16 | remoteString | semmle.label | remoteString |
225240
| sqlite3_c_api.swift:122:26:122:80 | call to String.init(contentsOf:) | semmle.label | call to String.init(contentsOf:) |
226241
| sqlite3_c_api.swift:133:33:133:33 | unsafeQuery1 | semmle.label | unsafeQuery1 |
227242
| sqlite3_c_api.swift:134:33:134:33 | unsafeQuery2 | semmle.label | unsafeQuery2 |
@@ -336,6 +351,12 @@ subpaths
336351
| SQLite.swift:117:16:117:16 | unsafeQuery1 | SQLite.swift:62:25:62:79 | call to String.init(contentsOf:) | SQLite.swift:117:16:117:16 | unsafeQuery1 | This query depends on a $@. | SQLite.swift:62:25:62:79 | call to String.init(contentsOf:) | user-provided value |
337352
| SQLite.swift:119:16:119:16 | unsafeQuery1 | SQLite.swift:62:25:62:79 | call to String.init(contentsOf:) | SQLite.swift:119:16:119:16 | unsafeQuery1 | This query depends on a $@. | SQLite.swift:62:25:62:79 | call to String.init(contentsOf:) | user-provided value |
338353
| SQLite.swift:132:20:132:20 | remoteString | SQLite.swift:62:25:62:79 | call to String.init(contentsOf:) | SQLite.swift:132:20:132:20 | remoteString | This query depends on a $@. | SQLite.swift:62:25:62:79 | call to String.init(contentsOf:) | user-provided value |
354+
| other.swift:50:22:50:22 | remoteString | other.swift:46:25:46:79 | call to String.init(contentsOf:) | other.swift:50:22:50:22 | remoteString | This query depends on a $@. | other.swift:46:25:46:79 | call to String.init(contentsOf:) | user-provided value |
355+
| other.swift:52:14:52:14 | remoteString | other.swift:46:25:46:79 | call to String.init(contentsOf:) | other.swift:52:14:52:14 | remoteString | This query depends on a $@. | other.swift:46:25:46:79 | call to String.init(contentsOf:) | user-provided value |
356+
| other.swift:53:14:53:14 | remoteString | other.swift:46:25:46:79 | call to String.init(contentsOf:) | other.swift:53:14:53:14 | remoteString | This query depends on a $@. | other.swift:46:25:46:79 | call to String.init(contentsOf:) | user-provided value |
357+
| other.swift:54:14:54:43 | call to NSString.init(string:) | other.swift:46:25:46:79 | call to String.init(contentsOf:) | other.swift:54:14:54:43 | call to NSString.init(string:) | This query depends on a $@. | other.swift:46:25:46:79 | call to String.init(contentsOf:) | user-provided value |
358+
| other.swift:55:14:55:14 | remoteString | other.swift:46:25:46:79 | call to String.init(contentsOf:) | other.swift:55:14:55:14 | remoteString | This query depends on a $@. | other.swift:46:25:46:79 | call to String.init(contentsOf:) | user-provided value |
359+
| other.swift:57:16:57:16 | remoteString | other.swift:46:25:46:79 | call to String.init(contentsOf:) | other.swift:57:16:57:16 | remoteString | This query depends on a $@. | other.swift:46:25:46:79 | call to String.init(contentsOf:) | user-provided value |
339360
| sqlite3_c_api.swift:133:33:133:33 | unsafeQuery1 | sqlite3_c_api.swift:122:26:122:80 | call to String.init(contentsOf:) | sqlite3_c_api.swift:133:33:133:33 | unsafeQuery1 | This query depends on a $@. | sqlite3_c_api.swift:122:26:122:80 | call to String.init(contentsOf:) | user-provided value |
340361
| sqlite3_c_api.swift:134:33:134:33 | unsafeQuery2 | sqlite3_c_api.swift:122:26:122:80 | call to String.init(contentsOf:) | sqlite3_c_api.swift:134:33:134:33 | unsafeQuery2 | This query depends on a $@. | sqlite3_c_api.swift:122:26:122:80 | call to String.init(contentsOf:) | user-provided value |
341362
| sqlite3_c_api.swift:135:33:135:33 | unsafeQuery3 | sqlite3_c_api.swift:122:26:122:80 | call to String.init(contentsOf:) | sqlite3_c_api.swift:135:33:135:33 | unsafeQuery3 | This query depends on a $@. | sqlite3_c_api.swift:122:26:122:80 | call to String.init(contentsOf:) | user-provided value |

‎swift/ql/test/query-tests/Security/CWE-089/other.swift

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
2+
// --- stubs ---
3+
4+
struct URL
5+
{
6+
init?(string: String) {}
7+
}
8+
9+
extension String {
10+
init(contentsOf: URL) throws {
11+
self.init("")
12+
}
13+
}
14+
15+
class NSObject {
16+
}
17+
18+
class NSString : NSObject {
19+
init(string: String) { }
20+
}
21+
22+
class Sql {
23+
}
24+
25+
class MyDatabase {
26+
init(sql code: String? = nil) { }
27+
28+
func execute1(_ sql: String) { }
29+
func execute2(_ sql: String?) { }
30+
func execute3(_ sql: NSString) { }
31+
func execute4(_ sql: Sql) { }
32+
33+
func query(sql: String) { }
34+
func query(sqlLiteral: String) { }
35+
func query(sqlStatement: String) { }
36+
func query(sqliteStatement: String) { }
37+
38+
// non-examples
39+
func doSomething(sqlIndex: Int) { }
40+
func doSomething(sqliteContext: Sql) { }
41+
}
42+
43+
// --- tests ---
44+
45+
func test_heuristic(db: MyDatabase) throws {
46+
let remoteString = try String(contentsOf: URL(string: "http://example.com/")!)
47+
48+
_ = MyDatabase() // GOOD
49+
_ = MyDatabase(sql: "some_fixed_sql") // GOOD
50+
_ = MyDatabase(sql: remoteString) // BAD
51+
52+
db.execute1(remoteString) // BAD
53+
db.execute2(remoteString) // BAD
54+
db.execute3(NSString(string: remoteString)) // BAD
55+
db.execute4(remoteString as! Sql) // BAD
56+
57+
db.query(sql: remoteString) // BAD
58+
db.query(sqlLiteral: remoteString) // BAD [NOT DETECTED]
59+
db.query(sqlStatement: remoteString) // BAD [NOT DETECTED]
60+
db.query(sqliteStatement: remoteString) // BAD [NOT DETECTED]
61+
62+
db.doSomething(sqlIndex: Int(remoteString) ?? 0) // GOOD
63+
db.doSomething(sqliteContext: remoteString as! Sql) // GOOD
64+
}

0 commit comments

Comments
 (0)