Merge pull request #14266 from geoffw0/quickfix

Swift: Improve taint models for NSString
github · Sep 21, 2023 · 0530981 · 0530981
2 parents 7e04ac5 + af315c5
commit 0530981
Show file tree

Hide file tree

Showing 5 changed files with 21 additions and 11 deletions.
diff --git a/swift/ql/lib/change-notes/2023-09-29-nsstring.md b/swift/ql/lib/change-notes/2023-09-29-nsstring.md
@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+
+* Modelled varargs function in `NSString` more accurately.
diff --git a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/NsString.qll b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/NsString.qll
@@ -50,8 +50,10 @@ private class NsStringSummaries extends SummaryModelCsv {
         ";NSString;true;init(format:arguments:);;;Argument[0..1];ReturnValue;taint",
         ";NSString;true;init(format:locale:arguments:);;;Argument[0];ReturnValue;taint",
         ";NSString;true;init(format:locale:arguments:);;;Argument[2];ReturnValue;taint",
-        ";NSString;true;init(format:_:);;;Argument[0];ReturnValue;taint", //0..
-        ";NSString;true;init(format:locale:_:);;;Argument[0];ReturnValue;taint", //0,2..
+        ";NSString;true;init(format:_:);;;Argument[0];ReturnValue;taint",
+        ";NSString;true;init(format:_:);;;Argument[1].CollectionElement;ReturnValue;taint",
+        ";NSString;true;init(format:locale:_:);;;Argument[0];ReturnValue;taint",
+        ";NSString;true;init(format:locale:_:);;;Argument[2].CollectionElement;ReturnValue;taint",
         ";NSString;true;init(data:encoding:);;;Argument[0];ReturnValue;taint",
         ";NSString;true;init(contentsOfFile:);;;Argument[0];ReturnValue;taint",
         ";NSString;true;init(contentsOfFile:encoding:);;;Argument[0];ReturnValue;taint",
@@ -60,7 +62,8 @@ private class NsStringSummaries extends SummaryModelCsv {
         ";NSString;true;init(contentsOf:encoding:);;;Argument[0];ReturnValue;taint",
         ";NSString;true;init(contentsOf:usedEncoding:);;;Argument[0];ReturnValue;taint",
         ";NSString;true;init(coder:);;;Argument[0];ReturnValue;taint",
-        ";NSString;true;localizedStringWithFormat(_:_:);;;Argument[0];ReturnValue;taint", //0..
+        ";NSString;true;localizedStringWithFormat(_:_:);;;Argument[0];ReturnValue;taint",
+        ";NSString;true;localizedStringWithFormat(_:_:);;;Argument[1].CollectionElement;ReturnValue;taint",
         ";NSString;true;character(at:);;;Argument[-1];ReturnValue;taint",
         ";NSString;true;getCharacters(_:);;;Argument[-1];Argument[0];taint",
         ";NSString;true;getCharacters(_:range:);;;Argument[-1];Argument[0];taint",
@@ -72,7 +75,8 @@ private class NsStringSummaries extends SummaryModelCsv {
         ";NSString;true;getCString(_:maxLength:);;;Argument[-1];Argument[0];taint",
         ";NSString;true;getCString(_:maxLength:encoding:);;;Argument[-1];Argument[0];taint",
         ";NSString;true;getCString(_:maxLength:range:remaining:);;;Argument[-1];Argument[0];taint",
-        ";NSString;true;appendingFormat(_:_:);;;Argument[-1..0];ReturnValue;taint", // -1..
+        ";NSString;true;appendingFormat(_:_:);;;Argument[-1..0];ReturnValue;taint",
+        ";NSString;true;appendingFormat(_:_:);;;Argument[1].CollectionElement;ReturnValue;taint",
         ";NSString;true;appending(_:);;;Argument[-1..0];ReturnValue;taint",
         ";NSString;true;padding(toLength:withPad:startingAt:);;;Argument[-1];ReturnValue;taint",
         ";NSString;true;padding(toLength:withPad:startingAt:);;;Argument[1];ReturnValue;taint",
@@ -119,7 +123,8 @@ private class NsStringSummaries extends SummaryModelCsv {
         ";NSMutableString;true;replaceCharacters(in:with:);;;Argument[1];Argument[-1];taint",
         ";NSMutableString;true;replaceOccurrences(of:with:options:range:);;;Argument[1];Argument[-1];taint",
         ";NSMutableString;true;setString(_:);;;Argument[0];Argument[-1];taint",
-        ";NSMutableString;true;appendFormat(_:_:);;;Argument[0];Argument[-1];taint", //0..
+        ";NSMutableString;true;appendFormat(_:_:);;;Argument[0];Argument[-1];taint",
+        ";NSMutableString;true;appendFormat(_:_:);;;Argument[1].CollectionElement;Argument[-1];taint",
       ]
   }
 }

diff --git a/swift/ql/test/library-tests/dataflow/taint/libraries/TaintInline.expected b/swift/ql/test/library-tests/dataflow/taint/libraries/TaintInline.expected
@@ -1,2 +1,2 @@
-failures
 testFailures
+failures
diff --git a/swift/ql/test/library-tests/dataflow/taint/libraries/nsstring.swift b/swift/ql/test/library-tests/dataflow/taint/libraries/nsstring.swift
@@ -52,7 +52,7 @@ class NSString : NSObject, NSCopying, NSMutableCopying {
   func copy(with zone: NSZone? = nil) -> Any { return 0 }
   func mutableCopy(with zone: NSZone? = nil) -> Any { return 0 }
 
-  class func localizedStringWithFormat(_ format: NSString, _ args: CVarArg) -> Self { return (nil as Self?)! }
+  class func localizedStringWithFormat(_ format: NSString, _ args: CVarArg...) -> Self { return (nil as Self?)! }
   class func path(withComponents components: [String]) -> String { return "" }
   class func string(withCString bytes: UnsafePointer<CChar>) -> Any? { return nil }
   class func string(withCString bytes: UnsafePointer<CChar>, length: Int) -> Any? { return nil }
@@ -185,7 +185,7 @@ func sourceUnsafeMutableRawPointer() -> UnsafeMutableRawPointer { return (nil as
 func sourceCString() -> UnsafePointer<CChar> { return (nil as UnsafePointer<CChar>?)! }
 func sourceData() -> Data { return Data(0) }
 func sourceStringArray() -> [String] { return [] }
-
+func sourceInt() -> Int { return 0 }
 func sink(arg: Any) {}
 
 func taintThroughInterpolatedStrings() {
@@ -244,8 +244,8 @@ func taintThroughInterpolatedStrings() {
 
   let harmless = NSString(string: "harmless")
   let myRange = NSRange(location:0, length: 128)
-
-  sink(arg: NSString.localizedStringWithFormat(sourceNSString(), (nil as CVarArg?)!)) // $ tainted=248
+  sink(arg: NSString.localizedStringWithFormat(NSString(string: "%i %i %i"), 1, sourceInt(), 3)) // $ tainted=247
+  sink(arg: NSString.localizedStringWithFormat(sourceNSString(), 1, 2, 3)) // $ tainted=248
   sink(arg: sourceNSString().character(at: 0)) // $ tainted=249
   sink(arg: sourceNSString().cString(using: 0)!) // $ tainted=250
   sink(arg: sourceNSString().cString()) // $ tainted=251

diff --git a/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift b/swift/ql/test/library-tests/dataflow/taint/libraries/string.swift
@@ -226,6 +226,7 @@ func taintThroughSimpleStringOperations() {
   sink(arg: String(format: tainted, locale: nil, 1, 2, 3)) // $ tainted=217
   sink(arg: String(format: tainted, locale: nil, arguments: [])) // $ tainted=217
   sink(arg: String.localizedStringWithFormat(tainted, 1, 2, 3)) // $ tainted=217
+  sink(arg: String.localizedStringWithFormat("%i %s %i", 1, tainted, 3)) // $ tainted=217
   sink(arg: String(format: "%s", tainted)) // $ tainted=217
   sink(arg: String(format: "%i %i %i", 1, 2, taintedInt)) // $ tainted=218
 
@@ -235,7 +236,6 @@ func taintThroughSimpleStringOperations() {
   sink(arg: tainted.dropFirst(10)) // $ tainted=217
   sink(arg: tainted.dropLast(10)) // $ tainted=217
   sink(arg: tainted.substring(from: tainted.startIndex)) // $ tainted=217
-
   sink(arg: tainted.lowercased()) // $ tainted=217
   sink(arg: tainted.uppercased()) // $ tainted=217
   sink(arg: tainted.lowercased(with: nil)) // $ tainted=217