Skip to content

Commit 557dd10

Browse files
erik-krogherik-krogh
committed
add a rb/unsafe-shell-command-construction query
1 parent 0d5da42 commit 557dd10

File tree

10 files changed

+278
-0
lines changed

10 files changed

+278
-0
lines changed

‎ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for reasoning about
3+
* shell command constructed from library input vulnerabilities, as
4+
* well as extension points for adding your own.
5+
*/
6+
7+
private import codeql.ruby.DataFlow
8+
private import codeql.ruby.DataFlow2
9+
private import codeql.ruby.ApiGraphs
10+
private import codeql.ruby.frameworks.core.Gem::Gem as Gem
11+
private import codeql.ruby.AST as Ast
12+
private import codeql.ruby.Concepts as Concepts
13+
14+
module UnsafeShellCommandConstruction {
15+
/** A source for shell command constructed from library input vulnerabilities. */
16+
abstract class Source extends DataFlow::Node { }
17+
18+
/** An input parameter to a gem seen as a source. */
19+
private class LibraryInputAsSource extends Source instanceof DataFlow::ParameterNode {
20+
LibraryInputAsSource() {
21+
this = Gem::getALibraryInput() and
22+
// we exclude arguments named `cmd` or similar, as they seem to execute commands on purpose
23+
not exists(string name | name = super.getName() |
24+
name = ["cmd", "command"]
25+
or
26+
name.regexpMatch(".*(Cmd|Command)$")
27+
)
28+
}
29+
}
30+
31+
/** A sink for shell command constructed from library input vulnerabilities. */
32+
abstract class Sink extends DataFlow::Node {
33+
/** Gets a description of how the string in this sink was constructed. */
34+
abstract string describe();
35+
36+
/** Gets the dataflow node where the string is constructed. */
37+
DataFlow::Node getStringConstruction() { result = this }
38+
39+
/** Gets the dataflow node that executed the string as a shell command. */
40+
abstract DataFlow::Node getCommandExecution();
41+
}
42+
43+
/** A dataflow-configuration for tracking flow from various string constructions to places where those strings are executed as shell commands. */
44+
class TrackSystemCommand extends DataFlow2::Configuration {
45+
TrackSystemCommand() { this = "StringConcatAsSink::TrackSystemCommand" }
46+
47+
override predicate isSource(DataFlow::Node source) {
48+
source instanceof TaintedFormat::PrintfStyleCall
49+
or
50+
source.asExpr().getExpr() =
51+
any(Ast::StringLiteral lit |
52+
lit.getComponent(_) instanceof Ast::StringInterpolationComponent
53+
)
54+
}
55+
56+
override predicate isSink(DataFlow::Node sink) {
57+
exists(Concepts::SystemCommandExecution s | s.isShellInterpreted(sink))
58+
}
59+
}
60+
61+
/** Holds if the string constructed at `source` is executed at `shellExec` */
62+
predicate isUsedAsShellCommand(DataFlow::Node source, Concepts::SystemCommandExecution shellExec) {
63+
any(TrackSystemCommand conf)
64+
.hasFlow(source, any(DataFlow::Node arg | shellExec.isShellInterpreted(arg)))
65+
}
66+
67+
/**
68+
* A string constructed from a string-literal (e.g. `"foo #{sink}"`),
69+
* where the resulting string ends up being executed as a shell command.
70+
*/
71+
class StringFormatAsSink extends Sink {
72+
Concepts::SystemCommandExecution s;
73+
Ast::StringLiteral lit;
74+
75+
StringFormatAsSink() {
76+
isUsedAsShellCommand(any(DataFlow::Node n | n.asExpr().getExpr() = lit), s) and
77+
this.asExpr().getExpr() = lit.getComponent(_)
78+
}
79+
80+
override string describe() { result = "string construction" }
81+
82+
override DataFlow::Node getCommandExecution() { result = s }
83+
84+
override DataFlow::Node getStringConstruction() { result.asExpr().getExpr() = lit }
85+
}
86+
87+
import codeql.ruby.security.TaintedFormatStringSpecific as TaintedFormat
88+
89+
/**
90+
* A string constructed from a printf-style call,
91+
* where the resulting string ends up being executed as a shell command.
92+
*/
93+
class TaintedFormatStringAsSink extends Sink {
94+
Concepts::SystemCommandExecution s;
95+
TaintedFormat::PrintfStyleCall call;
96+
97+
TaintedFormatStringAsSink() {
98+
isUsedAsShellCommand(call, s) and
99+
this = [call.getFormatArgument(_), call.getFormatString()]
100+
}
101+
102+
override string describe() { result = "formatted string" }
103+
104+
override DataFlow::Node getCommandExecution() { result = s }
105+
106+
override DataFlow::Node getStringConstruction() { result = call }
107+
}
108+
}

‎ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
/**
2+
* Provides a taint tracking configuration for reasoning about shell command
3+
* constructed from library input vulnerabilities
4+
*
5+
* Note, for performance reasons: only import this file if `Configuration` is needed,
6+
* otherwise `UnsafeShellCommandConstructionCustomizations` should be imported instead.
7+
*/
8+
9+
import codeql.ruby.DataFlow
10+
import UnsafeShellCommandConstructionCustomizations::UnsafeShellCommandConstruction
11+
private import codeql.ruby.TaintTracking
12+
private import CommandInjectionCustomizations::CommandInjection as CommandInjection
13+
private import codeql.ruby.dataflow.BarrierGuards
14+
15+
/**
16+
* A taint-tracking configuration for detecting shell command constructed from library input vulnerabilities.
17+
*/
18+
class Configuration extends TaintTracking::Configuration {
19+
Configuration() { this = "UnsafeShellCommandConstruction" }
20+
21+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
22+
23+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
24+
25+
override predicate isSanitizer(DataFlow::Node node) {
26+
node instanceof CommandInjection::Sanitizer or // using all sanitizers from `rb/command-injection`
27+
node instanceof StringConstCompareBarrier or
28+
node instanceof StringConstArrayInclusionCallBarrier
29+
}
30+
31+
// override to require the path doesn't have unmatched return steps
32+
override DataFlow::FlowFeature getAFeature() {
33+
result instanceof DataFlow::FeatureHasSourceCallContext
34+
}
35+
}

‎ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.ql

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
/**
2+
* @name Unsafe shell command constructed from library input
3+
* @description Using externally controlled strings in a command line may allow a malicious
4+
* user to change the meaning of the command.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @security-severity 6.3
8+
* @precision high
9+
* @id rb/shell-command-constructed-from-input
10+
* @tags correctness
11+
* security
12+
* external/cwe/cwe-078
13+
* external/cwe/cwe-088
14+
* external/cwe/cwe-073
15+
*/
16+
17+
import codeql.ruby.security.UnsafeShellCommandConstructionQuery
18+
import DataFlow::PathGraph
19+
20+
from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode
21+
where
22+
config.hasFlowPath(source, sink) and
23+
sinkNode = sink.getNode()
24+
select sinkNode.getStringConstruction(), source, sink,
25+
"This " + sinkNode.describe() + " which depends on $@ is later used in a $@.", source.getNode(),
26+
"library input", sinkNode.getCommandExecution(), "shell command"

‎ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
edges
2+
| impl/sub/notImported.rb:2:12:2:17 | target : | impl/sub/notImported.rb:3:19:3:27 | #{...} |
3+
| impl/sub/other2.rb:2:12:2:17 | target : | impl/sub/other2.rb:3:19:3:27 | #{...} |
4+
| impl/sub/other.rb:2:12:2:17 | target : | impl/sub/other.rb:3:19:3:27 | #{...} |
5+
| impl/unsafeShell.rb:2:12:2:17 | target : | impl/unsafeShell.rb:3:19:3:27 | #{...} |
6+
| impl/unsafeShell.rb:6:12:6:12 | x : | impl/unsafeShell.rb:7:32:7:32 | x |
7+
| impl/unsafeShell.rb:15:47:15:64 | innocent_file_path : | impl/unsafeShell.rb:20:21:20:41 | #{...} |
8+
| impl/unsafeShell.rb:23:15:23:23 | file_path : | impl/unsafeShell.rb:26:19:26:30 | #{...} |
9+
| impl/unsafeShell.rb:33:12:33:17 | target : | impl/unsafeShell.rb:34:19:34:27 | #{...} |
10+
| impl/unsafeShell.rb:37:10:37:10 | x : | impl/unsafeShell.rb:38:19:38:22 | #{...} |
11+
nodes
12+
| impl/sub/notImported.rb:2:12:2:17 | target : | semmle.label | target : |
13+
| impl/sub/notImported.rb:3:19:3:27 | #{...} | semmle.label | #{...} |
14+
| impl/sub/other2.rb:2:12:2:17 | target : | semmle.label | target : |
15+
| impl/sub/other2.rb:3:19:3:27 | #{...} | semmle.label | #{...} |
16+
| impl/sub/other.rb:2:12:2:17 | target : | semmle.label | target : |
17+
| impl/sub/other.rb:3:19:3:27 | #{...} | semmle.label | #{...} |
18+
| impl/unsafeShell.rb:2:12:2:17 | target : | semmle.label | target : |
19+
| impl/unsafeShell.rb:3:19:3:27 | #{...} | semmle.label | #{...} |
20+
| impl/unsafeShell.rb:6:12:6:12 | x : | semmle.label | x : |
21+
| impl/unsafeShell.rb:7:32:7:32 | x | semmle.label | x |
22+
| impl/unsafeShell.rb:15:47:15:64 | innocent_file_path : | semmle.label | innocent_file_path : |
23+
| impl/unsafeShell.rb:20:21:20:41 | #{...} | semmle.label | #{...} |
24+
| impl/unsafeShell.rb:23:15:23:23 | file_path : | semmle.label | file_path : |
25+
| impl/unsafeShell.rb:26:19:26:30 | #{...} | semmle.label | #{...} |
26+
| impl/unsafeShell.rb:33:12:33:17 | target : | semmle.label | target : |
27+
| impl/unsafeShell.rb:34:19:34:27 | #{...} | semmle.label | #{...} |
28+
| impl/unsafeShell.rb:37:10:37:10 | x : | semmle.label | x : |
29+
| impl/unsafeShell.rb:38:19:38:22 | #{...} | semmle.label | #{...} |
30+
subpaths
31+
#select
32+
| 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 |
33+
| impl/sub/other2.rb:3:14:3:28 | "cat #{...}" | impl/sub/other2.rb:2:12:2:17 | target : | impl/sub/other2.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/sub/other2.rb:2:12:2:17 | target | library input | impl/sub/other2.rb:3:5:3:34 | call to popen | shell command |
34+
| impl/sub/other.rb:3:14:3:28 | "cat #{...}" | impl/sub/other.rb:2:12:2:17 | target : | impl/sub/other.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/sub/other.rb:2:12:2:17 | target | library input | impl/sub/other.rb:3:5:3:34 | call to popen | shell command |
35+
| impl/unsafeShell.rb:3:14:3:28 | "cat #{...}" | impl/unsafeShell.rb:2:12:2:17 | target : | impl/unsafeShell.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:2:12:2:17 | target | library input | impl/unsafeShell.rb:3:5:3:34 | call to popen | shell command |
36+
| impl/unsafeShell.rb:7:14:7:33 | call to sprintf | impl/unsafeShell.rb:6:12:6:12 | x : | impl/unsafeShell.rb:7:32:7:32 | x | This formatted string which depends on $@ is later used in a $@. | impl/unsafeShell.rb:6:12:6:12 | x | library input | impl/unsafeShell.rb:8:5:8:25 | call to popen | shell command |
37+
| impl/unsafeShell.rb:20:14:20:42 | "which #{...}" | impl/unsafeShell.rb:15:47:15:64 | innocent_file_path : | impl/unsafeShell.rb:20:21:20:41 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:15:47:15:64 | innocent_file_path | library input | impl/unsafeShell.rb:20:5:20:48 | call to popen | shell command |
38+
| impl/unsafeShell.rb:26:14:26:31 | "cat #{...}" | impl/unsafeShell.rb:23:15:23:23 | file_path : | impl/unsafeShell.rb:26:19:26:30 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:23:15:23:23 | file_path | library input | impl/unsafeShell.rb:26:5:26:37 | call to popen | shell command |
39+
| impl/unsafeShell.rb:34:14:34:28 | "cat #{...}" | impl/unsafeShell.rb:33:12:33:17 | target : | impl/unsafeShell.rb:34:19:34:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:33:12:33:17 | target | library input | impl/unsafeShell.rb:34:5:34:34 | call to popen | shell command |
40+
| impl/unsafeShell.rb:38:14:38:23 | "cat #{...}" | impl/unsafeShell.rb:37:10:37:10 | x : | impl/unsafeShell.rb:38:19:38:22 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:37:10:37:10 | x | library input | impl/unsafeShell.rb:38:5:38:29 | call to popen | shell command |

‎ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
queries/security/cwe-078/UnsafeShellCommandConstruction.ql

‎ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/notImported.rb

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
class Foobar
2+
def foo1(target)
3+
IO.popen("cat #{target}", "w") # NOT OK - everything assumed to be imported...
4+
end
5+
end
6+

‎ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other.rb

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
class Foobar
2+
def foo1(target)
3+
IO.popen("cat #{target}", "w") # NOT OK
4+
end
5+
end
6+
7+
require 'sub/other2'

‎ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other2.rb

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
class Foobar
2+
def foo1(target)
3+
IO.popen("cat #{target}", "w") # NOT OK
4+
end
5+
end

‎ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
class Foobar
2+
def foo1(target)
3+
IO.popen("cat #{target}", "w") # NOT OK
4+
end
5+
6+
def foo2(x)
7+
format = sprintf("cat %s", x) # NOT OK
8+
IO.popen(format, "w")
9+
end
10+
11+
def fileRead1(path)
12+
File.read(path) # OK
13+
end
14+
15+
def my_exec(cmd, command, myCmd, myCommand, innocent_file_path)
16+
IO.popen("which #{cmd}", "w") # OK - the parameter is named `cmd`, so it's meant to be a command
17+
IO.popen("which #{command}", "w") # OK - the parameter is named `command`, so it's meant to be a command
18+
IO.popen("which #{myCmd}", "w") # OK - the parameter is named `myCmd`, so it's meant to be a command
19+
IO.popen("which #{myCommand}", "w") # OK - the parameter is named `myCommand`, so it's meant to be a command
20+
IO.popen("which #{innocent_file_path}", "w") # NOT OK - the parameter is named `innocent_file_path`, so it's not meant to be a command
21+
end
22+
23+
def escaped(file_path)
24+
IO.popen("cat #{file_path.shellescape}", "w") # OK - the parameter is escaped
25+
26+
IO.popen("cat #{file_path}", "w") # NOT OK - the parameter is not escaped
27+
end
28+
end
29+
30+
require File.join(File.dirname(__FILE__), 'sub', 'other')
31+
32+
class Foobar2
33+
def foo1(target)
34+
IO.popen("cat #{target}", "w") # NOT OK
35+
end
36+
37+
def id(x)
38+
IO.popen("cat #{x}", "w") # NOT OK - the parameter is not a constant.
39+
return x
40+
end
41+
42+
def thisIsSafe()
43+
IO.popen("echo #{id('foo')}", "w") # OK - only using constants.
44+
end
45+
end

‎ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/unsafe-shell.gemspec

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
Gem::Specification.new do |s|
2+
s.name = 'unsafe-shell'
3+
s.require_path = "impl"
4+
end
5+

0 commit comments

Comments
 (0)