Skip to content

Commit 52ebf66

Browse files
atorralbaatorralba
committed
Add basic path sanitizer
1 parent 1576ee9 commit 52ebf66

5 files changed

Lines changed: 120 additions & 75 deletions

File tree

‎swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ private import internal.FlowSummaryImplSpecific
8080
private module Frameworks {
8181
private import codeql.swift.frameworks.StandardLibrary.CustomUrlSchemes
8282
private import codeql.swift.frameworks.StandardLibrary.Data
83+
private import codeql.swift.frameworks.StandardLibrary.FilePath
8384
private import codeql.swift.frameworks.StandardLibrary.InputStream
8485
private import codeql.swift.frameworks.StandardLibrary.NSData
8586
private import codeql.swift.frameworks.StandardLibrary.NsUrl

‎swift/ql/lib/codeql/swift/frameworks/StandardLibrary/FilePath.qll‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
import swift
2+
private import codeql.swift.dataflow.ExternalFlow
3+
4+
/** The struct `FilePath`. */
5+
class FilePath extends StructDecl {
6+
FilePath() { this.getFullName() = "FilePath" }
7+
}
8+
9+
/**
10+
* A model for `FilePath` members that permit taint flow.
11+
*/
12+
private class FilePathSummaries extends SummaryModelCsv {
13+
override predicate row(string row) {
14+
// TODO: Properly model this class
15+
row = ";FilePath;true;init(stringLiteral:);(String);;Argument[0];ReturnValue;taint"
16+
}
17+
}

‎swift/ql/lib/codeql/swift/frameworks/StandardLibrary/NsUrl.qll‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,12 @@
11
import swift
2-
private import codeql.swift.dataflow.DataFlow
32
private import codeql.swift.dataflow.ExternalFlow
4-
private import codeql.swift.dataflow.FlowSteps
53

64
/**
75
* A model for `NSURL` members that permit taint flow.
86
*/
97
private class NsUrlSummaries extends SummaryModelCsv {
108
override predicate row(string row) {
11-
row =
12-
[
13-
";NSURL;true;init(string:);(String);;Argument[0];ReturnValue;taint",
14-
// TODO
15-
]
9+
// TODO: Properly model this class
10+
row = ";NSURL;true;init(string:);(String);;Argument[0];ReturnValue;taint"
1611
}
1712
}

‎swift/ql/lib/codeql/swift/security/PathInjection.qll‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
import swift
44
private import codeql.swift.dataflow.DataFlow
55
private import codeql.swift.dataflow.ExternalFlow
6+
private import codeql.swift.dataflow.TaintTracking
7+
private import codeql.swift.frameworks.StandardLibrary.FilePath
68

79
/** A data flow sink for path injection vulnerabilities. */
810
abstract class PathInjectionSink extends DataFlow::Node { }
@@ -29,7 +31,19 @@ private class DefaultPathInjectionSink extends PathInjectionSink {
2931
}
3032

3133
private class DefaultPathInjectionSanitizer extends PathInjectionSanitizer {
32-
DefaultPathInjectionSanitizer() { none() } // TODO: Implement a proper path sanitizer
34+
DefaultPathInjectionSanitizer() {
35+
// This is a simple implementation prone to FNs by sanitizing too many nodes.
36+
// TODO: Implement a complete path sanitizer when Guards are available.
37+
exists(CallExpr starts, CallExpr normalize |
38+
starts.getStaticTarget().getName() = "starts(with:)" and
39+
starts.getStaticTarget().getEnclosingDecl() instanceof FilePath and
40+
normalize.getStaticTarget().getName() = "lexicallyNormalized()" and
41+
normalize.getStaticTarget().getEnclosingDecl() instanceof FilePath
42+
|
43+
TaintTracking::localTaint(this, DataFlow::exprNode(normalize.getQualifier())) and
44+
DataFlow::localExprFlow(normalize, starts.getQualifier())
45+
)
46+
}
3347
}
3448

3549
private class PathInjectionSinks extends SinkModelCsv {

‎swift/ql/test/query-tests/Security/CWE-022/testPathInjection.swift‎

Lines changed: 85 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,12 @@ struct ObjCBool {}
3737

3838
struct AutoreleasingUnsafeMutablePointer<Pointee> {}
3939

40+
struct FilePath {
41+
init(stringLiteral: String) {}
42+
func lexicallyNormalized() -> FilePath { return FilePath(stringLiteral: "") }
43+
func starts(with: FilePath) -> Bool { return false }
44+
}
45+
4046
class FileManager {
4147
class DirectoryEnumerator {}
4248
struct DirectoryEnumerationOptions : OptionSet { let rawValue: Int }
@@ -107,76 +113,88 @@ func test() {
107113
let safeUrl = URL(string: "")!
108114
let safeNsUrl = NSURL(string: "")!
109115

110-
Data("").write(to: remoteUrl, options: []) // $ hasPathInjection=104
116+
Data("").write(to: remoteUrl, options: []) // $ hasPathInjection=110
111117

112118
let nsData = NSData()
113-
let _ = nsData.write(to: remoteUrl, atomically: false) // $ hasPathInjection=104
114-
nsData.write(to: remoteUrl, options: []) // $ hasPathInjection=104
115-
let _ = nsData.write(toFile: remoteString, atomically: false) // $ hasPathInjection=104
116-
nsData.write(toFile: remoteString, options: []) // $ hasPathInjection=104
119+
let _ = nsData.write(to: remoteUrl, atomically: false) // $ hasPathInjection=110
120+
nsData.write(to: remoteUrl, options: []) // $ hasPathInjection=110
121+
let _ = nsData.write(toFile: remoteString, atomically: false) // $ hasPathInjection=110
122+
nsData.write(toFile: remoteString, options: []) // $ hasPathInjection=110
117123

118124
let fm = FileManager()
119-
let _ = fm.contentsOfDirectory(at: remoteUrl, includingPropertiesForKeys: [], options: []) // $ hasPathInjection=104
120-
let _ = fm.contentsOfDirectory(atPath: remoteString) // $ hasPathInjection=104
121-
let _ = fm.enumerator(at: remoteUrl, includingPropertiesForKeys: [], options: [], errorHandler: nil) // $ hasPathInjection=104
122-
let _ = fm.enumerator(atPath: remoteString) // $ hasPathInjection=104
123-
let _ = fm.subpathsOfDirectory(atPath: remoteString) // $ hasPathInjection=104
124-
let _ = fm.subpaths(atPath: remoteString) // $ hasPathInjection=104
125-
fm.createDirectory(at: remoteUrl, withIntermediateDirectories: false, attributes: [:]) // $ hasPathInjection=104
126-
let _ = fm.createDirectory(atPath: remoteString, attributes: [:]) // $ hasPathInjection=104
127-
let _ = fm.createFile(atPath: remoteString, contents: nil, attributes: [:]) // $ hasPathInjection=104
128-
fm.removeItem(at: remoteUrl) // $ hasPathInjection=104
129-
fm.removeItem(atPath: remoteString) // $ hasPathInjection=104
130-
fm.trashItem(at: remoteUrl, resultingItemURL: AutoreleasingUnsafeMutablePointer<NSURL?>()) // $ hasPathInjection=104
131-
let _ = fm.replaceItemAt(remoteUrl, withItemAt: safeUrl, backupItemName: nil, options: []) // $ hasPathInjection=104
132-
let _ = fm.replaceItemAt(safeUrl, withItemAt: remoteUrl, backupItemName: nil, options: []) // $ hasPathInjection=104
133-
fm.replaceItem(at: remoteUrl, withItemAt: safeUrl, backupItemName: nil, options: [], resultingItemURL: AutoreleasingUnsafeMutablePointer<NSURL?>()) // $ hasPathInjection=104
134-
fm.replaceItem(at: safeUrl, withItemAt: remoteUrl, backupItemName: nil, options: [], resultingItemURL: AutoreleasingUnsafeMutablePointer<NSURL?>()) // $ hasPathInjection=104
135-
fm.copyItem(at: remoteUrl, to: safeUrl) // $ hasPathInjection=104
136-
fm.copyItem(at: safeUrl, to: remoteUrl) // $ hasPathInjection=104
137-
fm.copyItem(atPath: remoteString, toPath: "") // $ hasPathInjection=104
138-
fm.copyItem(atPath: "", toPath: remoteString) // $ hasPathInjection=104
139-
fm.moveItem(at: remoteUrl, to: safeUrl) // $ hasPathInjection=104
140-
fm.moveItem(at: safeUrl, to: remoteUrl) // $ hasPathInjection=104
141-
fm.moveItem(atPath: remoteString, toPath: "") // $ hasPathInjection=104
142-
fm.moveItem(atPath: "", toPath: remoteString) // $ hasPathInjection=104
143-
fm.createSymbolicLink(at: remoteUrl, withDestinationURL: safeUrl) // $ hasPathInjection=104
144-
fm.createSymbolicLink(at: safeUrl, withDestinationURL: remoteUrl) // $ hasPathInjection=104
145-
fm.createSymbolicLink(atPath: remoteString, withDestinationPath: "") // $ hasPathInjection=104
146-
fm.createSymbolicLink(atPath: "", withDestinationPath: remoteString) // $ hasPathInjection=104
147-
fm.linkItem(at: remoteUrl, to: safeUrl) // $ hasPathInjection=104
148-
fm.linkItem(at: safeUrl, to: remoteUrl) // $ hasPathInjection=104
149-
fm.linkItem(atPath: remoteString, toPath: "") // $ hasPathInjection=104
150-
fm.linkItem(atPath: "", toPath: remoteString) // $ hasPathInjection=104
151-
let _ = fm.destinationOfSymbolicLink(atPath: remoteString) // $ hasPathInjection=104
152-
let _ = fm.fileExists(atPath: remoteString) // $ hasPathInjection=104
153-
let _ = fm.fileExists(atPath: remoteString, isDirectory: UnsafeMutablePointer<ObjCBool>.init(bitPattern: 0)) // $ hasPathInjection=104
154-
let _ = fm.isReadableFile(atPath: remoteString) // $ hasPathInjection=104
155-
let _ = fm.isWritableFile(atPath: remoteString) // $ hasPathInjection=104
156-
let _ = fm.isDeletableFile(atPath: remoteString) // $ hasPathInjection=104
157-
let _ = fm.componentsToDisplay(forPath: remoteString) // $ hasPathInjection=104
158-
let _ = fm.displayName(atPath: remoteString) // $ hasPathInjection=104
159-
let _ = fm.attributesOfItem(atPath: remoteString) // $ hasPathInjection=104
160-
let _ = fm.attributesOfFileSystem(forPath: remoteString) // $ hasPathInjection=104
161-
fm.setAttributes([:], ofItemAtPath: remoteString) // $ hasPathInjection=104
162-
let _ = fm.contents(atPath: remoteString) // $ hasPathInjection=104
163-
let _ = fm.contentsEqual(atPath: remoteString, andPath: "") // $ hasPathInjection=104
164-
let _ = fm.contentsEqual(atPath: "", andPath: remoteString) // $ hasPathInjection=104
165-
fm.getRelationship(UnsafeMutablePointer<FileManager.URLRelationship>.allocate(capacity: 0), ofDirectoryAt: remoteUrl, toItemAt: safeUrl) // $ hasPathInjection=104
166-
fm.getRelationship(UnsafeMutablePointer<FileManager.URLRelationship>.allocate(capacity: 0), ofDirectoryAt: safeUrl, toItemAt: remoteUrl) // $ hasPathInjection=104
167-
fm.getRelationship(UnsafeMutablePointer<FileManager.URLRelationship>.allocate(capacity: 0), of: FileManager.SearchPathDirectory.none, in: FileManager.SearchPathDomainMask(), toItemAt: remoteUrl) // $ hasPathInjection=104
168-
let _ = fm.changeCurrentDirectoryPath(remoteString) // $ hasPathInjection=104
169-
let _ = fm.unmountVolume(at: remoteUrl, options: [], completionHandler: { _ in }) // $ hasPathInjection=104
170-
let _ = fm.NSHFSTypeOfFile(remoteString) // $ hasPathInjection=104
125+
let _ = fm.contentsOfDirectory(at: remoteUrl, includingPropertiesForKeys: [], options: []) // $ hasPathInjection=110
126+
let _ = fm.contentsOfDirectory(atPath: remoteString) // $ hasPathInjection=110
127+
let _ = fm.enumerator(at: remoteUrl, includingPropertiesForKeys: [], options: [], errorHandler: nil) // $ hasPathInjection=110
128+
let _ = fm.enumerator(atPath: remoteString) // $ hasPathInjection=110
129+
let _ = fm.subpathsOfDirectory(atPath: remoteString) // $ hasPathInjection=110
130+
let _ = fm.subpaths(atPath: remoteString) // $ hasPathInjection=110
131+
fm.createDirectory(at: remoteUrl, withIntermediateDirectories: false, attributes: [:]) // $ hasPathInjection=110
132+
let _ = fm.createDirectory(atPath: remoteString, attributes: [:]) // $ hasPathInjection=110
133+
let _ = fm.createFile(atPath: remoteString, contents: nil, attributes: [:]) // $ hasPathInjection=110
134+
fm.removeItem(at: remoteUrl) // $ hasPathInjection=110
135+
fm.removeItem(atPath: remoteString) // $ hasPathInjection=110
136+
fm.trashItem(at: remoteUrl, resultingItemURL: AutoreleasingUnsafeMutablePointer<NSURL?>()) // $ hasPathInjection=110
137+
let _ = fm.replaceItemAt(remoteUrl, withItemAt: safeUrl, backupItemName: nil, options: []) // $ hasPathInjection=110
138+
let _ = fm.replaceItemAt(safeUrl, withItemAt: remoteUrl, backupItemName: nil, options: []) // $ hasPathInjection=110
139+
fm.replaceItem(at: remoteUrl, withItemAt: safeUrl, backupItemName: nil, options: [], resultingItemURL: AutoreleasingUnsafeMutablePointer<NSURL?>()) // $ hasPathInjection=110
140+
fm.replaceItem(at: safeUrl, withItemAt: remoteUrl, backupItemName: nil, options: [], resultingItemURL: AutoreleasingUnsafeMutablePointer<NSURL?>()) // $ hasPathInjection=110
141+
fm.copyItem(at: remoteUrl, to: safeUrl) // $ hasPathInjection=110
142+
fm.copyItem(at: safeUrl, to: remoteUrl) // $ hasPathInjection=110
143+
fm.copyItem(atPath: remoteString, toPath: "") // $ hasPathInjection=110
144+
fm.copyItem(atPath: "", toPath: remoteString) // $ hasPathInjection=110
145+
fm.moveItem(at: remoteUrl, to: safeUrl) // $ hasPathInjection=110
146+
fm.moveItem(at: safeUrl, to: remoteUrl) // $ hasPathInjection=110
147+
fm.moveItem(atPath: remoteString, toPath: "") // $ hasPathInjection=110
148+
fm.moveItem(atPath: "", toPath: remoteString) // $ hasPathInjection=110
149+
fm.createSymbolicLink(at: remoteUrl, withDestinationURL: safeUrl) // $ hasPathInjection=110
150+
fm.createSymbolicLink(at: safeUrl, withDestinationURL: remoteUrl) // $ hasPathInjection=110
151+
fm.createSymbolicLink(atPath: remoteString, withDestinationPath: "") // $ hasPathInjection=110
152+
fm.createSymbolicLink(atPath: "", withDestinationPath: remoteString) // $ hasPathInjection=110
153+
fm.linkItem(at: remoteUrl, to: safeUrl) // $ hasPathInjection=110
154+
fm.linkItem(at: safeUrl, to: remoteUrl) // $ hasPathInjection=110
155+
fm.linkItem(atPath: remoteString, toPath: "") // $ hasPathInjection=110
156+
fm.linkItem(atPath: "", toPath: remoteString) // $ hasPathInjection=110
157+
let _ = fm.destinationOfSymbolicLink(atPath: remoteString) // $ hasPathInjection=110
158+
let _ = fm.fileExists(atPath: remoteString) // $ hasPathInjection=110
159+
let _ = fm.fileExists(atPath: remoteString, isDirectory: UnsafeMutablePointer<ObjCBool>.init(bitPattern: 0)) // $ hasPathInjection=110
160+
let _ = fm.isReadableFile(atPath: remoteString) // $ hasPathInjection=110
161+
let _ = fm.isWritableFile(atPath: remoteString) // $ hasPathInjection=110
162+
let _ = fm.isDeletableFile(atPath: remoteString) // $ hasPathInjection=110
163+
let _ = fm.componentsToDisplay(forPath: remoteString) // $ hasPathInjection=110
164+
let _ = fm.displayName(atPath: remoteString) // $ hasPathInjection=110
165+
let _ = fm.attributesOfItem(atPath: remoteString) // $ hasPathInjection=110
166+
let _ = fm.attributesOfFileSystem(forPath: remoteString) // $ hasPathInjection=110
167+
fm.setAttributes([:], ofItemAtPath: remoteString) // $ hasPathInjection=110
168+
let _ = fm.contents(atPath: remoteString) // $ hasPathInjection=110
169+
let _ = fm.contentsEqual(atPath: remoteString, andPath: "") // $ hasPathInjection=110
170+
let _ = fm.contentsEqual(atPath: "", andPath: remoteString) // $ hasPathInjection=110
171+
fm.getRelationship(UnsafeMutablePointer<FileManager.URLRelationship>.allocate(capacity: 0), ofDirectoryAt: remoteUrl, toItemAt: safeUrl) // $ hasPathInjection=110
172+
fm.getRelationship(UnsafeMutablePointer<FileManager.URLRelationship>.allocate(capacity: 0), ofDirectoryAt: safeUrl, toItemAt: remoteUrl) // $ hasPathInjection=110
173+
fm.getRelationship(UnsafeMutablePointer<FileManager.URLRelationship>.allocate(capacity: 0), of: FileManager.SearchPathDirectory.none, in: FileManager.SearchPathDomainMask(), toItemAt: remoteUrl) // $ hasPathInjection=110
174+
let _ = fm.changeCurrentDirectoryPath(remoteString) // $ hasPathInjection=110
175+
let _ = fm.unmountVolume(at: remoteUrl, options: [], completionHandler: { _ in }) // $ hasPathInjection=110
176+
let _ = fm.NSHFSTypeOfFile(remoteString) // $ hasPathInjection=110
171177
// Deprecated methods
172-
let _ = fm.changeFileAttributes([:], atPath: remoteString) // $ hasPathInjection=104
173-
let _ = fm.fileAttributes(atPath: remoteString, traverseLink: false) // $ hasPathInjection=104
174-
let _ = fm.fileSystemAttributes(atPath: remoteString) // $ hasPathInjection=104
175-
let _ = fm.directoryContents(atPath: remoteString) // $ hasPathInjection=104
176-
let _ = fm.createDirectory(atPath: remoteString, attributes: [:]) // $ hasPathInjection=104
177-
let _ = fm.createSymbolicLink(atPath: remoteString, pathContent: "") // $ hasPathInjection=104
178-
let _ = fm.createSymbolicLink(atPath: "", pathContent: remoteString) // $ hasPathInjection=104
179-
let _ = fm.pathContentOfSymbolicLink(atPath: remoteString) // $ hasPathInjection=104
180-
let _ = fm.replaceItemAtURL(originalItemURL: remoteNsUrl, withItemAtURL: safeNsUrl, backupItemName: nil, options: []) // $ hasPathInjection=104
181-
let _ = fm.replaceItemAtURL(originalItemURL: safeNsUrl, withItemAtURL: remoteNsUrl, backupItemName: nil, options: []) // $ hasPathInjection=104
178+
let _ = fm.changeFileAttributes([:], atPath: remoteString) // $ hasPathInjection=110
179+
let _ = fm.fileAttributes(atPath: remoteString, traverseLink: false) // $ hasPathInjection=110
180+
let _ = fm.fileSystemAttributes(atPath: remoteString) // $ hasPathInjection=110
181+
let _ = fm.directoryContents(atPath: remoteString) // $ hasPathInjection=110
182+
let _ = fm.createDirectory(atPath: remoteString, attributes: [:]) // $ hasPathInjection=110
183+
let _ = fm.createSymbolicLink(atPath: remoteString, pathContent: "") // $ hasPathInjection=110
184+
let _ = fm.createSymbolicLink(atPath: "", pathContent: remoteString) // $ hasPathInjection=110
185+
let _ = fm.pathContentOfSymbolicLink(atPath: remoteString) // $ hasPathInjection=110
186+
let _ = fm.replaceItemAtURL(originalItemURL: remoteNsUrl, withItemAtURL: safeNsUrl, backupItemName: nil, options: []) // $ hasPathInjection=110
187+
let _ = fm.replaceItemAtURL(originalItemURL: safeNsUrl, withItemAtURL: remoteNsUrl, backupItemName: nil, options: []) // $ hasPathInjection=110
188+
}
189+
190+
func testSanitizers() {
191+
let remoteString = String(contentsOf: URL(string: "http://example.com/")!)
192+
193+
let fm = FileManager()
194+
195+
let filePath = FilePath(stringLiteral: remoteString)
196+
if (filePath.lexicallyNormalized().starts(with: FilePath(stringLiteral: "/safe"))) {
197+
fm.contents(atPath: remoteString) // Safe
198+
}
199+
fm.contents(atPath: remoteString) // $ MISSING: $ hasPathInjection=191
182200
}

0 commit comments

Comments
 (0)