Skip to content

Commit e0f958f

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 1c866c7 commit e0f958f

12 files changed

Lines changed: 586 additions & 53 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

@@ -269,27 +328,57 @@ private DataFlow::Node getSourceFromCallable(Callable callable, AccessPathToken
269328
result.isReceiverParameter(callable)
270329
}
271330

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

284370
private predicate normalizedSinkModel(
285371
Selector selector, AccessPath output, string kind, string provenance
286372
) {
287-
exists(
288-
string type, boolean subtypes, string rawName, string name, string argLabels, string argTypes
373+
exists(string type, boolean subtypes, string rawName, string argTypes |
374+
legacySinkModel(_, type, subtypes, rawName, argTypes, _, output, kind, provenance)
289375
|
290-
legacySinkModel(_, type, subtypes, rawName, argTypes, _, output, kind, provenance) and
291-
parsedRawMethodName(rawName, name, argLabels) and
292-
selector = MkSelector(type, subtypes, name, argLabels, argTypes)
376+
exists(string name, string argLabels |
377+
parsedRawMethodName(rawName, name, argLabels) and
378+
selector = MkCallSelector(type, subtypes, name, argLabels, argTypes)
379+
)
380+
or
381+
selector = MkPropertySelector(type, subtypes, rawName)
293382
)
294383
}
295384

@@ -301,7 +390,7 @@ predicate normalizedSummaryModel(
301390
|
302391
legacySummaryModel(_, type, subtypes, rawName, argTypes, _, input, output, kind, provenance) and
303392
parsedRawMethodName(rawName, name, argLabels) and
304-
selector = MkSelector(type, subtypes, name, argLabels, argTypes)
393+
selector = MkCallSelector(type, subtypes, name, argLabels, argTypes)
305394
)
306395
}
307396

@@ -321,6 +410,11 @@ DataFlow::Node getASource(string kind, string provenance) {
321410
selector.matchesCallable(callable) and
322411
result = getSourceFromCallable(callable, output)
323412
)
413+
or
414+
exists(MemberAccess access |
415+
selector.matchesPropertyAccess(access) and
416+
result = getSourceFromPropertyAccess(access, output)
417+
)
324418
)
325419
}
326420

@@ -340,6 +434,11 @@ DataFlow::Node getASink(string kind, string provenance) {
340434
selector.matchesCallable(callable) and
341435
result = getSinkFromCallable(callable, input)
342436
)
437+
or
438+
exists(MemberAccess access |
439+
selector.matchesPropertyAccess(access) and
440+
result = getSinkFromPropertyAccess(access, input)
441+
)
343442
)
344443
}
345444

‎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: 32 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6,18 +6,30 @@ 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 |
19-
| test.swift:137:5:139:5 | [self arg] ... .sinkArgs(...) | shorthand-sink |
20-
| test.swift:138:9:138:14 | string | shorthand-sink |
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 |
31+
| test.swift:217:5:219:5 | [self arg] ... .sinkArgs(...) | shorthand-sink |
32+
| test.swift:218:9:218:14 | string | shorthand-sink |
2133
isSource
2234
| test.swift:4:5:4:27 | String(...) | remote |
2335
| test.swift:5:5:5:44 | String(...) | remote |
@@ -27,5 +39,11 @@ isSource
2739
| test.swift:10:5:12:24 | String(...) | local |
2840
| test.swift:13:5:15:28 | String(...) | local |
2941
| test.swift:29:29:29:32 | text | local |
30-
| test.swift:129:5:133:5 | [self param] func ShorthandTestSub.sourceParams | shorthand-source |
31-
| test.swift:130:11:130:11 | x | shorthand-source |
42+
| test.swift:52:9:52:20 | ... .count | string-length |
43+
| test.swift:53:9:53:22 | ... .text | local |
44+
| test.swift:89:9:89:24 | ... .text | local |
45+
| test.swift:176:13:176:16 | text | local |
46+
| test.swift:177:13:177:21 | ... .text | local |
47+
| test.swift:209:5:213:5 | [self param] func ShorthandTestSub.sourceParams | shorthand-source |
48+
| test.swift:210:11:210:11 | x | shorthand-source |
49+
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)