Skip to content

Commit b07ed01

Browse files
asgerfCopilot
asgerf
and
Copilot
committed
Unified: Support property source/sink models in LegacyMaD
Property models (e.g. UITextField.text, Process.arguments) are matched against member accesses using type inference on the base. Selector now covers both call-site and property selectors. Also add stubs for all types mentioned in models to the builtins, since dependency declarations are not extracted outside of tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent a1d3db7 commit b07ed01

11 files changed

Lines changed: 542 additions & 49 deletions

File tree

‎unified/ql/lib/codeql/unified/internal/mad/LegacyMaD.qll‎

Lines changed: 122 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44

55
private import unified
66
private import codeql.dataflow.internal.AccessPathSyntax
7+
private import codeql.unified.internal.typeinference.TypeInference as T
8+
private import codeql.unified.internal.typeinference.Type as Types
79

810
extensible predicate legacySourceModel(
911
string namespace, string type, boolean subtypes, string name, string signature, string ext,
@@ -29,7 +31,6 @@ private predicate modelContainsMethodName(string name) {
2931
}
3032

3133
private predicate parsedRawMethodName(string rawName, string name, string argLabels) {
32-
// NOTE: We only support method names at the moment, not property names like `String.count`.
3334
modelContainsMethodName(rawName) and
3435
exists(string regex |
3536
regex = "([^(]+)\\((.*)\\)" and
@@ -123,8 +124,36 @@ private predicate callableSelector(
123124
)
124125
}
125126

127+
/** Holds if `name` from a model refers to a property, such as `count` in `String.count`. */
128+
private predicate isRawPropertyName(string name) {
129+
modelContainsMethodName(name) and
130+
not name.matches("%(%")
131+
}
132+
133+
/** Holds if `cls` is named `type` or, if `subtypes` is true, is a subclass of `type`. */
134+
bindingset[cls, type]
135+
private predicate classMatchesType(ClassLikeDeclaration cls, string type, boolean subtypes) {
136+
cls.getQualifiedName() = type and subtypes = [true, false]
137+
or
138+
isSubclassOfType(cls, type) and subtypes = true
139+
}
140+
141+
/**
142+
* Holds if `access` may be an access to the property `name`, and its base has the inferred
143+
* type `cls`.
144+
*/
145+
pragma[nomagic]
146+
private predicate propertyAccessSelector(
147+
MemberAccessExpr access, ClassLikeDeclaration cls, string name
148+
) {
149+
name = access.getMemberName() and
150+
name = any(Selector s).getPropertyName() and
151+
not access = any(CallExpr call).getCallee() and
152+
cls = T::inferType(access.getBase()).(Types::ClassLikeDeclarationType).getClassLikeDeclaration()
153+
}
154+
126155
private newtype TSelector =
127-
MkSelector(string type, boolean subtypes, string name, string argLabels, string argTypes) {
156+
MkCallSelector(string type, boolean subtypes, string name, string argLabels, string argTypes) {
128157
exists(string rawName |
129158
legacySourceModel(_, type, subtypes, rawName, argTypes, _, _, _, _)
130159
or
@@ -134,21 +163,38 @@ private newtype TSelector =
134163
|
135164
parsedRawMethodName(rawName, name, argLabels)
136165
)
166+
} or
167+
MkPropertySelector(string type, boolean subtypes, string name) {
168+
(
169+
legacySourceModel(_, type, subtypes, name, _, _, _, _, _)
170+
or
171+
legacySinkModel(_, type, subtypes, name, _, _, _, _, _)
172+
) and
173+
isRawPropertyName(name)
137174
}
138175

139176
/**
140-
* A tuple from a model that identifies call sites.
177+
* A tuple from a model that identifies call sites, callables, or property accesses.
141178
*/
142179
class Selector extends TSelector {
143-
string getTypeString() { this = MkSelector(result, _, _, _, _) }
180+
string getTypeString() {
181+
this = MkCallSelector(result, _, _, _, _) or this = MkPropertySelector(result, _, _)
182+
}
183+
184+
boolean getSubtypesFlag() {
185+
this = MkCallSelector(_, result, _, _, _) or this = MkPropertySelector(_, result, _)
186+
}
144187

145-
boolean getSubtypesFlag() { this = MkSelector(_, result, _, _, _) }
188+
string getName() {
189+
this = MkCallSelector(_, _, result, _, _) or this = MkPropertySelector(_, _, result)
190+
}
146191

147-
string getName() { this = MkSelector(_, _, result, _, _) }
192+
/** Gets the property name, if this is a property selector. */
193+
string getPropertyName() { this = MkPropertySelector(_, _, result) }
148194

149-
string getArgLabels() { this = MkSelector(_, _, _, result, _) }
195+
string getArgLabels() { this = MkCallSelector(_, _, _, result, _) }
150196

151-
string getArgTypes() { this = MkSelector(_, _, _, _, result) }
197+
string getArgTypes() { this = MkCallSelector(_, _, _, _, result) }
152198

153199
private string ppSubtypes() {
154200
if this.getSubtypesFlag() = true then result = "+" else result = ""
@@ -158,19 +204,21 @@ class Selector extends TSelector {
158204
result =
159205
this.getTypeString() + this.ppSubtypes() + "." + this.getName() + "(" + this.getArgLabels() +
160206
")" + this.getArgTypes()
207+
or
208+
result = this.getTypeString() + this.ppSubtypes() + "." + this.getPropertyName()
161209
}
162210

163211
/** Holds if `name,argLabels` should be used to join with `methodCallSelector`. */
164212
pragma[nomagic]
165213
private predicate matchesMethodCallSelector(string name, string argLabels) {
166-
this = MkSelector(_, _, name, argLabels, _) and
214+
this = MkCallSelector(_, _, name, argLabels, _) and
167215
name != "init"
168216
}
169217

170218
/** Holds if `name,argLabels` should be used to join with `constructorCallSelector`. */
171219
pragma[nomagic]
172220
private predicate matchesConstructorCallSelector(string name, string argLabels) {
173-
this = MkSelector(name, _, "init", argLabels, _)
221+
this = MkCallSelector(name, _, "init", argLabels, _)
174222
}
175223

176224
predicate matchesCall(CallExpr call) {
@@ -186,7 +234,19 @@ class Selector extends TSelector {
186234
predicate matchesCallable(Callable callable) {
187235
exists(string type, string name, string argLabels |
188236
callableSelector(callable, type, name, argLabels) and
189-
this = MkSelector(type, true, name, argLabels, _)
237+
this = MkCallSelector(type, true, name, argLabels, _)
238+
)
239+
}
240+
241+
/**
242+
* Holds if `access` is an access to the property identified by this selector, as determined
243+
* by the inferred type of its base.
244+
*/
245+
predicate matchesPropertyAccess(MemberAccessExpr access) {
246+
exists(ClassLikeDeclaration cls, string type, boolean subtypes, string name |
247+
this = MkPropertySelector(type, subtypes, name) and
248+
propertyAccessSelector(access, cls, name) and
249+
classMatchesType(cls, type, subtypes)
190250
)
191251
}
192252

@@ -267,27 +327,57 @@ private DataFlow::Node getSourceFromCallable(Callable callable, AccessPathToken
267327
result.isReceiverParameter(callable)
268328
}
269329

330+
/**
331+
* Gets the source from the property `access` specified by `path`.
332+
*/
333+
bindingset[access, path]
334+
private DataFlow::Node getSourceFromPropertyAccess(MemberAccessExpr access, AccessPath path) {
335+
path = "" and
336+
result.isResultValue(access)
337+
}
338+
339+
/**
340+
* Gets the sink from the property `access` specified by `path`.
341+
*/
342+
bindingset[access, path]
343+
private DataFlow::Node getSinkFromPropertyAccess(MemberAccessExpr access, AccessPath path) {
344+
path = "PostUpdate" and
345+
(
346+
// The value being assigned to the property, as in `x.prop = value`
347+
result.isIncomingValue(access)
348+
or
349+
// The property value after being mutated in-place, as in `x.prop.append(value)`
350+
result.isPostUpdate(access)
351+
)
352+
}
353+
270354
private predicate normalizedSourceModel(
271355
Selector selector, AccessPath output, string kind, string provenance
272356
) {
273-
exists(
274-
string type, boolean subtypes, string rawName, string name, string argLabels, string argTypes
357+
exists(string type, boolean subtypes, string rawName, string argTypes |
358+
legacySourceModel(_, type, subtypes, rawName, argTypes, _, output, kind, provenance)
275359
|
276-
legacySourceModel(_, type, subtypes, rawName, argTypes, _, output, kind, provenance) and
277-
parsedRawMethodName(rawName, name, argLabels) and
278-
selector = MkSelector(type, subtypes, name, argLabels, argTypes)
360+
exists(string name, string argLabels |
361+
parsedRawMethodName(rawName, name, argLabels) and
362+
selector = MkCallSelector(type, subtypes, name, argLabels, argTypes)
363+
)
364+
or
365+
selector = MkPropertySelector(type, subtypes, rawName)
279366
)
280367
}
281368

282369
private predicate normalizedSinkModel(
283370
Selector selector, AccessPath output, string kind, string provenance
284371
) {
285-
exists(
286-
string type, boolean subtypes, string rawName, string name, string argLabels, string argTypes
372+
exists(string type, boolean subtypes, string rawName, string argTypes |
373+
legacySinkModel(_, type, subtypes, rawName, argTypes, _, output, kind, provenance)
287374
|
288-
legacySinkModel(_, type, subtypes, rawName, argTypes, _, output, kind, provenance) and
289-
parsedRawMethodName(rawName, name, argLabels) and
290-
selector = MkSelector(type, subtypes, name, argLabels, argTypes)
375+
exists(string name, string argLabels |
376+
parsedRawMethodName(rawName, name, argLabels) and
377+
selector = MkCallSelector(type, subtypes, name, argLabels, argTypes)
378+
)
379+
or
380+
selector = MkPropertySelector(type, subtypes, rawName)
291381
)
292382
}
293383

@@ -299,7 +389,7 @@ predicate normalizedSummaryModel(
299389
|
300390
legacySummaryModel(_, type, subtypes, rawName, argTypes, _, input, output, kind, provenance) and
301391
parsedRawMethodName(rawName, name, argLabels) and
302-
selector = MkSelector(type, subtypes, name, argLabels, argTypes)
392+
selector = MkCallSelector(type, subtypes, name, argLabels, argTypes)
303393
)
304394
}
305395

@@ -319,6 +409,11 @@ DataFlow::Node getASource(string kind, string provenance) {
319409
selector.matchesCallable(callable) and
320410
result = getSourceFromCallable(callable, output)
321411
)
412+
or
413+
exists(MemberAccessExpr access |
414+
selector.matchesPropertyAccess(access) and
415+
result = getSourceFromPropertyAccess(access, output)
416+
)
322417
)
323418
}
324419

@@ -338,6 +433,11 @@ DataFlow::Node getASink(string kind, string provenance) {
338433
selector.matchesCallable(callable) and
339434
result = getSinkFromCallable(callable, input)
340435
)
436+
or
437+
exists(MemberAccessExpr access |
438+
selector.matchesPropertyAccess(access) and
439+
result = getSinkFromPropertyAccess(access, input)
440+
)
341441
)
342442
}
343443

‎unified/ql/src/queries/security/CWE-078/CommandInjection.ql‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,7 @@ import unified
2020
module CommandInjectionConfig implements DataFlow::ConfigSig {
2121
predicate isSource(DataFlow::Node node) { Models::isSource(node, _) }
2222

23-
predicate isSink(DataFlow::Node node) {
24-
// TODO: Support Process property-write sinks from original Swift's CommandInjectionExtensions.qll.
25-
Models::isSink(node, "command-injection")
26-
}
23+
predicate isSink(DataFlow::Node node) { Models::isSink(node, "command-injection") }
2724

2825
predicate isBarrier(DataFlow::Node node) {
2926
// TODO: Port the numeric-type barrier from original Swift's CommandInjectionExtensions.qll.

‎unified/ql/test/library-tests/mad-builtins/test.expected‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
isSink
2+
| test.swift:15:5:15:52 | ... .tlsMinimumSupportedProtocolVersion | tls-protocol-version |
3+
| test.swift:17:5:17:22 | ... .launchPath | command-injection |
4+
| test.swift:18:5:18:21 | ... .arguments | command-injection |
5+
| test.swift:19:5:19:23 | ... .arguments | command-injection |
6+
| test.swift:22:5:22:26 | ... .arguments | command-injection |
7+
isSource
8+
| test.swift:13:9:13:22 | ... .text | local |
9+
missingBuiltinType

‎unified/ql/test/library-tests/mad-builtins/test.qlref‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
query: library-tests/mad/test.ql
2+
postprocess: utils/test/InlineExpectationsTestQuery.ql

‎unified/ql/test/library-tests/mad-builtins/test.swift‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
// No stubs are declared in this file, so the types below must be resolved via the builtins,
2+
// as is the case when analyzing real code whose dependencies are not extracted.
3+
4+
class MyProcess: Process {}
5+
6+
func testPropertiesWithoutStubs(
7+
string: String,
8+
textField: UITextField,
9+
configuration: URLSessionConfiguration,
10+
process: Process,
11+
myProcess: MyProcess
12+
) {
13+
_ = textField.text // $ isSource=local
14+
15+
configuration.tlsMinimumSupportedProtocolVersion = 0 // $ isSink=tls-protocol-version
16+
17+
process.launchPath = string // $ isSink=command-injection
18+
process.arguments = [string] // $ isSink=command-injection
19+
myProcess.arguments = [string] // $ isSink=command-injection
20+
21+
let localProcess = Process()
22+
localProcess.arguments = [string] // $ isSink=command-injection
23+
}

‎unified/ql/test/library-tests/mad/test.expected‎

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,16 +6,24 @@ isSink
66
| test.swift:21:5:21:16 | [self arg] ... .md5(...) | weak-hash-input-MD5 |
77
| test.swift:23:20:23:25 | string | regex-use |
88
| test.swift:24:20:24:25 | string | regex-use |
9-
| test.swift:71:37:71:49 | encodedOffset | string-length |
10-
| test.swift:72:42:72:54 | encodedOffset | string-length |
11-
| test.swift:76:24:76:36 | encryptionKey | encryption-key |
12-
| test.swift:77:18:77:24 | fileURL | path-injection |
13-
| test.swift:88:24:88:36 | encryptionKey | encryption-key |
14-
| test.swift:89:18:89:24 | fileURL | path-injection |
15-
| test.swift:100:24:100:36 | encryptionKey | encryption-key |
16-
| test.swift:101:18:101:24 | fileURL | path-injection |
17-
| test.swift:107:23:107:34 | seedFilePath | path-injection |
18-
| test.swift:119:29:119:34 | string | path-injection |
9+
| test.swift:55:5:55:45 | ... .tlsMaximumSupportedProtocol | tls-protocol-version |
10+
| test.swift:56:5:56:45 | ... .tlsMinimumSupportedProtocol | tls-protocol-version |
11+
| test.swift:57:5:57:52 | ... .tlsMaximumSupportedProtocolVersion | tls-protocol-version |
12+
| test.swift:58:5:58:52 | ... .tlsMinimumSupportedProtocolVersion | tls-protocol-version |
13+
| test.swift:93:5:93:22 | ... .launchPath | command-injection |
14+
| test.swift:94:5:94:21 | ... .arguments | command-injection |
15+
| test.swift:95:5:95:21 | [post] ... .arguments | command-injection |
16+
| test.swift:96:5:96:23 | ... .arguments | command-injection |
17+
| test.swift:123:37:123:49 | encodedOffset | string-length |
18+
| test.swift:124:42:124:54 | encodedOffset | string-length |
19+
| test.swift:128:24:128:36 | encryptionKey | encryption-key |
20+
| test.swift:129:18:129:24 | fileURL | path-injection |
21+
| test.swift:140:24:140:36 | encryptionKey | encryption-key |
22+
| test.swift:141:18:141:24 | fileURL | path-injection |
23+
| test.swift:152:24:152:36 | encryptionKey | encryption-key |
24+
| test.swift:153:18:153:24 | fileURL | path-injection |
25+
| test.swift:159:23:159:34 | seedFilePath | path-injection |
26+
| test.swift:171:29:171:34 | string | path-injection |
1927
isSource
2028
| test.swift:4:5:4:27 | String(...) | remote |
2129
| test.swift:5:5:5:44 | String(...) | remote |
@@ -25,3 +33,7 @@ isSource
2533
| test.swift:10:5:12:24 | String(...) | local |
2634
| test.swift:13:5:15:28 | String(...) | local |
2735
| test.swift:29:29:29:32 | text | local |
36+
| test.swift:52:9:52:20 | ... .count | string-length |
37+
| test.swift:53:9:53:22 | ... .text | local |
38+
| test.swift:89:9:89:24 | ... .text | local |
39+
missingBuiltinType

‎unified/ql/test/library-tests/mad/test.ql‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,19 @@
11
private import unified
2+
private import codeql.unified.internal.Builtins
3+
private import codeql.unified.internal.mad.LegacyMaD as LegacyMaD
24

35
query predicate isSource = Models::isSource/2;
46

57
query predicate isSink = Models::isSink/2;
8+
9+
/**
10+
* Holds if `type` is mentioned in a model, but has no representative in the builtins.
11+
*
12+
* Declarations from dependencies are not extracted, so such types would not be resolved
13+
* by type inference outside of tests.
14+
*/
15+
query predicate missingBuiltinType(string type) {
16+
type = any(LegacyMaD::Selector s).getTypeString() and
17+
type != "" and
18+
not type = any(BuiltinClassLikeDeclaration decl).getQualifiedName()
19+
}

0 commit comments

Comments
 (0)