Skip to content

Swift: Models for String methods involving closures. #14578

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
rdmarsh2 merged 7 commits into from
Nov 2, 2023
Merged

Swift: Models for String methods involving closures. #14578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4cf40ed
Swift: Add a test case for withMutableCharacters.
geoffw0 Oct 23, 2023
d3063e8
Swift: Model string closure methods.
geoffw0 Oct 23, 2023
a520602
Swift: Expand a test to explore why it fails (lack of pointer models …
geoffw0 Oct 24, 2023
79f675c
Swift: Fix a model.
geoffw0 Oct 24, 2023
d2d70cc
Swift: Change note.
geoffw0 Oct 24, 2023
bf6e6a1
Merge branch 'main' into stringwith
geoffw0 Oct 25, 2023
bf50384
Swift: Fixup line numbers after merge.
geoffw0 Oct 25, 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-10-24-string.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 for `String` methods involving closures such as `String.withUTF8(_:)`.
14 changes: 14 additions & 0 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 @@ -68,6 +68,10 @@ private class StringSummaries extends SummaryModelCsv {
";StringProtocol;true;trimmingCharacters(in:);;;Argument[-1];ReturnValue;taint",
";StringProtocol;true;uppercased();;;Argument[-1];ReturnValue;taint",
";StringProtocol;true;uppercased(with:);;;Argument[-1];ReturnValue;taint",
";StringProtocol;true;withCString(_:);;;Argument[-1];Argument[0].Parameter[0].CollectionElement;taint",
";StringProtocol;true;withCString(_:);;;Argument[0].ReturnValue;ReturnValue;value",
Comment on lines +71 to +72
Copy link
Contributor

@rdmarsh2 rdmarsh2 Oct 25, 2023

Choose a reason for hiding this comment

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

Should there be a step from the mutated parameter of the callback to the this parameter of the method?

Copy link
Contributor Author

@geoffw0 geoffw0 Oct 31, 2023

Choose a reason for hiding this comment

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

Discussed on the other comment.

";StringProtocol;true;withCString(encodedAs:_:);;;Argument[-1];Argument[1].Parameter[0].CollectionElement;taint",
";StringProtocol;true;withCString(encodedAs:_:);;;Argument[1].ReturnValue;ReturnValue;value",
";String;true;init(decoding:);;;Argument[0];ReturnValue;taint",
";String;true;init(_:);;;Argument[0];ReturnValue;taint",
";String;true;init(_:);;;Argument[0];ReturnValue.OptionalSome;taint",
Expand Down Expand Up @@ -110,6 +114,7 @@ private class StringSummaries extends SummaryModelCsv {
";String;true;init(validating:);;;Argument[0];ReturnValue.OptionalSome;taint",
";String;true;init(validatingPlatformString:);;;Argument[0];ReturnValue.OptionalSome;taint",
";String;true;init(validatingPlatformString:);;;Argument[0].CollectionElement;ReturnValue.OptionalSome;taint",
";String;true;init(unsafeUninitializedCapacity:initializingUTF8With:);;;Argument[1].Parameter[0].CollectionElement;ReturnValue;taint",
";String;true;localizedStringWithFormat(_:_:);;;Argument[0];ReturnValue;taint",
";String;true;localizedStringWithFormat(_:_:);;;Argument[1].CollectionElement;ReturnValue;taint",
";String;true;insert(contentsOf:at:);;;Argument[0];Argument[-1];taint",
Expand All @@ -125,6 +130,15 @@ private class StringSummaries extends SummaryModelCsv {
";String;true;encode(to:);;;Argument[-1];Argument[0];taint",
";String;true;decodeCString(_:as:repairingInvalidCodeUnits:);;;Argument[0];ReturnValue.TupleElement[0];taint",
";String;true;decodeCString(_:as:repairingInvalidCodeUnits:);;;Argument[0].CollectionElement;ReturnValue.TupleElement[0];taint",
";String;true;withUTF8(_:);;;Argument[-1];Argument[0].Parameter[0].CollectionElement;taint",
";String;true;withUTF8(_:);;;Argument[0].Parameter[0].CollectionElement;Argument[-1];taint",
";String;true;withUTF8(_:);;;Argument[0].ReturnValue;ReturnValue;value",
";String;true;withPlatformString(_:);;;Argument[-1];Argument[0].Parameter[0].CollectionElement;taint",
";String;true;withPlatformString(_:);;;Argument[0].ReturnValue;ReturnValue;value",
Comment on lines +136 to +137
Copy link
Contributor

@rdmarsh2 rdmarsh2 Oct 25, 2023

Choose a reason for hiding this comment

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

Same question here.

Copy link
Contributor Author

@geoffw0 geoffw0 Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think you can mutate the string with these particular methods. I'll check...

Copy link
Contributor Author

@geoffw0 geoffw0 Oct 31, 2023

Choose a reason for hiding this comment

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

Sorry for taking a while to get back to this. I think it's only possible to modify the object using the methods marked mutating, that is withMutableCharacters and withUTF8 in this PR (and I'm not sure it's practically possible with the latter, see the doc).

Some experimentation here - notice that the changes are never reflected back in s.

";String;true;withMutableCharacters(_:);;;Argument[-1];Argument[0].Parameter[0];value",
";String;true;withMutableCharacters(_:);;;Argument[0].Parameter[0];Argument[-1];value",
";String;true;withMutableCharacters(_:);;;Argument[0].Parameter[0].CollectionElement;Argument[-1];taint",
";String;true;withMutableCharacters(_:);;;Argument[0].ReturnValue;ReturnValue;value",
";LosslessStringConvertible;true;init(_:);;;Argument[0];ReturnValue;taint",
";Substring;true;withUTF8(_:);;;Argument[-1];Argument[0].Parameter[0].CollectionElement;taint",
";Substring;true;withUTF8(_:);;;Argument[0].Parameter[0].CollectionElement;Argument[-1];taint",
Expand Down
152 changes: 91 additions & 61 deletions swift/ql/test/library-tests/dataflow/taint/libraries/string.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ extension String : CVarArg {
init(platformString: UnsafePointer<CInterop.PlatformChar>) { self.init() }

func withPlatformString<Result>(_ body: (UnsafePointer<CInterop.PlatformChar>) throws -> Result) rethrows -> Result { return 0 as! Result }

mutating func withMutableCharacters<R>(_ body: (inout String) -> R) -> R { return 0 as! R }


mutating func replaceSubrange<C>(_ subrange: Range<String.Index>, with newElements: C)
Expand Down Expand Up @@ -372,7 +372,7 @@ func taintThroughEncodings() {
})
tainted.withUTF8({
buffer in
sink(arg: buffer[0]) // $ MISSING: tainted=366
sink(arg: buffer[0]) // $ tainted=366
sink(arg: buffer.baseAddress!) // $ MISSING: tainted=366
})

Expand All @@ -382,15 +382,15 @@ func taintThroughEncodings() {
})
tainted.withCString({
ptr in
sink(arg: ptr[0]) // $ MISSING: tainted=366
sink(arg: ptr[0]) // $ tainted=366
})
clean.withCString(encodedAs: UTF8.self, {
ptr in
sink(arg: ptr[0])
})
tainted.withCString(encodedAs: UTF8.self, {
ptr in
sink(arg: ptr[0]) // $ MISSING: tainted=366
sink(arg: ptr[0]) // $ tainted=366
})

let arrayString1 = clean.cString(using: String.Encoding.utf8)!
Expand Down Expand Up @@ -421,8 +421,8 @@ func taintThroughEncodings() {
})
tainted.withPlatformString({
ptr in
sink(arg: ptr[0]) // $ MISSING: tainted=366
sink(arg: String(platformString: ptr)) // $ MISSING: tainted=366
sink(arg: ptr[0]) // $ tainted=366
sink(arg: String(platformString: ptr)) // $ tainted=366

let buffer = UnsafeBufferPointer(start: ptr, count: 10)
let arrayString = Array(buffer)
Expand All @@ -449,26 +449,38 @@ func taintFromUInt8Array() {
var cleanUInt8Values: [UInt8] = [0x41, 0x42, 0x43, 0] // "ABC"
var taintedUInt8Values: [UInt8] = [source4()]

sink(arg: String(unsafeUninitializedCapacity: 256, initializingUTF8With: {
sink(arg: taintedUInt8Values[0]) // $ tainted=450
let r1 = String(unsafeUninitializedCapacity: 256, initializingUTF8With: {
(buffer: UnsafeMutableBufferPointer<UInt8>) -> Int in
sink(arg: buffer[0])
let _ = buffer.initialize(from: cleanUInt8Values)
sink(arg: buffer[0])
return 3
}
))
sink(arg: String(unsafeUninitializedCapacity: 256, initializingUTF8With: { // $ MISSING: tainted=450
)
sink(arg: r1)
let r2 = String(unsafeUninitializedCapacity: 256, initializingUTF8With: {
(buffer: UnsafeMutableBufferPointer<UInt8>) -> Int in
sink(arg: buffer[0])
sink(arg: taintedUInt8Values[0]) // $ MISSING: tainted=450
let _ = buffer.initialize(from: taintedUInt8Values)
sink(arg: buffer[0]) // $ MISSING: tainted=450
return 256
}
))
)
sink(arg: r2) // $ MISSING: tainted=450
let r3 = String(unsafeUninitializedCapacity: 256, initializingUTF8With: {
(buffer: UnsafeMutableBufferPointer<UInt8>) -> Int in
sink(arg: buffer[0])
buffer.update(repeating: source4())
sink(arg: buffer[0]) // $ tainted=475
return 256
}
)
sink(arg: r3) // $ tainted=475

sink(arg: String(bytes: cleanUInt8Values, encoding: String.Encoding.utf8)!)
sink(arg: String(bytes: taintedUInt8Values, encoding: String.Encoding.utf8)!) // $ tainted=450

sink(arg: String(cString: cleanUInt8Values))
sink(arg: String(cString: taintedUInt8Values)) // $ tainted=450

Expand All @@ -484,7 +496,6 @@ func taintFromUInt8Array() {
sink(arg: buffer.baseAddress!) // $ MISSING: tainted=450
sink(arg: String(cString: buffer.baseAddress!)) // $ MISSING: tainted=450
})

try! cleanUInt8Values.withUnsafeMutableBytes({
(buffer: UnsafeMutableRawBufferPointer) throws in
sink(arg: buffer[0])
Expand Down Expand Up @@ -515,15 +526,15 @@ func taintThroughCCharArray() {
})
taintedCCharValues.withUnsafeBufferPointer({
ptr in
sink(arg: ptr[0]) // $ tainted=506
sink(arg: ptr.baseAddress!) // $ MISSING: tainted=506
sink(arg: String(utf8String: ptr.baseAddress!)!) // $ MISSING: tainted=506
sink(arg: String(validatingUTF8: ptr.baseAddress!)!) // $ MISSING: tainted=506
sink(arg: String(cString: ptr.baseAddress!)) // $ MISSING: tainted=506
sink(arg: ptr[0]) // $ tainted=517
sink(arg: ptr.baseAddress!) // $ MISSING: tainted=517
sink(arg: String(utf8String: ptr.baseAddress!)!) // $ MISSING: tainted=517
sink(arg: String(validatingUTF8: ptr.baseAddress!)!) // $ MISSING: tainted=517
sink(arg: String(cString: ptr.baseAddress!)) // $ MISSING: tainted=517
})

sink(arg: String(cString: cleanCCharValues))
sink(arg: String(cString: taintedCCharValues)) // $ tainted=506
sink(arg: String(cString: taintedCCharValues)) // $ tainted=517
}

func source6() -> unichar { return 0 }
Expand All @@ -541,10 +552,10 @@ func taintThroughUnicharArray() {
})
taintedUnicharValues.withUnsafeBufferPointer({
ptr in
sink(arg: ptr[0]) // $ tainted=533
sink(arg: ptr.baseAddress!) // $ MISSING: tainted=533
sink(arg: String(utf16CodeUnits: ptr.baseAddress!, count: ptr.count)) // $ MISSING: tainted=533
sink(arg: String(utf16CodeUnitsNoCopy: ptr.baseAddress!, count: ptr.count, freeWhenDone: false)) // $ MISSING: tainted=533
sink(arg: ptr[0]) // $ tainted=544
sink(arg: ptr.baseAddress!) // $ MISSING: tainted=544
sink(arg: String(utf16CodeUnits: ptr.baseAddress!, count: ptr.count)) // $ MISSING: tainted=5344
sink(arg: String(utf16CodeUnitsNoCopy: ptr.baseAddress!, count: ptr.count, freeWhenDone: false)) // $ MISSING: tainted=544
})
}

Expand All @@ -553,43 +564,45 @@ func source7() -> Substring { return Substring() }
func taintThroughSubstring() {
let tainted = source2()

sink(arg: source7()) // $ tainted=556
sink(arg: source7()) // $ tainted=567

let sub1 = tainted[tainted.startIndex ..< tainted.endIndex]
sink(arg: sub1) // $ tainted=554
sink(arg: String(sub1)) // $ tainted=554
sink(arg: sub1) // $ tainted=565
sink(arg: String(sub1)) // $ tainted=565

let sub2 = tainted.prefix(10)
sink(arg: sub2) // $ tainted=554
sink(arg: String(sub2)) // $ tainted=554
sink(arg: sub2) // $ tainted=565
sink(arg: String(sub2)) // $ tainted=565

let sub3 = tainted.prefix(through: tainted.endIndex)
sink(arg: sub3) // $ tainted=554
sink(arg: String(sub3)) // $ tainted=554
sink(arg: sub3) // $ tainted=565
sink(arg: String(sub3)) // $ tainted=565

let sub4 = tainted.prefix(upTo: tainted.endIndex)
sink(arg: sub4) // $ tainted=554
sink(arg: String(sub4)) // $ tainted=554
sink(arg: sub4) // $ tainted=565
sink(arg: String(sub4)) // $ tainted=565

let sub5 = tainted.suffix(10)
sink(arg: sub5) // $ tainted=554
sink(arg: String(sub5)) // $ tainted=554
sink(arg: sub5) // $ tainted=565
sink(arg: String(sub5)) // $ tainted=565

let sub6 = tainted.suffix(from: tainted.startIndex)
sink(arg: sub6) // $ tainted=554
sink(arg: String(sub6)) // $ tainted=554
sink(arg: sub6) // $ tainted=565
sink(arg: String(sub6)) // $ tainted=565
}

func taintedThroughConversion() {
sink(arg: String(0))
sink(arg: String(source())) // $ tainted=585
sink(arg: String(source())) // $ tainted=596

sink(arg: Int(0).description)
sink(arg: source().description) // $ tainted=587
sink(arg: source().description) // $ tainted=599

sink(arg: String(describing: 0))
sink(arg: String(describing: source())) // $ tainted=589
sink(arg: String(describing: source())) // $ tainted=602

sink(arg: Int("123")!)
sink(arg: Int(source2())!) // $ tainted=592
sink(arg: Int(source2())!) // $ tainted=605
}

func untaintedFields() {
Expand All @@ -607,7 +620,7 @@ func callbackWithCleanPointer(ptr: UnsafeBufferPointer<String.Element>) throws -
}

func callbackWithTaintedPointer(ptr: UnsafeBufferPointer<String.Element>) throws -> Int {
sink(arg: ptr[0]) // $ tainted=617
sink(arg: ptr[0]) // $ tainted=630

return source()
}
Expand All @@ -626,7 +639,7 @@ func furtherTaintThroughCallbacks() {
ptr in
return source()
})
sink(arg: result2!) // $ tainted=627
sink(arg: result2!) // $ tainted=640

// return values from the closure (2)
if let result3 = clean.withContiguousStorageIfAvailable({
Expand All @@ -639,28 +652,28 @@ func furtherTaintThroughCallbacks() {
ptr in
return source()
}) {
sink(arg: result4) // $ tainted=640
sink(arg: result4) // $ tainted=653
}

// using a non-closure function
let result5 = try? clean.withContiguousStorageIfAvailable(callbackWithCleanPointer)
sink(arg: result5!)
let result6 = try? tainted.withContiguousStorageIfAvailable(callbackWithTaintedPointer)
sink(arg: result6!) // $ tainted=612
sink(arg: result6!) // $ tainted=625
}

func testAppendingFormat() {
var s1 = source2()
sink(arg: s1.appendingFormat("%s %i", "", 0)) // $ tainted=653
sink(arg: s1.appendingFormat("%s %i", "", 0)) // $ tainted=666

var s2 = ""
sink(arg: s2.appendingFormat(source2(), "", 0)) // $ tainted=657
sink(arg: s2.appendingFormat(source2(), "", 0)) // $ tainted=670

var s3 = ""
sink(arg: s3.appendingFormat("%s %i", source2(), 0)) // $ tainted=660
sink(arg: s3.appendingFormat("%s %i", source2(), 0)) // $ tainted=673

var s4 = ""
sink(arg: s4.appendingFormat("%s %i", "", source())) // $ tainted=663
sink(arg: s4.appendingFormat("%s %i", "", source())) // $ tainted=676
}

func sourceUInt8() -> UInt8 { return 0 }
Expand All @@ -669,22 +682,22 @@ func testDecodeCString() {
var input : [UInt8] = [1, 2, 3, sourceUInt8()]

let (str1, repaired1) = String.decodeCString(input, as: UTF8.self)!
sink(arg: str1) // $ tainted=669
sink(arg: str1) // $ tainted=682
sink(arg: repaired1)

input.withUnsafeBufferPointer({
ptr in
let (str2, repaired2) = String.decodeCString(ptr.baseAddress, as: UTF8.self)!
sink(arg: str2) // $ MISSING: tainted=669
sink(arg: str2) // $ MISSING: tainted=682
sink(arg: repaired2)
})

let (str3, repaired3) = String.decodeCString(source2(), as: UTF8.self)!
sink(arg: str3) // $ tainted=682
sink(arg: str3) // $ tainted=695
sink(arg: repaired3)

let (str4, repaired4) = String.decodeCString(&input, as: UTF8.self)!
sink(arg: str4) // $ tainted=669
sink(arg: str4) // $ tainted=682
sink(arg: repaired4)
}

Expand All @@ -693,26 +706,43 @@ func testSubstringMembers() {
let tainted = source2()

let sub1 = tainted[..<tainted.index(tainted.endIndex, offsetBy: -5)]
sink(arg: sub1) // $ tainted=693
sink(arg: sub1.base) // $ tainted=693
sink(arg: sub1.utf8) // $ tainted=693
sink(arg: sub1.capitalized) // $ tainted=693
sink(arg: sub1.description) // $ tainted=693
sink(arg: sub1) // $ tainted=706
sink(arg: sub1.base) // $ tainted=706
sink(arg: sub1.utf8) // $ tainted=706
sink(arg: sub1.capitalized) // $ tainted=706
sink(arg: sub1.description) // $ tainted=706

var sub2 = tainted[tainted.index(tainted.startIndex, offsetBy: 5)...]
sink(arg: sub2) // $ tainted=693
sink(arg: sub2) // $ tainted=706
let result1 = sub2.withUTF8({
buffer in
sink(arg: buffer[0]) // $ tainted=693
sink(arg: buffer[0]) // $ tainted=706
return source()
})
sink(arg: result1) // $ tainted=707
sink(arg: result1) // $ tainted=720

let sub3 = Substring(sub2.utf8)
sink(arg: sub3) // $ tainted=693
sink(arg: sub3) // $ tainted=706

var sub4 = clean.prefix(10)
sink(arg: sub4)
sub4.replaceSubrange(..<clean.endIndex, with: sub1)
sink(arg: sub4) // $ tainted=693
sink(arg: sub4) // $ tainted=706
}

// ---

func taintMutableCharacters() {
var str = ""

sink(arg: str)
let rtn = str.withMutableCharacters({
chars in
sink(arg: chars)
chars.append(source2())
sink(arg: chars) // $ tainted=742
return source()
})
sink(arg: rtn) // $ tainted=744
sink(arg: str) // $ tainted=742
}