Skip to content

Commit 2028b5e

Browse files
geoffw0geoffw0
committed
Swift: Fix imprecise sinks.
1 parent 1908033 commit 2028b5e

File tree

3 files changed

+73
-32
lines changed

3 files changed

+73
-32
lines changed

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

Lines changed: 49 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -180,14 +180,6 @@ private class StringLengthConflationSinks extends SinkModelCsv {
180180
override predicate row(string row) {
181181
row =
182182
[
183-
";Sequence;true;dropFirst(_:);;;Argument[0];string-length",
184-
";Sequence;true;dropLast(_:);;;Argument[0];string-length",
185-
";Sequence;true;prefix(_:);;;Argument[0];string-length",
186-
";Sequence;true;suffix(_:);;;Argument[0];string-length",
187-
";Collection;true;formIndex(_:offsetBy:);;;Argument[0..1];string-length",
188-
";Collection;true;formIndex(_:offsetBy:limitBy:);;;Argument[0..1];string-length",
189-
";Collection;true;removeFirst(_:);;;Argument[0];string-length",
190-
";RangeReplaceableCollection;true;removeLast(_:);;;Argument[0];string-length",
191183
";String;true;index(_:offsetBy:);;;Argument[0..1];string-length",
192184
";String;true;index(_:offsetBy:limitBy:);;;Argument[0..1];string-length",
193185
";String.Index;true;init(encodedOffset:);;;Argument[0];string-length",
@@ -203,3 +195,52 @@ private class StringLengthConflationSinks extends SinkModelCsv {
203195
]
204196
}
205197
}
198+
199+
/**
200+
* An extra sink that don't fit into the CSV scheme (because we care about the actual
201+
* type the method is being called on, not just the type it's declared on).
202+
*/
203+
private class ExtraStringLengthConflationSink extends StringLengthConflationSink {
204+
StringType stringType;
205+
206+
ExtraStringLengthConflationSink() {
207+
exists(CallExpr call, string typeName |
208+
(
209+
// `String`
210+
typeName = "String" and
211+
stringType = TString()
212+
or
213+
// `String.utf8`
214+
typeName = "String.UTF8View" and
215+
stringType = TStringUtf8()
216+
or
217+
// `String.utf16`
218+
typeName = "String.UTF16View" and
219+
stringType = TStringUtf16()
220+
or
221+
// `String.unicodeScalars`
222+
typeName = "String.UnicodeScalarView" and
223+
stringType = TStringUnicodeScalars()
224+
) and
225+
// sink is a length or offset argument to [type].[method]
226+
(
227+
call.getQualifier().getType().(NominalType).getName() = typeName or
228+
call.getQualifier().getType().(InOutType).getObjectType().(NominalType).getName() = typeName
229+
) and
230+
(
231+
call.getStaticTarget().getName() =
232+
[
233+
"dropFirst(_:)", "dropLast(_:)", "prefix(_:)", "suffix(_:)", "removeFirst(_:)",
234+
"removeLast(_:)"
235+
] and
236+
this.asExpr() = call.getArgument(0).getExpr()
237+
or
238+
call.getStaticTarget().getName() =
239+
["formIndex(_:offsetBy:)", "formIndex(_:offsetBy:limitBy:)"] and
240+
this.asExpr() = call.getArgument([0, 1]).getExpr()
241+
)
242+
)
243+
}
244+
245+
override StringType getCorrectStringType() { result = stringType }
246+
}

‎swift/ql/test/query-tests/Security/CWE-135/StringLengthConflation.expected

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,13 @@ edges
2323
| StringLengthConflation.swift:170:29:170:46 | .count | StringLengthConflation.swift:170:29:170:54 | ... .-(_:_:) ... |
2424
| StringLengthConflation.swift:171:29:171:32 | .length | StringLengthConflation.swift:171:29:171:41 | ... .-(_:_:) ... |
2525
| StringLengthConflation.swift:172:29:172:33 | .length | StringLengthConflation.swift:172:29:172:42 | ... .-(_:_:) ... |
26+
| StringLengthConflation.swift:173:35:173:37 | .count | StringLengthConflation.swift:173:35:173:45 | ... .-(_:_:) ... |
2627
| StringLengthConflation.swift:174:35:174:42 | .count | StringLengthConflation.swift:174:35:174:50 | ... .-(_:_:) ... |
2728
| StringLengthConflation.swift:175:35:175:43 | .count | StringLengthConflation.swift:175:35:175:51 | ... .-(_:_:) ... |
28-
| StringLengthConflation.swift:176:35:176:52 | .count | StringLengthConflation.swift:176:35:176:60 | ... .-(_:_:) ... |
2929
| StringLengthConflation.swift:177:35:177:38 | .length | StringLengthConflation.swift:177:35:177:47 | ... .-(_:_:) ... |
3030
| StringLengthConflation.swift:178:35:178:39 | .length | StringLengthConflation.swift:178:35:178:48 | ... .-(_:_:) ... |
31-
| StringLengthConflation.swift:180:37:180:44 | .count | StringLengthConflation.swift:180:37:180:52 | ... .-(_:_:) ... |
32-
| StringLengthConflation.swift:182:37:182:45 | .count | StringLengthConflation.swift:182:37:182:53 | ... .-(_:_:) ... |
31+
| StringLengthConflation.swift:179:37:179:39 | .count | StringLengthConflation.swift:179:37:179:47 | ... .-(_:_:) ... |
32+
| StringLengthConflation.swift:181:37:181:39 | .count | StringLengthConflation.swift:181:37:181:47 | ... .-(_:_:) ... |
3333
| file://:0:0:0:0 | .length | StringLengthConflation.swift:53:43:53:46 | .length |
3434
| file://:0:0:0:0 | .length | StringLengthConflation.swift:60:47:60:50 | .length |
3535
| file://:0:0:0:0 | .length | StringLengthConflation.swift:66:33:66:36 | .length |
@@ -102,20 +102,20 @@ nodes
102102
| StringLengthConflation.swift:171:29:171:41 | ... .-(_:_:) ... | semmle.label | ... .-(_:_:) ... |
103103
| StringLengthConflation.swift:172:29:172:33 | .length | semmle.label | .length |
104104
| StringLengthConflation.swift:172:29:172:42 | ... .-(_:_:) ... | semmle.label | ... .-(_:_:) ... |
105+
| StringLengthConflation.swift:173:35:173:37 | .count | semmle.label | .count |
106+
| StringLengthConflation.swift:173:35:173:45 | ... .-(_:_:) ... | semmle.label | ... .-(_:_:) ... |
105107
| StringLengthConflation.swift:174:35:174:42 | .count | semmle.label | .count |
106108
| StringLengthConflation.swift:174:35:174:50 | ... .-(_:_:) ... | semmle.label | ... .-(_:_:) ... |
107109
| StringLengthConflation.swift:175:35:175:43 | .count | semmle.label | .count |
108110
| StringLengthConflation.swift:175:35:175:51 | ... .-(_:_:) ... | semmle.label | ... .-(_:_:) ... |
109-
| StringLengthConflation.swift:176:35:176:52 | .count | semmle.label | .count |
110-
| StringLengthConflation.swift:176:35:176:60 | ... .-(_:_:) ... | semmle.label | ... .-(_:_:) ... |
111111
| StringLengthConflation.swift:177:35:177:38 | .length | semmle.label | .length |
112112
| StringLengthConflation.swift:177:35:177:47 | ... .-(_:_:) ... | semmle.label | ... .-(_:_:) ... |
113113
| StringLengthConflation.swift:178:35:178:39 | .length | semmle.label | .length |
114114
| StringLengthConflation.swift:178:35:178:48 | ... .-(_:_:) ... | semmle.label | ... .-(_:_:) ... |
115-
| StringLengthConflation.swift:180:37:180:44 | .count | semmle.label | .count |
116-
| StringLengthConflation.swift:180:37:180:52 | ... .-(_:_:) ... | semmle.label | ... .-(_:_:) ... |
117-
| StringLengthConflation.swift:182:37:182:45 | .count | semmle.label | .count |
118-
| StringLengthConflation.swift:182:37:182:53 | ... .-(_:_:) ... | semmle.label | ... .-(_:_:) ... |
115+
| StringLengthConflation.swift:179:37:179:39 | .count | semmle.label | .count |
116+
| StringLengthConflation.swift:179:37:179:47 | ... .-(_:_:) ... | semmle.label | ... .-(_:_:) ... |
117+
| StringLengthConflation.swift:181:37:181:39 | .count | semmle.label | .count |
118+
| StringLengthConflation.swift:181:37:181:47 | ... .-(_:_:) ... | semmle.label | ... .-(_:_:) ... |
119119
| file://:0:0:0:0 | .length | semmle.label | .length |
120120
subpaths
121121
#select
@@ -164,12 +164,12 @@ subpaths
164164
| StringLengthConflation.swift:171:29:171:41 | ... .-(_:_:) ... | file://:0:0:0:0 | .length | StringLengthConflation.swift:171:29:171:41 | ... .-(_:_:) ... | This NSString length is used in a String, but it may not be equivalent. |
165165
| StringLengthConflation.swift:172:29:172:42 | ... .-(_:_:) ... | StringLengthConflation.swift:172:29:172:33 | .length | StringLengthConflation.swift:172:29:172:42 | ... .-(_:_:) ... | This NSString length is used in a String, but it may not be equivalent. |
166166
| StringLengthConflation.swift:172:29:172:42 | ... .-(_:_:) ... | file://:0:0:0:0 | .length | StringLengthConflation.swift:172:29:172:42 | ... .-(_:_:) ... | This NSString length is used in a String, but it may not be equivalent. |
167-
| StringLengthConflation.swift:174:35:174:50 | ... .-(_:_:) ... | StringLengthConflation.swift:174:35:174:42 | .count | StringLengthConflation.swift:174:35:174:50 | ... .-(_:_:) ... | This String.utf8 length is used in a String, but it may not be equivalent. |
168-
| StringLengthConflation.swift:175:35:175:51 | ... .-(_:_:) ... | StringLengthConflation.swift:175:35:175:43 | .count | StringLengthConflation.swift:175:35:175:51 | ... .-(_:_:) ... | This String.utf16 length is used in a String, but it may not be equivalent. |
169-
| StringLengthConflation.swift:176:35:176:60 | ... .-(_:_:) ... | StringLengthConflation.swift:176:35:176:52 | .count | StringLengthConflation.swift:176:35:176:60 | ... .-(_:_:) ... | This String.unicodeScalars length is used in a String, but it may not be equivalent. |
170-
| StringLengthConflation.swift:177:35:177:47 | ... .-(_:_:) ... | StringLengthConflation.swift:177:35:177:38 | .length | StringLengthConflation.swift:177:35:177:47 | ... .-(_:_:) ... | This NSString length is used in a String, but it may not be equivalent. |
171-
| StringLengthConflation.swift:177:35:177:47 | ... .-(_:_:) ... | file://:0:0:0:0 | .length | StringLengthConflation.swift:177:35:177:47 | ... .-(_:_:) ... | This NSString length is used in a String, but it may not be equivalent. |
172-
| StringLengthConflation.swift:178:35:178:48 | ... .-(_:_:) ... | StringLengthConflation.swift:178:35:178:39 | .length | StringLengthConflation.swift:178:35:178:48 | ... .-(_:_:) ... | This NSString length is used in a String, but it may not be equivalent. |
173-
| StringLengthConflation.swift:178:35:178:48 | ... .-(_:_:) ... | file://:0:0:0:0 | .length | StringLengthConflation.swift:178:35:178:48 | ... .-(_:_:) ... | This NSString length is used in a String, but it may not be equivalent. |
174-
| StringLengthConflation.swift:180:37:180:52 | ... .-(_:_:) ... | StringLengthConflation.swift:180:37:180:44 | .count | StringLengthConflation.swift:180:37:180:52 | ... .-(_:_:) ... | This String.utf8 length is used in a String, but it may not be equivalent. |
175-
| StringLengthConflation.swift:182:37:182:53 | ... .-(_:_:) ... | StringLengthConflation.swift:182:37:182:45 | .count | StringLengthConflation.swift:182:37:182:53 | ... .-(_:_:) ... | This String.utf16 length is used in a String, but it may not be equivalent. |
167+
| StringLengthConflation.swift:173:35:173:45 | ... .-(_:_:) ... | StringLengthConflation.swift:173:35:173:37 | .count | StringLengthConflation.swift:173:35:173:45 | ... .-(_:_:) ... | This String length is used in a String.unicodeScalars, but it may not be equivalent. |
168+
| StringLengthConflation.swift:174:35:174:50 | ... .-(_:_:) ... | StringLengthConflation.swift:174:35:174:42 | .count | StringLengthConflation.swift:174:35:174:50 | ... .-(_:_:) ... | This String.utf8 length is used in a String.unicodeScalars, but it may not be equivalent. |
169+
| StringLengthConflation.swift:175:35:175:51 | ... .-(_:_:) ... | StringLengthConflation.swift:175:35:175:43 | .count | StringLengthConflation.swift:175:35:175:51 | ... .-(_:_:) ... | This String.utf16 length is used in a String.unicodeScalars, but it may not be equivalent. |
170+
| StringLengthConflation.swift:177:35:177:47 | ... .-(_:_:) ... | StringLengthConflation.swift:177:35:177:38 | .length | StringLengthConflation.swift:177:35:177:47 | ... .-(_:_:) ... | This NSString length is used in a String.unicodeScalars, but it may not be equivalent. |
171+
| StringLengthConflation.swift:177:35:177:47 | ... .-(_:_:) ... | file://:0:0:0:0 | .length | StringLengthConflation.swift:177:35:177:47 | ... .-(_:_:) ... | This NSString length is used in a String.unicodeScalars, but it may not be equivalent. |
172+
| StringLengthConflation.swift:178:35:178:48 | ... .-(_:_:) ... | StringLengthConflation.swift:178:35:178:39 | .length | StringLengthConflation.swift:178:35:178:48 | ... .-(_:_:) ... | This NSString length is used in a String.unicodeScalars, but it may not be equivalent. |
173+
| StringLengthConflation.swift:178:35:178:48 | ... .-(_:_:) ... | file://:0:0:0:0 | .length | StringLengthConflation.swift:178:35:178:48 | ... .-(_:_:) ... | This NSString length is used in a String.unicodeScalars, but it may not be equivalent. |
174+
| StringLengthConflation.swift:179:37:179:47 | ... .-(_:_:) ... | StringLengthConflation.swift:179:37:179:39 | .count | StringLengthConflation.swift:179:37:179:47 | ... .-(_:_:) ... | This String length is used in a String.utf8, but it may not be equivalent. |
175+
| StringLengthConflation.swift:181:37:181:47 | ... .-(_:_:) ... | StringLengthConflation.swift:181:37:181:39 | .count | StringLengthConflation.swift:181:37:181:47 | ... .-(_:_:) ... | This String length is used in a String.utf16, but it may not be equivalent. |

‎swift/ql/test/query-tests/Security/CWE-135/StringLengthConflation.swift

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -170,16 +170,16 @@ func test(s: String) {
170170
let _ = String(s.prefix(s.unicodeScalars.count - 10)) // BAD
171171
let _ = String(s.prefix(ns.length - 10)) // BAD
172172
let _ = String(s.prefix(nms.length - 10)) // BAD
173-
let _ = String(scalars.prefix(s.count - 10)) // BAD [NOT DETECTED]
173+
let _ = String(scalars.prefix(s.count - 10)) // BAD
174174
let _ = String(scalars.prefix(s.utf8.count - 10)) // BAD
175175
let _ = String(scalars.prefix(s.utf16.count - 10)) // BAD
176-
let _ = String(scalars.prefix(s.unicodeScalars.count - 10)) // GOOD [FALSE POSITIVE]
176+
let _ = String(scalars.prefix(s.unicodeScalars.count - 10)) // GOOD
177177
let _ = String(scalars.prefix(ns.length - 10)) // BAD
178178
let _ = String(scalars.prefix(nms.length - 10)) // BAD
179-
let _ = String(s.utf8.dropFirst(s.count - 10)) // BAD [NOT DETECTED]
180-
let _ = String(s.utf8.dropFirst(s.utf8.count - 10)) // GOOD [FALSE POSITIVE]
181-
let _ = String(s.utf16.dropLast(s.count - 10)) // BAD [NOT DETECTED]
182-
let _ = String(s.utf16.dropLast(s.utf16.count - 10)) // GOOD [FALSE POSITIVE]
179+
let _ = String(s.utf8.dropFirst(s.count - 10)) // BAD
180+
let _ = String(s.utf8.dropFirst(s.utf8.count - 10)) // GOOD
181+
let _ = String(s.utf16.dropLast(s.count - 10)) // BAD
182+
let _ = String(s.utf16.dropLast(s.utf16.count - 10)) // GOOD
183183
}
184184

185185
// `begin :thumbsup: end`, with thumbs up emoji and skin tone modifier

0 commit comments

Comments
 (0)