Skip to content

Commit c2b7300

Browse files
jketemajketema
authored
Merge pull request #9848 from geoffw0/stringlengthconflation5
Swift: More improvements for the string length conflation query
2 parents ca81957 + 541df9b commit c2b7300

File tree

3 files changed

+114
-76
lines changed

3 files changed

+114
-76
lines changed

‎swift/ql/src/queries/Security/CWE-135/StringLengthConflation.ql

Lines changed: 68 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -41,88 +41,80 @@ class StringLengthConflationConfiguration extends DataFlow::Configuration {
4141
}
4242

4343
override predicate isSink(DataFlow::Node node, string flowstate) {
44-
// arguments to method calls...
4544
exists(
46-
string className, string methodName, string paramName, ClassDecl c, AbstractFunctionDecl f,
47-
CallExpr call, int arg
45+
AbstractFunctionDecl funcDecl, CallExpr call, string funcName, string paramName, int arg
4846
|
4947
(
50-
// `NSRange.init`
51-
className = "NSRange" and
52-
methodName = "init(location:length:)" and
53-
paramName = ["location", "length"]
48+
// arguments to method calls...
49+
exists(string className, ClassDecl c |
50+
(
51+
// `NSRange.init`
52+
className = "NSRange" and
53+
funcName = "init(location:length:)" and
54+
paramName = ["location", "length"]
55+
or
56+
// `NSString.character`
57+
className = ["NSString", "NSMutableString"] and
58+
funcName = "character(at:)" and
59+
paramName = "at"
60+
or
61+
// `NSString.character`
62+
className = ["NSString", "NSMutableString"] and
63+
funcName = "substring(from:)" and
64+
paramName = "from"
65+
or
66+
// `NSString.character`
67+
className = ["NSString", "NSMutableString"] and
68+
funcName = "substring(to:)" and
69+
paramName = "to"
70+
or
71+
// `NSMutableString.insert`
72+
className = "NSMutableString" and
73+
funcName = "insert(_:at:)" and
74+
paramName = "at"
75+
) and
76+
c.getName() = className and
77+
c.getAMember() = funcDecl and
78+
call.getFunction().(ApplyExpr).getStaticTarget() = funcDecl and
79+
flowstate = "String" // `String` length flowing into `NSString`
80+
)
5481
or
55-
// `NSString.character`
56-
className = ["NSString", "NSMutableString"] and
57-
methodName = "character(at:)" and
58-
paramName = "at"
82+
// arguments to function calls...
83+
// `NSMakeRange`
84+
funcName = "NSMakeRange(_:_:)" and
85+
paramName = ["loc", "len"] and
86+
call.getStaticTarget() = funcDecl and
87+
flowstate = "String" // `String` length flowing into `NSString`
5988
or
60-
// `NSString.character`
61-
className = ["NSString", "NSMutableString"] and
62-
methodName = "substring(from:)" and
63-
paramName = "from"
64-
or
65-
// `NSString.character`
66-
className = ["NSString", "NSMutableString"] and
67-
methodName = "substring(to:)" and
68-
paramName = "to"
69-
or
70-
// `NSMutableString.insert`
71-
className = "NSMutableString" and
72-
methodName = "insert(_:at:)" and
73-
paramName = "at"
74-
) and
75-
c.getName() = className and
76-
c.getAMember() = f and // TODO: will this even work if its defined in a parent class?
77-
call.getFunction().(ApplyExpr).getStaticTarget() = f and
78-
f.getName() = methodName and
79-
f.getParam(pragma[only_bind_into](arg)).getName() = paramName and
80-
call.getArgument(pragma[only_bind_into](arg)).getExpr() = node.asExpr() and
81-
flowstate = "String" // `String` length flowing into `NSString`
82-
)
83-
or
84-
// arguments to function calls...
85-
exists(string funcName, string paramName, CallExpr call, int arg |
86-
// `NSMakeRange`
87-
funcName = "NSMakeRange(_:_:)" and
88-
paramName = ["loc", "len"] and
89-
call.getStaticTarget().getName() = funcName and
90-
call.getStaticTarget().getParam(pragma[only_bind_into](arg)).getName() = paramName and
91-
call.getArgument(pragma[only_bind_into](arg)).getExpr() = node.asExpr() and
92-
flowstate = "String" // `String` length flowing into `NSString`
93-
)
94-
or
95-
// arguments to function calls...
96-
exists(string funcName, string paramName, CallExpr call, int arg |
97-
(
98-
// `String.dropFirst`, `String.dropLast`, `String.removeFirst`, `String.removeLast`
99-
funcName = ["dropFirst(_:)", "dropLast(_:)", "removeFirst(_:)", "removeLast(_:)"] and
100-
paramName = "k"
101-
or
102-
// `String.prefix`, `String.suffix`
103-
funcName = ["prefix(_:)", "suffix(_:)"] and
104-
paramName = "maxLength"
105-
or
106-
// `String.Index.init`
107-
funcName = "init(encodedOffset:)" and
108-
paramName = "offset"
109-
or
110-
// `String.index`
111-
funcName = ["index(_:offsetBy:)", "index(_:offsetBy:limitBy:)"] and
112-
paramName = "n"
113-
or
114-
// `String.formIndex`
115-
funcName = ["formIndex(_:offsetBy:)", "formIndex(_:offsetBy:limitBy:)"] and
116-
paramName = "distance"
89+
// arguments to method calls...
90+
(
91+
// `String.dropFirst`, `String.dropLast`, `String.removeFirst`, `String.removeLast`
92+
funcName = ["dropFirst(_:)", "dropLast(_:)", "removeFirst(_:)", "removeLast(_:)"] and
93+
paramName = "k"
94+
or
95+
// `String.prefix`, `String.suffix`
96+
funcName = ["prefix(_:)", "suffix(_:)"] and
97+
paramName = "maxLength"
98+
or
99+
// `String.Index.init`
100+
funcName = "init(encodedOffset:)" and
101+
paramName = "offset"
102+
or
103+
// `String.index`
104+
funcName = ["index(_:offsetBy:)", "index(_:offsetBy:limitBy:)"] and
105+
paramName = "n"
106+
or
107+
// `String.formIndex`
108+
funcName = ["formIndex(_:offsetBy:)", "formIndex(_:offsetBy:limitBy:)"] and
109+
paramName = "distance"
110+
) and
111+
call.getFunction().(ApplyExpr).getStaticTarget() = funcDecl and
112+
flowstate = "NSString" // `NSString` length flowing into `String`
117113
) and
118-
call.getFunction().(ApplyExpr).getStaticTarget().getName() = funcName and
119-
call.getFunction()
120-
.(ApplyExpr)
121-
.getStaticTarget()
122-
.getParam(pragma[only_bind_into](arg))
123-
.getName() = paramName and
124-
call.getArgument(pragma[only_bind_into](arg)).getExpr() = node.asExpr() and
125-
flowstate = "NSString" // `NSString` length flowing into `String`
114+
// match up `funcName`, `paramName`, `arg`, `node`.
115+
funcDecl.getName() = funcName and
116+
funcDecl.getParam(pragma[only_bind_into](arg)).getName() = paramName and
117+
call.getArgument(pragma[only_bind_into](arg)).getExpr() = node.asExpr()
126118
)
127119
}
128120

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
edges
2+
| StringLengthConflation2.swift:37:34:37:36 | .count : | StringLengthConflation2.swift:37:34:37:44 | ... call to -(_:_:) ... |
23
| StringLengthConflation.swift:60:47:60:50 | .length : | StringLengthConflation.swift:60:47:60:59 | ... call to /(_:_:) ... |
34
| StringLengthConflation.swift:66:33:66:36 | .length : | StringLengthConflation.swift:66:33:66:45 | ... call to /(_:_:) ... |
45
| StringLengthConflation.swift:93:28:93:31 | .length : | StringLengthConflation.swift:93:28:93:40 | ... call to -(_:_:) ... |
@@ -15,6 +16,8 @@ edges
1516
| StringLengthConflation.swift:135:36:135:38 | .count : | StringLengthConflation.swift:135:36:135:46 | ... call to -(_:_:) ... |
1617
| StringLengthConflation.swift:141:28:141:30 | .count : | StringLengthConflation.swift:141:28:141:38 | ... call to -(_:_:) ... |
1718
nodes
19+
| StringLengthConflation2.swift:37:34:37:36 | .count : | semmle.label | .count : |
20+
| StringLengthConflation2.swift:37:34:37:44 | ... call to -(_:_:) ... | semmle.label | ... call to -(_:_:) ... |
1821
| StringLengthConflation.swift:53:43:53:46 | .length | semmle.label | .length |
1922
| StringLengthConflation.swift:60:47:60:50 | .length : | semmle.label | .length : |
2023
| StringLengthConflation.swift:60:47:60:59 | ... call to /(_:_:) ... | semmle.label | ... call to /(_:_:) ... |
@@ -50,6 +53,7 @@ nodes
5053
| StringLengthConflation.swift:141:28:141:38 | ... call to -(_:_:) ... | semmle.label | ... call to -(_:_:) ... |
5154
subpaths
5255
#select
56+
| StringLengthConflation2.swift:37:34:37:44 | ... call to -(_:_:) ... | StringLengthConflation2.swift:37:34:37:36 | .count : | StringLengthConflation2.swift:37:34:37:44 | ... call to -(_:_:) ... | This String length is used in an NSString, but it may not be equivalent. |
5357
| StringLengthConflation.swift:53:43:53:46 | .length | StringLengthConflation.swift:53:43:53:46 | .length | StringLengthConflation.swift:53:43:53:46 | .length | This NSString length is used in a String, but it may not be equivalent. |
5458
| StringLengthConflation.swift:60:47:60:59 | ... call to /(_:_:) ... | StringLengthConflation.swift:60:47:60:50 | .length : | StringLengthConflation.swift:60:47:60:59 | ... call to /(_:_:) ... | This NSString length is used in a String, but it may not be equivalent. |
5559
| StringLengthConflation.swift:66:33:66:45 | ... call to /(_:_:) ... | StringLengthConflation.swift:66:33:66:36 | .length : | StringLengthConflation.swift:66:33:66:45 | ... call to /(_:_:) ... | This NSString length is used in a String, but it may not be equivalent. |

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

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
// this test is in a separate file, because we want to test with a slightly different library
2+
// implementation. In this version, some of the functions of `NSString` are in fact implemented
3+
// in a base class `NSStringBase`.
4+
5+
// --- stubs ---
6+
7+
func print(_ items: Any...) {}
8+
9+
typealias unichar = UInt16
10+
11+
class NSObject
12+
{
13+
}
14+
15+
class NSStringBase : NSObject
16+
{
17+
func substring(from: Int) -> String { return "" }
18+
}
19+
20+
class NSString : NSStringBase
21+
{
22+
init(string: String) { length = string.count }
23+
24+
func substring(to: Int) -> String { return "" }
25+
26+
private(set) var length: Int
27+
}
28+
29+
// --- tests ---
30+
31+
func test(s: String) {
32+
let ns = NSString(string: s)
33+
34+
let nstr1 = ns.substring(from: ns.length - 1) // GOOD
35+
let nstr2 = ns.substring(from: s.count - 1) // BAD: String length used in NSString [NOT DETECTED]
36+
let nstr3 = ns.substring(to: ns.length - 1) // GOOD
37+
let nstr4 = ns.substring(to: s.count - 1) // BAD: String length used in NSString
38+
print("substrings '\(nstr1)' '\(nstr2)' / '\(nstr3)' '\(nstr4)'")
39+
}
40+
41+
// `begin :thumbsup: end`, with thumbs up emoji and skin tone modifier
42+
test(s: "begin \u{0001F44D}\u{0001F3FF} end")

0 commit comments

Comments
 (0)