diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll index 7b56f2e6a93..5a6883a5fe4 100644 --- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll +++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll @@ -147,6 +147,9 @@ class ExprNode extends Node, TExprNode { class ParameterNode extends Node, TParameterNode instanceof ParameterNodeImpl { /** Gets the parameter corresponding to this node, if any. */ final Parameter getParameter() { result = super.getParameter() } + + /** Gets the name of the parameter, if any. */ + final string getName() { result = this.getParameter().(NamedParameter).getName() } } /** diff --git a/ruby/ql/lib/codeql/ruby/frameworks/Core.qll b/ruby/ql/lib/codeql/ruby/frameworks/Core.qll index 0e3336f81d6..6411dad64b5 100644 --- a/ruby/ql/lib/codeql/ruby/frameworks/Core.qll +++ b/ruby/ql/lib/codeql/ruby/frameworks/Core.qll @@ -7,6 +7,7 @@ private import codeql.ruby.DataFlow private import codeql.ruby.dataflow.FlowSummary import core.BasicObject::BasicObject import core.Object::Object +import core.Gem::Gem import core.Kernel::Kernel import core.Module import core.Array diff --git a/ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll b/ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll new file mode 100644 index 00000000000..9e417ad371a --- /dev/null +++ b/ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll @@ -0,0 +1,102 @@ +/** + * Provides modeling for the `Gem` module and `.gemspec` files. + */ + +private import ruby +private import Ast +private import codeql.ruby.ApiGraphs + +/** Provides modeling for the `Gem` module and `.gemspec` files. */ +module Gem { + /** + * A .gemspec file that lists properties of a Ruby gem. + * The contents of a .gemspec file generally look like: + * ```Ruby + * Gem::Specification.new do |s| + * s.name = 'library-name' + * s.require_path = "lib" + * # more properties + * end + * ``` + */ + class GemSpec instanceof File { + API::Node specCall; + + GemSpec() { + this.getExtension() = "gemspec" and + specCall = API::root().getMember("Gem").getMember("Specification").getMethod("new") and + specCall.getLocation().getFile() = this + } + + /** Gets the name of this .gemspec file. */ + string toString() { result = File.super.getBaseName() } + + /** + * Gets a value of the `name` property of this .gemspec file. + * These properties are set using the `Gem::Specification.new` method. + */ + private Expr getSpecProperty(string name) { + exists(Expr rhs | + rhs = + specCall + .getBlock() + .getParameter(0) + .getMethod(name + "=") + .getParameter(0) + .asSink() + .asExpr() + .getExpr() + .(Ast::AssignExpr) + .getRightOperand() + | + result = rhs + or + // some properties are arrays, we just unfold them + result = rhs.(ArrayLiteral).getAnElement() + ) + } + + /** Gets the name of the gem */ + string getName() { result = getSpecProperty("name").getConstantValue().getString() } + + /** Gets a path that is loaded when the gem is required */ + private string getARequirePath() { + result = getSpecProperty(["require_paths", "require_path"]).getConstantValue().getString() + or + not exists(getSpecProperty(["require_paths", "require_path"]).getConstantValue().getString()) and + result = "lib" // the default is "lib" + } + + /** Gets a file that could be loaded when the gem is required. */ + private File getAPossiblyRequiredFile() { + result = File.super.getParentContainer().getFolder(getARequirePath()).getAChildContainer*() + } + + /** Gets a class/module that is exported by this gem. */ + private ModuleBase getAPublicModule() { + result.(Toplevel).getLocation().getFile() = getAPossiblyRequiredFile() + or + result = getAPublicModule().getAModule() + or + result = getAPublicModule().getAClass() + or + result = getAPublicModule().getStmt(_).(SingletonClass) + } + + /** Gets a parameter from an exported method, which is an input to this gem. */ + DataFlow::ParameterNode getAnInputParameter() { + exists(MethodBase method | method = getAPublicModule().getAMethod() | + result.getParameter() = method.getAParameter() and + method.isPublic() + ) + } + } + + /** Gets a parameter that is an input to a named gem. */ + DataFlow::ParameterNode getALibraryInput() { + exists(GemSpec spec | + exists(spec.getName()) and // we only consider `.gemspec` files that have a name + result = spec.getAnInputParameter() + ) + } +} diff --git a/ruby/ql/lib/codeql/ruby/security/CommandInjectionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/CommandInjectionCustomizations.qll index e67ae044a00..20cc8d5b26b 100644 --- a/ruby/ql/lib/codeql/ruby/security/CommandInjectionCustomizations.qll +++ b/ruby/ql/lib/codeql/ruby/security/CommandInjectionCustomizations.qll @@ -49,6 +49,9 @@ module CommandInjection { class ShellwordsEscapeAsSanitizer extends Sanitizer { ShellwordsEscapeAsSanitizer() { this = API::getTopLevelMember("Shellwords").getAMethodCall(["escape", "shellescape"]) + or + // The method is also added as `String#shellescape`. + this.(DataFlow::CallNode).getMethodName() = "shellescape" } } } diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll new file mode 100644 index 00000000000..515b563c1e1 --- /dev/null +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll @@ -0,0 +1,105 @@ +/** + * Provides default sources, sinks and sanitizers for reasoning about + * shell command constructed from library input vulnerabilities, as + * well as extension points for adding your own. + */ + +private import codeql.ruby.DataFlow +private import codeql.ruby.DataFlow2 +private import codeql.ruby.ApiGraphs +private import codeql.ruby.frameworks.core.Gem::Gem as Gem +private import codeql.ruby.AST as Ast +private import codeql.ruby.Concepts as Concepts + +/** + * Module containing sources, sinks, and sanitizers for shell command constructed from library input. + */ +module UnsafeShellCommandConstruction { + /** A source for shell command constructed from library input vulnerabilities. */ + abstract class Source extends DataFlow::Node { } + + /** An input parameter to a gem seen as a source. */ + private class LibraryInputAsSource extends Source instanceof DataFlow::ParameterNode { + LibraryInputAsSource() { + this = Gem::getALibraryInput() and + // we exclude arguments named `cmd` or similar, as they seem to execute commands on purpose + not exists(string name | name = super.getName() | + name = ["cmd", "command"] + or + name.regexpMatch(".*(Cmd|Command)$") + ) + } + } + + /** A sink for shell command constructed from library input vulnerabilities. */ + abstract class Sink extends DataFlow::Node { + /** Gets a description of how the string in this sink was constructed. */ + abstract string describe(); + + /** Gets the dataflow node where the string is constructed. */ + DataFlow::Node getStringConstruction() { result = this } + + /** Gets the dataflow node that executed the string as a shell command. */ + abstract DataFlow::Node getCommandExecution(); + } + + /** Holds if the string constructed at `source` is executed at `shellExec` */ + predicate isUsedAsShellCommand(DataFlow::Node source, Concepts::SystemCommandExecution shellExec) { + source = backtrackShellExec(TypeTracker::TypeBackTracker::end(), shellExec) + } + + import codeql.ruby.typetracking.TypeTracker as TypeTracker + + private DataFlow::LocalSourceNode backtrackShellExec( + TypeTracker::TypeBackTracker t, Concepts::SystemCommandExecution shellExec + ) { + t.start() and + result = any(DataFlow::Node n | shellExec.isShellInterpreted(n)).getALocalSource() + or + exists(TypeTracker::TypeBackTracker t2 | + result = backtrackShellExec(t2, shellExec).backtrack(t2, t) + ) + } + + /** + * A string constructed from a string-literal (e.g. `"foo #{sink}"`), + * where the resulting string ends up being executed as a shell command. + */ + class StringInterpolationAsSink extends Sink { + Concepts::SystemCommandExecution s; + Ast::StringLiteral lit; + + StringInterpolationAsSink() { + isUsedAsShellCommand(any(DataFlow::Node n | n.asExpr().getExpr() = lit), s) and + this.asExpr().getExpr() = lit.getComponent(_) + } + + override string describe() { result = "string construction" } + + override DataFlow::Node getCommandExecution() { result = s } + + override DataFlow::Node getStringConstruction() { result.asExpr().getExpr() = lit } + } + + import codeql.ruby.security.TaintedFormatStringSpecific as TaintedFormat + + /** + * A string constructed from a printf-style call, + * where the resulting string ends up being executed as a shell command. + */ + class TaintedFormatStringAsSink extends Sink { + Concepts::SystemCommandExecution s; + TaintedFormat::PrintfStyleCall call; + + TaintedFormatStringAsSink() { + isUsedAsShellCommand(call, s) and + this = [call.getFormatArgument(_), call.getFormatString()] + } + + override string describe() { result = "formatted string" } + + override DataFlow::Node getCommandExecution() { result = s } + + override DataFlow::Node getStringConstruction() { result = call } + } +} diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll new file mode 100644 index 00000000000..87b602911ae --- /dev/null +++ b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll @@ -0,0 +1,35 @@ +/** + * Provides a taint tracking configuration for reasoning about shell command + * constructed from library input vulnerabilities + * + * Note, for performance reasons: only import this file if `Configuration` is needed, + * otherwise `UnsafeShellCommandConstructionCustomizations` should be imported instead. + */ + +import codeql.ruby.DataFlow +import UnsafeShellCommandConstructionCustomizations::UnsafeShellCommandConstruction +private import codeql.ruby.TaintTracking +private import CommandInjectionCustomizations::CommandInjection as CommandInjection +private import codeql.ruby.dataflow.BarrierGuards + +/** + * A taint-tracking configuration for detecting shell command constructed from library input vulnerabilities. + */ +class Configuration extends TaintTracking::Configuration { + Configuration() { this = "UnsafeShellCommandConstruction" } + + override predicate isSource(DataFlow::Node source) { source instanceof Source } + + override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + + override predicate isSanitizer(DataFlow::Node node) { + node instanceof CommandInjection::Sanitizer or // using all sanitizers from `rb/command-injection` + node instanceof StringConstCompareBarrier or + node instanceof StringConstArrayInclusionCallBarrier + } + + // override to require the path doesn't have unmatched return steps + override DataFlow::FlowFeature getAFeature() { + result instanceof DataFlow::FeatureHasSourceCallContext + } +} diff --git a/ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md b/ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md new file mode 100644 index 00000000000..fba6a9304cf --- /dev/null +++ b/ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md @@ -0,0 +1,4 @@ +--- +category: newQuery +--- +* Added a new query, `rb/shell-command-constructed-from-input`, to detect libraries that unsafely construct shell commands from their inputs. diff --git a/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp b/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp new file mode 100644 index 00000000000..4231f7cb0b4 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp @@ -0,0 +1,73 @@ + + + +

+ Dynamically constructing a shell command with inputs from exported + functions may inadvertently change the meaning of the shell command. + + Clients using the exported function may use inputs containing + characters that the shell interprets in a special way, for instance + quotes and spaces. + + This can result in the shell command misbehaving, or even + allowing a malicious user to execute arbitrary commands on the system. +

+ + +
+ + +

+ If possible, provide the dynamic arguments to the shell as an array + to APIs such as system(..) to avoid interpretation by the shell. +

+ +

+ Alternatively, if the shell command must be constructed + dynamically, then add code to ensure that special characters + do not alter the shell command unexpectedly. +

+ +
+ + +

+ The following example shows a dynamically constructed shell + command that downloads a file from a remote URL. +

+ + + +

+ The shell command will, however, fail to work as intended if the + input contains spaces or other special characters interpreted in a + special way by the shell. +

+ +

+ Even worse, a client might pass in user-controlled + data, not knowing that the input is interpreted as a shell command. + This could allow a malicious user to provide the input http://example.org; cat /etc/passwd + in order to execute the command cat /etc/passwd. +

+ +

+ To avoid such potentially catastrophic behaviors, provide the + input from exported functions as an argument that does not + get interpreted by a shell: +

+ + + +
+ + +
  • + OWASP: + Command Injection. +
  • + +
    +
    diff --git a/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.ql b/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.ql new file mode 100644 index 00000000000..53c71cfc314 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.ql @@ -0,0 +1,26 @@ +/** + * @name Unsafe shell command constructed from library input + * @description Using externally controlled strings in a command line may allow a malicious + * user to change the meaning of the command. + * @kind path-problem + * @problem.severity error + * @security-severity 6.3 + * @precision high + * @id rb/shell-command-constructed-from-input + * @tags correctness + * security + * external/cwe/cwe-078 + * external/cwe/cwe-088 + * external/cwe/cwe-073 + */ + +import codeql.ruby.security.UnsafeShellCommandConstructionQuery +import DataFlow::PathGraph + +from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode +where + config.hasFlowPath(source, sink) and + sinkNode = sink.getNode() +select sinkNode.getStringConstruction(), source, sink, + "This " + sinkNode.describe() + " which depends on $@ is later used in a $@.", source.getNode(), + "library input", sinkNode.getCommandExecution(), "shell command" diff --git a/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction.rb b/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction.rb new file mode 100644 index 00000000000..500bd49e890 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction.rb @@ -0,0 +1,5 @@ +module Utils + def download(path) + system("wget #{path}") # NOT OK + end +end \ No newline at end of file diff --git a/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction_fixed.rb b/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction_fixed.rb new file mode 100644 index 00000000000..cb8730eee09 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction_fixed.rb @@ -0,0 +1,6 @@ +module Utils + def download(path) + # using an array to call `system` is safe + system("wget", path) # OK + end +end \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected index b2e4990daec..1d00f293dfd 100644 --- a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected +++ b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected @@ -15,6 +15,8 @@ edges | CommandInjection.rb:81:20:81:25 | **args : | CommandInjection.rb:82:22:82:25 | args : | | CommandInjection.rb:82:22:82:25 | args : | CommandInjection.rb:82:22:82:37 | ...[...] : | | CommandInjection.rb:82:22:82:37 | ...[...] : | CommandInjection.rb:82:14:82:39 | "echo #{...}" | +| CommandInjection.rb:94:16:94:21 | call to params : | CommandInjection.rb:94:16:94:28 | ...[...] : | +| CommandInjection.rb:94:16:94:28 | ...[...] : | CommandInjection.rb:95:16:95:28 | "cat #{...}" | nodes | CommandInjection.rb:6:15:6:20 | call to params : | semmle.label | call to params : | | CommandInjection.rb:6:15:6:26 | ...[...] : | semmle.label | ...[...] : | @@ -37,6 +39,9 @@ nodes | CommandInjection.rb:82:14:82:39 | "echo #{...}" | semmle.label | "echo #{...}" | | CommandInjection.rb:82:22:82:25 | args : | semmle.label | args : | | CommandInjection.rb:82:22:82:37 | ...[...] : | semmle.label | ...[...] : | +| CommandInjection.rb:94:16:94:21 | call to params : | semmle.label | call to params : | +| CommandInjection.rb:94:16:94:28 | ...[...] : | semmle.label | ...[...] : | +| CommandInjection.rb:95:16:95:28 | "cat #{...}" | semmle.label | "cat #{...}" | subpaths #select | CommandInjection.rb:7:10:7:15 | #{...} | CommandInjection.rb:6:15:6:20 | call to params : | CommandInjection.rb:7:10:7:15 | #{...} | This command depends on a $@. | CommandInjection.rb:6:15:6:20 | call to params | user-provided value | @@ -51,3 +56,4 @@ subpaths | CommandInjection.rb:65:14:65:29 | "echo #{...}" | CommandInjection.rb:64:18:64:23 | number : | CommandInjection.rb:65:14:65:29 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:64:18:64:23 | number | user-provided value | | CommandInjection.rb:73:14:73:34 | "echo #{...}" | CommandInjection.rb:72:23:72:33 | blah_number : | CommandInjection.rb:73:14:73:34 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:72:23:72:33 | blah_number | user-provided value | | CommandInjection.rb:82:14:82:39 | "echo #{...}" | CommandInjection.rb:81:20:81:25 | **args : | CommandInjection.rb:82:14:82:39 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:81:20:81:25 | **args | user-provided value | +| CommandInjection.rb:95:16:95:28 | "cat #{...}" | CommandInjection.rb:94:16:94:21 | call to params : | CommandInjection.rb:95:16:95:28 | "cat #{...}" | This command depends on a $@. | CommandInjection.rb:94:16:94:21 | call to params | user-provided value | diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb index ed9750128cc..75219e2131c 100644 --- a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb +++ b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb @@ -88,3 +88,13 @@ module Types end end end + +class Foo < ActionController::Base + def create + file = params[:file] + system("cat #{file}") + # .shellescape + system("cat #{file.shellescape}") # OK, because file is shell escaped + + end +end \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected new file mode 100644 index 00000000000..964fb4433ce --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected @@ -0,0 +1,44 @@ +edges +| impl/sub/notImported.rb:2:12:2:17 | target : | impl/sub/notImported.rb:3:19:3:27 | #{...} | +| impl/sub/other2.rb:2:12:2:17 | target : | impl/sub/other2.rb:3:19:3:27 | #{...} | +| impl/sub/other.rb:2:12:2:17 | target : | impl/sub/other.rb:3:19:3:27 | #{...} | +| impl/unsafeShell.rb:2:12:2:17 | target : | impl/unsafeShell.rb:3:19:3:27 | #{...} | +| impl/unsafeShell.rb:6:12:6:12 | x : | impl/unsafeShell.rb:7:32:7:32 | x | +| impl/unsafeShell.rb:15:47:15:64 | innocent_file_path : | impl/unsafeShell.rb:20:21:20:41 | #{...} | +| impl/unsafeShell.rb:23:15:23:23 | file_path : | impl/unsafeShell.rb:26:19:26:30 | #{...} | +| impl/unsafeShell.rb:33:12:33:17 | target : | impl/unsafeShell.rb:34:19:34:27 | #{...} | +| impl/unsafeShell.rb:37:10:37:10 | x : | impl/unsafeShell.rb:38:19:38:22 | #{...} | +| impl/unsafeShell.rb:47:16:47:21 | target : | impl/unsafeShell.rb:48:19:48:27 | #{...} | +nodes +| impl/sub/notImported.rb:2:12:2:17 | target : | semmle.label | target : | +| impl/sub/notImported.rb:3:19:3:27 | #{...} | semmle.label | #{...} | +| impl/sub/other2.rb:2:12:2:17 | target : | semmle.label | target : | +| impl/sub/other2.rb:3:19:3:27 | #{...} | semmle.label | #{...} | +| impl/sub/other.rb:2:12:2:17 | target : | semmle.label | target : | +| impl/sub/other.rb:3:19:3:27 | #{...} | semmle.label | #{...} | +| impl/unsafeShell.rb:2:12:2:17 | target : | semmle.label | target : | +| impl/unsafeShell.rb:3:19:3:27 | #{...} | semmle.label | #{...} | +| impl/unsafeShell.rb:6:12:6:12 | x : | semmle.label | x : | +| impl/unsafeShell.rb:7:32:7:32 | x | semmle.label | x | +| impl/unsafeShell.rb:15:47:15:64 | innocent_file_path : | semmle.label | innocent_file_path : | +| impl/unsafeShell.rb:20:21:20:41 | #{...} | semmle.label | #{...} | +| impl/unsafeShell.rb:23:15:23:23 | file_path : | semmle.label | file_path : | +| impl/unsafeShell.rb:26:19:26:30 | #{...} | semmle.label | #{...} | +| impl/unsafeShell.rb:33:12:33:17 | target : | semmle.label | target : | +| impl/unsafeShell.rb:34:19:34:27 | #{...} | semmle.label | #{...} | +| impl/unsafeShell.rb:37:10:37:10 | x : | semmle.label | x : | +| impl/unsafeShell.rb:38:19:38:22 | #{...} | semmle.label | #{...} | +| impl/unsafeShell.rb:47:16:47:21 | target : | semmle.label | target : | +| impl/unsafeShell.rb:48:19:48:27 | #{...} | semmle.label | #{...} | +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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| impl/unsafeShell.rb:48:14:48:28 | "cat #{...}" | impl/unsafeShell.rb:47:16:47:21 | target : | impl/unsafeShell.rb:48:19:48:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:47:16:47:21 | target | library input | impl/unsafeShell.rb:48:5:48:34 | call to popen | shell command | diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.qlref b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.qlref new file mode 100644 index 00000000000..99292da7663 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.qlref @@ -0,0 +1 @@ +queries/security/cwe-078/UnsafeShellCommandConstruction.ql \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/notImported.rb b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/notImported.rb new file mode 100644 index 00000000000..0a385f5f6bc --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/notImported.rb @@ -0,0 +1,6 @@ +class Foobar + def foo1(target) + IO.popen("cat #{target}", "w") # NOT OK - everything assumed to be imported... + end +end + \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other.rb b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other.rb new file mode 100644 index 00000000000..22eaa13bcc0 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other.rb @@ -0,0 +1,7 @@ +class Foobar + def foo1(target) + IO.popen("cat #{target}", "w") # NOT OK + end +end + +require 'sub/other2' \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other2.rb b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other2.rb new file mode 100644 index 00000000000..007dae343ff --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other2.rb @@ -0,0 +1,5 @@ +class Foobar + def foo1(target) + IO.popen("cat #{target}", "w") # NOT OK + end +end \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb new file mode 100644 index 00000000000..f1fd9f685a8 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb @@ -0,0 +1,50 @@ +class Foobar + def foo1(target) + IO.popen("cat #{target}", "w") # NOT OK + end + + def foo2(x) + format = sprintf("cat %s", x) # NOT OK + IO.popen(format, "w") + end + + def fileRead1(path) + File.read(path) # OK + end + + def my_exec(cmd, command, myCmd, myCommand, innocent_file_path) + IO.popen("which #{cmd}", "w") # OK - the parameter is named `cmd`, so it's meant to be a command + IO.popen("which #{command}", "w") # OK - the parameter is named `command`, so it's meant to be a command + IO.popen("which #{myCmd}", "w") # OK - the parameter is named `myCmd`, so it's meant to be a command + IO.popen("which #{myCommand}", "w") # OK - the parameter is named `myCommand`, so it's meant to be a command + 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 + end + + def escaped(file_path) + IO.popen("cat #{file_path.shellescape}", "w") # OK - the parameter is escaped + + IO.popen("cat #{file_path}", "w") # NOT OK - the parameter is not escaped + end +end + +require File.join(File.dirname(__FILE__), 'sub', 'other') + +class Foobar2 + def foo1(target) + IO.popen("cat #{target}", "w") # NOT OK + end + + def id(x) + IO.popen("cat #{x}", "w") # NOT OK - the parameter is not a constant. + return x + end + + def thisIsSafe() + IO.popen("echo #{id('foo')}", "w") # OK - only using constants. + end + + # class methods + def self.foo(target) + IO.popen("cat #{target}", "w") # NOT OK + end +end diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/unsafe-shell.gemspec b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/unsafe-shell.gemspec new file mode 100644 index 00000000000..545bc14a7a6 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/unsafe-shell.gemspec @@ -0,0 +1,5 @@ +Gem::Specification.new do |s| + s.name = 'unsafe-shell' + s.require_path = "impl" + end + \ No newline at end of file