Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ruby: Add Insecure Randomness Query #14554

Merged
merged 7 commits into from
Jan 31, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
/**
* Provides default sources, sinks, and sanitizers for reasoning about random values that
* are not cryptographically secure, as well as extension points for adding your own.
*/

private import codeql.ruby.CFG
private import codeql.ruby.AST
private import codeql.ruby.DataFlow
private import codeql.ruby.security.SensitiveActions
private import codeql.ruby.Concepts
private import codeql.ruby.ApiGraphs
import codeql.ruby.frameworks.core.Kernel

/**
* Provides default sources, sinks, and sanitizers for reasoning about random values that
* are not cryptographically secure, as well as extension points for adding your own.
*/
module InsecureRandomness {
/**
* A data flow source for random values that are not cryptographically secure.
*/
abstract class Source extends DataFlow::Node { }

/**
* AA data flow sink for random values that are not cryptographically secure.
*/
abstract class Sink extends DataFlow::Node { }

/**
* A sanitizer for random values that are not cryptographically secure.
*/
abstract class Sanitizer extends DataFlow::Node { }

/**
* A simple random number generator that is not cryptographically secure.
*/
class DefaultSource extends Source, DataFlow::CallNode {
DefaultSource() {
this = API::getTopLevelMember("Random").getAMethodCall(["rand"])
or
this.asExpr().getExpr() instanceof UnknownMethodCall and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you import codeql.ruby.frameworks.core.Kernel then you can just do

or this.(Kernel::KernelMethodCall).getMethodName() = "rand"

(
this.getReceiver().asExpr().getExpr() instanceof SelfVariableAccess and
super.getMethodName() = "rand"
)
or this.(Kernel::KernelMethodCall).getMethodName() = "rand"
}
}

/**
* A sensitive write, considered as a sink for random values that are not cryptographically
* secure.
*/
class SensitiveWriteSink extends Sink instanceof SensitiveWrite { }

/**
* A cryptographic key, considered as a sink for random values that are not cryptographically
* secure.
*/
class CryptoKeySink extends Sink {
CryptoKeySink() {
exists(Cryptography::CryptographicOperation operation | this = operation.getAnInput())
}
}

/**
* A index call, considered as a sink for random values that are not cryptographiocally
* secure
*/
class CharacterIndexing extends Sink {
CharacterIndexing() {
exists(DataFlow::CallNode c | this = c.getAMethodCall("[]").getArgument(0))
}
}
}
21 changes: 21 additions & 0 deletions ruby/ql/lib/codeql/ruby/security/InsecureRandomnessQuery.qll
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/**
* Provides default sources, sinks and sanitizers for detecting Insecure Randomness
* vulnerabilities, as well as extension points for adding your own.
*/

private import codeql.ruby.DataFlow
private import codeql.ruby.TaintTracking
import InsecureRandomnessCustomizations::InsecureRandomness

private module InsecureRandomnessConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) { source instanceof Source }

predicate isSink(DataFlow::Node sink) { sink instanceof Sink }

predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
}

/**
* Taint-tracking for detecting Insecure Randomness vulnerabilities.
*/
module InsecureRandomnessFlow = TaintTracking::Global<InsecureRandomnessConfig>;
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: newQuery
---
* Added a new experimental query, `rb/insecure-randomness`, to detect when application uses random values that are not cryptographically secure.
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>

<overview>
<p>
Using a cryptographically weak pseudo-random number generator to generate a security-sensitive value,
such as a password, makes it easier for an attacker to predict the value.

Pseudo-random number generators generate a sequence of numbers that only approximates the
properties of random numbers. The sequence is not truly random because it is completely
determined by a relatively small set of initial values, the seed. If the random number generator is
cryptographically weak, then this sequence may be easily predictable through outside observations.
</p>
</overview>

<recommendation>
<p>
When generating values for use in security-sensitive contexts, it's essential to utilize a
cryptographically secure pseudo-random number generator. As a general guideline, a value
should be deemed "security-sensitive" if its predictability would empower an attacker to
perform actions that would otherwise be beyond their reach. For instance, if an attacker could
predict a newly generated user's random password, they would gain unauthorized access to that user's
account.

For Ruby, `SecureRandom` provides a cryptographically secure pseudo-random number generator.
`rand` is not cryptographically secure, and should be avoided in security contexts.
For contexts which are not security sensitive, Random may be preferable as it has a more convenient
interface.

</p>
</recommendation>

<example>
<p>
The following examples show different ways of generating a password.
</p>

<p>The first example uses `Random.rand()` which is not for security purposes</p>

<sample src="examples/InsecureRandomnessBad.rb" />

<p>In the second example, the password is generated using `SecureRandom.random_bytes` which is a
cryptographically secure method.</p>

<sample src="examples/InsecureRandomnessGood.rb" />
</example>

<references>
<li>Wikipedia: <a href="http://en.wikipedia.org/wiki/Pseudorandom_number_generator">Pseudo-random number generator</a>.</li>
<li>Common Weakness Enumeration: <a href="https://cwe.mitre.org/data/definitions/338.html">CWE-338</a>.</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be added automatically based on the tags in the .ql file I think.

<li>Ruby-doc: <a href="https://ruby-doc.org/core-3.1.2/Random.html">Random</a>.</li>
</references>
</qhelp>
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
/**
* @name Insecure randomness
* @description Using a cryptographically weak pseudo-random number generator to generate a
* security-sensitive value may allow an attacker to predict what value will
* be generated.
* @kind path-problem
* @problem.severity warning
* @security-severity 7.8
* @precision high
* @id rb/insecure-randomness
* @tags security
* external/cwe/cwe-338
*/

import codeql.ruby.DataFlow
import codeql.ruby.security.InsecureRandomnessQuery
import InsecureRandomnessFlow::PathGraph

from InsecureRandomnessFlow::PathNode source, InsecureRandomnessFlow::PathNode sink
where InsecureRandomnessFlow::flowPath(source, sink)
select sink.getNode(), source, sink,
"This uses a cryptographically insecure random number generated at $@ in a security context.",
source.getNode(), source.getNode().toString()
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
def generate_password()
chars = ('a'..'z').to_a + ('A'..'Z').to_a + ('0'..'9').to_a + ['!', '@', '#', '$', '%']
# BAD: rand is not cryptographically secure
password = (1..10).collect { chars[rand(chars.size)] }.join
end

password = generate_password
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
require 'securerandom'

def generate_password()
chars = ('a'..'z').to_a + ('A'..'Z').to_a + ('0'..'9').to_a + ['!', '@', '#', '$', '%']

# GOOD: SecureRandom is cryptographically secure
password = SecureRandom.random_bytes(10).each_byte.map do |byte|
chars[byte % chars.length]
end.join
end

password = generate_password()
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
edges
nodes
| InsecureRandomness.rb:6:42:6:57 | call to rand | semmle.label | call to rand |
subpaths
#select
| InsecureRandomness.rb:6:42:6:57 | call to rand | InsecureRandomness.rb:6:42:6:57 | call to rand | InsecureRandomness.rb:6:42:6:57 | call to rand | This uses a cryptographically insecure random number generated at $@ in a security context. | InsecureRandomness.rb:6:42:6:57 | call to rand | call to rand |
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
experimental/insecure-randomness/InsecureRandomness.ql
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
require 'securerandom'

def generate_password_1(length)
chars = ('a'..'z').to_a + ('A'..'Z').to_a + ('0'..'9').to_a + ['!', '@', '#', '$', '%']
# BAD: rand is not cryptographically secure
password = (1..length).collect { chars[rand(chars.size)] }.join
end

def generate_password_2(length)
chars = ('a'..'z').to_a + ('A'..'Z').to_a + ('0'..'9').to_a + ['!', '@', '#', '$', '%']

# GOOD: SecureRandom is cryptographically secure
password = SecureRandom.random_bytes(length).each_byte.map do |byte|
chars[byte % chars.length]
end.join
end

password = generate_password_1(10)
password = generate_password_2(10)