-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Swift: Modernize the swift/string-length-conflation query #12642
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.
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
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
879dea2
Swift: Additional test cases.
geoffw0 dfcad7f
Swift: Split the query into the usual three files.
geoffw0 4c0d02a
Swift: Standardize the sources, sinks etc.
geoffw0 e266132
Swift: Replace sinks with (extendable) CSV.
geoffw0 9529bc5
Swift: The regressed test is not realistic, update it to be more like…
geoffw0 a5bb934
Swift: Replace sources with (extendable) CSV.
geoffw0 de5cf84
Swift: Address check failures.
geoffw0 c158f83
Swift: Fix regression.
geoffw0 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
189 changes: 189 additions & 0 deletions
189
swift/ql/lib/codeql/swift/security/StringLengthConflationExtensions.qll
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,189 @@ | ||
/** | ||
* Provides classes and predicates for reasoning about string length | ||
* conflation vulnerabilities. | ||
*/ | ||
|
||
import swift | ||
import codeql.swift.dataflow.DataFlow | ||
import codeql.swift.dataflow.ExternalFlow | ||
|
||
/** | ||
* A type of Swift string encoding. This class is used as a flow state for | ||
* the string length conflation taint tracking configuration. | ||
*/ | ||
class StringType extends string { | ||
string singular; | ||
string equivClass; | ||
string csvLabel; | ||
|
||
StringType() { | ||
this = "String" and | ||
singular = "a String" and | ||
equivClass = "String" and | ||
csvLabel = "string-length" | ||
or | ||
this = "NSString" and | ||
singular = "an NSString" and | ||
equivClass = "NSString" and | ||
csvLabel = "nsstring-length" | ||
or | ||
this = "String.utf8" and | ||
singular = "a String.utf8" and | ||
equivClass = "String.utf8" and | ||
csvLabel = "string-utf8-length" | ||
or | ||
this = "String.utf16" and | ||
singular = "a String.utf16" and | ||
equivClass = "NSString" and | ||
csvLabel = "string-utf16-length" | ||
or | ||
this = "String.unicodeScalars" and | ||
singular = "a String.unicodeScalars" and | ||
equivClass = "String.unicodeScalars" and | ||
csvLabel = "string-unicodescalars-length" | ||
} | ||
|
||
/** | ||
* Gets the equivalence class for this string type. If these are equal, | ||
* they should be treated as equivalent. | ||
*/ | ||
string getEquivClass() { result = equivClass } | ||
|
||
/** | ||
* Gets text for the singular form of this string type. | ||
*/ | ||
string getSingular() { result = singular } | ||
|
||
/** | ||
* Gets the label for this string type in CSV models. | ||
*/ | ||
string getCsvLabel() { result = csvLabel } | ||
} | ||
|
||
/** | ||
* A dataflow source for string length conflation vulnerabilities. That is, | ||
* a `DataFlow::Node` where a string length is generated. | ||
*/ | ||
abstract class StringLengthConflationSource extends DataFlow::Node { | ||
/** | ||
* Gets the `StringType` for this source. | ||
*/ | ||
abstract StringType getStringType(); | ||
} | ||
|
||
/** | ||
* A dataflow sink for string length conflation vulnerabilities. That is, | ||
* a `DataFlow::Node` where a string length is used. | ||
*/ | ||
abstract class StringLengthConflationSink extends DataFlow::Node { | ||
/** | ||
* Gets the correct `StringType` for this sink. | ||
*/ | ||
abstract StringType getCorrectStringType(); | ||
} | ||
|
||
/** | ||
* A sanitizer for string length conflation vulnerabilities. | ||
*/ | ||
abstract class StringLengthConflationSanitizer extends DataFlow::Node { } | ||
|
||
/** | ||
* A unit class for adding additional taint steps. | ||
*/ | ||
class StringLengthConflationAdditionalTaintStep extends Unit { | ||
/** | ||
* Holds if the step from `node1` to `node2` should be considered a taint | ||
* step for paths related to string length conflation vulnerabilities. | ||
*/ | ||
abstract predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo); | ||
} | ||
|
||
/** | ||
* A source defined in a CSV model. | ||
*/ | ||
private class DefaultStringLengthConflationSource extends StringLengthConflationSource { | ||
StringType stringType; | ||
|
||
DefaultStringLengthConflationSource() { sourceNode(this, stringType.getCsvLabel()) } | ||
|
||
override StringType getStringType() { result = stringType } | ||
} | ||
|
||
private class StringLengthConflationSources extends SourceModelCsv { | ||
override predicate row(string row) { | ||
row = | ||
[ | ||
";String;true;count;;;;string-length", ";NSString;true;length;;;;nsstring-length", | ||
";NSMutableString;true;length;;;;nsstring-length", | ||
] | ||
} | ||
} | ||
|
||
/** | ||
* An extra source that don't currently fit into the CSV scheme. | ||
*/ | ||
private class ExtraStringLengthConflationSource extends StringLengthConflationSource { | ||
StringType stringType; | ||
|
||
ExtraStringLengthConflationSource() { | ||
exists(MemberRefExpr memberRef, string typeName | | ||
( | ||
// result of a call to `String.utf8.count` | ||
typeName = "String.UTF8View" and | ||
stringType = "String.utf8" | ||
or | ||
// result of a call to `String.utf16.count` | ||
typeName = "String.UTF16View" and | ||
stringType = "String.utf16" | ||
or | ||
// result of a call to `String.unicodeScalars.count` | ||
typeName = "String.UnicodeScalarView" and | ||
stringType = "String.unicodeScalars" | ||
) and | ||
memberRef.getBase().getType().(NominalType).getName() = typeName and | ||
memberRef.getMember().(VarDecl).getName() = "count" and | ||
this.asExpr() = memberRef | ||
) | ||
} | ||
|
||
override StringType getStringType() { result = stringType } | ||
} | ||
|
||
/** | ||
* A sink defined in a CSV model. | ||
*/ | ||
private class DefaultStringLengthConflationSink extends StringLengthConflationSink { | ||
StringType stringType; | ||
|
||
DefaultStringLengthConflationSink() { sinkNode(this, stringType.getCsvLabel()) } | ||
|
||
override StringType getCorrectStringType() { result = stringType } | ||
} | ||
|
||
private class StringLengthConflationSinks extends SinkModelCsv { | ||
override predicate row(string row) { | ||
row = | ||
[ | ||
";Sequence;true;dropFirst(_:);;;Argument[0];string-length", | ||
";Sequence;true;dropLast(_:);;;Argument[0];string-length", | ||
";Sequence;true;prefix(_:);;;Argument[0];string-length", | ||
";Sequence;true;suffix(_:);;;Argument[0];string-length", | ||
";Collection;true;formIndex(_:offsetBy:);;;Argument[0..1];string-length", | ||
";Collection;true;formIndex(_:offsetBy:limitBy:);;;Argument[0..1];string-length", | ||
";Collection;true;removeFirst(_:);;;Argument[0];string-length", | ||
";RangeReplaceableCollection;true;removeLast(_:);;;Argument[0];string-length", | ||
";String;true;index(_:offsetBy:);;;Argument[0..1];string-length", | ||
";String;true;index(_:offsetBy:limitBy:);;;Argument[0..1];string-length", | ||
";String.Index;true;init(encodedOffset:);;;Argument[0];string-length", | ||
";NSRange;true;init(location:length:);;;Argument[0..1];nsstring-length", | ||
";NSString;true;character(at:);;;Argument[0];nsstring-length", | ||
";NSString;true;substring(from:);;;Argument[0];nsstring-length", | ||
";NSString;true;substring(to:);;;Argument[0];nsstring-length", | ||
";NSMutableString;true;character(at:);;;Argument[0];nsstring-length", | ||
";NSMutableString;true;substring(from:);;;Argument[0];nsstring-length", | ||
";NSMutableString;true;substring(to:);;;Argument[0];nsstring-length", | ||
";NSMutableString;true;insert(_:at:);;;Argument[1];nsstring-length", | ||
";;false;NSMakeRange(_:_:);;;Argument[0..1];nsstring-length", | ||
] | ||
} | ||
} |
39 changes: 39 additions & 0 deletions
39
swift/ql/lib/codeql/swift/security/StringLengthConflationQuery.qll
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
/** | ||
* Provides a taint-tracking configuration for reasoning about string length | ||
* conflation vulnerabilities. | ||
*/ | ||
|
||
import swift | ||
import codeql.swift.dataflow.DataFlow | ||
import codeql.swift.dataflow.TaintTracking | ||
import codeql.swift.security.StringLengthConflationExtensions | ||
|
||
/** | ||
* A configuration for tracking string lengths originating from source that is | ||
* a `String` or an `NSString` object, to a sink of a different kind that | ||
* expects an incompatible measure of length. | ||
*/ | ||
class StringLengthConflationConfiguration extends TaintTracking::Configuration { | ||
StringLengthConflationConfiguration() { this = "StringLengthConflationConfiguration" } | ||
|
||
override predicate isSource(DataFlow::Node node, string flowstate) { | ||
flowstate = node.(StringLengthConflationSource).getStringType() | ||
} | ||
|
||
override predicate isSink(DataFlow::Node node, string flowstate) { | ||
// Permit any *incorrect* flowstate, as those are the results the query | ||
// should report. | ||
exists(string correctFlowState | | ||
correctFlowState = node.(StringLengthConflationSink).getCorrectStringType() and | ||
flowstate.(StringType).getEquivClass() != correctFlowState.(StringType).getEquivClass() | ||
) | ||
} | ||
|
||
override predicate isSanitizer(DataFlow::Node sanitizer) { | ||
sanitizer instanceof StringLengthConflationSanitizer | ||
} | ||
|
||
override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { | ||
any(StringLengthConflationAdditionalTaintStep s).step(nodeFrom, nodeTo) | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once Swift starts converting dataflow configurations to modules this
flowstate
could be simplified to a newtype with 5 unit branches.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I noticed we will benefit from the new configurations here. I've added a note of your exact suggestion to https://github.com/github/codeql-c-team/issues/1625.