mirror of
https://github.com/github/codeql.git
synced 2025-12-24 04:36:35 +01:00
Merge branch 'main' into unsafeRbCmd
This commit is contained in:
erik-krogh
46
ruby/ql/src/queries/security/cwe-078/KernelOpen.inc.qhelp
Normal file
46
ruby/ql/src/queries/security/cwe-078/KernelOpen.inc.qhelp
Normal file
@@ -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>
|
||||
@@ -1,46 +1,4 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<!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>
|
||||
<include src="KernelOpen.inc.qhelp" />
|
||||
</qhelp>
|
||||
@@ -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.TaintTracking
|
||||
import codeql.ruby.dataflow.BarrierGuards
|
||||
import codeql.ruby.dataflow.RemoteFlowSources
|
||||
import codeql.ruby.DataFlow
|
||||
import codeql.ruby.TaintTracking
|
||||
import codeql.ruby.dataflow.RemoteFlowSources
|
||||
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"
|
||||
|
||||
@@ -0,0 +1,4 @@
|
||||
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
|
||||
<qhelp>
|
||||
<include src="KernelOpen.inc.qhelp" />
|
||||
</qhelp>
|
||||
@@ -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() + "."
|
||||
@@ -12,7 +12,7 @@ to execute arbitrary code.
|
||||
<recommendation>
|
||||
<p>
|
||||
Avoid deserialization of untrusted data if possible. If the architecture permits
|
||||
it, use serialization formats that cannot represent arbitarary objects. For
|
||||
it, use serialization formats that cannot represent arbitrary objects. For
|
||||
libraries that support it, such as the Ruby standard library's <code>JSON</code>
|
||||
module, ensure that the parser is configured to disable
|
||||
deserialization of arbitrary objects.
|
||||
|
||||
43
ruby/ql/src/queries/security/cwe-598/SensitiveGetQuery.qhelp
Normal file
43
ruby/ql/src/queries/security/cwe-598/SensitiveGetQuery.qhelp
Normal file
@@ -0,0 +1,43 @@
|
||||
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
|
||||
<qhelp>
|
||||
<overview>
|
||||
<p>
|
||||
Sensitive information such as passwords should not be transmitted within the query string of the requested URL.
|
||||
Sensitive information within URLs may be logged in various locations, including the user's browser, the web server,
|
||||
and any proxy servers between the two endpoints. URLs may also be displayed on-screen, bookmarked
|
||||
or emailed around by users. They may be disclosed to third parties via the Referer header when any off-site links are
|
||||
followed. Placing sensitive information into the URL therefore increases the risk that it will be captured by an attacker.
|
||||
</p>
|
||||
</overview>
|
||||
|
||||
<recommendation>
|
||||
<p>
|
||||
Use HTTP POST to send sensitive information as part of the request body; for example, as form data.
|
||||
</p>
|
||||
</recommendation>
|
||||
|
||||
<example>
|
||||
<p>
|
||||
The following example shows two route handlers that both receive a username and a password.
|
||||
The first receives this sensitive information from the query parameters of a GET request, which is
|
||||
transmitted in the URL. The second receives this sensitive information from the request body of a POST request.
|
||||
</p>
|
||||
<sample src="examples/routes.rb" />
|
||||
<sample src="examples/users_controller.rb" />
|
||||
</example>
|
||||
|
||||
<references>
|
||||
<li>
|
||||
CWE:
|
||||
<a href="https://cwe.mitre.org/data/definitions/598.html">CWE-598: Use of GET Request Method with Sensitive Query Strings</a>
|
||||
</li>
|
||||
<li>
|
||||
PortSwigger (Burp):
|
||||
<a href="https://portswigger.net/kb/issues/00400300_password-submitted-using-get-method">Password Submitted using GET Method</a>
|
||||
</li>
|
||||
<li>
|
||||
OWASP:
|
||||
<a href="https://owasp.org/www-community/vulnerabilities/Information_exposure_through_query_strings_in_url">Information Exposure through Query Strings in URL</a>
|
||||
</li>
|
||||
</references>
|
||||
</qhelp>
|
||||
23
ruby/ql/src/queries/security/cwe-598/SensitiveGetQuery.ql
Normal file
23
ruby/ql/src/queries/security/cwe-598/SensitiveGetQuery.ql
Normal file
@@ -0,0 +1,23 @@
|
||||
/**
|
||||
* @name Sensitive data read from GET request
|
||||
* @description Placing sensitive data in a GET request increases the risk of
|
||||
* the data being exposed to an attacker.
|
||||
* @kind path-problem
|
||||
* @problem.severity warning
|
||||
* @security-severity 6.5
|
||||
* @precision high
|
||||
* @id rb/sensitive-get-query
|
||||
* @tags security
|
||||
* external/cwe/cwe-598
|
||||
*/
|
||||
|
||||
import ruby
|
||||
import DataFlow::PathGraph
|
||||
import codeql.ruby.security.SensitiveGetQueryQuery
|
||||
import codeql.ruby.security.SensitiveActions
|
||||
|
||||
from DataFlow::PathNode source, DataFlow::PathNode sink, SensitiveGetQuery::Configuration config
|
||||
where config.hasFlowPath(source, sink)
|
||||
select source.getNode(), source, sink,
|
||||
"$@ for GET requests uses query parameter as sensitive data.",
|
||||
source.getNode().(SensitiveGetQuery::Source).getHandler(), "Route handler"
|
||||
4
ruby/ql/src/queries/security/cwe-598/examples/routes.rb
Normal file
4
ruby/ql/src/queries/security/cwe-598/examples/routes.rb
Normal file
@@ -0,0 +1,4 @@
|
||||
Rails.application.routes.draw do
|
||||
get "users/login", to: "#login_get" # BAD: sensitive data transmitted through query parameters
|
||||
post "users/login", to: "users#login_post" # GOOD: sensitive data transmitted in the request body
|
||||
end
|
||||
@@ -0,0 +1,16 @@
|
||||
class UsersController < ActionController::Base
|
||||
def login_get
|
||||
password = params[:password]
|
||||
authenticate_user(params[:username], password)
|
||||
end
|
||||
|
||||
def login_post
|
||||
password = params[:password]
|
||||
authenticate_user(params[:username], password)
|
||||
end
|
||||
|
||||
private
|
||||
def authenticate_user(username, password)
|
||||
# ... authenticate the user here
|
||||
end
|
||||
end
|
||||
@@ -4,7 +4,7 @@
|
||||
<qhelp>
|
||||
<overview>
|
||||
<p>
|
||||
Downloading executeables or other sensitive files over an unencrypted connection
|
||||
Downloading executables or other sensitive files over an unencrypted connection
|
||||
can leave a server open to man-in-the-middle attacks (MITM).
|
||||
Such an attack can allow an attacker to insert arbitrary content
|
||||
into the downloaded file, and in the worst case, allow the attacker to execute
|
||||
|
||||
Reference in New Issue
Block a user