Merge pull request github#13034 from geoffw0/swifttodos

geoffw0 · web-flow · commit 358720440328 · 2023-05-10T11:09:27.000+01:00
Swift: Delete some TODO comments
diff --git a/swift/ql/lib/codeql/swift/elements/decl/PrecedenceGroupDecl.qll b/swift/ql/lib/codeql/swift/elements/decl/PrecedenceGroupDecl.qll
@@ -1,7 +1,5 @@
 private import codeql.swift.generated.decl.PrecedenceGroupDecl
 
 class PrecedenceGroupDecl extends Generated::PrecedenceGroupDecl {
-  override string toString() {
-    result = "precedencegroup ..." // TODO: Once we extract the name we can improve this.
-  }
+  override string toString() { result = "precedencegroup ..." }
 }
diff --git a/swift/ql/lib/codeql/swift/elements/expr/TupleElementExpr.qll b/swift/ql/lib/codeql/swift/elements/expr/TupleElementExpr.qll
@@ -1,7 +1,5 @@
 private import codeql.swift.generated.expr.TupleElementExpr
 
 class TupleElementExpr extends Generated::TupleElementExpr {
-  override string toString() {
-    result = "." + this.getIndex() // TODO: Can be improved once we extract the name
-  }
+  override string toString() { result = "." + this.getIndex() }
 }
diff --git a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/FilePath.qll b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/FilePath.qll
@@ -15,7 +15,6 @@ class FilePath extends StructDecl {
  */
 private class FilePathSummaries extends SummaryModelCsv {
   override predicate row(string row) {
-    // TODO: Properly model this class
     row = ";FilePath;true;init(stringLiteral:);(String);;Argument[0];ReturnValue;taint"
   }
 }
diff --git a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/NsUrl.qll b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/NsUrl.qll
@@ -10,7 +10,6 @@ private import codeql.swift.dataflow.ExternalFlow
  */
 private class NsUrlSummaries extends SummaryModelCsv {
   override predicate row(string row) {
-    // TODO: Properly model this class
     row = ";NSURL;true;init(string:);(String);;Argument[0];ReturnValue;taint"
   }
 }
diff --git a/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/WebView.qll b/swift/ql/lib/codeql/swift/frameworks/StandardLibrary/WebView.qll
@@ -138,7 +138,6 @@ private class JsValueSummaries extends SummaryModelCsv {
         ";JSValue;true;toRange();;;Argument[-1];ReturnValue;taint",
         ";JSValue;true;toRect();;;Argument[-1];ReturnValue;taint",
         ";JSValue;true;toSize();;;Argument[-1];ReturnValue;taint",
-        // TODO: These models could use content flow to be more precise
         ";JSValue;true;atIndex(_:);;;Argument[-1];ReturnValue;taint",
         ";JSValue;true;defineProperty(_:descriptor:);;;Argument[1];Argument[-1];taint",
         ";JSValue;true;forProperty(_:);;;Argument[-1];ReturnValue;taint",
diff --git a/swift/ql/lib/codeql/swift/security/CleartextStoragePreferencesExtensions.qll b/swift/ql/lib/codeql/swift/security/CleartextStoragePreferencesExtensions.qll
@@ -63,10 +63,12 @@ private class NSUbiquitousKeyValueStore extends CleartextStoragePreferencesSink
  * A more complicated case, this is a macOS-only way of writing to
  * NSUserDefaults by modifying the `NSUserDefaultsController.values: Any`
  * object via reflection (`perform(Selector)`) or the `NSKeyValueCoding`,
- * `NSKeyValueBindingCreation` APIs. (TODO)
+ * `NSKeyValueBindingCreation` APIs.
  */
 private class NSUserDefaultsControllerStore extends CleartextStoragePreferencesSink {
-  NSUserDefaultsControllerStore() { none() }
+  NSUserDefaultsControllerStore() {
+    none() // not yet implemented
+  }
 
   override string getStoreName() { result = "the user defaults database" }
 }
diff --git a/swift/ql/lib/codeql/swift/security/PathInjectionExtensions.qll b/swift/ql/lib/codeql/swift/security/PathInjectionExtensions.qll
@@ -36,7 +36,6 @@ private class DefaultPathInjectionSink extends PathInjectionSink {
 private class DefaultPathInjectionBarrier extends PathInjectionBarrier {
   DefaultPathInjectionBarrier() {
     // This is a simplified implementation.
-    // TODO: Implement a complete path barrier when Guards are available.
     exists(CallExpr starts, CallExpr normalize, DataFlow::Node validated |
       starts.getStaticTarget().getName() = "starts(with:)" and
       starts.getStaticTarget().getEnclosingDecl() instanceof FilePath and
diff --git a/swift/ql/test/query-tests/Security/CWE-611/testLibxmlXXE.swift b/swift/ql/test/query-tests/Security/CWE-611/testLibxmlXXE.swift
@@ -1,18 +1,52 @@
 // --- stubs ---
 
+class NSObject {
+}
+
 class Data {
     init<S>(_ elements: S) {}
-    func copyBytes(to: UnsafeMutablePointer<UInt8>, count: Int) {}
 }
 
 struct URL {
 	init?(string: String) {}
+
+    func appendingPathComponent(_ pathComponent: String) -> URL { return self }
+}
+
+class FileManager : NSObject {
+    class var `default`: FileManager { get { return FileManager() } }
+
+    var homeDirectoryForCurrentUser: URL { get { return URL(string: "")! } }
+}
+
+class Bundle : NSObject {
+    class var main: Bundle { get { return Bundle() } }
+
+    func path(forResource name: String?, ofType ext: String?) -> String? { return "" }
 }
 
-extension String {
-	init(contentsOf: URL) {
-        let data = ""
-        self.init(data)
+struct FilePath {
+    init?(_ url: URL) { }
+    init(_ string: String) { }
+}
+
+struct FileDescriptor {
+    let rawValue: CInt
+
+    struct AccessMode : RawRepresentable {
+        var rawValue: CInt
+
+        static let readOnly = AccessMode(rawValue: 0)
+    }
+
+    static func open(
+        _ path: FilePath,
+        _ mode: FileDescriptor.AccessMode,
+        options: Int? = nil,
+        permissions: Int? = nil,
+        retryOnInterrupt: Bool = true
+    ) throws -> FileDescriptor {
+        return FileDescriptor(rawValue: 0)
     }
 }
 
@@ -46,53 +80,93 @@ func xmlParseInNodeContext(_ node: xmlNodePtr!, _ data: UnsafePointer<CChar>!, _
 func xmlCtxtReadMemory(_ ctxt: xmlParserCtxtPtr!, _ buffer: UnsafePointer<CChar>!, _ size: Int32, _ URL: UnsafePointer<CChar>!, _ encoding: UnsafePointer<CChar>!, _ options: Int32) -> xmlDocPtr! { return xmlDocPtr.allocate(capacity: 0) }
 func xmlCtxtReadFd(_ ctxt: xmlParserCtxtPtr!, _ fd: Int32, _ URL: UnsafePointer<CChar>!, _ encoding: UnsafePointer<CChar>!, _ options: Int32) -> xmlDocPtr! { return xmlDocPtr.allocate(capacity: 0) }
 func xmlCtxtReadIO(_ ctxt: xmlParserCtxtPtr!, _ ioread: xmlInputReadCallback!, _ ioclose: xmlInputCloseCallback!, _ ioctx: UnsafeMutableRawPointer!, _ URL: UnsafePointer<CChar>!, _ encoding: UnsafePointer<CChar>!, _ options: Int32) -> xmlDocPtr! { return xmlDocPtr.allocate(capacity: 0) }
+func xmlParseChunk(_ ctxt: xmlParserCtxtPtr!, _ chunk: UnsafePointer<CChar>?, _ size: Int32, _ terminate: Int32) -> Int32 { return 0 }
 
 // --- tests ---
 
 func sourcePtr() -> UnsafeMutablePointer<UInt8> { return UnsafeMutablePointer<UInt8>.allocate(capacity: 0) }
 func sourceCharPtr() -> UnsafeMutablePointer<CChar> { return UnsafeMutablePointer<CChar>.allocate(capacity: 0) }
 
+func getACtxt() -> xmlParserCtxtPtr {
+    return 0 as! xmlParserCtxtPtr
+}
+
 func test() {
     let remotePtr = sourcePtr()
     let remoteCharPtr = sourceCharPtr()
+    let safeCharPtr = UnsafeMutablePointer<CChar>.allocate(capacity: 1024)
+    safeCharPtr.initialize(repeating: 0, count: 1024)
+
     let _ = xmlReadFile(remoteCharPtr, nil, 0) // NO XXE: external entities not enabled
-    let _ = xmlReadFile(remoteCharPtr, nil, Int32(XML_PARSE_NOENT.rawValue)) // $ hasXXE=57
-    let _ = xmlReadFile(remoteCharPtr, nil, Int32(XML_PARSE_DTDLOAD.rawValue)) // $ hasXXE=57
-    let _ = xmlReadFile(remoteCharPtr, nil, Int32(XML_PARSE_NOENT.rawValue | XML_PARSE_DTDLOAD.rawValue)) // $ hasXXE=57
-    let _ = xmlReadFile(remoteCharPtr, nil, Int32(XML_PARSE_NOENT.rawValue | 0)) // $ hasXXE=57
+    let _ = xmlReadFile(remoteCharPtr, nil, Int32(XML_PARSE_NOENT.rawValue)) // $ hasXXE=96
+    let _ = xmlReadFile(remoteCharPtr, nil, Int32(XML_PARSE_DTDLOAD.rawValue)) // $ hasXXE=96
+    let _ = xmlReadFile(remoteCharPtr, nil, Int32(XML_PARSE_NOENT.rawValue | XML_PARSE_DTDLOAD.rawValue)) // $ hasXXE=96
+    let _ = xmlReadFile(remoteCharPtr, nil, Int32(XML_PARSE_NOENT.rawValue | 0)) // $ hasXXE=96
     let _ = xmlReadDoc(remotePtr, nil, nil, 0) // NO XXE: external entities not enabled
-    let _ = xmlReadDoc(remotePtr, nil, nil, Int32(XML_PARSE_NOENT.rawValue)) // $ hasXXE=56
-    let _ = xmlReadDoc(remotePtr, nil, nil, Int32(XML_PARSE_DTDLOAD.rawValue)) // $ hasXXE=56
+    let _ = xmlReadDoc(remotePtr, nil, nil, Int32(XML_PARSE_NOENT.rawValue)) // $ hasXXE=95
+    let _ = xmlReadDoc(remotePtr, nil, nil, Int32(XML_PARSE_DTDLOAD.rawValue)) // $ hasXXE=95
     let _ = xmlCtxtReadFile(nil, remoteCharPtr, nil, 0) // NO XXE: external entities not enabled
-    let _ = xmlCtxtReadFile(nil, remoteCharPtr, nil, Int32(XML_PARSE_NOENT.rawValue)) // $ hasXXE=57
-    let _ = xmlCtxtReadFile(nil, remoteCharPtr, nil, Int32(XML_PARSE_DTDLOAD.rawValue)) // $ hasXXE=57
+    let _ = xmlCtxtReadFile(nil, remoteCharPtr, nil, Int32(XML_PARSE_NOENT.rawValue)) // $ hasXXE=96
+    let _ = xmlCtxtReadFile(nil, remoteCharPtr, nil, Int32(XML_PARSE_DTDLOAD.rawValue)) // $ hasXXE=96
     let _ = xmlParseInNodeContext(nil, remoteCharPtr, -1, 0, nil) // NO XXE: external entities not enabled
-    let _ = xmlParseInNodeContext(nil, remoteCharPtr, -1, Int32(XML_PARSE_DTDLOAD.rawValue), nil) // $ hasXXE=57
-    let _ = xmlParseInNodeContext(nil, remoteCharPtr, -1, Int32(XML_PARSE_NOENT.rawValue), nil) // $ hasXXE=57
+    let _ = xmlParseInNodeContext(nil, remoteCharPtr, -1, Int32(XML_PARSE_DTDLOAD.rawValue), nil) // $ hasXXE=96
+    let _ = xmlParseInNodeContext(nil, remoteCharPtr, -1, Int32(XML_PARSE_NOENT.rawValue), nil) // $ hasXXE=96
     let _ = xmlCtxtReadDoc(nil, remotePtr, nil, nil, 0) // NO XXE: external entities not enabled
-    let _ = xmlCtxtReadDoc(nil, remotePtr, nil, nil, Int32(XML_PARSE_NOENT.rawValue)) // $ hasXXE=56
-    let _ = xmlCtxtReadDoc(nil, remotePtr, nil, nil, Int32(XML_PARSE_DTDLOAD.rawValue)) // $ hasXXE=56
+    let _ = xmlCtxtReadDoc(nil, remotePtr, nil, nil, Int32(XML_PARSE_NOENT.rawValue)) // $ hasXXE=95
+    let _ = xmlCtxtReadDoc(nil, remotePtr, nil, nil, Int32(XML_PARSE_DTDLOAD.rawValue)) // $ hasXXE=95
     let _ = xmlReadMemory(remoteCharPtr, -1, nil, nil, 0) // NO XXE: external entities not enabled
-    let _ = xmlReadMemory(remoteCharPtr, -1, nil, nil, Int32(XML_PARSE_NOENT.rawValue)) // $ hasXXE=57
-    let _ = xmlReadMemory(remoteCharPtr, -1, nil, nil, Int32(XML_PARSE_DTDLOAD.rawValue)) // $ hasXXE=57
+    let _ = xmlReadMemory(remoteCharPtr, -1, nil, nil, Int32(XML_PARSE_NOENT.rawValue)) // $ hasXXE=96
+    let _ = xmlReadMemory(remoteCharPtr, -1, nil, nil, Int32(XML_PARSE_DTDLOAD.rawValue)) // $ hasXXE=96
     let _ = xmlCtxtReadMemory(nil, remoteCharPtr, -1, nil, nil, 0) // NO XXE: external entities not enabled
-    let _ = xmlCtxtReadMemory(nil, remoteCharPtr, -1, nil, nil, Int32(XML_PARSE_NOENT.rawValue)) // $ hasXXE=57
-    let _ = xmlCtxtReadMemory(nil, remoteCharPtr, -1, nil, nil, Int32(XML_PARSE_DTDLOAD.rawValue)) // $ hasXXE=57
-    // TODO: We would need to model taint around `xmlParserCtxtPtr`, file descriptors, and `xmlInputReadCallback`
-    // to be able to alert on these methods. Not doing it for now because of the effort required vs the expected gain.
-    let _ = xmlCtxtUseOptions(nil, 0)
-    let _ = xmlCtxtUseOptions(nil, Int32(XML_PARSE_NOENT.rawValue))
-    let _ = xmlCtxtUseOptions(nil, Int32(XML_PARSE_DTDLOAD.rawValue))
-    let _ = xmlReadFd(0, nil, nil, 0)
-    let _ = xmlReadFd(0, nil, nil, Int32(XML_PARSE_NOENT.rawValue))
-    let _ = xmlReadFd(0, nil, nil, Int32(XML_PARSE_DTDLOAD.rawValue))
-    let _ = xmlCtxtReadFd(nil, 0, nil, nil, 0)
-    let _ = xmlCtxtReadFd(nil, 0, nil, nil, Int32(XML_PARSE_NOENT.rawValue))
-    let _ = xmlCtxtReadFd(nil, 0, nil, nil, Int32(XML_PARSE_DTDLOAD.rawValue))
-    let _ = xmlReadIO(nil, nil, nil, nil, nil, 0)
-    let _ = xmlReadIO(nil, nil, nil, nil, nil, Int32(XML_PARSE_NOENT.rawValue))
-    let _ = xmlReadIO(nil, nil, nil, nil, nil, Int32(XML_PARSE_DTDLOAD.rawValue))
-    let _ = xmlCtxtReadIO(nil, nil, nil, nil, nil, nil, 0)
-    let _ = xmlCtxtReadIO(nil, nil, nil, nil, nil, nil, Int32(XML_PARSE_NOENT.rawValue))
-    let _ = xmlCtxtReadIO(nil, nil, nil, nil, nil, nil, Int32(XML_PARSE_DTDLOAD.rawValue))
+    let _ = xmlCtxtReadMemory(nil, remoteCharPtr, -1, nil, nil, Int32(XML_PARSE_NOENT.rawValue)) // $ hasXXE=96
+    let _ = xmlCtxtReadMemory(nil, remoteCharPtr, -1, nil, nil, Int32(XML_PARSE_DTDLOAD.rawValue)) // $ hasXXE=96
+
+    let ctxt1 = getACtxt()
+    if (xmlCtxtUseOptions(ctxt1, 0) == 0) { // NO XXE: external entities not enabled
+        let _ = xmlParseChunk(ctxt1, remoteCharPtr, 1024, 0)
+    }
+
+    let ctxt2 = getACtxt()
+    if (xmlCtxtUseOptions(ctxt2, Int32(XML_PARSE_NOENT.rawValue)) == 0) { // $ MISSING: hasXXE=96
+        let _ = xmlParseChunk(ctxt2, remoteCharPtr, 1024, 0)
+    }
+
+    let ctxt3 = getACtxt()
+    if (xmlCtxtUseOptions(ctxt3, Int32(XML_PARSE_DTDLOAD.rawValue)) == 0) { // $ MISSING: hasXXE=96
+        let _ = xmlParseChunk(ctxt3, remoteCharPtr, 1024, 0)
+    }
+
+    let ctxt4 = getACtxt()
+    if (xmlCtxtUseOptions(ctxt4, Int32(XML_PARSE_DTDLOAD.rawValue)) == 0) { // $ NO XXE: the input chunk isn't tainted
+        let _ = xmlParseChunk(ctxt4, safeCharPtr, 1024, 0)
+    }
+
+    let remotePath = FileManager.default.homeDirectoryForCurrentUser.appendingPathComponent("shared.xml")
+    let safePath = Bundle.main.path(forResource: "my", ofType: "xml")
+    let remoteFd = try! FileDescriptor.open(FilePath(remotePath)!, FileDescriptor.AccessMode.readOnly)
+    let safeFd = try! FileDescriptor.open(FilePath(safePath!), FileDescriptor.AccessMode.readOnly)
+
+    let _ = xmlReadFd(remoteFd.rawValue, nil, nil, 0) // NO XXE: external entities not enabled
+    let _ = xmlReadFd(remoteFd.rawValue, nil, nil, Int32(XML_PARSE_NOENT.rawValue)) // $ MISSING: hasXXE=146
+    let _ = xmlReadFd(remoteFd.rawValue, nil, nil, Int32(XML_PARSE_DTDLOAD.rawValue)) // $ MISSING: hasXXE=146
+    let _ = xmlReadFd(safeFd.rawValue, nil, nil, Int32(XML_PARSE_DTDLOAD.rawValue)) // NO XXE: the input file is trusted
+
+    let ctxt5 = getACtxt()
+    let _ = xmlCtxtReadFd(ctxt5, remoteFd.rawValue, nil, nil, 0) // NO XXE: external entities not enabled
+    let _ = xmlCtxtReadFd(ctxt5, remoteFd.rawValue, nil, nil, Int32(XML_PARSE_NOENT.rawValue)) // $ MISSING: hasXXE=146
+    let _ = xmlCtxtReadFd(ctxt5, remoteFd.rawValue, nil, nil, Int32(XML_PARSE_DTDLOAD.rawValue)) // $ MISSING: hasXXE=146
+    let _ = xmlCtxtReadFd(ctxt5, safeFd.rawValue, nil, nil, Int32(XML_PARSE_DTDLOAD.rawValue)) // NO XXE: the input file is trusted
+
+    let ctxt6 = getACtxt()
+    if (xmlCtxtUseOptions(ctxt6, Int32(XML_PARSE_NOENT.rawValue)) == 0) { // $ MISSING: hasXXE=146
+        let _ = xmlCtxtReadFd(ctxt6, remoteFd.rawValue, nil, nil, 0)
+    }
+
+    let _ = xmlReadIO(nil, nil, nil, nil, nil, 0) // NO XXE: external entities not enabled
+    let _ = xmlReadIO(nil, nil, nil, nil, nil, Int32(XML_PARSE_NOENT.rawValue)) // $ MISSING: hasXXE=?
+    let _ = xmlReadIO(nil, nil, nil, nil, nil, Int32(XML_PARSE_DTDLOAD.rawValue)) // $ MISSING: hasXXE=?
+
+    let _ = xmlCtxtReadIO(nil, nil, nil, nil, nil, nil, 0) // NO XXE: external entities not enabled
+    let _ = xmlCtxtReadIO(nil, nil, nil, nil, nil, nil, Int32(XML_PARSE_NOENT.rawValue)) // $ MISSING: hasXXE=?
+    let _ = xmlCtxtReadIO(nil, nil, nil, nil, nil, nil, Int32(XML_PARSE_DTDLOAD.rawValue)) // $ MISSING: hasXXE=?
 }

Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,5 @@`
`1`	`1`	`private import codeql.swift.generated.decl.PrecedenceGroupDecl`
`2`	`2`
`3`	`3`	`class PrecedenceGroupDecl extends Generated::PrecedenceGroupDecl {`
`4`		`- override string toString() {`
`5`		`- result = "precedencegroup ..." // TODO: Once we extract the name we can improve this.`
`6`		`- }`
	`4`	`+ override string toString() { result = "precedencegroup ..." }`
`7`	`5`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,5 @@`
`1`	`1`	`private import codeql.swift.generated.expr.TupleElementExpr`
`2`	`2`
`3`	`3`	`class TupleElementExpr extends Generated::TupleElementExpr {`
`4`		`- override string toString() {`
`5`		`- result = "." + this.getIndex() // TODO: Can be improved once we extract the name`
`6`		`- }`
	`4`	`+ override string toString() { result = "." + this.getIndex() }`
`7`	`5`	`}`
Original file line number	Diff line number	Diff line change
`@@ -15,7 +15,6 @@ class FilePath extends StructDecl {`
`15`	`15`	`*/`
`16`	`16`	`private class FilePathSummaries extends SummaryModelCsv {`
`17`	`17`	`override predicate row(string row) {`
`18`		`- // TODO: Properly model this class`
`19`	`18`	`row = ";FilePath;true;init(stringLiteral:);(String);;Argument[0];ReturnValue;taint"`
`20`	`19`	`}`
`21`	`20`	`}`
Original file line number	Diff line number	Diff line change
`@@ -10,7 +10,6 @@ private import codeql.swift.dataflow.ExternalFlow`
`10`	`10`	`*/`
`11`	`11`	`private class NsUrlSummaries extends SummaryModelCsv {`
`12`	`12`	`override predicate row(string row) {`
`13`		`- // TODO: Properly model this class`
`14`	`13`	`row = ";NSURL;true;init(string:);(String);;Argument[0];ReturnValue;taint"`
`15`	`14`	`}`
`16`	`15`	`}`