Skip to content

Commit b203a9e

Browse files
atorralbaatorralba
committed
Add a sanitizer for OSLogPrivacy options
Add test cases to verify how the sanitizer behaves depending on the argument type and the privacy option being used.
1 parent aad5609 commit b203a9e

File tree

2 files changed

+100
-31
lines changed

2 files changed

+100
-31
lines changed

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

Lines changed: 43 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -29,15 +29,51 @@ private class DefaultCleartextLoggingSink extends CleartextLoggingSink {
2929
DefaultCleartextLoggingSink() { sinkNode(this, "logging") }
3030
}
3131

32-
// TODO: Remove this. It shouldn't be necessary.
33-
private class EncryptionCleartextLoggingSanitizer extends CleartextLoggingSanitizer {
34-
EncryptionCleartextLoggingSanitizer() { this.asExpr() instanceof EncryptedExpr }
32+
/**
33+
* A sanitizer for `OSLogMessage`s configured with the appropriate privacy option.
34+
* Numeric and boolean arguments aren't redacted unless the `public` option is used.
35+
* Arguments of other types are always redacted unless the `private` or `sensitive` options are used.
36+
*/
37+
private class OsLogPrivacyCleartextLoggingSanitizer extends CleartextLoggingSanitizer {
38+
OsLogPrivacyCleartextLoggingSanitizer() {
39+
exists(CallExpr c, AutoClosureExpr e |
40+
c.getStaticTarget().getName().matches("appendInterpolation(_:%privacy:%)") and
41+
c.getArgument(0).getExpr() = e and
42+
this.asExpr() = e
43+
|
44+
e.getExpr().getType() instanceof OsLogNonRedactedType and
45+
c.getArgumentWithLabel("privacy").getExpr().(OsLogPrivacyRef).isSafe()
46+
or
47+
not e.getExpr().getType() instanceof OsLogNonRedactedType and
48+
not c.getArgumentWithLabel("privacy").getExpr().(OsLogPrivacyRef).isPublic()
49+
)
50+
}
3551
}
3652

37-
/*
38-
* TODO: Add a sanitizer for the OsLogMessage interpolation with .private/.sensitive privacy options,
39-
* or restrict the sinks to require .public interpolation depending on what the default behavior is.
40-
*/
53+
/** A type that isn't redacted by default in an `OSLogMessage`. */
54+
private class OsLogNonRedactedType extends Type {
55+
OsLogNonRedactedType() {
56+
this.getName() = [["", "U"] + "Int" + ["", "16", "32", "64"], "Double", "Float", "Bool"]
57+
}
58+
}
59+
60+
/** A reference to a field of `OsLogPrivacy`. */
61+
private class OsLogPrivacyRef extends MemberRefExpr {
62+
string optionName;
63+
64+
OsLogPrivacyRef() {
65+
exists(FieldDecl f | this.getMember() = f |
66+
f.getEnclosingDecl().(NominalTypeDecl).getName() = "OSLogPrivacy" and
67+
optionName = f.getName()
68+
)
69+
}
70+
71+
/** Holds if this is a safe privacy option (private or sensitive). */
72+
predicate isSafe() { optionName = ["private", "sensitive"] }
73+
74+
/** Holds if this is a public (that is, unsafe) privacy option. */
75+
predicate isPublic() { optionName = "public" }
76+
}
4177

4278
private class LoggingSinks extends SinkModelCsv {
4379
override predicate row(string row) {

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

Lines changed: 57 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,16 @@ struct OSLogStringAlignment {
1313
static var none = OSLogStringAlignment()
1414
}
1515

16+
enum OSLogIntegerFormatting { case decimal }
17+
enum OSLogInt32ExtendedFormat { case none }
18+
enum OSLogFloatFormatting { case fixed }
19+
enum OSLogBoolFormat { case truth }
20+
1621
struct OSLogPrivacy {
1722
enum Mask { case none }
1823
static var auto = OSLogPrivacy()
1924
static var `private` = OSLogPrivacy()
25+
static var `public` = OSLogPrivacy()
2026
static var sensitive = OSLogPrivacy()
2127

2228
static func auto(mask: OSLogPrivacy.Mask) -> OSLogPrivacy { return .auto }
@@ -30,6 +36,23 @@ struct OSLogInterpolation : StringInterpolationProtocol {
3036
func appendLiteral(_: Self.StringLiteralType) {}
3137
mutating func appendInterpolation(_ argumentString: @autoclosure @escaping () -> String, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {}
3238
mutating func appendInterpolation(_ argumentString: @autoclosure @escaping () -> String, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto, attributes: String) {}
39+
mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Int, format: OSLogIntegerFormatting = .decimal, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {}
40+
mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Int8, format: OSLogIntegerFormatting = .decimal, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {}
41+
mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Int16, format: OSLogIntegerFormatting = .decimal, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {}
42+
mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Int32, format: OSLogInt32ExtendedFormat, privacy: OSLogPrivacy = .auto) {}
43+
mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Int32, format: OSLogInt32ExtendedFormat, privacy: OSLogPrivacy = .auto, attributes: String) {}
44+
mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Int32, format: OSLogIntegerFormatting = .decimal, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {}
45+
mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Int64, format: OSLogIntegerFormatting = .decimal, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {}
46+
mutating func appendInterpolation(_ number: @autoclosure @escaping () -> UInt, format: OSLogIntegerFormatting = .decimal, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {}
47+
mutating func appendInterpolation(_ number: @autoclosure @escaping () -> UInt8, format: OSLogIntegerFormatting = .decimal, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {}
48+
mutating func appendInterpolation(_ number: @autoclosure @escaping () -> UInt16, format: OSLogIntegerFormatting = .decimal, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {}
49+
mutating func appendInterpolation(_ number: @autoclosure @escaping () -> UInt32, format: OSLogIntegerFormatting = .decimal, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {}
50+
mutating func appendInterpolation(_ number: @autoclosure @escaping () -> UInt64, format: OSLogIntegerFormatting = .decimal, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {}
51+
mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Double, format: OSLogFloatFormatting = .fixed, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {}
52+
mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Double, format: OSLogFloatFormatting = .fixed, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto, attributes: String) {}
53+
mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Float,format: OSLogFloatFormatting = .fixed, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto) {}
54+
mutating func appendInterpolation(_ number: @autoclosure @escaping () -> Float, format: OSLogFloatFormatting = .fixed, align: OSLogStringAlignment = .none, privacy: OSLogPrivacy = .auto, attributes: String) {}
55+
mutating func appendInterpolation(_ boolean: @autoclosure @escaping () -> Bool, format: OSLogBoolFormat = .truth, privacy: OSLogPrivacy = .auto) {}
3356
}
3457

3558
struct OSLogMessage : ExpressibleByStringInterpolation {
@@ -60,32 +83,42 @@ struct Logger {
6083
// --- tests ---
6184

6285
func test1(password: String, passwordHash : String) {
63-
print(password) // $ MISSING: hasCleartextLogging=63
64-
print(password, separator: "") // $ MISSING: $ hasCleartextLogging=64
65-
print("", separator: password) // $ hasCleartextLogging=65
66-
print(password, separator: "", terminator: "") // $ MISSING: hasCleartextLogging=66
67-
print("", separator: password, terminator: "") // $ hasCleartextLogging=67
68-
print("", separator: "", terminator: password) // $ hasCleartextLogging=68
69-
70-
NSLog(password) // $ hasCleartextLogging=70
71-
NSLog("%@", password as! CVarArg) // $ MISSING: hasCleartextLogging=71
72-
NSLog("%@ %@", "" as! CVarArg, password as! CVarArg) // $ MISSING: hasCleartextLogging=72
73-
NSLog("\(password)") // $ hasCleartextLogging=73
74-
NSLogv("%@", getVaList([password as! CVarArg])) // $ MISSING: hasCleartextLogging=74
75-
NSLogv("%@ %@", getVaList(["" as! CVarArg, password as! CVarArg])) // $ MISSING: hasCleartextLogging=75
76-
86+
print(password) // $ MISSING: hasCleartextLogging=86
87+
print(password, separator: "") // $ MISSING: $ hasCleartextLogging=97
88+
print("", separator: password) // $ hasCleartextLogging=88
89+
print(password, separator: "", terminator: "") // $ MISSING: hasCleartextLogging=89
90+
print("", separator: password, terminator: "") // $ hasCleartextLogging=90
91+
print("", separator: "", terminator: password) // $ hasCleartextLogging=91
92+
93+
NSLog(password) // $ hasCleartextLogging=93
94+
NSLog("%@", password as! CVarArg) // $ MISSING: hasCleartextLogging=94
95+
NSLog("%@ %@", "" as! CVarArg, password as! CVarArg) // $ MISSING: hasCleartextLogging=95
96+
NSLog("\(password)") // $ hasCleartextLogging=96
97+
NSLogv("%@", getVaList([password as! CVarArg])) // $ MISSING: hasCleartextLogging=97
98+
NSLogv("%@ %@", getVaList(["" as! CVarArg, password as! CVarArg])) // $ MISSING: hasCleartextLogging=98
99+
100+
let bankAccount: Int = 0
77101
let log = Logger()
78-
log.log("\(password)") // $ hasCleartextLogging=78
102+
// These MISSING test cases will be fixed when we properly generate the CFG around autoclosures.
103+
log.log("\(password)") // Safe
104+
log.log("\(password, privacy: .auto)") // Safe
79105
log.log("\(password, privacy: .private)") // Safe
80-
log.log(level: .default, "\(password)") // $ hasCleartextLogging=80
81-
log.trace("\(password)") // $ hasCleartextLogging=81
82-
log.debug("\(password)") // $ hasCleartextLogging=82
83-
log.info("\(password)") // $ hasCleartextLogging=83
84-
log.notice("\(password)") // $ hasCleartextLogging=84
85-
log.warning("\(password)") // $ hasCleartextLogging=85
86-
log.error("\(password)") // $ hasCleartextLogging=86
87-
log.critical("\(password)") // $ hasCleartextLogging=87
88-
log.fault("\(password)") // $ hasCleartextLogging=88
106+
log.log("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=106
107+
log.log("\(password, privacy: .sensitive)") // Safe
108+
log.log("\(bankAccount)") // $ MISSING: hasCleartextLogging=108
109+
log.log("\(bankAccount, privacy: .auto)") // $ MISSING: hasCleartextLogging=109
110+
log.log("\(bankAccount, privacy: .private)") // Safe
111+
log.log("\(bankAccount, privacy: .public)") // $ MISSING: hasCleartextLogging=111
112+
log.log("\(bankAccount, privacy: .sensitive)") // Safe
113+
log.log(level: .default, "\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=113
114+
log.trace("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=114
115+
log.debug("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=115
116+
log.info("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=116
117+
log.notice("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=117
118+
log.warning("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=118
119+
log.error("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=119
120+
log.critical("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=120
121+
log.fault("\(password, privacy: .public)") // $ MISSING: hasCleartextLogging=121
89122
}
90123
/*
91124
class MyClass {

0 commit comments

Comments
 (0)