Skip to content

Commit 470d915

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 a61f577 commit 470d915

12 files changed

Lines changed: 582 additions & 49 deletions

File tree

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

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

55
private import unified
66
private import codeql.dataflow.internal.AccessPathSyntax
7+
private import codeql.unified.internal.typeinference.TypeInference as T
78

89
extensible predicate legacySourceModel(
910
string namespace, string type, boolean subtypes, string name, string signature, string ext,
@@ -29,7 +30,6 @@ private predicate modelContainsMethodName(string name) {
2930
}
3031

3132
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`.
3333
modelContainsMethodName(rawName) and
3434
exists(string regex |
3535
regex = "([^(]+)\\((.*)\\)" and
@@ -123,8 +123,36 @@ private predicate callableSelector(
123123
)
124124
}
125125

126+
/** Holds if `name` from a model refers to a property, such as `count` in `String.count`. */
127+
private predicate isRawPropertyName(string name) {
128+
modelContainsMethodName(name) and
129+
not name.matches("%(%")
130+
}
131+
132+
/** Holds if `cls` is named `type` or, if `subtypes` is true, is a subclass of `type`. */
133+
bindingset[cls, type]
134+
private predicate classMatchesType(ClassLikeDeclaration cls, string type, boolean subtypes) {
135+
cls.getQualifiedName() = type and subtypes = [true, false]
136+
or
137+
isSubclassOfType(cls, type) and subtypes = true
138+
}
139+
140+
/**
141+
* Holds if `access` may be an access to the property `name`, and its base has the inferred
142+
* type `cls`.
143+
*/
144+
pragma[nomagic]
145+
private predicate propertyAccessSelector(
146+
MemberAccess access, ClassLikeDeclaration cls, string name
147+
) {
148+
name = access.getMemberName() and
149+
name = any(Selector s).getPropertyName() and
150+
not access = any(CallExpr call).getCallee() and
151+
cls = T::inferBaseType(access).asClass()
152+
}
153+
126154
private newtype TSelector =
127-
MkSelector(string type, boolean subtypes, string name, string argLabels, string argTypes) {
155+
MkCallSelector(string type, boolean subtypes, string name, string argLabels, string argTypes) {
128156
exists(string rawName |
129157
legacySourceModel(_, type, subtypes, rawName, argTypes, _, _, _, _)
130158
or
@@ -134,21 +162,38 @@ private newtype TSelector =
134162
|
135163
parsedRawMethodName(rawName, name, argLabels)
136164
)
165+
} or
166+
MkPropertySelector(string type, boolean subtypes, string name) {
167+
(
168+
legacySourceModel(_, type, subtypes, name, _, _, _, _, _)
169+
or
170+
legacySinkModel(_, type, subtypes, name, _, _, _, _, _)
171+
) and
172+
isRawPropertyName(name)
137173
}
138174

139175
/**
140-
* A tuple from a model that identifies call sites.
176+
* A tuple from a model that identifies call sites, callables, or property accesses.
141177
*/
142178
class Selector extends TSelector {
143-
string getTypeString() { this = MkSelector(result, _, _, _, _) }
179+
string getTypeString() {
180+
this = MkCallSelector(result, _, _, _, _) or this = MkPropertySelector(result, _, _)
181+
}
182+
183+
boolean getSubtypesFlag() {
184+
this = MkCallSelector(_, result, _, _, _) or this = MkPropertySelector(_, result, _)
185+
}
144186

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

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

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

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

153198
private string ppSubtypes() {
154199
if this.getSubtypesFlag() = true then result = "+" else result = ""
@@ -158,19 +203,21 @@ class Selector extends TSelector {
158203
result =
159204
this.getTypeString() + this.ppSubtypes() + "." + this.getName() + "(" + this.getArgLabels() +
160205
")" + this.getArgTypes()
206+
or
207+
result = this.getTypeString() + this.ppSubtypes() + "." + this.getPropertyName()
161208
}
162209

163210
/** Holds if `name,argLabels` should be used to join with `methodCallSelector`. */
164211
pragma[nomagic]
165212
private predicate matchesMethodCallSelector(string name, string argLabels) {
166-
this = MkSelector(_, _, name, argLabels, _) and
213+
this = MkCallSelector(_, _, name, argLabels, _) and
167214
name != "init"
168215
}
169216

170217
/** Holds if `name,argLabels` should be used to join with `constructorCallSelector`. */
171218
pragma[nomagic]
172219
private predicate matchesConstructorCallSelector(string name, string argLabels) {
173-
this = MkSelector(name, _, "init", argLabels, _)
220+
this = MkCallSelector(name, _, "init", argLabels, _)
174221
}
175222

176223
predicate matchesCall(CallExpr call) {
@@ -186,7 +233,19 @@ class Selector extends TSelector {
186233
predicate matchesCallable(Callable callable) {
187234
exists(string type, string name, string argLabels |
188235
callableSelector(callable, type, name, argLabels) and
189-
this = MkSelector(type, true, name, argLabels, _)
236+
this = MkCallSelector(type, true, name, argLabels, _)
237+
)
238+
}
239+
240+
/**
241+
* Holds if `access` is an access to the property identified by this selector, as determined
242+
* by the inferred type of its base.
243+
*/
244+
predicate matchesPropertyAccess(MemberAccess access) {
245+
exists(ClassLikeDeclaration cls, string type, boolean subtypes, string name |
246+
this = MkPropertySelector(type, subtypes, name) and
247+
propertyAccessSelector(access, cls, name) and
248+
classMatchesType(cls, type, subtypes)
190249
)
191250
}
192251

@@ -267,27 +326,57 @@ private DataFlow::Node getSourceFromCallable(Callable callable, AccessPathToken
267326
result.isReceiverParameter(callable)
268327
}
269328

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

282368
private predicate normalizedSinkModel(
283369
Selector selector, AccessPath output, string kind, string provenance
284370
) {
285-
exists(
286-
string type, boolean subtypes, string rawName, string name, string argLabels, string argTypes
371+
exists(string type, boolean subtypes, string rawName, string argTypes |
372+
legacySinkModel(_, type, subtypes, rawName, argTypes, _, output, kind, provenance)
287373
|
288-
legacySinkModel(_, type, subtypes, rawName, argTypes, _, output, kind, provenance) and
289-
parsedRawMethodName(rawName, name, argLabels) and
290-
selector = MkSelector(type, subtypes, name, argLabels, argTypes)
374+
exists(string name, string argLabels |
375+
parsedRawMethodName(rawName, name, argLabels) and
376+
selector = MkCallSelector(type, subtypes, name, argLabels, argTypes)
377+
)
378+
or
379+
selector = MkPropertySelector(type, subtypes, rawName)
291380
)
292381
}
293382

@@ -299,7 +388,7 @@ predicate normalizedSummaryModel(
299388
|
300389
legacySummaryModel(_, type, subtypes, rawName, argTypes, _, input, output, kind, provenance) and
301390
parsedRawMethodName(rawName, name, argLabels) and
302-
selector = MkSelector(type, subtypes, name, argLabels, argTypes)
391+
selector = MkCallSelector(type, subtypes, name, argLabels, argTypes)
303392
)
304393
}
305394

@@ -319,6 +408,11 @@ DataFlow::Node getASource(string kind, string provenance) {
319408
selector.matchesCallable(callable) and
320409
result = getSourceFromCallable(callable, output)
321410
)
411+
or
412+
exists(MemberAccess access |
413+
selector.matchesPropertyAccess(access) and
414+
result = getSourceFromPropertyAccess(access, output)
415+
)
322416
)
323417
}
324418

@@ -338,6 +432,11 @@ DataFlow::Node getASink(string kind, string provenance) {
338432
selector.matchesCallable(callable) and
339433
result = getSinkFromCallable(callable, input)
340434
)
435+
or
436+
exists(MemberAccess access |
437+
selector.matchesPropertyAccess(access) and
438+
result = getSinkFromPropertyAccess(access, input)
439+
)
341440
)
342441
}
343442

‎unified/ql/lib/codeql/unified/internal/typeinference/TypeInference.qll‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -862,6 +862,13 @@ predicate inferTypeUnique = M3::inferTypeUnique/2;
862862

863863
predicate inferTypeCertain = M3::inferTypeCertain/2;
864864

865+
Type inferBaseType(MemberAccess access) {
866+
result = inferType(access.(MemberAccessExpr).getBase())
867+
or
868+
result.asClass().getAMemberIncludingExtensions() =
869+
access.(UnqualifiedMemberAccess).getImplicitQualifierVariable().getDeclaringCallable()
870+
}
871+
865872
module Consistency = M3::Consistency;
866873

867874
module CachedStage = M3::CachedStage;

‎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: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,16 +6,28 @@ 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 |
27+
| test.swift:183:9:183:18 | launchPath | command-injection |
28+
| test.swift:184:9:184:17 | arguments | command-injection |
29+
| test.swift:185:9:185:17 | [post] arguments | command-injection |
30+
| test.swift:191:9:191:18 | launchPath | command-injection |
1931
isSource
2032
| test.swift:4:5:4:27 | String(...) | remote |
2133
| test.swift:5:5:5:44 | String(...) | remote |
@@ -25,3 +37,9 @@ isSource
2537
| test.swift:10:5:12:24 | String(...) | local |
2638
| test.swift:13:5:15:28 | String(...) | local |
2739
| test.swift:29:29:29:32 | text | local |
40+
| test.swift:52:9:52:20 | ... .count | string-length |
41+
| test.swift:53:9:53:22 | ... .text | local |
42+
| test.swift:89:9:89:24 | ... .text | local |
43+
| test.swift:176:13:176:16 | text | local |
44+
| test.swift:177:13:177:21 | ... .text | local |
45+
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)