Merge remote-tracking branch 'origin/main' into nickrolfe/oj

ql/src/queries/security/cwe-078/KernelOpen.qhelp

@@ -0,0 +1,46 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>If <code>Kernel.open</code> is given a file name that starts with a <code>|</code>
+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 <code>IO.read</code>.
+</p>
+
+</overview>
+<recommendation>
+
+<p>Use <code>File.open</code> instead of <code>Kernel.open</code>, as the former
+does not have this vulnerability. Similarly, use <code>File.read</code> instead
+of <code>IO.read</code>.</p>
+
+</recommendation>
+<example>
+
+<p>
+The following example shows code that calls <code>Kernel.open</code> on a
+user-supplied file path.
+</p>
+
+<sample src="examples/kernel_open.rb" />
+
+<p>Instead, <code>File.open</code> should be used, as in the following example.</p>
+
+<sample src="examples/file_open.rb" />
+
+</example>
+<references>
+
+<li>
+OWASP:
+<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
+</li>
+
+<li>
+Example CVE: <a href="https://www.ruby-lang.org/en/news/2021/05/02/os-command-injection-in-rdoc/">Command Injection in RDoc</a>.
+</li>
+
+</references>
+</qhelp>

ql/src/queries/security/cwe-078/KernelOpen.ql

@@ -0,0 +1,76 @@
+/**
+ * @name Use of `Kernel.open` or `IO.read`
+ * @description Using `Kernel.open` or `IO.read` may allow a malicious
+ *              user to execute arbitrary system commands.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 9.8
+ * @precision high
+ * @id rb/kernel-open
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-078
+ *       external/cwe/cwe-088
+ *       external/cwe/cwe-073
+ */
+
+import ruby
+import codeql.ruby.ApiGraphs
+import codeql.ruby.frameworks.StandardLibrary
+import codeql.ruby.TaintTracking
+import codeql.ruby.dataflow.BarrierGuards
+import codeql.ruby.dataflow.RemoteFlowSources
+import DataFlow::PathGraph
+
+/**
+ * Method calls that have 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" }
+}
+
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "KernelOpen" }
+
+  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)
+  }
+
+  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+    guard instanceof StringConstCompare or
+    guard instanceof StringConstArrayInclusionCall
+  }
+}
+
+from
+  Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink,
+  DataFlow::Node sourceNode, DataFlow::CallNode call
+where
+  config.hasFlowPath(source, sink) and
+  sourceNode = source.getNode() and
+  call.asExpr().getExpr().(MethodCall).getArgument(0) = sink.getNode().asExpr().getExpr()
+select sink.getNode(), source, sink,
+  "This call to " + call.(Replacement).getFrom() +
+    " depends on a user-provided value. Replace it with " + call.(Replacement).getTo() + "."

ql/src/queries/security/cwe-078/examples/file_open.rb

@@ -0,0 +1,6 @@
+class UsersController < ActionController::Base
+    def create
+      filename = params[:filename]
+      File.open(filename)
+    end
+  end  

ql/src/queries/security/cwe-078/examples/kernel_open.rb

@@ -0,0 +1,6 @@
+class UsersController < ActionController::Base
+  def create
+    filename = params[:filename]
+    open(filename) # BAD
+  end
+end  

ql/src/queries/security/cwe-611/Xxe.qhelp

@@ -0,0 +1,55 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Parsing untrusted XML files with a weakly configured XML parser may lead to an
+XML External Entity (XXE) attack. This type of attack uses external entity references
+to access arbitrary files on a system, carry out denial-of-service (DoS) attacks, or server-side
+request forgery. Even when the result of parsing is not returned to the user, DoS attacks are still possible
+and out-of-band data retrieval techniques may allow attackers to steal sensitive data.
+</p>
+</overview>
+
+<recommendation>
+<p>
+The easiest way to prevent XXE attacks is to disable external entity handling when
+parsing untrusted data. How this is done depends on the library being used. Note that some
+libraries, such as <code>rexml</code>, <code>nokogiri</code> and <code>libxml-ruby</code>, 
+disable entity expansion by default, so unless you have explicitly enabled entity expansion,
+no further action needs to be taken.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example uses the <code>nokogiri</code> XML parser to parse a string <code>xmlSrc</code>.
+If that string is from an untrusted source, this code may be vulnerable to an XXE attack, since
+the parser is invoked with the <code>noent</code> option set:
+</p>
+<sample src="examples/Xxe.rb"/>
+
+<p>
+To guard against XXE attacks, the <code>noent</code> option should be omitted or cleared
+(e.g. using <code>nonoent</code>). This means that no entity expansion is undertaken at all, 
+not even for standard internal entities such as <code>&amp;amp;</code> or <code>&amp;gt;</code>.
+If desired, these entities can be expanded in a separate step using utility functions.
+</p>
+<sample src="examples/XxeGood.rb"/>
+</example>
+
+<references>
+<li>
+OWASP:
+<a href="https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing">XML External Entity (XXE) Processing</a>.
+</li>
+<li>
+Timothy Morgen:
+<a href="https://research.nccgroup.com/2014/05/19/xml-schema-dtd-and-entity-attacks-a-compendium-of-known-techniques/">XML Schema, DTD, and Entity Attacks</a>.
+</li>
+<li>
+Timur Yunusov, Alexey Osipov:
+<a href="https://www.slideshare.net/qqlan/bh-ready-v4">XML Out-Of-Band Data Retrieval</a>.
+</li>
+</references>
+</qhelp>

ql/src/queries/security/cwe-611/Xxe.ql

@@ -0,0 +1,43 @@
+/**
+ * @name XML external entity expansion
+ * @description Parsing user input as an XML document with external
+ *              entity expansion is vulnerable to XXE attacks.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 9.1
+ * @precision high
+ * @id rb/xxe
+ * @tags security
+ *       external/cwe/cwe-611
+ *       external/cwe/cwe-776
+ *       external/cwe/cwe-827
+ */
+
+import ruby
+import codeql.ruby.dataflow.RemoteFlowSources
+import codeql.ruby.TaintTracking
+import codeql.ruby.Concepts
+import codeql.ruby.DataFlow
+import DataFlow::PathGraph
+
+class UnsafeXxeSink extends DataFlow::ExprNode {
+  UnsafeXxeSink() {
+    exists(XmlParserCall parse |
+      parse.getInput() = this and
+      parse.externalEntitiesEnabled()
+    )
+  }
+}
+
+class XxeConfig extends TaintTracking::Configuration {
+  XxeConfig() { this = "XXE.ql::XxeConfig" }
+
+  override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeXxeSink }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, XxeConfig conf
+where conf.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Unsafe parsing of XML file from $@.", source.getNode(),
+  "user input"

ql/src/queries/security/cwe-611/examples/Xxe.rb

@@ -0,0 +1,12 @@
+require "nokogiri"
+
+def process_data1
+   xmlSrc = request.body
+   doc = Nokogiri::XML.parse(xmlSrc, nil, nil, Nokogiri::XML::ParseOptions::NOENT) # BAD
+end
+
+def process_data2
+   xmlSrc = request.body
+   doc = Nokogiri::XML.parse(xmlSrc) { |config| config.noent } # BAD
+end
+

ql/src/queries/security/cwe-611/examples/XxeGood.rb

@@ -0,0 +1,12 @@
+require "nokogiri"
+
+def process_data1
+   xmlSrc = request.body
+   doc = Nokogiri::XML.parse(xmlSrc) # GOOD
+end
+
+def process_data2
+   xmlSrc = request.body
+   doc = Nokogiri::XML.parse(xmlSrc) { |config| config.nonoent } # GOOD
+end
+