Skip to content

Commit 34b33e1

Browse files
geoffw0geoffw0
authored
Merge pull request #14328 from geoffw0/debugdesc
Swift: Model .description, .debugDescription more generally
2 parents c518f39 + 98b2ef0 commit 34b33e1

9 files changed

Lines changed: 45 additions & 20 deletions

File tree

‎swift/ql/lib/change-notes/2023-09-27-debugdesc.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+
* Modelled `CustomStringConvertible.description` and `CustomDebugStringConvertible.debugDescription`, replacing ad-hoc models of these properties on derived classes.

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

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -61,11 +61,7 @@ private class FilePathFieldsInheritTaint extends TaintInheritingContent,
6161
FilePathFieldsInheritTaint() {
6262
exists(FieldDecl f | this.getField() = f |
6363
f.getEnclosingDecl().asNominalTypeDecl() instanceof FilePath and
64-
f.getName() =
65-
[
66-
"description", "debugDescription", "components", "extension", "lastComponent", "root",
67-
"stem", "string"
68-
]
64+
f.getName() = ["components", "extension", "lastComponent", "root", "stem", "string"]
6965
)
7066
}
7167
}

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

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -131,8 +131,8 @@ private class StringSummaries extends SummaryModelCsv {
131131
}
132132

133133
/**
134-
* A content implying that, if a `String` is tainted, then many of its fields are
135-
* tainted. This also includes fields declared in `StringProtocol`.
134+
* A content implying that, if a `String`, `StringProtocol` or related class is tainted, then many
135+
* of its fields are tainted.
136136
*/
137137
private class StringFieldsInheritTaint extends TaintInheritingContent,
138138
DataFlow::Content::FieldContent
@@ -141,12 +141,24 @@ private class StringFieldsInheritTaint extends TaintInheritingContent,
141141
this.getField()
142142
.hasQualifiedName(["String", "StringProtocol"],
143143
[
144-
"unicodeScalars", "utf8", "utf16", "lazy", "utf8CString", "description",
145-
"debugDescription", "dataValue", "identifierValue", "capitalized",
146-
"localizedCapitalized", "localizedLowercase", "localizedUppercase",
147-
"decomposedStringWithCanonicalMapping", "decomposedStringWithCompatibilityMapping",
148-
"precomposedStringWithCanonicalMapping", "precomposedStringWithCompatibilityMapping",
149-
"removingPercentEncoding"
144+
"unicodeScalars", "utf8", "utf16", "lazy", "utf8CString", "dataValue",
145+
"identifierValue", "capitalized", "localizedCapitalized", "localizedLowercase",
146+
"localizedUppercase", "decomposedStringWithCanonicalMapping",
147+
"decomposedStringWithCompatibilityMapping", "precomposedStringWithCanonicalMapping",
148+
"precomposedStringWithCompatibilityMapping", "removingPercentEncoding"
150149
])
150+
or
151+
exists(FieldDecl fieldDecl, Decl declaringDecl, TypeDecl namedTypeDecl |
152+
(
153+
namedTypeDecl.getFullName() = "CustomStringConvertible" and
154+
fieldDecl.getName() = "description"
155+
or
156+
namedTypeDecl.getFullName() = "CustomDebugStringConvertible" and
157+
fieldDecl.getName() = "debugDescription"
158+
) and
159+
declaringDecl.getAMember() = fieldDecl and
160+
declaringDecl.asNominalTypeDecl() = namedTypeDecl.getADerivedTypeDecl*() and
161+
this.getField() = fieldDecl
162+
)
151163
}
152164
}

‎swift/ql/test/library-tests/dataflow/taint/core/LocalTaint.expected‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,8 +89,10 @@
8989
| conversions.swift:90:12:90:12 | [post] ms1 | conversions.swift:91:12:91:12 | ms1 |
9090
| conversions.swift:90:12:90:12 | ms1 | conversions.swift:91:12:91:12 | ms1 |
9191
| conversions.swift:91:12:91:12 | [post] ms1 | conversions.swift:92:12:92:12 | ms1 |
92+
| conversions.swift:91:12:91:12 | ms1 | conversions.swift:91:12:91:16 | .description |
9293
| conversions.swift:91:12:91:12 | ms1 | conversions.swift:92:12:92:12 | ms1 |
9394
| conversions.swift:92:12:92:12 | [post] ms1 | conversions.swift:93:12:93:12 | ms1 |
95+
| conversions.swift:92:12:92:12 | ms1 | conversions.swift:92:12:92:16 | .debugDescription |
9496
| conversions.swift:92:12:92:12 | ms1 | conversions.swift:93:12:93:12 | ms1 |
9597
| conversions.swift:95:6:95:6 | SSA def(ms2) | conversions.swift:96:12:96:12 | ms2 |
9698
| conversions.swift:95:6:95:6 | ms2 | conversions.swift:95:6:95:6 | SSA def(ms2) |
@@ -100,8 +102,10 @@
100102
| conversions.swift:96:12:96:12 | [post] ms2 | conversions.swift:97:12:97:12 | ms2 |
101103
| conversions.swift:96:12:96:12 | ms2 | conversions.swift:97:12:97:12 | ms2 |
102104
| conversions.swift:97:12:97:12 | [post] ms2 | conversions.swift:98:12:98:12 | ms2 |
105+
| conversions.swift:97:12:97:12 | ms2 | conversions.swift:97:12:97:16 | .description |
103106
| conversions.swift:97:12:97:12 | ms2 | conversions.swift:98:12:98:12 | ms2 |
104107
| conversions.swift:98:12:98:12 | [post] ms2 | conversions.swift:99:12:99:12 | ms2 |
108+
| conversions.swift:98:12:98:12 | ms2 | conversions.swift:98:12:98:16 | .debugDescription |
105109
| conversions.swift:98:12:98:12 | ms2 | conversions.swift:99:12:99:12 | ms2 |
106110
| conversions.swift:103:6:103:6 | SSA def(parent) | conversions.swift:104:12:104:12 | parent |
107111
| conversions.swift:103:6:103:6 | parent | conversions.swift:103:6:103:6 | SSA def(parent) |

‎swift/ql/test/library-tests/dataflow/taint/core/Taint.expected‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,12 @@ edges
4747
| conversions.swift:87:19:87:32 | call to sourceString() | conversions.swift:87:12:87:33 | call to String.init(_:) |
4848
| conversions.swift:95:12:95:35 | call to MyString.init(_:) | conversions.swift:95:12:95:35 | call to MyString.init(_:) [some:0] |
4949
| conversions.swift:95:12:95:35 | call to MyString.init(_:) | conversions.swift:96:12:96:12 | ms2 |
50+
| conversions.swift:95:12:95:35 | call to MyString.init(_:) | conversions.swift:97:12:97:16 | .description |
51+
| conversions.swift:95:12:95:35 | call to MyString.init(_:) | conversions.swift:98:12:98:16 | .debugDescription |
5052
| conversions.swift:95:12:95:35 | call to MyString.init(_:) [some:0] | conversions.swift:95:12:95:36 | ...! |
5153
| conversions.swift:95:12:95:36 | ...! | conversions.swift:96:12:96:12 | ms2 |
54+
| conversions.swift:95:12:95:36 | ...! | conversions.swift:97:12:97:16 | .description |
55+
| conversions.swift:95:12:95:36 | ...! | conversions.swift:98:12:98:16 | .debugDescription |
5256
| conversions.swift:95:21:95:34 | call to sourceString() | conversions.swift:95:12:95:35 | call to MyString.init(_:) |
5357
| conversions.swift:103:31:103:44 | call to sourceString() | conversions.swift:104:12:104:12 | parent |
5458
| conversions.swift:103:31:103:44 | call to sourceString() | conversions.swift:105:12:105:12 | parent |
@@ -223,6 +227,8 @@ nodes
223227
| conversions.swift:95:12:95:36 | ...! | semmle.label | ...! |
224228
| conversions.swift:95:21:95:34 | call to sourceString() | semmle.label | call to sourceString() |
225229
| conversions.swift:96:12:96:12 | ms2 | semmle.label | ms2 |
230+
| conversions.swift:97:12:97:16 | .description | semmle.label | .description |
231+
| conversions.swift:98:12:98:16 | .debugDescription | semmle.label | .debugDescription |
226232
| conversions.swift:103:31:103:44 | call to sourceString() | semmle.label | call to sourceString() |
227233
| conversions.swift:104:12:104:12 | parent | semmle.label | parent |
228234
| conversions.swift:105:12:105:12 | parent | semmle.label | parent |
@@ -395,6 +401,8 @@ subpaths
395401
| conversions.swift:86:12:86:25 | call to sourceString() | conversions.swift:86:12:86:25 | call to sourceString() | conversions.swift:86:12:86:25 | call to sourceString() | result |
396402
| conversions.swift:87:12:87:33 | call to String.init(_:) | conversions.swift:87:19:87:32 | call to sourceString() | conversions.swift:87:12:87:33 | call to String.init(_:) | result |
397403
| conversions.swift:96:12:96:12 | ms2 | conversions.swift:95:21:95:34 | call to sourceString() | conversions.swift:96:12:96:12 | ms2 | result |
404+
| conversions.swift:97:12:97:16 | .description | conversions.swift:95:21:95:34 | call to sourceString() | conversions.swift:97:12:97:16 | .description | result |
405+
| conversions.swift:98:12:98:16 | .debugDescription | conversions.swift:95:21:95:34 | call to sourceString() | conversions.swift:98:12:98:16 | .debugDescription | result |
398406
| conversions.swift:104:12:104:12 | parent | conversions.swift:103:31:103:44 | call to sourceString() | conversions.swift:104:12:104:12 | parent | result |
399407
| conversions.swift:105:12:105:12 | parent | conversions.swift:103:31:103:44 | call to sourceString() | conversions.swift:105:12:105:12 | parent | result |
400408
| conversions.swift:108:12:108:12 | v3 | conversions.swift:103:31:103:44 | call to sourceString() | conversions.swift:108:12:108:12 | v3 | result |

‎swift/ql/test/library-tests/dataflow/taint/core/conversions.swift‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,8 +94,8 @@ func testConversions() {
9494

9595
let ms2 = MyString(sourceString())!
9696
sink(arg: ms2) // $ tainted=95
97-
sink(arg: ms2.description) // $ MISSING: tainted=
98-
sink(arg: ms2.debugDescription) // $ MISSING: tainted=
97+
sink(arg: ms2.description) // $ tainted=95
98+
sink(arg: ms2.debugDescription) // $ tainted=95
9999
sink(arg: ms2.clean)
100100

101101
// ---

‎swift/ql/test/library-tests/dataflow/taint/libraries/files.swift‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ enum CInterop {
99
typealias PlatformChar = CInterop.Char
1010
}
1111

12-
struct FilePath {
12+
struct FilePath : CustomStringConvertible, CustomDebugStringConvertible {
1313
struct Component {
1414
init?(_ string: String) { }
1515

‎swift/ql/test/library-tests/dataflow/taint/libraries/string.swift‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -584,7 +584,7 @@ func taintedThroughConversion() {
584584
sink(arg: String(0))
585585
sink(arg: String(source())) // $ tainted=585
586586
sink(arg: Int(0).description)
587-
sink(arg: source().description) // $ MISSING: tainted=587
587+
sink(arg: source().description) // $ tainted=587
588588
sink(arg: String(describing: 0))
589589
sink(arg: String(describing: source())) // $ tainted=589
590590

‎swift/ql/test/library-tests/dataflow/taint/libraries/url.swift‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ struct URLResource {
154154
let subdirectory: String?
155155
}
156156

157-
struct URLRequest {
157+
struct URLRequest : CustomStringConvertible, CustomDebugStringConvertible {
158158
enum CachePolicy { case none }
159159
enum NetworkServiceType { case none }
160160
enum Attribution { case none }
@@ -463,9 +463,9 @@ func taintThroughUrlRequest() {
463463
sink(any: clean.attribution)
464464
sink(any: tainted.attribution)
465465
sink(any: clean.description)
466-
sink(any: tainted.description)
466+
sink(any: tainted.description) // $ tainted=431
467467
sink(any: clean.debugDescription)
468-
sink(any: tainted.debugDescription)
468+
sink(any: tainted.debugDescription) // $ tainted=431
469469
sink(any: clean.customMirror)
470470
sink(any: tainted.customMirror)
471471
sink(any: clean.hashValue)

0 commit comments

Comments
 (0)