diff --git a/ruby/ql/lib/codeql/ruby/security/KernelOpenQuery.qll b/ruby/ql/lib/codeql/ruby/security/KernelOpenQuery.qll new file mode 100644 index 000000000000..309a26ac9df8 --- /dev/null +++ b/ruby/ql/lib/codeql/ruby/security/KernelOpenQuery.qll @@ -0,0 +1,36 @@ +/** + * Provides utility classes and predicates for reasoning about `Kernel.open` and related methods. + */ + +private import codeql.ruby.AST +private import codeql.ruby.DataFlow +private import codeql.ruby.AST +private import codeql.ruby.ApiGraphs +private import codeql.ruby.frameworks.core.Kernel::Kernel + +/** A call to a method that might access a file or start a process. */ +class AmbiguousPathCall extends DataFlow::CallNode { + string name; + + AmbiguousPathCall() { + this.(KernelMethodCall).getMethodName() = "open" and + name = "Kernel.open" + or + this = API::getTopLevelMember("IO").getAMethodCall("read") and + not this = API::getTopLevelMember("File").getAMethodCall("read") and // needed in e.g. opal/opal, where some calls have both paths, but I'm not sure why + name = "IO.read" + } + + /** Gets the name for the method being called. */ + string getName() { result = name } + + /** Gets the name for a safer method that can be used instead. */ + string getReplacement() { + result = "File.read" and name = "IO.read" + or + result = "File.open" and name = "Kernel.open" + } + + /** Gets the argument that specifies the path to be accessed. */ + DataFlow::Node getPathArgument() { result = this.getArgument(0) } +} diff --git a/ruby/ql/src/change-notes/2022-10-06-non-constant-kernel-open.md b/ruby/ql/src/change-notes/2022-10-06-non-constant-kernel-open.md new file mode 100644 index 000000000000..b64f39305555 --- /dev/null +++ b/ruby/ql/src/change-notes/2022-10-06-non-constant-kernel-open.md @@ -0,0 +1,4 @@ +--- +category: newQuery +--- +* Added a new query, `rb/non-constant-kernel-open`, to detect uses of Kernel.open and related methods with non-constant values. \ No newline at end of file diff --git a/ruby/ql/src/queries/security/cwe-078/KernelOpen.inc.qhelp b/ruby/ql/src/queries/security/cwe-078/KernelOpen.inc.qhelp new file mode 100644 index 000000000000..eea2281cc4a2 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-078/KernelOpen.inc.qhelp @@ -0,0 +1,46 @@ + + + +

If Kernel.open is given a file name that starts with a | +character, it will execute the remaining string as a shell command. If a +malicious user can control the file name, they can execute arbitrary code. +The same vulnerability applies to IO.read. +

+ + + + +

Use File.open instead of Kernel.open, as the former +does not have this vulnerability. Similarly, use File.read instead +of IO.read.

+ + + + +

+The following example shows code that calls Kernel.open on a +user-supplied file path. +

+ + + +

Instead, File.open should be used, as in the following example.

+ + + + + + +
  • +OWASP: +Command Injection. +
    • + +
  • +Example CVE: Command Injection in RDoc. +
    • + +     +     diff --git a/ruby/ql/src/queries/security/cwe-078/KernelOpen.qhelp b/ruby/ql/src/queries/security/cwe-078/KernelOpen.qhelp index eea2281cc4a2..304aefcfbb24 100644 --- a/ruby/ql/src/queries/security/cwe-078/KernelOpen.qhelp +++ b/ruby/ql/src/queries/security/cwe-078/KernelOpen.qhelp @@ -1,46 +1,4 @@ - + - -

    If Kernel.open is given a file name that starts with a | -character, it will execute the remaining string as a shell command. If a -malicious user can control the file name, they can execute arbitrary code. -The same vulnerability applies to IO.read. -

    - -     - - -

    Use File.open instead of Kernel.open, as the former -does not have this vulnerability. Similarly, use File.read instead -of IO.read.

    - -     - - -

    -The following example shows code that calls Kernel.open on a -user-supplied file path. -

    - - - -

    Instead, File.open should be used, as in the following example.

    - - - -     - - -
  • -OWASP: -Command Injection. -
    • - -
  • -Example CVE: Command Injection in RDoc. -
    • - -     -     + + \ No newline at end of file diff --git a/ruby/ql/src/queries/security/cwe-078/KernelOpen.ql b/ruby/ql/src/queries/security/cwe-078/KernelOpen.ql index e2390944c1e1..6e03bcb06c26 100644 --- a/ruby/ql/src/queries/security/cwe-078/KernelOpen.ql +++ b/ruby/ql/src/queries/security/cwe-078/KernelOpen.ql @@ -1,5 +1,5 @@ /** - * @name Use of `Kernel.open` or `IO.read` + * @name Use of `Kernel.open` or `IO.read` with user-controlled input * @description Using `Kernel.open` or `IO.read` may allow a malicious * user to execute arbitrary system commands. * @kind path-problem @@ -14,39 +14,12 @@ * external/cwe/cwe-073 */ -import codeql.ruby.AST -import codeql.ruby.ApiGraphs -import codeql.ruby.frameworks.core.Kernel::Kernel +import codeql.ruby.DataFlow import codeql.ruby.TaintTracking -import codeql.ruby.dataflow.BarrierGuards import codeql.ruby.dataflow.RemoteFlowSources -import codeql.ruby.DataFlow +import codeql.ruby.dataflow.BarrierGuards import DataFlow::PathGraph - -/** - * A method call that has a suggested replacement. - */ -abstract class Replacement extends DataFlow::CallNode { - abstract string getFrom(); - - abstract string getTo(); -} - -class KernelOpenCall extends KernelMethodCall, Replacement { - KernelOpenCall() { this.getMethodName() = "open" } - - override string getFrom() { result = "Kernel.open" } - - override string getTo() { result = "File.open" } -} - -class IOReadCall extends DataFlow::CallNode, Replacement { - IOReadCall() { this = API::getTopLevelMember("IO").getAMethodCall("read") } - - override string getFrom() { result = "IO.read" } - - override string getTo() { result = "File.read" } -} +import codeql.ruby.security.KernelOpenQuery class Configuration extends TaintTracking::Configuration { Configuration() { this = "KernelOpen" } @@ -54,9 +27,7 @@ class Configuration extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource } override predicate isSink(DataFlow::Node sink) { - exists(KernelOpenCall c | c.getArgument(0) = sink) - or - exists(IOReadCall c | c.getArgument(0) = sink) + sink = any(AmbiguousPathCall r).getPathArgument() } override predicate isSanitizer(DataFlow::Node node) { @@ -73,5 +44,6 @@ where sourceNode = source.getNode() and call.getArgument(0) = sink.getNode() select sink.getNode(), source, sink, - "This call to " + call.(Replacement).getFrom() + " depends on a $@. Replace it with " + - call.(Replacement).getTo() + ".", source.getNode(), "user-provided value" + "This call to " + call.(AmbiguousPathCall).getName() + + " depends on a $@. Consider replacing it with " + call.(AmbiguousPathCall).getReplacement() + + ".", source.getNode(), "user-provided value" diff --git a/ruby/ql/src/queries/security/cwe-078/NonConstantKernelOpen.qhelp b/ruby/ql/src/queries/security/cwe-078/NonConstantKernelOpen.qhelp new file mode 100644 index 000000000000..304aefcfbb24 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-078/NonConstantKernelOpen.qhelp @@ -0,0 +1,4 @@ + + + + \ No newline at end of file diff --git a/ruby/ql/src/queries/security/cwe-078/NonConstantKernelOpen.ql b/ruby/ql/src/queries/security/cwe-078/NonConstantKernelOpen.ql new file mode 100644 index 000000000000..da490fd9ae31 --- /dev/null +++ b/ruby/ql/src/queries/security/cwe-078/NonConstantKernelOpen.ql @@ -0,0 +1,29 @@ +/** + * @name Use of `Kernel.open` or `IO.read` with a non-constant value + * @description Using `Kernel.open` or `IO.read` may allow a malicious + * user to execute arbitrary system commands. + * @kind problem + * @problem.severity warning + * @security-severity 6.5 + * @precision high + * @id rb/non-constant-kernel-open + * @tags correctness + * security + * external/cwe/cwe-078 + * external/cwe/cwe-088 + * external/cwe/cwe-073 + */ + +import codeql.ruby.security.KernelOpenQuery +import codeql.ruby.ast.Literal + +from AmbiguousPathCall call +where + // there is not a constant string argument + not exists(call.getPathArgument().asExpr().getExpr().getConstantValue()) and + // if it's a format string, then the first argument is not a constant string + not call.getPathArgument().getALocalSource().asExpr().getExpr().(StringLiteral).getComponent(0) + instanceof StringTextComponent +select call, + "Call to " + call.getName() + " with a non-constant value. Consider replacing it with " + + call.getReplacement() + "." diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection.expected b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected similarity index 100% rename from ruby/ql/test/query-tests/security/cwe-078/CommandInjection.expected rename to ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection.qlref b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.qlref similarity index 100% rename from ruby/ql/test/query-tests/security/cwe-078/CommandInjection.qlref rename to ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.qlref diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection.rb b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb similarity index 100% rename from ruby/ql/test/query-tests/security/cwe-078/CommandInjection.rb rename to ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb diff --git a/ruby/ql/test/query-tests/security/cwe-078/KernelOpen.expected b/ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.expected similarity index 75% rename from ruby/ql/test/query-tests/security/cwe-078/KernelOpen.expected rename to ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.expected index fc87de5c1037..140dbca5371d 100644 --- a/ruby/ql/test/query-tests/security/cwe-078/KernelOpen.expected +++ b/ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.expected @@ -9,5 +9,5 @@ nodes | KernelOpen.rb:5:13:5:16 | file | semmle.label | file | subpaths #select -| KernelOpen.rb:4:10:4:13 | file | KernelOpen.rb:3:12:3:17 | call to params : | KernelOpen.rb:4:10:4:13 | file | This call to Kernel.open depends on a $@. Replace it with File.open. | KernelOpen.rb:3:12:3:17 | call to params | user-provided value | -| KernelOpen.rb:5:13:5:16 | file | KernelOpen.rb:3:12:3:17 | call to params : | KernelOpen.rb:5:13:5:16 | file | This call to IO.read depends on a $@. Replace it with File.read. | KernelOpen.rb:3:12:3:17 | call to params | user-provided value | +| KernelOpen.rb:4:10:4:13 | file | KernelOpen.rb:3:12:3:17 | call to params : | KernelOpen.rb:4:10:4:13 | file | This call to Kernel.open depends on a $@. Consider replacing it with File.open. | KernelOpen.rb:3:12:3:17 | call to params | user-provided value | +| KernelOpen.rb:5:13:5:16 | file | KernelOpen.rb:3:12:3:17 | call to params : | KernelOpen.rb:5:13:5:16 | file | This call to IO.read depends on a $@. Consider replacing it with File.read. | KernelOpen.rb:3:12:3:17 | call to params | user-provided value | diff --git a/ruby/ql/test/query-tests/security/cwe-078/KernelOpen.qlref b/ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.qlref similarity index 100% rename from ruby/ql/test/query-tests/security/cwe-078/KernelOpen.qlref rename to ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.qlref diff --git a/ruby/ql/test/query-tests/security/cwe-078/KernelOpen.rb b/ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.rb similarity index 100% rename from ruby/ql/test/query-tests/security/cwe-078/KernelOpen.rb rename to ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.rb diff --git a/ruby/ql/test/query-tests/security/cwe-078/NonConstantKernelOpen/NonConstantKernelOpen.expected b/ruby/ql/test/query-tests/security/cwe-078/NonConstantKernelOpen/NonConstantKernelOpen.expected new file mode 100644 index 000000000000..920592aae3ed --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/NonConstantKernelOpen/NonConstantKernelOpen.expected @@ -0,0 +1,4 @@ +| NonConstantKernelOpen.rb:4:5:4:14 | call to open | Call to Kernel.open with a non-constant value. Consider replacing it with File.open. | +| NonConstantKernelOpen.rb:5:5:5:17 | call to read | Call to IO.read with a non-constant value. Consider replacing it with File.read. | +| NonConstantKernelOpen.rb:9:5:9:21 | call to open | Call to Kernel.open with a non-constant value. Consider replacing it with File.open. | +| NonConstantKernelOpen.rb:19:5:19:33 | call to open | Call to Kernel.open with a non-constant value. Consider replacing it with File.open. | diff --git a/ruby/ql/test/query-tests/security/cwe-078/NonConstantKernelOpen/NonConstantKernelOpen.qlref b/ruby/ql/test/query-tests/security/cwe-078/NonConstantKernelOpen/NonConstantKernelOpen.qlref new file mode 100644 index 000000000000..0b23d9102b9a --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/NonConstantKernelOpen/NonConstantKernelOpen.qlref @@ -0,0 +1 @@ +queries/security/cwe-078/NonConstantKernelOpen.ql \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-078/NonConstantKernelOpen/NonConstantKernelOpen.rb b/ruby/ql/test/query-tests/security/cwe-078/NonConstantKernelOpen/NonConstantKernelOpen.rb new file mode 100644 index 000000000000..b22ba0517227 --- /dev/null +++ b/ruby/ql/test/query-tests/security/cwe-078/NonConstantKernelOpen/NonConstantKernelOpen.rb @@ -0,0 +1,23 @@ +class UsersController < ActionController::Base + def create + file = params[:file] + open(file) # BAD + IO.read(file) # BAD + + File.open(file).read # GOOD + + Kernel.open(file) # BAD + + File.open(file, "r") # GOOD + + Kernel.open("constant") # GOOD + + IO.read("constant") # GOOD + + Kernel.open("this is #{fine}") # GOOD + + Kernel.open("#{this_is} bad") # BAD + + open("| #{this_is_an_explicit_command} foo bar") # GOOD + end +end