From 1cac879e58c7a6f6406ccbc00e1fa7ecae6af6dc Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Mon, 10 Jul 2023 17:24:28 +0000 Subject: [PATCH 1/9] Initial test for pointer escaping withUnsafeBytes --- .../test/query-tests/Security/CWE-825/test.ql | 5 +++ .../Security/CWE-825/withUnsafeBytes.swift | 38 +++++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 swift/ql/test/query-tests/Security/CWE-825/test.ql create mode 100644 swift/ql/test/query-tests/Security/CWE-825/withUnsafeBytes.swift diff --git a/swift/ql/test/query-tests/Security/CWE-825/test.ql b/swift/ql/test/query-tests/Security/CWE-825/test.ql new file mode 100644 index 000000000000..8516e8320197 --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-825/test.ql @@ -0,0 +1,5 @@ +import codeql.swift.dataflow.DataFlow + +module Conf implements ConfigSig { + +} \ 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..661dc29c5450 --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-825/withUnsafeBytes.swift @@ -0,0 +1,38 @@ +// 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}) // BAD + print(v.field) + + ints.withUnsafeBytes(myPrint) // GOOD + + myPrint(p: ints.withUnsafeBytes(id)) // BAD +} + +func id(pointer: T) -> T { + return pointer +} + +struct PointerHolder { + var field: UnsafeRawBufferPointer? +} + +func myPrint(p: UnsafeRawBufferPointer) { + print(p) +} From 83a787ecfc62eff24de937bf7560c38d5058a02b Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Mon, 10 Jul 2023 19:23:18 +0000 Subject: [PATCH 2/9] Swift: initial query for unsafe closure arg escape --- .../Security/CWE-825/test.expected | 121 ++++++++++++++++++ .../test/query-tests/Security/CWE-825/test.ql | 60 ++++++++- 2 files changed, 179 insertions(+), 2 deletions(-) create mode 100644 swift/ql/test/query-tests/Security/CWE-825/test.expected diff --git a/swift/ql/test/query-tests/Security/CWE-825/test.expected b/swift/ql/test/query-tests/Security/CWE-825/test.expected new file mode 100644 index 000000000000..357af6b83af5 --- /dev/null +++ b/swift/ql/test/query-tests/Security/CWE-825/test.expected @@ -0,0 +1,121 @@ +WARNING: Unused variable c (test.ql:51,73-74) +edges +| file://:0:0:0:0 | [post] self [field, some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] | +| file://:0:0:0:0 | [post] self [field, some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0, some:0, some:0, some:0] | +| file://:0:0:0:0 | [post] self [field, some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] | +| file://:0:0:0:0 | [post] self [field, some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0, some:0, some:0] | +| file://:0:0:0:0 | [post] self [field, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] | +| file://:0:0:0:0 | [post] self [field, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0, some:0] | +| file://:0:0:0:0 | [post] self [field, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] | +| file://:0:0:0:0 | [post] self [field, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0] | +| file://:0:0:0:0 | [post] self [field] | withUnsafeBytes.swift:33:9:33:9 | self[return] | +| file://:0:0:0:0 | [post] self [field] | withUnsafeBytes.swift:33:9:33:9 | self[return] [field] | +| file://:0:0:0:0 | value | file://:0:0:0:0 | [post] self [field] | +| file://:0:0:0:0 | value [some:0, some:0, some:0, some:0] | file://:0:0:0:0 | [post] self [field, some:0, some:0, some:0, some:0] | +| file://:0:0:0:0 | value [some:0, some:0, some:0] | file://:0:0:0:0 | [post] self [field, some:0, some:0, some:0] | +| file://:0:0:0:0 | value [some:0, some:0] | file://:0:0:0:0 | [post] self [field, some:0, some:0] | +| file://:0:0:0:0 | value [some:0] | file://:0:0:0:0 | [post] self [field, some:0] | +| 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:28:12:28:21 | pointer | +| withUnsafeBytes.swift:20:28:20:31 | p | withUnsafeBytes.swift:20:68:20:68 | p | +| withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | +| withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | +| withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | +| withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | +| withUnsafeBytes.swift:20:58:20:68 | ... = ... [field] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | +| withUnsafeBytes.swift:20:68:20:68 | p | withUnsafeBytes.swift:20:58:20:68 | ... = ... | +| withUnsafeBytes.swift:20:68:20:68 | p | withUnsafeBytes.swift:20:58:20:68 | ... = ... [field] | +| withUnsafeBytes.swift:20:68:20:68 | p | withUnsafeBytes.swift:20:68:20:68 | p [some:0] | +| withUnsafeBytes.swift:20:68:20:68 | p | withUnsafeBytes.swift:33:9:33:9 | value | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0, some:0, some:0] | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0, some:0, some:0] | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0, some:0] | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0] | withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0, some:0] | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0, some:0] | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0] | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0] | withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0] | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0] | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0] | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0] | withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0] | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0] | +| withUnsafeBytes.swift:28:12:28:21 | pointer | withUnsafeBytes.swift:29:12:29:12 | pointer | +| withUnsafeBytes.swift:28:12:28:21 | pointer | withUnsafeBytes.swift:29:12:29:12 | pointer | +| withUnsafeBytes.swift:33:9:33:9 | value | file://:0:0:0:0 | value | +| withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0, some:0, some:0] | file://:0:0:0:0 | value [some:0, some:0, some:0, some:0] | +| withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0, some:0] | file://:0:0:0:0 | value [some:0, some:0, some:0] | +| withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0] | file://:0:0:0:0 | value [some:0, some:0] | +| withUnsafeBytes.swift:33:9:33:9 | value [some:0] | file://:0:0:0:0 | value [some:0] | +nodes +| file://:0:0:0:0 | [post] self [field, some:0, some:0, some:0, some:0] | semmle.label | [post] self [field, some:0, some:0, some:0, some:0] | +| file://:0:0:0:0 | [post] self [field, some:0, some:0, some:0] | semmle.label | [post] self [field, some:0, some:0, some:0] | +| file://:0:0:0:0 | [post] self [field, some:0, some:0] | semmle.label | [post] self [field, some:0, some:0] | +| file://:0:0:0:0 | [post] self [field, some:0] | semmle.label | [post] self [field, some:0] | +| file://:0:0:0:0 | [post] self [field] | semmle.label | [post] self [field] | +| file://:0:0:0:0 | value | semmle.label | value | +| file://:0:0:0:0 | value [some:0, some:0, some:0, some:0] | semmle.label | value [some:0, some:0, some:0, some:0] | +| file://:0:0:0:0 | value [some:0, some:0, some:0] | semmle.label | value [some:0, some:0, some:0] | +| file://:0:0:0:0 | value [some:0, some:0] | semmle.label | value [some:0, some:0] | +| file://:0:0:0:0 | value [some:0] | semmle.label | value [some:0] | +| 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:20:28:20:31 | p | semmle.label | p | +| withUnsafeBytes.swift:20:58:20:68 | ... = ... | semmle.label | ... = ... | +| withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0, some:0, some:0] | semmle.label | ... = ... [field, some:0, some:0, some:0, some:0] | +| withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0, some:0] | semmle.label | ... = ... [field, some:0, some:0, some:0] | +| withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0] | semmle.label | ... = ... [field, some:0, some:0] | +| withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0] | semmle.label | ... = ... [field, some:0] | +| withUnsafeBytes.swift:20:58:20:68 | ... = ... [field] | semmle.label | ... = ... [field] | +| withUnsafeBytes.swift:20:68:20:68 | p | semmle.label | p | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0, some:0] | semmle.label | p [some:0, some:0, some:0, some:0] | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0] | semmle.label | p [some:0, some:0, some:0] | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0] | semmle.label | p [some:0, some:0] | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0] | semmle.label | p [some:0] | +| withUnsafeBytes.swift:28:12:28:21 | pointer | semmle.label | pointer | +| withUnsafeBytes.swift:28:12:28:21 | pointer | semmle.label | pointer | +| withUnsafeBytes.swift:29:12:29:12 | pointer | semmle.label | pointer | +| withUnsafeBytes.swift:29:12:29:12 | pointer | semmle.label | pointer | +| withUnsafeBytes.swift:33:9:33:9 | self[return] | semmle.label | self[return] | +| withUnsafeBytes.swift:33:9:33:9 | self[return] | semmle.label | self[return] | +| withUnsafeBytes.swift:33:9:33:9 | self[return] | semmle.label | self[return] | +| withUnsafeBytes.swift:33:9:33:9 | self[return] | semmle.label | self[return] | +| withUnsafeBytes.swift:33:9:33:9 | self[return] | semmle.label | self[return] | +| withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0, some:0, some:0, some:0] | semmle.label | self[return] [field, some:0, some:0, some:0, some:0] | +| withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0, some:0, some:0] | semmle.label | self[return] [field, some:0, some:0, some:0] | +| withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0, some:0] | semmle.label | self[return] [field, some:0, some:0] | +| withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0] | semmle.label | self[return] [field, some:0] | +| withUnsafeBytes.swift:33:9:33:9 | self[return] [field] | semmle.label | self[return] [field] | +| withUnsafeBytes.swift:33:9:33:9 | value | semmle.label | value | +| withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0, some:0, some:0] | semmle.label | value [some:0, some:0, some:0, some:0] | +| withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0, some:0] | semmle.label | value [some:0, some:0, some:0] | +| withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0] | semmle.label | value [some:0, some:0] | +| withUnsafeBytes.swift:33:9:33:9 | value [some:0] | semmle.label | value [some:0] | +subpaths +| withUnsafeBytes.swift:15:109:15:109 | p | withUnsafeBytes.swift:28:12:28:21 | pointer | withUnsafeBytes.swift:29:12:29:12 | pointer | withUnsafeBytes.swift:15:97:15:110 | call to id(pointer:) | +| withUnsafeBytes.swift:20:68:20:68 | p | withUnsafeBytes.swift:33:9:33:9 | value | withUnsafeBytes.swift:33:9:33:9 | self[return] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | +| withUnsafeBytes.swift:20:68:20:68 | p | withUnsafeBytes.swift:33:9:33:9 | value | withUnsafeBytes.swift:33:9:33:9 | self[return] [field] | withUnsafeBytes.swift:20:58:20:68 | ... = ... [field] | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0, some:0, some:0] | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0, some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0, some:0] | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0] | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | +| withUnsafeBytes.swift:20:68:20:68 | p [some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0] | +#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:20:58:20:68 | ... = ... | withUnsafeBytes.swift:20:28:20:31 | p | withUnsafeBytes.swift:20:58:20:68 | ... = ... | This unsafe parameter may escape its invocation | +| withUnsafeBytes.swift:29:12:29:12 | pointer | withUnsafeBytes.swift:28:12:28:21 | pointer | withUnsafeBytes.swift:29:12:29:12 | pointer | This unsafe parameter may escape its invocation | diff --git a/swift/ql/test/query-tests/Security/CWE-825/test.ql b/swift/ql/test/query-tests/Security/CWE-825/test.ql index 8516e8320197..3c00c1a28abd 100644 --- a/swift/ql/test/query-tests/Security/CWE-825/test.ql +++ b/swift/ql/test/query-tests/Security/CWE-825/test.ql @@ -1,5 +1,61 @@ +import swift import codeql.swift.dataflow.DataFlow +import Flow::PathGraph -module Conf implements ConfigSig { +module Conf implements DataFlow::StateConfigSig { + class FlowState = Callable; -} \ No newline at end of file + predicate isSource(DataFlow::Node node, FlowState state) { + // parameter of closure or function passed to withUnsafeBytes + exists( + CallExpr call | + call.getStaticTarget().hasName("withUnsafeBytes(_:)") and + state = node.(DataFlow::ParameterNode).getDeclaringFunction().getUnderlyingCallable() and + ( + // if the declaring callable is a closure expr + state = call.getArgument(0).getExpr() + or + state.(Function).getAnAccess() = call.getArgument(0).getExpr() + ) + ) + } + + predicate isSink(DataFlow::Node node, FlowState state) { + 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) { none() } + 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(DataFlow::ContentSet set) + } +} + +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" From f27522d9965840a26efb9f9fbc111efa8409b9b6 Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Tue, 11 Jul 2023 22:19:48 +0000 Subject: [PATCH 3/9] Swift: relocate UnsafePointerEscapesClosure --- .../queries/Security/CWE-825/UnsafePointerEscapesClosure.ql} | 0 .../{test.expected => UnsafePointerEscapesClosure.expected} | 1 - .../Security/CWE-825/UnsafePointerEscapesClosure.qlref | 1 + 3 files changed, 1 insertion(+), 1 deletion(-) rename swift/ql/{test/query-tests/Security/CWE-825/test.ql => src/queries/Security/CWE-825/UnsafePointerEscapesClosure.ql} (100%) rename swift/ql/test/query-tests/Security/CWE-825/{test.expected => UnsafePointerEscapesClosure.expected} (99%) create mode 100644 swift/ql/test/query-tests/Security/CWE-825/UnsafePointerEscapesClosure.qlref diff --git a/swift/ql/test/query-tests/Security/CWE-825/test.ql b/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.ql similarity index 100% rename from swift/ql/test/query-tests/Security/CWE-825/test.ql rename to swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.ql diff --git a/swift/ql/test/query-tests/Security/CWE-825/test.expected b/swift/ql/test/query-tests/Security/CWE-825/UnsafePointerEscapesClosure.expected similarity index 99% rename from swift/ql/test/query-tests/Security/CWE-825/test.expected rename to swift/ql/test/query-tests/Security/CWE-825/UnsafePointerEscapesClosure.expected index 357af6b83af5..85c4315f0441 100644 --- a/swift/ql/test/query-tests/Security/CWE-825/test.expected +++ b/swift/ql/test/query-tests/Security/CWE-825/UnsafePointerEscapesClosure.expected @@ -1,4 +1,3 @@ -WARNING: Unused variable c (test.ql:51,73-74) edges | file://:0:0:0:0 | [post] self [field, some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] | | file://:0:0:0:0 | [post] self [field, some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0, some:0, some:0, some:0] | 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 From db1891579ef1640efacb9d7ed469707249c74d49 Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Wed, 12 Jul 2023 15:22:02 +0000 Subject: [PATCH 4/9] Swift: add more funcs to unsafe closure query --- .../CWE-825/UnsafePointerEscapesClosure.ql | 39 ++++++++++--------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.ql b/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.ql index 3c00c1a28abd..3a19e62cc9b1 100644 --- a/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.ql +++ b/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.ql @@ -7,9 +7,13 @@ module Conf implements DataFlow::StateConfigSig { predicate isSource(DataFlow::Node node, FlowState state) { // parameter of closure or function passed to withUnsafeBytes - exists( - CallExpr call | - call.getStaticTarget().hasName("withUnsafeBytes(_:)") and + exists(CallExpr call | + call.getStaticTarget() + .hasName([ + "withUnsafeBytes(_:)", "withCString(_:)", "withUnsafeMutableBytes(_:)", + "withContiguousMutableStorageIfAvailable(_:)", "withContiguousStorageIfAvailable(_:)", + "withUTF8(_:)", "withUnsafeBufferPointer(_:)", "withUnsafeBufferPointer(_:)" + ]) and state = node.(DataFlow::ParameterNode).getDeclaringFunction().getUnderlyingCallable() and ( // if the declaring callable is a closure expr @@ -29,26 +33,25 @@ module Conf implements DataFlow::StateConfigSig { ) } - 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 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) { none() } - predicate isAdditionalFlowStep(DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2) { + + 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) { + + predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) { isSink(node, _) and c = any(DataFlow::ContentSet set) } From 8120c8b9fdd161035b49759979d73a0e1579bb72 Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Wed, 12 Jul 2023 17:46:27 +0000 Subject: [PATCH 5/9] Swift: refactor to remove cartesian product --- .../CWE-825/UnsafePointerEscapesClosure.ql | 68 +++++++++++++------ 1 file changed, 49 insertions(+), 19 deletions(-) diff --git a/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.ql b/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.ql index 3a19e62cc9b1..44bc8401ecf7 100644 --- a/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.ql +++ b/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.ql @@ -1,3 +1,12 @@ +/** + * @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 @@ -5,31 +14,52 @@ import Flow::PathGraph module Conf implements DataFlow::StateConfigSig { class FlowState = Callable; - predicate isSource(DataFlow::Node node, FlowState state) { - // parameter of closure or function passed to withUnsafeBytes + additional predicate isUnsafePointerCall(CallExpr call) { + call.getStaticTarget() + .hasName([ + "withUnsafeBytes(_:)", "withCString(_:)", "withUnsafeMutableBytes(_:)", + "withContiguousMutableStorageIfAvailable(_:)", "withContiguousStorageIfAvailable(_:)", + "withUTF8(_:)", "withUnsafeBufferPointer(_:)", "withUnsafeBufferPointer(_:)" + ]) + } + + + additional predicate isUnsafePointerClosure(ClosureExpr expr) { + exists(CallExpr call | + isUnsafePointerCall(call) and + expr = call.getArgument(0).getExpr() + ) + } + + additional predicate isUnsafePointerFunction(Function f) { exists(CallExpr call | - call.getStaticTarget() - .hasName([ - "withUnsafeBytes(_:)", "withCString(_:)", "withUnsafeMutableBytes(_:)", - "withContiguousMutableStorageIfAvailable(_:)", "withContiguousStorageIfAvailable(_:)", - "withUTF8(_:)", "withUnsafeBufferPointer(_:)", "withUnsafeBufferPointer(_:)" - ]) and - state = node.(DataFlow::ParameterNode).getDeclaringFunction().getUnderlyingCallable() and + isUnsafePointerCall(call) and + f.getAnAccess() = call.getArgument(0).getExpr() + ) + } + predicate isSource(DataFlow::Node node, FlowState state) { ( - // if the declaring callable is a closure expr - state = call.getArgument(0).getExpr() + isUnsafePointerClosure(state) or - state.(Function).getAnAccess() = call.getArgument(0).getExpr() - ) - ) + isUnsafePointerFunction(state) + ) and + state = node.(DataFlow::ParameterNode).getDeclaringFunction().getUnderlyingCallable() } predicate isSink(DataFlow::Node node, FlowState state) { - node.(DataFlow::InoutReturnNode).getParameter().getDeclaringFunction() = state - or - exists(ReturnStmt stmt | - node.asExpr() = stmt.getResult() and - stmt.getEnclosingCallable() = 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 + ) ) } From 459eea51e98cde338b79351c742ec246520ed9c4 Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Wed, 12 Jul 2023 19:51:08 +0000 Subject: [PATCH 6/9] Swift: avoid implicit return in tests --- .../UnsafePointerEscapesClosure.expected | 111 ++---------------- .../Security/CWE-825/withUnsafeBytes.swift | 12 +- 2 files changed, 20 insertions(+), 103 deletions(-) diff --git a/swift/ql/test/query-tests/Security/CWE-825/UnsafePointerEscapesClosure.expected b/swift/ql/test/query-tests/Security/CWE-825/UnsafePointerEscapesClosure.expected index 85c4315f0441..4c9c14772620 100644 --- a/swift/ql/test/query-tests/Security/CWE-825/UnsafePointerEscapesClosure.expected +++ b/swift/ql/test/query-tests/Security/CWE-825/UnsafePointerEscapesClosure.expected @@ -1,67 +1,12 @@ edges -| file://:0:0:0:0 | [post] self [field, some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] | -| file://:0:0:0:0 | [post] self [field, some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0, some:0, some:0, some:0] | -| file://:0:0:0:0 | [post] self [field, some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] | -| file://:0:0:0:0 | [post] self [field, some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0, some:0, some:0] | -| file://:0:0:0:0 | [post] self [field, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] | -| file://:0:0:0:0 | [post] self [field, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0, some:0] | -| file://:0:0:0:0 | [post] self [field, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] | -| file://:0:0:0:0 | [post] self [field, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0] | -| file://:0:0:0:0 | [post] self [field] | withUnsafeBytes.swift:33:9:33:9 | self[return] | -| file://:0:0:0:0 | [post] self [field] | withUnsafeBytes.swift:33:9:33:9 | self[return] [field] | -| file://:0:0:0:0 | value | file://:0:0:0:0 | [post] self [field] | -| file://:0:0:0:0 | value [some:0, some:0, some:0, some:0] | file://:0:0:0:0 | [post] self [field, some:0, some:0, some:0, some:0] | -| file://:0:0:0:0 | value [some:0, some:0, some:0] | file://:0:0:0:0 | [post] self [field, some:0, some:0, some:0] | -| file://:0:0:0:0 | value [some:0, some:0] | file://:0:0:0:0 | [post] self [field, some:0, some:0] | -| file://:0:0:0:0 | value [some:0] | file://:0:0:0:0 | [post] self [field, some:0] | | 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:28:12:28:21 | pointer | -| withUnsafeBytes.swift:20:28:20:31 | p | withUnsafeBytes.swift:20:68:20:68 | p | -| withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | -| withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | -| withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | -| withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | -| withUnsafeBytes.swift:20:58:20:68 | ... = ... [field] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | -| withUnsafeBytes.swift:20:68:20:68 | p | withUnsafeBytes.swift:20:58:20:68 | ... = ... | -| withUnsafeBytes.swift:20:68:20:68 | p | withUnsafeBytes.swift:20:58:20:68 | ... = ... [field] | -| withUnsafeBytes.swift:20:68:20:68 | p | withUnsafeBytes.swift:20:68:20:68 | p [some:0] | -| withUnsafeBytes.swift:20:68:20:68 | p | withUnsafeBytes.swift:33:9:33:9 | value | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0, some:0, some:0] | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0, some:0, some:0] | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0, some:0] | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0] | withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0, some:0] | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0, some:0] | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0] | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0] | withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0] | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0] | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0] | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0] | withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0] | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0] | -| withUnsafeBytes.swift:28:12:28:21 | pointer | withUnsafeBytes.swift:29:12:29:12 | pointer | -| withUnsafeBytes.swift:28:12:28:21 | pointer | withUnsafeBytes.swift:29:12:29:12 | pointer | -| withUnsafeBytes.swift:33:9:33:9 | value | file://:0:0:0:0 | value | -| withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0, some:0, some:0] | file://:0:0:0:0 | value [some:0, some:0, some:0, some:0] | -| withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0, some:0] | file://:0:0:0:0 | value [some:0, some:0, some:0] | -| withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0] | file://:0:0:0:0 | value [some:0, some:0] | -| withUnsafeBytes.swift:33:9:33:9 | value [some:0] | file://:0:0:0:0 | value [some:0] | +| 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 -| file://:0:0:0:0 | [post] self [field, some:0, some:0, some:0, some:0] | semmle.label | [post] self [field, some:0, some:0, some:0, some:0] | -| file://:0:0:0:0 | [post] self [field, some:0, some:0, some:0] | semmle.label | [post] self [field, some:0, some:0, some:0] | -| file://:0:0:0:0 | [post] self [field, some:0, some:0] | semmle.label | [post] self [field, some:0, some:0] | -| file://:0:0:0:0 | [post] self [field, some:0] | semmle.label | [post] self [field, some:0] | -| file://:0:0:0:0 | [post] self [field] | semmle.label | [post] self [field] | -| file://:0:0:0:0 | value | semmle.label | value | -| file://:0:0:0:0 | value [some:0, some:0, some:0, some:0] | semmle.label | value [some:0, some:0, some:0, some:0] | -| file://:0:0:0:0 | value [some:0, some:0, some:0] | semmle.label | value [some:0, some:0, some:0] | -| file://:0:0:0:0 | value [some:0, some:0] | semmle.label | value [some:0, some:0] | -| file://:0:0:0:0 | value [some:0] | semmle.label | value [some:0] | | 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 | @@ -69,52 +14,14 @@ nodes | 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:20:28:20:31 | p | semmle.label | p | -| withUnsafeBytes.swift:20:58:20:68 | ... = ... | semmle.label | ... = ... | -| withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0, some:0, some:0] | semmle.label | ... = ... [field, some:0, some:0, some:0, some:0] | -| withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0, some:0] | semmle.label | ... = ... [field, some:0, some:0, some:0] | -| withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0] | semmle.label | ... = ... [field, some:0, some:0] | -| withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0] | semmle.label | ... = ... [field, some:0] | -| withUnsafeBytes.swift:20:58:20:68 | ... = ... [field] | semmle.label | ... = ... [field] | -| withUnsafeBytes.swift:20:68:20:68 | p | semmle.label | p | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0, some:0] | semmle.label | p [some:0, some:0, some:0, some:0] | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0] | semmle.label | p [some:0, some:0, some:0] | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0] | semmle.label | p [some:0, some:0] | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0] | semmle.label | p [some:0] | -| withUnsafeBytes.swift:28:12:28:21 | pointer | semmle.label | pointer | -| withUnsafeBytes.swift:28:12:28:21 | pointer | semmle.label | pointer | -| withUnsafeBytes.swift:29:12:29:12 | pointer | semmle.label | pointer | -| withUnsafeBytes.swift:29:12:29:12 | pointer | semmle.label | pointer | -| withUnsafeBytes.swift:33:9:33:9 | self[return] | semmle.label | self[return] | -| withUnsafeBytes.swift:33:9:33:9 | self[return] | semmle.label | self[return] | -| withUnsafeBytes.swift:33:9:33:9 | self[return] | semmle.label | self[return] | -| withUnsafeBytes.swift:33:9:33:9 | self[return] | semmle.label | self[return] | -| withUnsafeBytes.swift:33:9:33:9 | self[return] | semmle.label | self[return] | -| withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0, some:0, some:0, some:0] | semmle.label | self[return] [field, some:0, some:0, some:0, some:0] | -| withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0, some:0, some:0] | semmle.label | self[return] [field, some:0, some:0, some:0] | -| withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0, some:0] | semmle.label | self[return] [field, some:0, some:0] | -| withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0] | semmle.label | self[return] [field, some:0] | -| withUnsafeBytes.swift:33:9:33:9 | self[return] [field] | semmle.label | self[return] [field] | -| withUnsafeBytes.swift:33:9:33:9 | value | semmle.label | value | -| withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0, some:0, some:0] | semmle.label | value [some:0, some:0, some:0, some:0] | -| withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0, some:0] | semmle.label | value [some:0, some:0, some:0] | -| withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0] | semmle.label | value [some:0, some:0] | -| withUnsafeBytes.swift:33:9:33:9 | value [some:0] | semmle.label | value [some:0] | +| 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:28:12:28:21 | pointer | withUnsafeBytes.swift:29:12:29:12 | pointer | withUnsafeBytes.swift:15:97:15:110 | call to id(pointer:) | -| withUnsafeBytes.swift:20:68:20:68 | p | withUnsafeBytes.swift:33:9:33:9 | value | withUnsafeBytes.swift:33:9:33:9 | self[return] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | -| withUnsafeBytes.swift:20:68:20:68 | p | withUnsafeBytes.swift:33:9:33:9 | value | withUnsafeBytes.swift:33:9:33:9 | self[return] [field] | withUnsafeBytes.swift:20:58:20:68 | ... = ... [field] | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0, some:0, some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0, some:0, some:0] | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0, some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0, some:0] | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0, some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0, some:0] | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] | withUnsafeBytes.swift:20:58:20:68 | ... = ... | -| withUnsafeBytes.swift:20:68:20:68 | p [some:0] | withUnsafeBytes.swift:33:9:33:9 | value [some:0] | withUnsafeBytes.swift:33:9:33:9 | self[return] [field, some:0] | withUnsafeBytes.swift:20:58:20:68 | ... = ... [field, some:0] | +| 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:20:58:20:68 | ... = ... | withUnsafeBytes.swift:20:28:20:31 | p | withUnsafeBytes.swift:20:58:20:68 | ... = ... | This unsafe parameter may escape its invocation | -| withUnsafeBytes.swift:29:12:29:12 | pointer | withUnsafeBytes.swift:28:12:28:21 | pointer | withUnsafeBytes.swift:29:12:29:12 | 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/withUnsafeBytes.swift b/swift/ql/test/query-tests/Security/CWE-825/withUnsafeBytes.swift index 661dc29c5450..60493fb0866e 100644 --- a/swift/ql/test/query-tests/Security/CWE-825/withUnsafeBytes.swift +++ b/swift/ql/test/query-tests/Security/CWE-825/withUnsafeBytes.swift @@ -17,12 +17,22 @@ func arrayTest() { ints.withUnsafeBytes({(p: UnsafeRawBufferPointer) in print(p)}) // GOOD var v = PointerHolder() - ints.withUnsafeBytes({(p: UnsafeRawBufferPointer) in v.field = p}) // BAD + 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 { From f125fa29473934c38ac0b1f861e351d7f5c5c90a Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Thu, 13 Jul 2023 14:50:33 +0000 Subject: [PATCH 7/9] Swift: respond to PR comments --- .../CWE-825/UnsafePointerEscapesClosure.ql | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.ql b/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.ql index 44bc8401ecf7..f495403a49e4 100644 --- a/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.ql +++ b/swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.ql @@ -11,7 +11,7 @@ import swift import codeql.swift.dataflow.DataFlow import Flow::PathGraph -module Conf implements DataFlow::StateConfigSig { +module Config implements DataFlow::StateConfigSig { class FlowState = Callable; additional predicate isUnsafePointerCall(CallExpr call) { @@ -19,7 +19,9 @@ module Conf implements DataFlow::StateConfigSig { .hasName([ "withUnsafeBytes(_:)", "withCString(_:)", "withUnsafeMutableBytes(_:)", "withContiguousMutableStorageIfAvailable(_:)", "withContiguousStorageIfAvailable(_:)", - "withUTF8(_:)", "withUnsafeBufferPointer(_:)", "withUnsafeBufferPointer(_:)" + "withUTF8(_:)", "withUnsafeBufferPointer(_:)", "withUnsafeMutableBufferPointer(_:)", + "withMemoryRebound(to:_:)", "withUnsafeTemporaryAllocation(of:capacity:_:)", + "withUnsafeCurrentTask(body:)", "withCheckedContinuation(function:_:)" ]) } @@ -27,14 +29,14 @@ module Conf implements DataFlow::StateConfigSig { additional predicate isUnsafePointerClosure(ClosureExpr expr) { exists(CallExpr call | isUnsafePointerCall(call) and - expr = call.getArgument(0).getExpr() + expr = call.getAnArgument().getExpr() ) } additional predicate isUnsafePointerFunction(Function f) { exists(CallExpr call | isUnsafePointerCall(call) and - f.getAnAccess() = call.getArgument(0).getExpr() + f.getAnAccess() = call.getAnArgument().getExpr() ) } predicate isSource(DataFlow::Node node, FlowState state) { @@ -71,7 +73,9 @@ module Conf implements DataFlow::StateConfigSig { predicate isBarrier(DataFlow::Node node, FlowState state) { none() } - predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { 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 @@ -83,11 +87,11 @@ module Conf implements DataFlow::StateConfigSig { predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) { isSink(node, _) and - c = any(DataFlow::ContentSet set) + c = any(c) } } -module Flow = DataFlow::GlobalWithState; +module Flow = DataFlow::GlobalWithState; from Flow::PathNode source, Flow::PathNode sink where Flow::flowPath(source, sink) From 5f39a1abafa2f076a3b05a55e4753e8baf7ea072 Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Thu, 13 Jul 2023 15:32:29 +0000 Subject: [PATCH 8/9] Swift: qhelp for UnsafePointerEscapesClosure --- .../CWE-825/UnsafePointerEscapesClosure.qhelp | 21 +++++++++++++++++++ .../CWE-825/UnsafePointerEscapesClosure.swift | 14 +++++++++++++ 2 files changed, 35 insertions(+) create mode 100644 swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.qhelp create mode 100644 swift/ql/src/queries/Security/CWE-825/UnsafePointerEscapesClosure.swift 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.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) + } +} From 37d69e59d61d903af4812028653a40bd6ce5930f Mon Sep 17 00:00:00 2001 From: Robert Marsh Date: Thu, 13 Jul 2023 14:07:59 -0400 Subject: [PATCH 9/9] Swift: add change note for unsafe closure query --- .../change-notes/2023-07-13-unsafe-pointer-escapes-closure.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 swift/ql/src/change-notes/2023-07-13-unsafe-pointer-escapes-closure.md 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