diff --git a/swift/ql/src/change-notes/2023-07-13-unsafe-pointer-escapes-closure.md b/swift/ql/src/change-notes/2023-07-13-unsafe-pointer-escapes-closure.md new file mode 100644 index 000000000000..75b0c1e58d59 --- /dev/null +++ b/swift/ql/src/change-notes/2023-07-13-unsafe-pointer-escapes-closure.md @@ -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. \ No newline at end of file diff --git a/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.qhelp b/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.qhelp new file mode 100644 index 000000000000..16484eaa448e --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.qhelp @@ -0,0 +1,21 @@ + + + +

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.

+
+ + +

Do not use temporary pointers outside the closure they are passed to.

+
+ + +

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.

+ +
+ + +
  • withUnsafeBytes
  • +
    +
    diff --git a/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.ql b/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.ql new file mode 100644 index 000000000000..f495403a49e4 --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.ql @@ -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 + */ + +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() + ) + } + 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; + +from Flow::PathNode source, Flow::PathNode sink +where Flow::flowPath(source, sink) +select sink, source, sink, "This unsafe parameter may escape its invocation" diff --git a/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.swift b/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.swift new file mode 100644 index 000000000000..9870b83be70a --- /dev/null +++ b/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.swift @@ -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) + } +} diff --git a/swift/ql/test/query-tests/Security/CWE-825/UnsafePointerEscapesClosure.expected b/swift/ql/test/query-tests/Security/CWE-825/UnsafePointerEscapesClosure.expected new file mode 100644 index 000000000000..4c9c14772620 --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-825/UnsafePointerEscapesClosure.expected @@ -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 | diff --git a/swift/ql/test/query-tests/Security/CWE-825/UnsafePointerEscapesClosure.qlref b/swift/ql/test/query-tests/Security/CWE-825/UnsafePointerEscapesClosure.qlref new file mode 100644 index 000000000000..49808c83d589 --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-825/UnsafePointerEscapesClosure.qlref @@ -0,0 +1 @@ +queries/Security/CWE-825/UnsafePointerEscapesClosure.ql \ No newline at end of file diff --git a/swift/ql/test/query-tests/Security/CWE-825/withUnsafeBytes.swift b/swift/ql/test/query-tests/Security/CWE-825/withUnsafeBytes.swift new file mode 100644 index 000000000000..60493fb0866e --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-825/withUnsafeBytes.swift @@ -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 + print(v.field) + + ints.withUnsafeBytes(myPrint) // GOOD + + myPrint(p: ints.withUnsafeBytes(id)) // BAD + + var v2: UnsafeRawBufferPointer? = nil + ints.withUnsafeBytes({(p: UnsafeRawBufferPointer) in + v2 = p + return 1 + }) // BAD + print(v2) +} + +func id(pointer: T) -> T { + return pointer +} + +struct PointerHolder { + var field: UnsafeRawBufferPointer? +} + +func myPrint(p: UnsafeRawBufferPointer) { + print(p) +}