-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Java: Add Weak Randomness Query (CWE-330/338) #13608
Merged
egregius313
merged 29 commits into
github:main
from
egregius313:egregius313/weak-randomness
Dec 12, 2023
Merged
Changes from all commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
9f986ca
Add Weak Randomness Query
egregius313 e69ff7b
Move to library and add docs
egregius313 1daa83b
Add test cases
egregius313 bf0123d
Add org.apache.commons.lang.RandomStringUtils as a source
egregius313 b713efb
Add ThreadLocalRandom.current as another source
egregius313 0313f39
Cryptographic sinks
egregius313 14fdfa4
Add new sink kind and change note
egregius313 bc06555
Simplifications
egregius313 ce7690b
Make imports private
egregius313 dc3e4cd
Refactored method accesses to the RandomDataSource library
egregius313 ba3c38c
Restrict `addCookie` to specific interface
egregius313 fb875f5
More variety of test cases
egregius313 057a74d
Remove unnused class
egregius313 646254c
Add credentials sinks from SensitiveApi
egregius313 b8b2de2
Remove use of `crypto-parameter` sink kind
egregius313 a1e9564
Add more sources
egregius313 e9ca4a2
Update to new `MethodCall` name
egregius313 7241e09
Replace `convertBytesToString` with models
egregius313 7f3995f
Remove extra `encryption-iv` models
egregius313 b9d2a26
Move ESAPI models into the Weak Randomness query
egregius313 4bdf2b5
Bump change note date
egregius313 bbf9937
Alter cookie sinks to instead focus on creation of a cookie
egregius313 4678302
Update query metadata
egregius313 6e70e6c
Use pre-exisiting type for SecureRandom
egregius313 3ca039b
Rename to InsecureRandomness
egregius313 1271cd3
Remove unnecessary crypto sinks
egregius313 7362158
Fix test case
egregius313 ce20c4a
Docs review suggestions
egregius313 06eef93
Docs review suggestions
egregius313 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
97 changes: 97 additions & 0 deletions
97
java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
/** Provides classes and predicates for reasoning about insecure randomness. */ | ||
|
||
import java | ||
private import semmle.code.java.frameworks.Servlets | ||
private import semmle.code.java.security.SensitiveActions | ||
private import semmle.code.java.security.SensitiveApi | ||
private import semmle.code.java.dataflow.TaintTracking | ||
private import semmle.code.java.dataflow.ExternalFlow | ||
private import semmle.code.java.security.RandomQuery | ||
|
||
/** | ||
* A node representing a source of insecure randomness. | ||
* | ||
* For example, use of `java.util.Random` or `java.lang.Math.random`. | ||
*/ | ||
abstract class InsecureRandomnessSource extends DataFlow::Node { } | ||
|
||
private class RandomMethodSource extends InsecureRandomnessSource { | ||
RandomMethodSource() { | ||
exists(RandomDataSource s | this.asExpr() = s.getOutput() | | ||
not s.getQualifier().getType() instanceof SafeRandomImplementation | ||
) | ||
} | ||
} | ||
|
||
/** | ||
* A type which is an implementation of `java.util.Random` but considered to be safe. | ||
* | ||
* For example, `java.security.SecureRandom`. | ||
*/ | ||
abstract private class SafeRandomImplementation extends RefType { } | ||
|
||
private class TypeSecureRandom extends SafeRandomImplementation instanceof SecureRandomNumberGenerator | ||
{ } | ||
|
||
private class TypeHadoopOsSecureRandom extends SafeRandomImplementation { | ||
TypeHadoopOsSecureRandom() { | ||
this.hasQualifiedName("org.apache.hadoop.crypto.random", "OsSecureRandom") | ||
} | ||
} | ||
|
||
/** | ||
* A node representing an operation which should not use a Insecurely random value. | ||
*/ | ||
abstract class InsecureRandomnessSink extends DataFlow::Node { } | ||
|
||
/** | ||
* A node which sets the value of a cookie. | ||
*/ | ||
private class CookieSink extends InsecureRandomnessSink { | ||
CookieSink() { | ||
exists(Call c | | ||
c.(ClassInstanceExpr).getConstructedType() instanceof TypeCookie and | ||
this.asExpr() = c.getArgument(1) | ||
or | ||
c.(MethodCall).getMethod().getDeclaringType() instanceof TypeCookie and | ||
c.(MethodCall).getMethod().hasName("setValue") and | ||
this.asExpr() = c.getArgument(0) | ||
) | ||
} | ||
} | ||
|
||
private class SensitiveActionSink extends InsecureRandomnessSink { | ||
SensitiveActionSink() { this.asExpr() instanceof SensitiveExpr } | ||
} | ||
|
||
private class CredentialsSink extends InsecureRandomnessSink instanceof CredentialsSinkNode { } | ||
|
||
/** | ||
* A taint-tracking configuration for Insecure randomness. | ||
*/ | ||
module InsecureRandomnessConfig implements DataFlow::ConfigSig { | ||
predicate isSource(DataFlow::Node src) { src instanceof InsecureRandomnessSource } | ||
|
||
predicate isSink(DataFlow::Node sink) { sink instanceof InsecureRandomnessSink } | ||
|
||
predicate isBarrierIn(DataFlow::Node n) { isSource(n) } | ||
|
||
predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) { | ||
n1.asExpr() = n2.asExpr().(BinaryExpr).getAnOperand() | ||
or | ||
n1.asExpr() = n2.asExpr().(UnaryExpr).getExpr() | ||
or | ||
exists(MethodCall mc, string methodName | | ||
mc.getMethod().hasQualifiedName("org.owasp.esapi", "Encoder", methodName) and | ||
methodName.matches("encode%") | ||
| | ||
n1.asExpr() = mc.getArgument(0) and | ||
n2.asExpr() = mc | ||
) | ||
} | ||
} | ||
|
||
/** | ||
* Taint-tracking flow of a Insecurely random value into a sensitive sink. | ||
*/ | ||
module InsecureRandomnessFlow = TaintTracking::Global<InsecureRandomnessConfig>; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
<!DOCTYPE qhelp PUBLIC | ||
"-//Semmle//qhelp//EN" | ||
"qhelp.dtd"> | ||
<qhelp> | ||
<overview> | ||
<p> | ||
If you use a cryptographically weak pseudo-random number generator to generate security-sensitive values, | ||
such as passwords, attackers can more easily predict those values. | ||
</p> | ||
<p> | ||
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> | ||
The <code>java.util.Random</code> random number generator is not cryptographically secure. Use a secure random number generator such as <code>java.security.SecureRandom</code> instead. | ||
</p> | ||
<p> | ||
Use a cryptographically secure pseudo-random number generator if the output is to be used in a | ||
security-sensitive context. As a general rule, a value should be considered "security-sensitive" | ||
if predicting it would allow the attacker to perform an action that they would otherwise be unable | ||
to perform. For example, if an attacker could predict the random password generated for a new user, | ||
they would be able to log in as that new user. | ||
</p> | ||
</recommendation> | ||
|
||
<example> | ||
|
||
<p> | ||
The following examples show different ways of generating a cookie with a random value. | ||
</p> | ||
|
||
<p> | ||
In the first (BAD) case, we generate a fresh cookie by appending a random integer to the end of a static | ||
string. The random number generator used (<code>Random</code>) is not cryptographically secure, | ||
so it may be possible for an attacker to predict the generated cookie. | ||
</p> | ||
|
||
<sample src="examples/InsecureRandomnessCookie.java" /> | ||
|
||
<p> | ||
In the second (GOOD) case, we generate a fresh cookie by appending a random integer to the end of a static | ||
string. The random number generator used (<code>SecureRandom</code>) is cryptographically secure, | ||
so it is not possible for an attacker to predict the generated cookie. | ||
</p> | ||
|
||
<sample src="examples/SecureRandomnessCookie.java" /> | ||
|
||
</example> | ||
|
||
<references> | ||
<li>Wikipedia: <a href="http://en.wikipedia.org/wiki/Pseudorandom_number_generator">Pseudo-random number generator</a>.</li> | ||
<li> | ||
Java Docs: <a href="http://docs.oracle.com/javase/8/docs/api/java/util/Random.html">Random</a>. | ||
</li> | ||
<li> | ||
Java Docs: <a href="http://docs.oracle.com/javase/8/docs/api/java/security/SecureRandom.html">SecureRandom</a>. | ||
</li> | ||
</references> | ||
</qhelp> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
/** | ||
* @name Insecure randomness | ||
* @description Using a cryptographically Insecure 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 java/insecure-randomness | ||
* @tags security | ||
* external/cwe/cwe-330 | ||
* external/cwe/cwe-338 | ||
*/ | ||
|
||
import java | ||
import semmle.code.java.security.InsecureRandomnessQuery | ||
import InsecureRandomnessFlow::PathGraph | ||
|
||
from InsecureRandomnessFlow::PathNode source, InsecureRandomnessFlow::PathNode sink | ||
where InsecureRandomnessFlow::flowPath(source, sink) | ||
select sink.getNode(), source, sink, "Potential Insecure randomness due to a $@.", source.getNode(), | ||
"Insecure randomness source." | ||
9 changes: 9 additions & 0 deletions
9
java/ql/src/Security/CWE/CWE-330/examples/InsecureRandomnessCookie.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
Random r = new Random(); | ||
|
||
byte[] bytes = new byte[16]; | ||
r.nextBytes(bytes); | ||
|
||
String cookieValue = encode(bytes); | ||
|
||
Cookie cookie = new Cookie("name", cookieValue); | ||
response.addCookie(cookie); |
9 changes: 9 additions & 0 deletions
9
java/ql/src/Security/CWE/CWE-330/examples/SecureRandomnessCookie.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
SecureRandom r = new SecureRandom(); | ||
|
||
byte[] bytes = new byte[16]; | ||
r.nextBytes(bytes); | ||
|
||
String cookieValue = encode(bytes); | ||
|
||
Cookie cookie = new Cookie("name", cookieValue); | ||
response.addCookie(cookie); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
category: newQuery | ||
--- | ||
* Added the `java/insecure-randomness` query to detect uses of weakly random values which an attacker may be able to predict. Also added the `crypto-parameter` sink kind for sinks which represent the parameters and keys of cryptographic operations. | ||
|
2 changes: 2 additions & 0 deletions
2
java/ql/test/query-tests/security/CWE-330/InsecureRandomnessTest.expected
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
failures | ||
testFailures |
19 changes: 19 additions & 0 deletions
19
java/ql/test/query-tests/security/CWE-330/InsecureRandomnessTest.ql
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
import java | ||
import semmle.code.java.dataflow.DataFlow | ||
import semmle.code.java.security.InsecureRandomnessQuery | ||
import TestUtilities.InlineExpectationsTest | ||
|
||
module WeakRandomTest implements TestSig { | ||
string getARelevantTag() { result = "hasWeakRandomFlow" } | ||
|
||
predicate hasActualResult(Location location, string element, string tag, string value) { | ||
tag = "hasWeakRandomFlow" and | ||
exists(DataFlow::Node sink | InsecureRandomnessFlow::flowTo(sink) | | ||
sink.getLocation() = location and | ||
element = sink.toString() and | ||
value = "" | ||
) | ||
} | ||
} | ||
|
||
import MakeTest<WeakRandomTest> |
61 changes: 61 additions & 0 deletions
61
java/ql/test/query-tests/security/CWE-330/WeakRandomCookies.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
import java.io.IOException; | ||
import java.util.Random; | ||
import java.util.concurrent.ThreadLocalRandom; | ||
import java.security.SecureRandom; | ||
import javax.servlet.ServletException; | ||
import javax.servlet.http.HttpServlet; | ||
import javax.servlet.http.HttpServletRequest; | ||
import javax.servlet.http.HttpServletResponse; | ||
import javax.servlet.http.Cookie; | ||
import org.apache.commons.lang3.RandomStringUtils; | ||
import org.owasp.esapi.Encoder; | ||
|
||
public class WeakRandomCookies extends HttpServlet { | ||
HttpServletResponse response; | ||
|
||
public void doGet() { | ||
Random r = new Random(); | ||
|
||
int c = r.nextInt(); | ||
// BAD: The cookie value may be predictable. | ||
Cookie cookie = new Cookie("name", Integer.toString(c)); // $hasWeakRandomFlow | ||
|
||
Encoder enc = null; | ||
int c2 = r.nextInt(); | ||
String value = enc.encodeForHTML(Integer.toString(c2)); | ||
// BAD: The cookie value may be predictable. | ||
Cookie cookie2 = new Cookie("name", value); // $hasWeakRandomFlow | ||
|
||
byte[] bytes = new byte[16]; | ||
r.nextBytes(bytes); | ||
// BAD: The cookie value may be predictable. | ||
Cookie cookie3 = new Cookie("name", new String(bytes)); // $hasWeakRandomFlow | ||
|
||
SecureRandom sr = new SecureRandom(); | ||
|
||
byte[] bytes2 = new byte[16]; | ||
sr.nextBytes(bytes2); | ||
// GOOD: The cookie value is unpredictable. | ||
Cookie cookie4 = new Cookie("name", new String(bytes2)); | ||
|
||
ThreadLocalRandom tlr = ThreadLocalRandom.current(); | ||
|
||
Cookie cookie5 = new Cookie("name", Integer.toString(tlr.nextInt())); // $hasWeakRandomFlow | ||
|
||
Cookie cookie6 = new Cookie("name", RandomStringUtils.random(10)); // $hasWeakRandomFlow | ||
|
||
Cookie cookie7 = new Cookie("name", RandomStringUtils.randomAscii(10)); // $hasWeakRandomFlow | ||
|
||
long c3 = r.nextLong(); | ||
// BAD: The cookie value may be predictable. | ||
Cookie cookie8 = new Cookie("name", Long.toString(c3 * 5)); // $hasWeakRandomFlow | ||
|
||
double c4 = Math.random(); | ||
// BAD: The cookie value may be predictable. | ||
Cookie cookie9 = new Cookie("name", Double.toString(c4)); // $hasWeakRandomFlow | ||
|
||
double c5 = Math.random(); | ||
// BAD: The cookie value may be predictable. | ||
Cookie cookie10 = new Cookie("name", Double.toString(++c5)); // $hasWeakRandomFlow | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/servlet-api-2.4:${testdir}/../../../stubs/apache-commons-lang3-3.7:${testdir}/../../../stubs/esapi-2.0.1 |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
1 change: 1 addition & 0 deletions
1
java/ql/test/stubs/esapi-2.0.1/org/owasp/esapi/reference/DefaultEncoder.java
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Check warning
Code scanning / CodeQL
Consistent alert message Warning