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

Conversation

maikypedia
Copy link
Contributor

This pull request adds a query for Insecure Randomness.

Looking forward to your suggestions.

@ghsecuritylab ghsecuritylab marked this pull request as draft October 21, 2023 16:02
@ghsecuritylab ghsecuritylab marked this pull request as ready for review October 30, 2023 10:51
@sidshank sidshank requested a review from alexrford November 6, 2023 14:21
@sidshank sidshank requested review from hmac and removed request for alexrford December 4, 2023 12:11
Copy link
Contributor

@hmac hmac left a comment

Choose a reason for hiding this comment

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

This looks good! I have some minor comments, and I've kicked off a variant analysis run to see what results we get.

@@ -0,0 +1,73 @@
/**
* Provides default sources, sinks, and sanitizers for reasoning about bypass of
* sensitive action guards, as well as extension points for adding your own.
Copy link
Contributor

Choose a reason for hiding this comment

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

about random values that are not cryptographically secure?


/**
* Provides default sources, sinks, and sanitizers for reasoning about bypass of
* sensitive action guards, as well as extension points for adding your own.
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

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"

* A taint-tracking configuration for detecting Insecure Randomness vulnerabilities.
* DEPRECATED: Use `InsecureRandomnessFlow`
*/
deprecated class Configuration extends TaintTracking::Configuration {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there's any need to add a deprecated class. We should remove this.

Copy link
Contributor

github-actions bot commented Dec 20, 2023

QHelp previews:

ruby/ql/src/experimental/insecure-randomness/InsecureRandomness.qhelp

Insecure randomness

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.

Recommendation

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.

Example

The following examples show different ways of generating a password.

The first example uses Random.rand() which is not for security purposes

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

In the second example, the password is generated using SecureRandom.random_bytes() which is a cryptographically secure method.

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()

References

@hmac
Copy link
Contributor

hmac commented Jan 2, 2024

There are also two code scanning warnings that should be fixed up.

@hmac
Copy link
Contributor

hmac commented Jan 9, 2024

It looks like ruby/ql/lib/codeql/ruby/security/InsecureRandomnessCustomizations.qll is not formatted correctly - run codeql query format on it to resolve that.


<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.

maikypedia and others added 2 commits January 27, 2024 14:07
@hmac
Copy link
Contributor

hmac commented Jan 31, 2024

Thanks!

@hmac hmac merged commit 06334ee into github:main Jan 31, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants