Swift: Query for escaping parameters of unsafe closures #13706
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| --- | ||
| category: newQuery | ||
| --- | ||
| * Added a new query, `swift/unsafe-pointer-escapes-closure` to detect code that passes temporary closure arguments outside the closure. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| <!DOCTYPE qhelp PUBLIC | ||
| "-//Semmle//qhelp//EN" | ||
| "qhelp.dtd"> | ||
| <qhelp> | ||
| <overview> | ||
| <p>Certain functions take a closure and pass a temporary pointer into it. If this pointer escapes from the closure and is used outside it, memory corruption may occur.</p> | ||
| </overview> | ||
|
|
||
| <recommendation> | ||
| <p>Do not use temporary pointers outside the closure they are passed to.</p> | ||
| </recommendation> | ||
|
|
||
| <example> | ||
| <p>In the first example below, the pointer is returned from the closure, potentially leading to memory corruption. In the second example, all work with the pointer is done inside the closure, and it is not returned.</p> | ||
| <sample src="UnsafePointerEscapesClosure.swift" /> | ||
| </example> | ||
|
|
||
| <references> | ||
| <li><a href="https://developer.apple.com/documentation/swift/array/withunsafebytes(_:)">withUnsafeBytes</a></li> | ||
| </references> | ||
| </qhelp> | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| /** | ||
| * @name Unsafe pointer escapes closure | ||
| * @description Certain functions pass a low-level pointer to a closure. If this pointer outlives the closure, unpredictable results may occur. | ||
| * @kind path-problem | ||
| * @id swift/unsafe-pointer-escapes-closure | ||
| * @tags security | ||
| * external/cwe/cwe-825 | ||
|
There was a problem hiding this comment. Needs precision, severity and security-severity tags. I guess we want to test with a near-final version before choosing a precision. |
||
| */ | ||
|
|
||
| import swift | ||
| import codeql.swift.dataflow.DataFlow | ||
| import Flow::PathGraph | ||
|
|
||
| module Config implements DataFlow::StateConfigSig { | ||
| class FlowState = Callable; | ||
|
|
||
| additional predicate isUnsafePointerCall(CallExpr call) { | ||
| call.getStaticTarget() | ||
| .hasName([ | ||
| "withUnsafeBytes(_:)", "withCString(_:)", "withUnsafeMutableBytes(_:)", | ||
| "withContiguousMutableStorageIfAvailable(_:)", "withContiguousStorageIfAvailable(_:)", | ||
| "withUTF8(_:)", "withUnsafeBufferPointer(_:)", "withUnsafeMutableBufferPointer(_:)", | ||
| "withMemoryRebound(to:_:)", "withUnsafeTemporaryAllocation(of:capacity:_:)", | ||
| "withUnsafeCurrentTask(body:)", "withCheckedContinuation(function:_:)" | ||
| ]) | ||
| } | ||
|
|
||
|
|
||
| additional predicate isUnsafePointerClosure(ClosureExpr expr) { | ||
| exists(CallExpr call | | ||
| isUnsafePointerCall(call) and | ||
| expr = call.getAnArgument().getExpr() | ||
| ) | ||
| } | ||
|
|
||
| additional predicate isUnsafePointerFunction(Function f) { | ||
| exists(CallExpr call | | ||
| isUnsafePointerCall(call) and | ||
| f.getAnAccess() = call.getAnArgument().getExpr() | ||
|
There was a problem hiding this comment. Is it safe that this can match arguments other than the intended closure? If it's possible to pass a closure or function to another argument that is. There was a problem hiding this comment. We might need to revisit this if there's a case where the function takes multiple closures, but everything I've modeled only takes one closure argument. |
||
| ) | ||
| } | ||
| predicate isSource(DataFlow::Node node, FlowState state) { | ||
| ( | ||
| isUnsafePointerClosure(state) | ||
| or | ||
| isUnsafePointerFunction(state) | ||
| ) and | ||
| state = node.(DataFlow::ParameterNode).getDeclaringFunction().getUnderlyingCallable() | ||
| } | ||
|
|
||
| predicate isSink(DataFlow::Node node, FlowState state) { | ||
| ( | ||
| isUnsafePointerClosure(state) | ||
| or | ||
| isUnsafePointerFunction(state) | ||
| ) | ||
| and | ||
| ( | ||
| node.(DataFlow::InoutReturnNode).getParameter().getDeclaringFunction() = state | ||
| or | ||
| exists(ReturnStmt stmt | | ||
| node.asExpr() = stmt.getResult() and | ||
| stmt.getEnclosingCallable() = state | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| predicate isBarrier(DataFlow::Node node) { none() } | ||
|
|
||
| predicate isBarrierIn(DataFlow::Node node) { none() } | ||
|
|
||
| predicate isBarrierOut(DataFlow::Node node) { none() } | ||
|
|
||
| predicate isBarrier(DataFlow::Node node, FlowState state) { none() } | ||
|
|
||
| predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { | ||
| node2.asExpr().convertsFrom(node1.asExpr()) | ||
| } | ||
|
|
||
| predicate isAdditionalFlowStep( | ||
| DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2 | ||
| ) { | ||
| none() | ||
| } | ||
|
|
||
| int fieldFlowBranchLimit() { result = 2 } | ||
|
|
||
| predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) { | ||
| isSink(node, _) and | ||
| c = any(c) | ||
| } | ||
| } | ||
|
|
||
| module Flow = DataFlow::GlobalWithState<Config>; | ||
|
|
||
| from Flow::PathNode source, Flow::PathNode sink | ||
| where Flow::flowPath(source, sink) | ||
| select sink, source, sink, "This unsafe parameter may escape its invocation" | ||
Check warningCode scanning / CodeQL Alert message style violation |
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| func bad() { | ||
| let ints = [1,2,3] | ||
| let bytes = ints.withUnsafeBytes{ | ||
| return $0 | ||
| } | ||
| print(bytes) | ||
| } | ||
|
|
||
| func good() { | ||
| let ints = [1,2,3] | ||
| let bytes = ints.withUnsafeBytes{ | ||
| print($0) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| edges | ||
| | withUnsafeBytes.swift:9:25:9:25 | $0 | withUnsafeBytes.swift:10:16:10:16 | $0 | | ||
| | withUnsafeBytes.swift:13:34:13:37 | p | withUnsafeBytes.swift:13:97:13:97 | p | | ||
| | withUnsafeBytes.swift:15:34:15:37 | p | withUnsafeBytes.swift:15:109:15:109 | p | | ||
| | withUnsafeBytes.swift:15:109:15:109 | p | withUnsafeBytes.swift:15:97:15:110 | call to id(pointer:) | | ||
| | withUnsafeBytes.swift:15:109:15:109 | p | withUnsafeBytes.swift:38:12:38:21 | pointer | | ||
| | withUnsafeBytes.swift:38:12:38:21 | pointer | withUnsafeBytes.swift:39:12:39:12 | pointer | | ||
| | withUnsafeBytes.swift:38:12:38:21 | pointer | withUnsafeBytes.swift:39:12:39:12 | pointer | | ||
| nodes | ||
| | withUnsafeBytes.swift:9:25:9:25 | $0 | semmle.label | $0 | | ||
| | withUnsafeBytes.swift:10:16:10:16 | $0 | semmle.label | $0 | | ||
| | withUnsafeBytes.swift:13:34:13:37 | p | semmle.label | p | | ||
| | withUnsafeBytes.swift:13:97:13:97 | p | semmle.label | p | | ||
| | withUnsafeBytes.swift:15:34:15:37 | p | semmle.label | p | | ||
| | withUnsafeBytes.swift:15:97:15:110 | call to id(pointer:) | semmle.label | call to id(pointer:) | | ||
| | withUnsafeBytes.swift:15:109:15:109 | p | semmle.label | p | | ||
| | withUnsafeBytes.swift:38:12:38:21 | pointer | semmle.label | pointer | | ||
| | withUnsafeBytes.swift:38:12:38:21 | pointer | semmle.label | pointer | | ||
| | withUnsafeBytes.swift:39:12:39:12 | pointer | semmle.label | pointer | | ||
| | withUnsafeBytes.swift:39:12:39:12 | pointer | semmle.label | pointer | | ||
| subpaths | ||
| | withUnsafeBytes.swift:15:109:15:109 | p | withUnsafeBytes.swift:38:12:38:21 | pointer | withUnsafeBytes.swift:39:12:39:12 | pointer | withUnsafeBytes.swift:15:97:15:110 | call to id(pointer:) | | ||
| #select | ||
| | withUnsafeBytes.swift:10:16:10:16 | $0 | withUnsafeBytes.swift:9:25:9:25 | $0 | withUnsafeBytes.swift:10:16:10:16 | $0 | This unsafe parameter may escape its invocation | | ||
| | withUnsafeBytes.swift:13:97:13:97 | p | withUnsafeBytes.swift:13:34:13:37 | p | withUnsafeBytes.swift:13:97:13:97 | p | This unsafe parameter may escape its invocation | | ||
| | withUnsafeBytes.swift:15:97:15:110 | call to id(pointer:) | withUnsafeBytes.swift:15:34:15:37 | p | withUnsafeBytes.swift:15:97:15:110 | call to id(pointer:) | This unsafe parameter may escape its invocation | | ||
| | withUnsafeBytes.swift:39:12:39:12 | pointer | withUnsafeBytes.swift:38:12:38:21 | pointer | withUnsafeBytes.swift:39:12:39:12 | pointer | This unsafe parameter may escape its invocation | |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| queries/Security/CWE-825/UnsafePointerEscapesClosure.ql |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| // stubs | ||
|
|
||
|
|
||
|
|
||
| // tests | ||
|
|
||
| func arrayTest() { | ||
| let ints = [1,2,3] | ||
| ints.withUnsafeBytes{ // BAD | ||
| return $0 | ||
| } | ||
|
|
||
| print(ints.withUnsafeBytes({(p: UnsafeRawBufferPointer) -> UnsafeRawBufferPointer in return p})) // BAD | ||
|
|
||
| print(ints.withUnsafeBytes({(p: UnsafeRawBufferPointer) -> UnsafeRawBufferPointer in return id(pointer: p)})) // BAD | ||
|
|
||
| ints.withUnsafeBytes({(p: UnsafeRawBufferPointer) in print(p)}) // GOOD | ||
|
|
||
| var v = PointerHolder() | ||
| ints.withUnsafeBytes({(p: UnsafeRawBufferPointer) in | ||
| v.field = p | ||
| return 1 | ||
| }) // BAD | ||
|
There was a problem hiding this comment. What's the issue with implicit returns? Surely we will see them in real world code. There was a problem hiding this comment. The issue is that we were picking up the implicit return of the assignment, but not the field being assigned. This is currently not detected... I think there's some missing flow for closure captures. |
||
| print(v.field) | ||
geoffw0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ints.withUnsafeBytes(myPrint) // GOOD | ||
|
There was a problem hiding this comment. Another variant here would be calling a member function that stores the pointer argument in the object it's called on for later use. The effect is similar to the |
||
|
|
||
| myPrint(p: ints.withUnsafeBytes(id)) // BAD | ||
|
|
||
| var v2: UnsafeRawBufferPointer? = nil | ||
| ints.withUnsafeBytes({(p: UnsafeRawBufferPointer) in | ||
| v2 = p | ||
| return 1 | ||
| }) // BAD | ||
| print(v2) | ||
| } | ||
|
|
||
| func id<T>(pointer: T) -> T { | ||
| return pointer | ||
| } | ||
|
|
||
| struct PointerHolder { | ||
| var field: UnsafeRawBufferPointer? | ||
| } | ||
|
|
||
| func myPrint(p: UnsafeRawBufferPointer) { | ||
| print(p) | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about linking to https://www.kodeco.com/7181017-unsafe-swift-using-pointers-and-interacting-with-c as well?