Skip to content

Swift: Model withUnsafeBytes and similar closure methods #13827

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
MathiasVP merged 23 commits into from
Aug 25, 2023
Merged

Swift: Model withUnsafeBytes and similar closure methods #13827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
63c71f0
Swift: Add tests of with* closure methods.
geoffw0 Jul 24, 2023
feadd71
Swift: Add tests with some different container types.
geoffw0 Jul 25, 2023
315cb32
Swift: Remove special case from UnsafeJsEval query.
geoffw0 Jul 24, 2023
49d1556
Swift: Model update(repeating:), to support the tests.
geoffw0 Jul 26, 2023
664dc01
Swift: Add closure function models.
geoffw0 Jul 24, 2023
c48d474
Swift: Fix mistake in the string taint test.
geoffw0 Jul 27, 2023
b41d47b
Swift: Array.withUnsafeBytes doesn't reliably match ContiguousBytes, …
geoffw0 Jul 27, 2023
d24db3f
Swift: Use .ArrayElement in the models, where appropriate.
geoffw0 Jul 26, 2023
af8d4e5
Swift: Change note.
geoffw0 Aug 4, 2023
063ab1c
Merge branch 'main' into closuremodels
geoffw0 Aug 8, 2023
6ccf47e
Swift: Accept test changes resulting from merge.
geoffw0 Aug 8, 2023
c954324
Swift: Correct a test case (but preserve the original as well since i…
geoffw0 Aug 8, 2023
59e2b04
Merge branch 'main' into closuremodels
geoffw0 Aug 17, 2023
1ac9d2c
Swift: Update models with CollectionElement, value flow.
geoffw0 Aug 17, 2023
0fd4f61
Swift: Allow subscript content reads from collections.
geoffw0 Aug 17, 2023
86a73fa
Swift: Accept fixed spurious test results.
geoffw0 Aug 17, 2023
a6f29fa
Swift: Address pointer/pointee conflation in the string tests themsel…
geoffw0 Aug 17, 2023
b4db68a
Swift: Add content to the string models.
geoffw0 Aug 17, 2023
a54747f
Swift: Fix mysterious taint flow issue.
geoffw0 Aug 21, 2023
997984c
Swift: Minor test .expected changes.
geoffw0 Aug 21, 2023
6ef6be7
Swift: UnsafeJSEval regression.
geoffw0 Aug 21, 2023
317757b
Swift: Create proper models for JavaScriptCore.
geoffw0 Aug 21, 2023
f7776f8
Swift: 'good enough' fix for UnsafeJsEval flow.
geoffw0 Aug 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions swift/ql/lib/change-notes/2023-08-04-closure-models.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
category: minorAnalysis
---

* Added flow models of collection `.withContiguous[Mutable]StorageIfAvailable`, `.withUnsafe[Mutable]BufferPointer` and `.withUnsafe[Mutable]Bytes` methods.
8 changes: 6 additions & 2 deletions swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -803,8 +803,12 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
exists(SubscriptExpr subscript |
subscript.getBase() = node1.asExpr() and
subscript = node2.asExpr() and
subscript.getBase().getType() instanceof ArrayType and
c.isSingleton(any(Content::ArrayContent ac))
(
subscript.getBase().getType() instanceof ArrayType and
c.isSingleton(any(Content::ArrayContent ac))
or
c.isSingleton(any(Content::CollectionContent ac))
)
Comment on lines +806 to +811
Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP Aug 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope that a future PR can either collapse ArrayContent and CollectionContent into a single IPA branch, or that we can ensure that a given read/store step always reads a single Content. But obviously, that's not for this PR to handle. So this LGTM for now

Copy link
Copy Markdown
Contributor Author

@geoffw0 geoffw0 Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree 100%.

)
or
FlowSummaryImpl::Private::Steps::summaryReadStep(node1.(FlowSummaryNode).getSummaryNode(), c,
Expand Down
7 changes: 6 additions & 1 deletion swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPublic.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,12 @@ module Content {
override string toString() { result = "Array element" }
}

/** An element of a collection. */
/**
* An element of a collection. This is a broad class including:
* - elements of collections, such as `Set<Element>`.
* - elements of buffers, such as `UnsafeBufferPointer<Element>`.
* - the pointee of a pointer, such as `UnsafePointer<Pointee>`.
Comment on lines +230 to +232
Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP Aug 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be a small performance win to split up these three things into separate Contents in the future. But we can delay that for now, obviously.

Copy link
Copy Markdown
Contributor Author

@geoffw0 geoffw0 Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UnsafeBufferPointer is in fact yet another type of Collection, so it falls into the existing discussion about merging or splitting collection content types properly.

UnsafePointer appears to not be a collection though. I'll create an issue for giving it its own content type.

*/
class CollectionContent extends Content, TCollectionContent {
override string toString() { result = "Collection element" }
}
Expand Down
1 change: 1 addition & 0 deletions swift/ql/lib/codeql/swift/frameworks/Frameworks.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/

private import Alamofire.Alamofire
private import JavaScriptCore.JavaScriptCore
private import StandardLibrary.StandardLibrary
private import UIKit.UIKit
private import Xml.Xml
20 changes: 20 additions & 0 deletions swift/ql/lib/codeql/swift/frameworks/JavaScriptCore/JavaScriptCore.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/**
* Provides models for the `JavaScriptCore` library.
*/

import swift
private import codeql.swift.dataflow.ExternalFlow

/**
* A model for `JavaScriptCore` functions and class members that permit taint flow.
*/
private class JSStringSummaries extends SummaryModelCsv {
override predicate row(string row) {
row =
[
";;false;JSStringCreateWithUTF8CString(_:);;;Argument[0];ReturnValue;taint",
";;false;JSStringCreateWithCharacters(_:_:);;;Argument[0];ReturnValue;taint",
";;false;JSStringRetain(_:);;;Argument[0];ReturnValue;taint",
]
}
}
32 changes: 30 additions & 2 deletions swift/ql/lib/codeql/swift/frameworks/StandardLibrary/Array.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,42 @@ class ArrayType extends Type {
}

/**
* A model for `Array` and related class members that permit data flow.
* A model for `Array` and related Swift class members that permit taint flow.
*/
private class ArraySummaries extends SummaryModelCsv {
override predicate row(string row) {
row =
[
";Array;true;insert(_:at:);;;Argument[0];Argument[-1].ArrayElement;value",
";Array;true;insert(_:at:);;;Argument[1];Argument[-1];taint"
";Array;true;insert(_:at:);;;Argument[1];Argument[-1];taint",
";Array;true;withUnsafeBufferPointer(_:);;;Argument[-1];Argument[0].Parameter[0].CollectionElement;taint",
";Array;true;withUnsafeBufferPointer(_:);;;Argument[-1].ArrayElement;Argument[0].Parameter[0].CollectionElement;value",
";Array;true;withUnsafeBufferPointer(_:);;;Argument[0].ReturnValue;ReturnValue;value",
";Array;true;withUnsafeMutableBufferPointer(_:);;;Argument[-1];Argument[0].Parameter[0].CollectionElement;taint",
";Array;true;withUnsafeMutableBufferPointer(_:);;;Argument[-1].ArrayElement;Argument[0].Parameter[0].CollectionElement;value",
";Array;true;withUnsafeMutableBufferPointer(_:);;;Argument[0].Parameter[0].CollectionElement;Argument[-1].CollectionElement;value",
";Array;true;withUnsafeMutableBufferPointer(_:);;;Argument[0].ReturnValue;ReturnValue;value",
";Array;true;withUnsafeBytes(_:);;;Argument[-1];Argument[0].Parameter[0].CollectionElement;taint",
";Array;true;withUnsafeBytes(_:);;;Argument[-1].ArrayElement;Argument[0].Parameter[0].CollectionElement;taint",
";Array;true;withUnsafeBytes(_:);;;Argument[0].ReturnValue;ReturnValue;value",
";Array;true;withUnsafeMutableBytes(_:);;;Argument[-1];Argument[0].Parameter[0].CollectionElement;taint",
";Array;true;withUnsafeMutableBytes(_:);;;Argument[-1].ArrayElement;Argument[0].Parameter[0].CollectionElement;taint",
";Array;true;withUnsafeMutableBytes(_:);;;Argument[0].Parameter[0].CollectionElement;Argument[-1].CollectionElement;value",
";Array;true;withUnsafeMutableBytes(_:);;;Argument[0].ReturnValue;ReturnValue;value",
";ContiguousArray;true;withUnsafeBufferPointer(_:);;;Argument[-1];Argument[0].Parameter[0].CollectionElement;taint",
";ContiguousArray;true;withUnsafeBufferPointer(_:);;;Argument[-1].CollectionElement;Argument[0].Parameter[0].CollectionElement;value",
";ContiguousArray;true;withUnsafeBufferPointer(_:);;;Argument[0].ReturnValue;ReturnValue;value",
";ContiguousArray;true;withUnsafeMutableBufferPointer(_:);;;Argument[-1];Argument[0].Parameter[0].CollectionElement;taint",
";ContiguousArray;true;withUnsafeMutableBufferPointer(_:);;;Argument[-1].CollectionElement;Argument[0].Parameter[0].CollectionElement;value",
";ContiguousArray;true;withUnsafeMutableBufferPointer(_:);;;Argument[0].Parameter[0].CollectionElement;Argument[-1].CollectionElement;value",
";ContiguousArray;true;withUnsafeMutableBufferPointer(_:);;;Argument[0].ReturnValue;ReturnValue;value",
";ContiguousArray;true;withUnsafeMutableBytes(_:);;;Argument[-1];Argument[0].Parameter[0].CollectionElement;taint",
";ContiguousArray;true;withUnsafeMutableBytes(_:);;;Argument[-1].CollectionElement;Argument[0].Parameter[0].CollectionElement;taint",
";ContiguousArray;true;withUnsafeMutableBytes(_:);;;Argument[0].Parameter[0].CollectionElement;Argument[-1].CollectionElement;taint",
";ContiguousArray;true;withUnsafeMutableBytes(_:);;;Argument[0].ReturnValue;ReturnValue;value",
";ContiguousBytes;true;withUnsafeBytes(_:);;;Argument[-1];Argument[0].Parameter[0].CollectionElement;taint",
";ContiguousBytes;true;withUnsafeBytes(_:);;;Argument[-1].CollectionElement;Argument[0].Parameter[0].CollectionElement;taint",
";ContiguousBytes;true;withUnsafeBytes(_:);;;Argument[0].ReturnValue;ReturnValue;value",
]
}
}
7 changes: 6 additions & 1 deletion swift/ql/lib/codeql/swift/frameworks/StandardLibrary/Collection.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ private import codeql.swift.dataflow.ExternalFlow
private import codeql.swift.dataflow.FlowSteps

/**
* A model for `Collection` members that permit taint flow.
* A model for `Collection` and related Swift class members that permit taint flow.
*/
private class CollectionSummaries extends SummaryModelCsv {
override predicate row(string row) {
Expand Down Expand Up @@ -37,6 +37,11 @@ private class CollectionSummaries extends SummaryModelCsv {
";BidirectionalCollection;true;joined(separator:);;;Argument[-1..0];ReturnValue;taint",
";BidirectionalCollection;true;last(where:);;;Argument[-1];ReturnValue;taint",
";BidirectionalCollection;true;popLast();;;Argument[-1];ReturnValue;taint",
";MutableCollection;true;withContiguousMutableStorageIfAvailable(_:);;;Argument[-1];Argument[0].Parameter[0].CollectionElement;taint",
";MutableCollection;true;withContiguousMutableStorageIfAvailable(_:);;;Argument[-1].ArrayElement;Argument[0].Parameter[0].CollectionElement;value",
";MutableCollection;true;withContiguousMutableStorageIfAvailable(_:);;;Argument[-1].CollectionElement;Argument[0].Parameter[0].CollectionElement;value",
";MutableCollection;true;withContiguousMutableStorageIfAvailable(_:);;;Argument[0].Parameter[0].CollectionElement;Argument[-1].CollectionElement;value",
";MutableCollection;true;withContiguousMutableStorageIfAvailable(_:);;;Argument[0].ReturnValue;ReturnValue.OptionalSome;value",
]
}
}
Expand Down
11 changes: 11 additions & 0 deletions swift/ql/lib/codeql/swift/frameworks/StandardLibrary/PointerTypes.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*/

import swift
private import codeql.swift.dataflow.ExternalFlow

/**
* A Swift unsafe typed pointer type such as `UnsafePointer`,
Expand Down Expand Up @@ -57,3 +58,13 @@ class CVaListPointerType extends NominalType {
class ManagedBufferPointerType extends BoundGenericType {
ManagedBufferPointerType() { this.getName().matches("ManagedBufferPointer<%") }
}

/**
* A model for `UnsafePointer` and related Swift class members that permit taint flow.
*/
private class PointerSummaries extends SummaryModelCsv {
override predicate row(string row) {
row =
";UnsafeMutableBufferPointer;true;update(repeating:);;;Argument[0];Argument[-1].CollectionElement;value"
}
}
4 changes: 3 additions & 1 deletion swift/ql/lib/codeql/swift/frameworks/StandardLibrary/Sequence.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ private class SequenceSummaries extends SummaryModelCsv {
";Sequence;true;joined();;;Argument[-1];ReturnValue;taint",
";Sequence;true;joined(separator:);;;Argument[-1..0];ReturnValue;taint",
";Sequence;true;first(where:);;;Argument[-1];ReturnValue;taint",
";Sequence;true;withContiguousStorageIfAvailable(_:);;;Argument[-1];Argument[0].Parameter[0];taint",
";Sequence;true;withContiguousStorageIfAvailable(_:);;;Argument[-1];Argument[0].Parameter[0].CollectionElement;taint",
";Sequence;true;withContiguousStorageIfAvailable(_:);;;Argument[-1].ArrayElement;Argument[0].Parameter[0].CollectionElement;value",
";Sequence;true;withContiguousStorageIfAvailable(_:);;;Argument[-1].CollectionElement;Argument[0].Parameter[0].CollectionElement;value",
";Sequence;true;withContiguousStorageIfAvailable(_:);;;Argument[0].ReturnValue;ReturnValue.OptionalSome;value",
]
}
Expand Down
58 changes: 39 additions & 19 deletions swift/ql/lib/codeql/swift/frameworks/StandardLibrary/String.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,22 +34,24 @@ private class StringSummaries extends SummaryModelCsv {
row =
[
";StringProtocol;true;init(cString:);;;Argument[0];ReturnValue;taint",
";StringProtocol;true;init(cString:);;;Argument[0].ArrayElement;ReturnValue;taint",
";StringProtocol;true;init(cString:);;;Argument[0].CollectionElement;ReturnValue;taint",
";StringProtocol;true;init(decoding:as:);;;Argument[0];ReturnValue;taint",
";StringProtocol;true;init(decodingCString:as:);;;Argument[0];ReturnValue;taint",
";StringProtocol;true;init(decodingCString:as:);;;Argument[0].OptionalSome.CollectionElement;ReturnValue.OptionalSome.TupleElement[0];taint",
";StringProtocol;true;addingPercentEncoding(withAllowedCharacter:);;;Argument[-1];ReturnValue;taint",
";StringProtocol;true;addingPercentEscapes(using:);;;Argument[-1];ReturnValue;taint",
";StringProtocol;true;appending(_:);;;Argument[-1..0];ReturnValue;taint",
";StringProtocol;true;appendingFormat(_:_:);;;Argument[-1..0];ReturnValue;taint", //-1..
";StringProtocol;true;applyingTransform(_:reverse:);;;Argument[-1];ReturnValue;taint",
";StringProtocol;true;cString(using:);;;Argument[-1];ReturnValue;taint",
";StringProtocol;true;capitalized(with:);;;Argument[-1];ReturnValue;taint",
";StringProtocol;true;completePath(into:caseSensitive:matchesInto:filterTypes:);;;Argument[-1];Argument[0];taint",
";StringProtocol;true;completePath(into:caseSensitive:matchesInto:filterTypes:);;;Argument[-1];Argument[2];taint",
";StringProtocol;true;completePath(into:caseSensitive:matchesInto:filterTypes:);;;Argument[-1];Argument[0].OptionalSome.CollectionElement;taint",
";StringProtocol;true;completePath(into:caseSensitive:matchesInto:filterTypes:);;;Argument[-1];Argument[2].OptionalSome.CollectionElement.ArrayElement;taint",
";StringProtocol;true;components(separatedBy:);;;Argument[-1];ReturnValue;taint",
";StringProtocol;true;data(using:allowLossyConversion:);;;Argument[-1];ReturnValue;taint",
";StringProtocol;true;folding(options:locale:);;;Argument[-1];ReturnValue;taint",
";StringProtocol;true;getBytes(_:maxLength:usedLength:encoding:options:range:remaining:);;;Argument[-1];Argument[0];taint",
";StringProtocol;true;getCString(_:maxLength:encoding:);;;Argument[-1];Argument[0];taint",
";StringProtocol;true;getBytes(_:maxLength:usedLength:encoding:options:range:remaining:);;;Argument[-1];Argument[0].ArrayElement;taint",
";StringProtocol;true;getCString(_:maxLength:encoding:);;;Argument[-1];Argument[0].ArrayElement;taint",
";StringProtocol;true;lowercased();;;Argument[-1];ReturnValue;taint",
";StringProtocol;true;lowercased(with:);;;Argument[-1];ReturnValue;taint",
";StringProtocol;true;padding(toLength:withPad:startingAt:);;;Argument[-1];ReturnValue;taint",
Expand All @@ -68,18 +70,26 @@ private class StringSummaries extends SummaryModelCsv {
";StringProtocol;true;uppercased(with:);;;Argument[-1];ReturnValue;taint",
";String;true;init(decoding:);;;Argument[0];ReturnValue;taint",
";String;true;init(_:);;;Argument[0];ReturnValue;taint",
";String;true;init(_:);;;Argument[0];ReturnValue.OptionalSome;taint",
";String;true;init(repeating:count:);;;Argument[0];ReturnValue;taint",
";String;true;init(data:encoding:);;;Argument[0];ReturnValue;taint",
";String;true;init(validatingUTF8:);;;Argument[0];ReturnValue;taint",
";String;true;init(utf16CodeUnits:count:);;;Argument[0];ReturnValue;taint",
";String;true;init(utf16CodeUnitsNoCopy:count:freeWhenDone:);;;Argument[0];ReturnValue;taint",
";String;true;init(format:_:);;;Argument[0];ReturnValue;taint", //0..
";String;true;init(format:arguments:);;;Argument[0..1];ReturnValue;taint",
";String;true;init(format:locale:_:);;;Argument[0];ReturnValue;taint", //0,2..
";String;true;init(data:encoding:);;;Argument[0];ReturnValue.OptionalSome;taint",
";String;true;init(validatingUTF8:);;;Argument[0];ReturnValue.OptionalSome;taint",
";String;true;init(validatingUTF8:);;;Argument[0].ArrayElement;ReturnValue.OptionalSome;taint",
";String;true;init(validatingUTF8:);;;Argument[0].CollectionElement;ReturnValue.OptionalSome;taint",
";String;true;init(utf16CodeUnits:count:);;;Argument[0].CollectionElement;ReturnValue;taint",
";String;true;init(utf16CodeUnitsNoCopy:count:freeWhenDone:);;;Argument[0].CollectionElement;ReturnValue;taint",
";String;true;init(format:_:);;;Argument[0];ReturnValue;taint",
";String;true;init(format:_:);;;Argument[1].ArrayElement;ReturnValue;taint",
";String;true;init(format:arguments:);;;Argument[0];ReturnValue;taint",
";String;true;init(format:arguments:);;;Argument[1].ArrayElement;ReturnValue;taint",
";String;true;init(format:locale:_:);;;Argument[0];ReturnValue;taint",
";String;true;init(format:locale:_:);;;Argument[2].ArrayElement;ReturnValue;taint",
";String;true;init(format:locale:arguments:);;;Argument[0];ReturnValue;taint",
";String;true;init(format:locale:arguments:);;;Argument[2].ArrayElement;ReturnValue;taint",
";String;true;init(_:radix:uppercase:);;;Argument[0];ReturnValue;taint",
";String;true;init(bytes:encoding:);;;Argument[0];ReturnValue;taint",
";String;true;init(bytesNoCopy:length:encoding:freeWhenDone:);;;Argument[0];ReturnValue;taint",
";String;true;init(bytes:encoding:);;;Argument[0].ArrayElement;ReturnValue.OptionalSome;taint",
";String;true;init(bytes:encoding:);;;Argument[0].CollectionElement;ReturnValue.OptionalSome;taint",
";String;true;init(bytesNoCopy:length:encoding:freeWhenDone:);;;Argument[0].CollectionElement;ReturnValue.OptionalSome;taint",
";String;true;init(describing:);;;Argument[0];ReturnValue;taint",
";String;true;init(contentsOf:);;;Argument[0];ReturnValue;taint",
";String;true;init(contentsOf:encoding:);;;Argument[0];ReturnValue;taint",
Expand All @@ -88,16 +98,26 @@ private class StringSummaries extends SummaryModelCsv {
";String;true;init(contentsOfFile:encoding:);;;Argument[0];ReturnValue;taint",
";String;true;init(contentsOfFile:usedEncoding:);;;Argument[0];ReturnValue;taint",
";String;true;init(from:);;;Argument[0];ReturnValue;taint",
";String;true;init(from:);;;Argument[0];ReturnValue.OptionalSome;taint",
";String;true;init(stringInterpolation:);;;Argument[0];ReturnValue;taint",
";String;true;init(stringLiteral:);;;Argument[0];ReturnValue;taint",
";String;true;init(unicodeScalarLiteral:);;;Argument[0];ReturnValue;taint",
";String;true;init(extendedGraphemeClusterLiteral:);;;Argument[0];ReturnValue;taint",
";String;true;init(cString:encoding:);;;Argument[0];ReturnValue;taint",
";String;true;init(cString:encoding:);;;Argument[0];ReturnValue.OptionalSome;taint",
";String;true;init(cString:encoding:);;;Argument[0].ArrayElement;ReturnValue.OptionalSome;taint",
";String;true;init(cString:encoding:);;;Argument[0].CollectionElement;ReturnValue.OptionalSome;taint",
";String;true;init(platformString:);;;Argument[0];ReturnValue;taint",
";String;true;init(utf8String:);;;Argument[0];ReturnValue;taint",
";String;true;init(validating:);;;Argument[0];ReturnValue;taint",
";String;true;init(validatingPlatformString:);;;Argument[0];ReturnValue;taint",
";String;true;localizedStringWithFormat(_:_:);;;Argument[0..1];ReturnValue;taint",
";String;true;init(platformString:);;;Argument[0].ArrayElement;ReturnValue;taint",
";String;true;init(platformString:);;;Argument[0].CollectionElement;ReturnValue;taint",
";String;true;init(utf8String:);;;Argument[0];ReturnValue.OptionalSome;taint",
";String;true;init(utf8String:);;;Argument[0].ArrayElement;ReturnValue.OptionalSome;taint",
";String;true;init(utf8String:);;;Argument[0].CollectionElement;ReturnValue.OptionalSome;taint",
";String;true;init(validating:);;;Argument[0];ReturnValue.OptionalSome;taint",
";String;true;init(validatingPlatformString:);;;Argument[0];ReturnValue.OptionalSome;taint",
";String;true;init(validatingPlatformString:);;;Argument[0].ArrayElement;ReturnValue.OptionalSome;taint",
";String;true;init(validatingPlatformString:);;;Argument[0].CollectionElement;ReturnValue.OptionalSome;taint",
";String;true;localizedStringWithFormat(_:_:);;;Argument[0];ReturnValue;taint",
";String;true;localizedStringWithFormat(_:_:);;;Argument[1].ArrayContent;ReturnValue;taint",
";String;true;write(_:);;;Argument[0];Argument[-1];taint",
";String;true;write(to:);;;Argument[-1];Argument[0];taint",
";String;true;insert(contentsOf:at:);;;Argument[0];Argument[-1];taint",
Expand Down
24 changes: 0 additions & 24 deletions swift/ql/lib/codeql/swift/security/UnsafeJsEvalExtensions.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,30 +102,6 @@ private class JSEvaluateScriptDefaultUnsafeJsEvalSink extends UnsafeJsEvalSink {
*/
private class DefaultUnsafeJsEvalAdditionalFlowStep extends UnsafeJsEvalAdditionalFlowStep {
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
exists(Argument arg |
arg =
any(CallExpr ce |
ce.getStaticTarget()
.(FreeFunction)
.hasName([
"JSStringCreateWithUTF8CString(_:)", "JSStringCreateWithCharacters(_:_:)",
"JSStringRetain(_:)"
])
).getArgument(0)
|
nodeFrom.asExpr() = arg.getExpr() and
nodeTo.asExpr() = arg.getApplyExpr()
)
or
exists(CallExpr ce, Expr self, ClosureExpr closure |
ce.getStaticTarget().getName().matches("withUnsafeBufferPointer(%)") and
self = ce.getQualifier() and
ce.getArgument(0).getExpr() = closure
|
nodeFrom.asExpr() = self and
nodeTo.(DataFlow::ParameterNode).getParameter() = closure.getParam(0)
)
or
exists(MemberRefExpr e, Expr self, VarDecl member |
self.getType().getFullName().matches(["Unsafe%Buffer%", "Unsafe%Pointer%"]) and
member.getName() = "baseAddress"
Expand Down
13 changes: 13 additions & 0 deletions swift/ql/lib/codeql/swift/security/UnsafeJsEvalQuery.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,19 @@ module UnsafeJsEvalConfig implements DataFlow::ConfigSig {
predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
any(UnsafeJsEvalAdditionalFlowStep s).step(nodeFrom, nodeTo)
}

predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
// flow out from content a the sink
(
isSink(node)
or
isAdditionalFlowStep(node, _)
Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP Aug 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm.... Why do we need implicit read steps at the additional flow steps, and not just at the sink? I assume this is related to the ad-hoc model you had to provide for .baseAddress?

Copy link
Copy Markdown
Contributor Author

@geoffw0 geoffw0 Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think that was why, and there's an issue for turning that ad-hoc model into a general one and getting rid of the remaining special cases here.

) and
(
c.getAReadContent() instanceof DataFlow::Content::ArrayContent or
c.getAReadContent() instanceof DataFlow::Content::CollectionContent
Comment on lines +34 to +35
Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP Aug 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once again, I really hope we can merge these eventually. We're straining the dataflow library quite a lot with these duplications (i.e., if we have a path that should have an access path like [a, CollectionContent, b, CollectionContent] we're really forcing the dataflow library to have:

[Field[a], CollectionContent, Field[b], CollectionContent]
[Field[a], CollectionContent, Field[b], ArrayContent]
[Field[a], ArrayContent, Field[b], CollectionContent]
[Field[a], ArrayContent, Field[b], ArrayContent]

Copy link
Copy Markdown
Contributor Author

@geoffw0 geoffw0 Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

)
}
}

/**
Expand Down
Loading