Skip to content

Rb: more taint-steps for shell-command-construction #11478

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Mar 20, 2023
Merged

Rb: more taint-steps for shell-command-construction #11478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a4c42aa
more custom array steps from unsafe-code-construction to a utility pr…
erik-krogh Jan 30, 2023
962465f
add array-taint-steps to unsafe-shell-command-construction
erik-krogh Jan 30, 2023
c2e8206
add more array taint steps that taint the entire array
erik-krogh Jan 30, 2023
d2bd70d
Merge branch 'main' into more-shell-taint
erik-krogh Feb 15, 2023
17f7ba2
rewrite the taint-step for join() to a flowsummary
erik-krogh Feb 15, 2023
b0797a2
Merge branch 'main' into more-shell-taint
erik-krogh Feb 27, 2023
36b3376
use allowImplicitRead instead of a taint-step from elements to the array
erik-krogh Feb 28, 2023
31336b0
add summary for the `Array` method on `Kernel`
erik-krogh Mar 1, 2023
6cd4cd3
remove redundant imports
erik-krogh Mar 1, 2023
25a6d49
Merge branch 'main' into HEAD
erik-krogh Mar 13, 2023
8b99e8a
fix bad join by removing bad recursion
erik-krogh Mar 13, 2023
4307889
specialize allowImplicitRead in unsafe-shell-command-construction to …
erik-krogh Mar 14, 2023
984a589
don't depend on the callgraph in KernelArraySummary
erik-krogh Mar 14, 2023
2133d1a
Merge branch 'main' into more-shell-taint
erik-krogh Mar 15, 2023
cc3efcd
also restrict allowImplicitRead in unsafe-code-construction
erik-krogh Mar 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions ruby/ql/lib/codeql/ruby/frameworks/core/Array.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1186,6 +1186,16 @@ module Array {
}
}

private class JoinSummary extends SimpleSummarizedCallable {
JoinSummary() { this = ["join"] }

Check warning

Code scanning / CodeQL

Singleton set literal

Singleton set literal can be replaced by its member.

override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
input = "Argument[self].Element[any]" and
output = "ReturnValue" and
preservesValue = false
}
}

private class PushSummary extends SimpleSummarizedCallable {
// `append` is an alias for `push`
PushSummary() { this = ["push", "append"] }
Expand Down
34 changes: 34 additions & 0 deletions ruby/ql/lib/codeql/ruby/frameworks/core/Kernel.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,4 +197,38 @@ module Kernel {
super.getMethodName() = ["autoload", "autoload?"]
}
}

private import codeql.ruby.ast.internal.Module as Module

/**
* A call to `Array()`, that converts it's singular argument to an array.
* This summary is based on https://ruby-doc.org/3.2.1/Kernel.html#method-i-Array
*/
private class KernelArraySummary extends SummarizedCallable {
KernelArraySummary() { this = "Array()" }

override MethodCall getACallSimple() {
result.getMethodName() = "Array" and
// I have to have a simplified "KernelMethodCall" implementation inlined here, because relying on `UnknownMethodCall` results in non-monotonic recursion (even if using `getACall`).
(
// similar to `getAStaticArrayCall` from Array.qll
Module::resolveConstantReadAccess(result.getReceiver()) = Module::TResolved("Kernel")
or
result.getReceiver() instanceof SelfVariableAccess
)
}

override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
(
// already an array
input = "Argument[0].WithElement[0..]" and
output = "ReturnValue"
or
// not already an array
input = "Argument[0].WithoutElement[0..]" and
output = "ReturnValue.Element[0]"
) and
preservesValue = true
}
}
}
20 changes: 6 additions & 14 deletions ruby/ql/lib/codeql/ruby/security/UnsafeCodeConstructionQuery.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,19 +31,11 @@ class Configuration extends TaintTracking::Configuration {
result instanceof DataFlow::FeatureHasSourceCallContext
}

override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
// if an array element gets tainted, then we treat the entire array as tainted
exists(DataFlow::CallNode call |
call.getMethodName() = ["<<", "push", "append"] and
call.getReceiver() = succ and
pred = call.getArgument(0) and
call.getNumberOfArguments() = 1
)
or
exists(DataFlow::CallNode call |
call.getMethodName() = "[]" and
succ = call and
pred = call.getArgument(_)
)
override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet set) {
// allow implicit reads of array elements
this.isSink(node) and
set.isKnownOrUnknownElement(any(DataFlow::Content::KnownElementContent content |
content.getIndex().getValueType() = "int"
))
}
}
8 changes: 8 additions & 0 deletions ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,12 @@ class Configuration extends TaintTracking::Configuration {
override DataFlow::FlowFeature getAFeature() {
result instanceof DataFlow::FeatureHasSourceCallContext
}

override predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet set) {
// allow implicit reads of array elements
this.isSink(node) and
set.isKnownOrUnknownElement(any(DataFlow::Content::KnownElementContent content |
content.getIndex().getValueType() = "int"
))
}
}
2 changes: 2 additions & 0 deletions ruby/ql/test/library-tests/frameworks/pathname/Pathname.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ pathnameInstances
| file://:0:0:0:0 | parameter self of Pathname;Method[relative_path_from] |
| file://:0:0:0:0 | parameter self of Pathname;Method[sub_ext] |
| file://:0:0:0:0 | parameter self of Pathname;Method[to_path] |
| file://:0:0:0:0 | parameter self of join |
| file://:0:0:0:0 | parameter self of sub |
| file://:0:0:0:0 | parameter self of to_s |
fileSystemAccesses
Expand Down Expand Up @@ -141,6 +142,7 @@ fileNameSources
| file://:0:0:0:0 | parameter self of Pathname;Method[relative_path_from] |
| file://:0:0:0:0 | parameter self of Pathname;Method[sub_ext] |
| file://:0:0:0:0 | parameter self of Pathname;Method[to_path] |
| file://:0:0:0:0 | parameter self of join |
| file://:0:0:0:0 | parameter self of sub |
| file://:0:0:0:0 | parameter self of to_s |
fileSystemReadAccesses
Expand Down
11 changes: 11 additions & 0 deletions ...s/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ edges
| impl/unsafeShell.rb:51:17:51:17 | x : | impl/unsafeShell.rb:52:14:52:14 | x |
| impl/unsafeShell.rb:51:17:51:17 | x : | impl/unsafeShell.rb:54:29:54:29 | x |
| impl/unsafeShell.rb:57:21:57:21 | x : | impl/unsafeShell.rb:58:23:58:23 | x |
| impl/unsafeShell.rb:61:20:61:20 | x : | impl/unsafeShell.rb:63:14:63:14 | x : |
| impl/unsafeShell.rb:63:5:63:7 | [post] arr [element] : | impl/unsafeShell.rb:64:14:64:16 | arr |
| impl/unsafeShell.rb:63:5:63:7 | [post] arr [element] : | impl/unsafeShell.rb:68:14:68:16 | arr |
| impl/unsafeShell.rb:63:14:63:14 | x : | impl/unsafeShell.rb:63:5:63:7 | [post] arr [element] : |
nodes
| impl/sub/notImported.rb:2:12:2:17 | target : | semmle.label | target : |
| impl/sub/notImported.rb:3:19:3:27 | #{...} | semmle.label | #{...} |
Expand All @@ -38,6 +42,11 @@ nodes
| impl/unsafeShell.rb:54:29:54:29 | x | semmle.label | x |
| impl/unsafeShell.rb:57:21:57:21 | x : | semmle.label | x : |
| impl/unsafeShell.rb:58:23:58:23 | x | semmle.label | x |
| impl/unsafeShell.rb:61:20:61:20 | x : | semmle.label | x : |
| impl/unsafeShell.rb:63:5:63:7 | [post] arr [element] : | semmle.label | [post] arr [element] : |
| impl/unsafeShell.rb:63:14:63:14 | x : | semmle.label | x : |
| impl/unsafeShell.rb:64:14:64:16 | arr | semmle.label | arr |
| impl/unsafeShell.rb:68:14:68:16 | arr | semmle.label | arr |
subpaths
#select
| impl/sub/notImported.rb:3:14:3:28 | "cat #{...}" | impl/sub/notImported.rb:2:12:2:17 | target : | impl/sub/notImported.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/sub/notImported.rb:2:12:2:17 | target | library input | impl/sub/notImported.rb:3:5:3:34 | call to popen | shell command |
Expand All @@ -53,3 +62,5 @@ subpaths
| impl/unsafeShell.rb:52:14:52:24 | call to join | impl/unsafeShell.rb:51:17:51:17 | x : | impl/unsafeShell.rb:52:14:52:14 | x | This array which depends on $@ is later used in a $@. | impl/unsafeShell.rb:51:17:51:17 | x | library input | impl/unsafeShell.rb:52:5:52:30 | call to popen | shell command |
| impl/unsafeShell.rb:54:14:54:40 | call to join | impl/unsafeShell.rb:51:17:51:17 | x : | impl/unsafeShell.rb:54:29:54:29 | x | This array which depends on $@ is later used in a $@. | impl/unsafeShell.rb:51:17:51:17 | x | library input | impl/unsafeShell.rb:54:5:54:46 | call to popen | shell command |
| impl/unsafeShell.rb:58:14:58:23 | ... + ... | impl/unsafeShell.rb:57:21:57:21 | x : | impl/unsafeShell.rb:58:23:58:23 | x | This string concatenation which depends on $@ is later used in a $@. | impl/unsafeShell.rb:57:21:57:21 | x | library input | impl/unsafeShell.rb:58:5:58:29 | call to popen | shell command |
| impl/unsafeShell.rb:64:14:64:26 | call to join | impl/unsafeShell.rb:61:20:61:20 | x : | impl/unsafeShell.rb:64:14:64:16 | arr | This array which depends on $@ is later used in a $@. | impl/unsafeShell.rb:61:20:61:20 | x | library input | impl/unsafeShell.rb:64:5:64:32 | call to popen | shell command |
| impl/unsafeShell.rb:68:14:68:26 | call to join | impl/unsafeShell.rb:61:20:61:20 | x : | impl/unsafeShell.rb:68:14:68:16 | arr | This array which depends on $@ is later used in a $@. | impl/unsafeShell.rb:61:20:61:20 | x | library input | impl/unsafeShell.rb:68:5:68:32 | call to popen | shell command |
10 changes: 10 additions & 0 deletions ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,14 @@ def arrayJoin(x)
def string_concat(x)
IO.popen("cat " + x, "w") # NOT OK
end

def array_taint (x, y)
arr = ["cat"]
arr.push(x)
IO.popen(arr.join(' '), "w") # NOT OK

arr2 = ["cat"]
arr2 << y
IO.popen(arr.join(' '), "w") # NOT OK
end
end
30 changes: 27 additions & 3 deletions .../test/query-tests/security/cwe-094/UnsafeCodeConstruction/UnsafeCodeConstruction.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,21 @@ edges
| impl/unsafeCode.rb:7:12:7:12 | x : | impl/unsafeCode.rb:8:30:8:30 | x |
| impl/unsafeCode.rb:12:12:12:12 | x : | impl/unsafeCode.rb:13:33:13:33 | x |
| impl/unsafeCode.rb:28:17:28:22 | my_arr : | impl/unsafeCode.rb:29:10:29:15 | my_arr |
| impl/unsafeCode.rb:32:21:32:21 | x : | impl/unsafeCode.rb:34:10:34:12 | arr |
| impl/unsafeCode.rb:37:15:37:15 | x : | impl/unsafeCode.rb:40:10:40:12 | arr |
| impl/unsafeCode.rb:37:15:37:15 | x : | impl/unsafeCode.rb:44:10:44:12 | arr |
| impl/unsafeCode.rb:32:21:32:21 | x : | impl/unsafeCode.rb:33:12:33:12 | x : |
| impl/unsafeCode.rb:33:12:33:12 | x : | impl/unsafeCode.rb:34:10:34:12 | arr |
| impl/unsafeCode.rb:37:15:37:15 | x : | impl/unsafeCode.rb:39:14:39:14 | x : |
| impl/unsafeCode.rb:39:5:39:7 | [post] arr [element] : | impl/unsafeCode.rb:40:10:40:12 | arr |
| impl/unsafeCode.rb:39:5:39:7 | [post] arr [element] : | impl/unsafeCode.rb:44:10:44:12 | arr |
| impl/unsafeCode.rb:39:14:39:14 | x : | impl/unsafeCode.rb:39:5:39:7 | [post] arr [element] : |
| impl/unsafeCode.rb:47:15:47:15 | x : | impl/unsafeCode.rb:49:9:49:12 | #{...} |
| impl/unsafeCode.rb:54:21:54:21 | x : | impl/unsafeCode.rb:55:22:55:22 | x |
| impl/unsafeCode.rb:59:21:59:21 | x : | impl/unsafeCode.rb:60:17:60:17 | x : |
| impl/unsafeCode.rb:59:24:59:24 | y : | impl/unsafeCode.rb:63:30:63:30 | y : |
| impl/unsafeCode.rb:60:11:60:18 | call to Array [element 0] : | impl/unsafeCode.rb:61:10:61:12 | arr |
| impl/unsafeCode.rb:60:17:60:17 | x : | impl/unsafeCode.rb:60:11:60:18 | call to Array [element 0] : |
| impl/unsafeCode.rb:63:13:63:32 | call to Array [element 1] : | impl/unsafeCode.rb:63:13:63:42 | call to join : |
| impl/unsafeCode.rb:63:13:63:42 | call to join : | impl/unsafeCode.rb:64:10:64:13 | arr2 |
| impl/unsafeCode.rb:63:30:63:30 | y : | impl/unsafeCode.rb:63:13:63:32 | call to Array [element 1] : |
nodes
| impl/unsafeCode.rb:2:12:2:17 | target : | semmle.label | target : |
| impl/unsafeCode.rb:3:17:3:25 | #{...} | semmle.label | #{...} |
Expand All @@ -18,14 +28,26 @@ nodes
| impl/unsafeCode.rb:28:17:28:22 | my_arr : | semmle.label | my_arr : |
| impl/unsafeCode.rb:29:10:29:15 | my_arr | semmle.label | my_arr |
| impl/unsafeCode.rb:32:21:32:21 | x : | semmle.label | x : |
| impl/unsafeCode.rb:33:12:33:12 | x : | semmle.label | x : |
| impl/unsafeCode.rb:34:10:34:12 | arr | semmle.label | arr |
| impl/unsafeCode.rb:37:15:37:15 | x : | semmle.label | x : |
| impl/unsafeCode.rb:39:5:39:7 | [post] arr [element] : | semmle.label | [post] arr [element] : |
| impl/unsafeCode.rb:39:14:39:14 | x : | semmle.label | x : |
| impl/unsafeCode.rb:40:10:40:12 | arr | semmle.label | arr |
| impl/unsafeCode.rb:44:10:44:12 | arr | semmle.label | arr |
| impl/unsafeCode.rb:47:15:47:15 | x : | semmle.label | x : |
| impl/unsafeCode.rb:49:9:49:12 | #{...} | semmle.label | #{...} |
| impl/unsafeCode.rb:54:21:54:21 | x : | semmle.label | x : |
| impl/unsafeCode.rb:55:22:55:22 | x | semmle.label | x |
| impl/unsafeCode.rb:59:21:59:21 | x : | semmle.label | x : |
| impl/unsafeCode.rb:59:24:59:24 | y : | semmle.label | y : |
| impl/unsafeCode.rb:60:11:60:18 | call to Array [element 0] : | semmle.label | call to Array [element 0] : |
| impl/unsafeCode.rb:60:17:60:17 | x : | semmle.label | x : |
| impl/unsafeCode.rb:61:10:61:12 | arr | semmle.label | arr |
| impl/unsafeCode.rb:63:13:63:32 | call to Array [element 1] : | semmle.label | call to Array [element 1] : |
| impl/unsafeCode.rb:63:13:63:42 | call to join : | semmle.label | call to join : |
| impl/unsafeCode.rb:63:30:63:30 | y : | semmle.label | y : |
| impl/unsafeCode.rb:64:10:64:13 | arr2 | semmle.label | arr2 |
subpaths
#select
| impl/unsafeCode.rb:3:17:3:25 | #{...} | impl/unsafeCode.rb:2:12:2:17 | target : | impl/unsafeCode.rb:3:17:3:25 | #{...} | This string interpolation which depends on $@ is later $@. | impl/unsafeCode.rb:2:12:2:17 | target | library input | impl/unsafeCode.rb:3:5:3:27 | call to eval | interpreted as code |
Expand All @@ -37,3 +59,5 @@ subpaths
| impl/unsafeCode.rb:44:10:44:12 | arr | impl/unsafeCode.rb:37:15:37:15 | x : | impl/unsafeCode.rb:44:10:44:12 | arr | This array which depends on $@ is later $@. | impl/unsafeCode.rb:37:15:37:15 | x | library input | impl/unsafeCode.rb:44:5:44:24 | call to eval | interpreted as code |
| impl/unsafeCode.rb:49:9:49:12 | #{...} | impl/unsafeCode.rb:47:15:47:15 | x : | impl/unsafeCode.rb:49:9:49:12 | #{...} | This string interpolation which depends on $@ is later $@. | impl/unsafeCode.rb:47:15:47:15 | x | library input | impl/unsafeCode.rb:51:5:51:13 | call to eval | interpreted as code |
| impl/unsafeCode.rb:55:22:55:22 | x | impl/unsafeCode.rb:54:21:54:21 | x : | impl/unsafeCode.rb:55:22:55:22 | x | This string concatenation which depends on $@ is later $@. | impl/unsafeCode.rb:54:21:54:21 | x | library input | impl/unsafeCode.rb:56:5:56:13 | call to eval | interpreted as code |
| impl/unsafeCode.rb:61:10:61:12 | arr | impl/unsafeCode.rb:59:21:59:21 | x : | impl/unsafeCode.rb:61:10:61:12 | arr | This array which depends on $@ is later $@. | impl/unsafeCode.rb:59:21:59:21 | x | library input | impl/unsafeCode.rb:61:5:61:23 | call to eval | interpreted as code |
| impl/unsafeCode.rb:64:10:64:13 | arr2 | impl/unsafeCode.rb:59:24:59:24 | y : | impl/unsafeCode.rb:64:10:64:13 | arr2 | This array which depends on $@ is later $@. | impl/unsafeCode.rb:59:24:59:24 | y | library input | impl/unsafeCode.rb:64:5:64:25 | call to eval | interpreted as code |
8 changes: 8 additions & 0 deletions ruby/ql/test/query-tests/security/cwe-094/UnsafeCodeConstruction/impl/unsafeCode.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,12 @@ def string_concat(x)
foo = "foo = " + x
eval(foo) # NOT OK
end

def join_indirect(x, y)
arr = Array(x)
eval(arr.join(" ")) # NOT OK

arr2 = [Array(["foo = ", y]).join(" ")]
eval(arr2.join("\n")) # NOT OK
end
end