Skip to content

Commit fb0016e

Browse files
MathiasVPMathiasVP
authored
Merge pull request #14485 from geoffw0/logging
Swift: Add more sinks to `swift/cleartext-logging`
2 parents 9a2ac65 + 5c00858 commit fb0016e

File tree

7 files changed

+202
-62
lines changed

7 files changed

+202
-62
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ private import Numeric
2020
private import PointerTypes
2121
private import Sequence
2222
private import Set
23+
private import Stream
2324
private import String
2425
private import Url
2526
private import UrlSession

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
/**
2+
* Provides models for `TextOutputStream` and related Swift classes.
3+
*/
4+
5+
import swift
6+
private import codeql.swift.dataflow.ExternalFlow
7+
8+
/**
9+
* A model for members of `TextOutputStream` and similar classes and methods that permit taint flow.
10+
*/
11+
private class StringSummaries extends SummaryModelCsv {
12+
override predicate row(string row) {
13+
row =
14+
[
15+
";TextOutputStream;true;write(_:);;;Argument[0];Argument[-1];taint",
16+
";TextOutputStreamable;true;write(to:);;;Argument[-1];Argument[0];taint",
17+
";;false;print(_:separator:terminator:to:);;;Argument[0].CollectionElement;Argument[3];taint",
18+
";;false;print(_:separator:terminator:to:);;;Argument[1..2];Argument[3];taint",
19+
";;false;debugPrint(_:separator:terminator:to:);;;Argument[0].CollectionElement;Argument[3];taint",
20+
";;false;debugPrint(_:separator:terminator:to:);;;Argument[1..2];Argument[3];taint",
21+
";;false;dump(_:to:name:indent:maxDepth:maxItems:);;;Argument[0,2];Argument[1];taint",
22+
]
23+
}
24+
}

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,8 +111,6 @@ private class StringSummaries extends SummaryModelCsv {
111111
";String;true;init(validatingPlatformString:);;;Argument[0].CollectionElement;ReturnValue.OptionalSome;taint",
112112
";String;true;localizedStringWithFormat(_:_:);;;Argument[0];ReturnValue;taint",
113113
";String;true;localizedStringWithFormat(_:_:);;;Argument[1].CollectionElement;ReturnValue;taint",
114-
";String;true;write(_:);;;Argument[0];Argument[-1];taint",
115-
";String;true;write(to:);;;Argument[-1];Argument[0];taint",
116114
";String;true;insert(contentsOf:at:);;;Argument[0];Argument[-1];taint",
117115
";String;true;replaceSubrange(_:with:);;;Argument[1];Argument[-1];taint",
118116
";String;true;max();;;Argument[-1];ReturnValue;taint",

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

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -99,16 +99,16 @@ private class LoggingSinks extends SinkModelCsv {
9999
[
100100
";;false;print(_:separator:terminator:);;;Argument[0..2];log-injection",
101101
";;false;print(_:separator:terminator:toStream:);;;Argument[0..2];log-injection",
102-
";;false;print(_:separator:terminator:to:);;;Argument[0..2];log-injection",
103102
";;false;debugPrint(_:separator:terminator:);;;Argument[0..2];log-injection",
104-
";;false;debugPrint(_:separator:terminator:to:);;;Argument[0..2];log-injection",
105103
";;false;dump(_:name:indent:maxDepth:maxItems:);;;Argument[0..1];log-injection",
106-
";;false;dump(_:to:name:indent:maxDepth:maxItems:);;;Argument[0];log-injection",
107-
";;false;dump(_:to:name:indent:maxDepth:maxItems:);;;Argument[2];log-injection",
104+
";;false;assert(_:_:file:line:);;;Argument[1];log-injection",
105+
";;false;assertionFailure(_:file:line:);;;Argument[0];log-injection",
106+
";;false;precondition(_:_:file:line:);;;Argument[1];log-injection",
107+
";;false;preconditionFailure(_:file:line:);;;Argument[0];log-injection",
108108
";;false;fatalError(_:file:line:);;;Argument[0];log-injection",
109109
";;false;NSLog(_:_:);;;Argument[0..1];log-injection",
110110
";;false;NSLogv(_:_:);;;Argument[0..1];log-injection",
111-
";;false;vfprintf(_:_:_:);;;Agument[1..2];log-injection",
111+
";;false;vfprintf(_:_:_:);;;Argument[1..2];log-injection",
112112
";Logger;true;log(_:);;;Argument[0];log-injection",
113113
";Logger;true;log(level:_:);;;Argument[1];log-injection",
114114
";Logger;true;trace(_:);;;Argument[1];log-injection",
@@ -119,6 +119,10 @@ private class LoggingSinks extends SinkModelCsv {
119119
";Logger;true;error(_:);;;Argument[1];log-injection",
120120
";Logger;true;critical(_:);;;Argument[1];log-injection",
121121
";Logger;true;fault(_:);;;Argument[1];log-injection",
122+
";;false;os_log(_:);;;Argument[0];log-injection",
123+
";;false;os_log(_:log:_:);;;Argument[2];log-injection",
124+
";;false;os_log(_:dso:log:_:_:);;;Argument[0,4];log-injection",
125+
";;false;os_log(_:dso:log:type:_:);;;Argument[0,4];log-injection",
122126
]
123127
}
124128
}

‎swift/ql/src/change-notes/2023-10-12-cleartext-logging.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 more new logging sinks to the `swift/cleartext-logging` query.

‎swift/ql/test/query-tests/Security/CWE-312/CleartextLoggingTest.expected

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
1-
failures
21
testFailures
2+
failures

‎swift/ql/test/query-tests/Security/CWE-312/cleartextLoggingTest.swift

Lines changed: 162 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
// --- stubs ---
22

3+
class NSObject { }
4+
35
func NSLog(_ format: String, _ args: CVarArg...) {}
46
func NSLogv(_ format: String, _ args: CVaListPointer) {}
57
func getVaList(_ args: [CVarArg]) -> CVaListPointer { return CVaListPointer(_fromUnsafeMutablePointer: UnsafeMutablePointer(bitPattern: 0)!) }
@@ -78,63 +80,88 @@ struct Logger {
7880
func warning(_: OSLogMessage) {}
7981
func fault(_: OSLogMessage) {}
8082
func critical(_: OSLogMessage) {}
83+
}
8184

85+
class OSLog : NSObject {
86+
static let `default` = OSLog(rawValue: 0)
87+
let rawValue: UInt8
88+
init(rawValue: UInt8) { self.rawValue = rawValue}
8289
}
8390

91+
extension String : CVarArg {
92+
public var _cVarArgEncoding: [Int] { get { return [] } }
93+
}
94+
95+
// from ObjC API; slightly simplified.
96+
func os_log(_ message: StaticString,
97+
dso: UnsafeRawPointer? = nil,
98+
log: OSLog = .default,
99+
type: OSLogType = .default,
100+
_ args: CVarArg...) { }
101+
84102
// --- tests ---
85103

86104
func test1(password: String, passwordHash : String, passphrase: String, pass_phrase: String) {
87-
print(password) // $ hasCleartextLogging=87
88-
print(password, separator: "") // $ $ hasCleartextLogging=88
89-
print("", separator: password) // $ hasCleartextLogging=89
90-
print(password, separator: "", terminator: "") // $ hasCleartextLogging=90
91-
print("", separator: password, terminator: "") // $ hasCleartextLogging=91
92-
print("", separator: "", terminator: password) // $ hasCleartextLogging=92
93-
print(passwordHash) // Safe
94-
95-
NSLog(password) // $ hasCleartextLogging=95
96-
NSLog("%@", password as! CVarArg) // $ hasCleartextLogging=96
97-
NSLog("%@ %@", "" as! CVarArg, password as! CVarArg) // $ hasCleartextLogging=97
98-
NSLog("\(password)") // $ hasCleartextLogging=98
99-
NSLogv("%@", getVaList([password as! CVarArg])) // $ hasCleartextLogging=99
100-
NSLogv("%@ %@", getVaList(["" as! CVarArg, password as! CVarArg])) // $ hasCleartextLogging=100
101-
NSLog(passwordHash) // SAfe
102-
NSLogv("%@", getVaList([passwordHash as! CVarArg])) // Safe
105+
print(password) // $ hasCleartextLogging=105
106+
print(password, separator: "") // $ $ hasCleartextLogging=106
107+
print("", separator: password) // $ hasCleartextLogging=107
108+
print(password, separator: "", terminator: "") // $ hasCleartextLogging=108
109+
print("", separator: password, terminator: "") // $ hasCleartextLogging=109
110+
print("", separator: "", terminator: password) // $ hasCleartextLogging=110
111+
print(passwordHash) // safe
112+
113+
debugPrint(password) // $ hasCleartextLogging=113
114+
115+
dump(password) // $ hasCleartextLogging=115
116+
117+
NSLog(password) // $ hasCleartextLogging=117
118+
NSLog("%@", password) // $ hasCleartextLogging=118
119+
NSLog("%@ %@", "", password) // $ hasCleartextLogging=119
120+
NSLog("\(password)") // $ hasCleartextLogging=120
121+
NSLogv("%@", getVaList([password])) // $ hasCleartextLogging=121
122+
NSLogv("%@ %@", getVaList(["", password])) // $ hasCleartextLogging=122
123+
NSLog(passwordHash) // safe
124+
NSLogv("%@", getVaList([passwordHash])) // safe
103125

104126
let bankAccount: Int = 0
105127
let log = Logger()
106128
// These MISSING test cases will be fixed when we properly generate the CFG around autoclosures.
107-
log.log("\(password)") // Safe
108-
log.log("\(password, privacy: .auto)") // Safe
109-
log.log("\(password, privacy: .private)") // Safe
110-
log.log("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=110
111-
log.log("\(passwordHash, privacy: .public)") // Safe
112-
log.log("\(password, privacy: .sensitive)") // Safe
113-
log.log("\(bankAccount)") // $ MISSING: hasCleartextLogging=113
114-
log.log("\(bankAccount, privacy: .auto)") // $ MISSING: hasCleartextLogging=114
115-
log.log("\(bankAccount, privacy: .private)") // Safe
116-
log.log("\(bankAccount, privacy: .public)") // $ MISSING: hasCleartextLogging=116
117-
log.log("\(bankAccount, privacy: .sensitive)") // Safe
118-
log.log(level: .default, "\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=118
119-
log.trace("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=119
120-
log.trace("\(passwordHash, privacy: .public)") // Safe
121-
log.debug("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=121
122-
log.debug("\(passwordHash, privacy: .public)") // Safe
123-
log.info("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=123
124-
log.info("\(passwordHash, privacy: .public)") // Safe
125-
log.notice("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=125
126-
log.notice("\(passwordHash, privacy: .public)") // Safe
127-
log.warning("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=127
128-
log.warning("\(passwordHash, privacy: .public)") // Safe
129-
log.error("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=129
130-
log.error("\(passwordHash, privacy: .public)") // Safe
131-
log.critical("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=131
132-
log.critical("\(passwordHash, privacy: .public)") // Safe
133-
log.fault("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=133
134-
log.fault("\(passwordHash, privacy: .public)") // Safe
135-
136-
NSLog(passphrase) // $ hasCleartextLogging=136
137-
NSLog(pass_phrase) // $ hasCleartextLogging=137
129+
log.log("\(password)") // safe
130+
log.log("\(password, privacy: .auto)") // safe
131+
log.log("\(password, privacy: .private)") // safe
132+
log.log("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=132
133+
log.log("\(passwordHash, privacy: .public)") // safe
134+
log.log("\(password, privacy: .sensitive)") // safe
135+
log.log("\(bankAccount)") // $ MISSING: hasCleartextLogging=135
136+
log.log("\(bankAccount, privacy: .auto)") // $ MISSING: hasCleartextLogging=136
137+
log.log("\(bankAccount, privacy: .private)") // safe
138+
log.log("\(bankAccount, privacy: .public)") // $ MISSING: hasCleartextLogging=138
139+
log.log("\(bankAccount, privacy: .sensitive)") // safe
140+
log.log(level: .default, "\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=140
141+
log.trace("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=141
142+
log.trace("\(passwordHash, privacy: .public)") // safe
143+
log.debug("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=143
144+
log.debug("\(passwordHash, privacy: .public)") // safe
145+
log.info("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=145
146+
log.info("\(passwordHash, privacy: .public)") // safe
147+
log.notice("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=147
148+
log.notice("\(passwordHash, privacy: .public)") // safe
149+
log.warning("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=149
150+
log.warning("\(passwordHash, privacy: .public)") // safe
151+
log.error("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=151
152+
log.error("\(passwordHash, privacy: .public)") // safe
153+
log.critical("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=153
154+
log.critical("\(passwordHash, privacy: .public)") // safe
155+
log.fault("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=155
156+
log.fault("\(passwordHash, privacy: .public)") // safe
157+
158+
NSLog(passphrase) // $ hasCleartextLogging=158
159+
NSLog(pass_phrase) // $ hasCleartextLogging=159
160+
161+
os_log("%@", log: .default, type: .default, "") // safe
162+
os_log("%@", log: .default, type: .default, password) // $ hasCleartextLogging=162
163+
os_log("%@ %@ %@", log: .default, type: .default, "", "", password) // $ hasCleartextLogging=163
164+
138165
}
139166

140167
class MyClass {
@@ -148,16 +175,16 @@ func doSomething(password: String) { }
148175
func test3(x: String) {
149176
// alternative evidence of sensitivity...
150177

151-
NSLog(x) // $ MISSING: hasCleartextLogging=152
178+
NSLog(x) // $ MISSING: hasCleartextLogging=179
152179
doSomething(password: x);
153-
NSLog(x) // $ hasCleartextLogging=152
180+
NSLog(x) // $ hasCleartextLogging=179
154181

155182
let y = getPassword();
156-
NSLog(y) // $ hasCleartextLogging=155
183+
NSLog(y) // $ hasCleartextLogging=182
157184

158185
let z = MyClass()
159-
NSLog(z.harmless) // Safe
160-
NSLog(z.password) // $ hasCleartextLogging=160
186+
NSLog(z.harmless) // safe
187+
NSLog(z.password) // $ hasCleartextLogging=187
161188
}
162189

163190
struct MyOuter {
@@ -170,6 +197,87 @@ struct MyOuter {
170197
}
171198

172199
func test3(mo : MyOuter) {
173-
NSLog(mo.password.value) // $ hasCleartextLogging=173
174-
NSLog(mo.harmless.value) // Safe
200+
// struct members...
201+
202+
NSLog(mo.password.value) // $ hasCleartextLogging=202
203+
NSLog(mo.harmless.value) // safe
204+
}
205+
206+
func test4(harmless: String, password: String) {
207+
// functions with an `in:` target for the write...
208+
var myString1 = ""
209+
var myString2 = ""
210+
var myString3 = ""
211+
var myString4 = ""
212+
var myString5 = ""
213+
var myString6 = ""
214+
var myString7 = ""
215+
var myString8 = ""
216+
var myString9 = ""
217+
var myString10 = ""
218+
var myString11 = ""
219+
var myString12 = ""
220+
var myString13 = ""
221+
222+
print(harmless, to: &myString1)
223+
print(myString1) // safe
224+
225+
print(password, to: &myString2)
226+
print(myString2) // $ hasCleartextLogging=225
227+
228+
print("log: " + password, to: &myString3)
229+
print(myString3) // $ hasCleartextLogging=228
230+
231+
debugPrint(harmless, to: &myString4)
232+
debugPrint(myString4) // safe
233+
234+
debugPrint(password, to: &myString5)
235+
debugPrint(myString5) // $ hasCleartextLogging=234
236+
237+
dump(harmless, to: &myString6)
238+
dump(myString6) // safe
239+
240+
dump(password, to: &myString7)
241+
dump(myString7) // $ hasCleartextLogging=240
242+
243+
myString8.write(harmless)
244+
print(myString8)
245+
246+
myString9.write(password)
247+
print(myString9) // $ hasCleartextLogging=246
248+
249+
myString10.write(harmless)
250+
myString10.write(password)
251+
myString10.write(harmless)
252+
print(myString10) // $ hasCleartextLogging=250
253+
254+
harmless.write(to: &myString11)
255+
print(myString11)
256+
257+
password.write(to: &myString12)
258+
print(myString12) // $ hasCleartextLogging=257
259+
260+
print(password, to: &myString13) // $ safe - only printed to another string
261+
debugPrint(password, to: &myString13) // $ safe - only printed to another string
262+
dump(password, to: &myString13) // $ safe - only printed to another string
263+
myString13.write(password) // safe - only printed to another string
264+
password.write(to: &myString13) // safe - only printed to another string
265+
}
266+
267+
func test5(password: String, caseNum: Int) {
268+
// `assert` methods...
269+
// (these would only be a danger in certain builds)
270+
271+
switch caseNum {
272+
case 0:
273+
assert(false, password) // $ MISSING: hasCleartextLogging=273
274+
case 1:
275+
assertionFailure(password) // $ MISSING: hasCleartextLogging=275
276+
case 2:
277+
precondition(false, password) // $ MISSING: hasCleartextLogging=277
278+
case 3:
279+
preconditionFailure(password) // $ MISSING: hasCleartextLogging=279
280+
default:
281+
fatalError(password) // $ MISSING: hasCleartextLogging=281
282+
}
175283
}

0 commit comments

Comments
 (0)