From 4cf40ed259938de3d56973e97fba093a2c102397 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Mon, 23 Oct 2023 23:16:55 +0100 Subject: [PATCH 1/6] Swift: Add a test case for withMutableCharacters. --- .../dataflow/taint/libraries/string.swift | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift b/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift index 294f57ecf048..ff74bd41b41b 100644 --- a/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift +++ b/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift @@ -79,7 +79,7 @@ extension String : CVarArg { init(platformString: UnsafePointer) { self.init() } func withPlatformString(_ body: (UnsafePointer) throws -> Result) rethrows -> Result { return 0 as! Result } - + mutating func withMutableCharacters(_ body: (inout String) -> R) -> R { return 0 as! R } mutating func replaceSubrange(_ subrange: Range, with newElements: C) @@ -687,3 +687,18 @@ func testDecodeCString() { sink(arg: str4) // $ tainted=669 sink(arg: repaired4) } + +func taintMutableCharacters() { + var str = "" + + sink(arg: str) + let rtn = str.withMutableCharacters({ + chars in + sink(arg: chars) + chars.append(source2()) + sink(arg: chars) // $ tainted=698 + return source() + }) + sink(arg: rtn) // $ MISSING: tainted=700 + sink(arg: str) // $ MISSING: tainted=698 +} From d3063e849e231a49aee86f6a7fd7e1e7dc49545d Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Mon, 23 Oct 2023 23:44:08 +0100 Subject: [PATCH 2/6] Swift: Model string closure methods. --- .../swift/frameworks/StandardLibrary/String.qll | 14 ++++++++++++++ .../dataflow/taint/libraries/string.swift | 14 +++++++------- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/String.qll b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/String.qll index 93a46c32e687..99f5c98e48d2 100644 --- a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/String.qll +++ b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/String.qll @@ -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", + ";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", @@ -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].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", @@ -126,6 +131,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", + ";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", ] } diff --git a/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift b/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift index ff74bd41b41b..d0c9b658f615 100644 --- a/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift +++ b/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift @@ -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 }) @@ -382,7 +382,7 @@ 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 @@ -390,7 +390,7 @@ func taintThroughEncodings() { }) 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)! @@ -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) @@ -699,6 +699,6 @@ func taintMutableCharacters() { sink(arg: chars) // $ tainted=698 return source() }) - sink(arg: rtn) // $ MISSING: tainted=700 - sink(arg: str) // $ MISSING: tainted=698 + sink(arg: rtn) // $ tainted=700 + sink(arg: str) // $ tainted=698 } From a5206028b085251f07d70a479efbc1ba7a081736 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Tue, 24 Oct 2023 14:42:03 +0100 Subject: [PATCH 3/6] Swift: Expand a test to explore why it fails (lack of pointer models and closure capture flow). --- .../dataflow/taint/libraries/string.swift | 109 ++++++++++-------- 1 file changed, 61 insertions(+), 48 deletions(-) diff --git a/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift b/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift index d0c9b658f615..e65f41f0ecd1 100644 --- a/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift +++ b/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift @@ -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) -> 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) -> 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) -> Int in + sink(arg: buffer[0]) + buffer.update(repeating: source4()) + sink(arg: buffer[0]) // $ tainted=475 + return 256 + } + ) + sink(arg: r3) // $ MISSING: 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 @@ -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]) @@ -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 } @@ -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 }) } @@ -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() { @@ -607,7 +620,7 @@ func callbackWithCleanPointer(ptr: UnsafeBufferPointer) throws - } func callbackWithTaintedPointer(ptr: UnsafeBufferPointer) throws -> Int { - sink(arg: ptr[0]) // $ tainted=617 + sink(arg: ptr[0]) // $ tainted=630 return source() } @@ -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({ @@ -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 } @@ -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) } @@ -696,9 +709,9 @@ func taintMutableCharacters() { chars in sink(arg: chars) chars.append(source2()) - sink(arg: chars) // $ tainted=698 + sink(arg: chars) // $ tainted=711 return source() }) - sink(arg: rtn) // $ tainted=700 - sink(arg: str) // $ tainted=698 + sink(arg: rtn) // $ tainted=713 + sink(arg: str) // $ tainted=711 } From 79f675cdb7705cfeda190d35e1a8308f6099ca9b Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Tue, 24 Oct 2023 15:43:06 +0100 Subject: [PATCH 4/6] Swift: Fix a model. --- swift/ql/lib/codeql/swift/frameworks/StandardLibrary/String.qll | 2 +- .../ql/test/library-tests/dataflow/taint/libraries/string.swift | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/String.qll b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/String.qll index 99f5c98e48d2..749dc5b8940b 100644 --- a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/String.qll +++ b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/String.qll @@ -114,7 +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].CollectionElement;ReturnValue;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", diff --git a/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift b/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift index e65f41f0ecd1..27cc73bd67e1 100644 --- a/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift +++ b/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift @@ -477,7 +477,7 @@ func taintFromUInt8Array() { return 256 } ) - sink(arg: r3) // $ MISSING: tainted=475 + 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 From d2d70cc78201ecdebb24e5f93ae5b9377b3cf819 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Tue, 24 Oct 2023 15:49:33 +0100 Subject: [PATCH 5/6] Swift: Change note. --- swift/ql/lib/change-notes/2023-10-24-string.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 swift/ql/lib/change-notes/2023-10-24-string.md diff --git a/swift/ql/lib/change-notes/2023-10-24-string.md b/swift/ql/lib/change-notes/2023-10-24-string.md new file mode 100644 index 000000000000..6020e708d3bb --- /dev/null +++ b/swift/ql/lib/change-notes/2023-10-24-string.md @@ -0,0 +1,5 @@ +--- +category: minorAnalysis +--- + +* Added flow models for `String` methods involving closures such as `String.withUTF8(_:)`. From bf503849eaace470b9b89f995af787ae7f322996 Mon Sep 17 00:00:00 2001 From: Geoffrey White <40627776+geoffw0@users.noreply.github.com> Date: Wed, 25 Oct 2023 11:06:56 +0100 Subject: [PATCH 6/6] Swift: Fixup line numbers after merge. --- .../dataflow/taint/libraries/string.swift | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift b/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift index 86ad32420bd6..266f80a22084 100644 --- a/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift +++ b/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift @@ -706,28 +706,28 @@ func testSubstringMembers() { let tainted = source2() let sub1 = tainted[..