Java: QL Query to Detect Security Sensitive non-CSPRNG usage #2694
Conversation
JLLeitschuh left a comment
JLLeitschuh left a comment There was a problem hiding this comment.
Just some initial thoughts and self-review
java/ql/src/Security/CWE/CWE-338/PredictableRandomNumberGenerator.java Outdated Show resolved Hide resolved
java/ql/src/Security/CWE/CWE-338/PredictableRandomNumberGenerator.qhelp Outdated Show resolved Hide resolved
| import java | ||
| import types | ||
| import semmle.code.java.dataflow.FlowSources | ||
| import DataFlow::PathGraph | ||
| import semmle.code.java.dataflow.FlowSources | ||
| import semmle.code.java.dataflow.TaintTracking2 | ||
| | ||
| abstract class PredictableRandomFlowSource extends DataFlow::Node { } | ||
| | ||
| abstract class PredictableRandomMethodAccess extends MethodAccess { } | ||
| | ||
| private class PredictableApacheRandomStringUtilsMethod extends Method { | ||
| PredictableApacheRandomStringUtilsMethod() { | ||
| this.getDeclaringType() instanceof ApacheRandomStringUtilsType | ||
| } | ||
| } | ||
| | ||
| private class PredictableApacheRandomStringUtilsMethodAccess extends PredictableRandomMethodAccess { | ||
| PredictableApacheRandomStringUtilsMethodAccess() { | ||
| this.getMethod() instanceof PredictableApacheRandomStringUtilsMethod and | ||
| // The one valid use of this type that uses SecureRandom as a source of data. | ||
| not ( | ||
| this.getMethod().getName() = "random" and | ||
| ( | ||
| this.getArgument(6).getProperExpr().getType() instanceof TypeSecureRandom or | ||
| isSecureRuntimeVarAccess(this.getArgument(6).getProperExpr()) | ||
| ) | ||
| ) | ||
| } | ||
| } | ||
| | ||
| private class FinalSecureRandomVariableAssignment extends VariableAssign { | ||
| FinalSecureRandomVariableAssignment() { | ||
| this.getSource().getType() instanceof TypeSecureRandom and | ||
| this.getDestVar().isFinal() | ||
| } | ||
| } | ||
| | ||
| /** | ||
| * Determines if this VarAccess can be safely assumed to be 'Secure' | ||
| * at runtime due to the implied type of the variable due to assignments. | ||
| */ | ||
| private predicate isSecureRuntimeVarAccess(VarAccess varAccess) { | ||
| /* | ||
| * TODO: Perhaps also check for multiple constructors that assign different things. | ||
| * This currently verifies that there is at least one final assignment that is 'Secure'. | ||
| * This does not verify that this is the _exclusive_ assignment to this variable. | ||
| * | ||
| * In reality, we are trying to guess at what the type will be at runtime, which is a turning | ||
| * complete problem, so we can only use this to eliminate the common cases. | ||
| */ | ||
| | ||
| exists(FinalSecureRandomVariableAssignment finalSecureVarAssign | | ||
| varAccess.getVariable() = finalSecureVarAssign.getDestVar() | ||
| ) | ||
| } | ||
| | ||
| private class PredictableJavaStdlibRandomMethodAcccess extends PredictableRandomMethodAccess { | ||
| PredictableJavaStdlibRandomMethodAcccess() { | ||
| this.getReceiverType() instanceof JavaStdlibInsecureRandomType and | ||
| not isSecureRuntimeVarAccess(this.getQualifier()) | ||
| } | ||
| } | ||
| | ||
| /** | ||
| * A byte array tainted by insecure RNG. | ||
| * | ||
| * Examples: | ||
| * | ||
| * ``` | ||
| * byte[] array = new byte[]; | ||
| * new Random().nextBytes(array); | ||
| * ``` | ||
| */ | ||
| private class TaintedByteArrayWrite extends VarAccess { | ||
| TaintedByteArrayWrite() { | ||
| exists(PredictableJavaStdlibRandomMethodAcccess insecureMethodAccess, MethodAccess ma | | ||
| ma.getMethod().hasName("nextBytes") and | ||
| ma = insecureMethodAccess and | ||
| ma.getArgument(0) = this | ||
| ) | ||
| } | ||
| } | ||
| | ||
| class SecureRandomMethodAccess extends MethodAccess { | ||
| SecureRandomMethodAccess() { this.getReceiverType() instanceof TypeSecureRandom } | ||
| } | ||
| | ||
| /** | ||
| * Intermediate tracking step to find instances of RandomStringGenerator that are "safely" created. | ||
| * A "safely" created RandomStringGenerator is defined as having used a SecureRandom method in the | ||
| * RandomStringGenerator.Builder::usingRandom lambda. | ||
| */ | ||
| class SafeRandomStringGeneratorFlowConfig extends TaintTracking2::Configuration { | ||
| SafeRandomStringGeneratorFlowConfig() { this = "RNG:SafeRandomStringGeneratorFlowConfig" } | ||
| | ||
| private predicate isUsingSafeUsingRandomSupplier(MethodAccess source) { | ||
| exists( | ||
| ApacheRandomStringGeneratorBuilderUsingRandomMethod usingRandomMethod, | ||
| FunctionalExpr functionalExpr, SecureRandomMethodAccess secureRandomMethodAccess | ||
| | | ||
| usingRandomMethod = source.getMethod() and | ||
| source.getArgument(0).getProperExpr() = functionalExpr and | ||
| // Some SecureRandom method is used in the scope of the functional expression | ||
| functionalExpr.asMethod() = secureRandomMethodAccess.getEnclosingCallable() | ||
| ) | ||
| } | ||
| | ||
| override predicate isSource(DataFlow::Node src) { isUsingSafeUsingRandomSupplier(src.asExpr()) } | ||
| | ||
| /** | ||
| * A sink is any RandomStringGenerator::generate method. | ||
| */ | ||
| override predicate isSink(DataFlow::Node sink) { | ||
| exists(ApacheRandomStringGeneratorType generatorType | | ||
| sink.asExpr().(MethodAccess).getMethod() = generatorType.getGenerateMethod() | ||
| ) | ||
| } | ||
| | ||
| private predicate isBuilderStep(MethodAccess previous, MethodAccess next) { | ||
| exists(ApacheRandomStringGeneratorBuilderType builderType | | ||
| builderType.getAMethod() = previous.getMethod() and | ||
| previous = next.getQualifier() | ||
| ) | ||
| } | ||
| | ||
| private predicate isGeneratorCallStep(Expr previous, MethodAccess next) { | ||
| exists(ApacheRandomStringGeneratorType generatorType | | ||
| generatorType.getGenerateMethod() = next.getMethod() and | ||
| previous = next.getQualifier() | ||
| ) | ||
| } | ||
| | ||
| override predicate isAdditionalTaintStep(DataFlow::Node previous, DataFlow::Node next) { | ||
| isBuilderStep(previous.asExpr(), next.asExpr()) or | ||
| isGeneratorCallStep(previous.asExpr(), next.asExpr()) | ||
| } | ||
| } | ||
| | ||
| private class TaintedRandomStringGeneratorMethodAccess extends PredictableRandomMethodAccess { | ||
| TaintedRandomStringGeneratorMethodAccess() { | ||
| this.getMethod().getDeclaringType() instanceof ApacheRandomStringGeneratorType and | ||
| not exists(SafeRandomStringGeneratorFlowConfig safeFlowSource | | ||
| safeFlowSource.hasFlowTo(DataFlow::exprNode(this)) | ||
| ) | ||
| } | ||
| } | ||
| | ||
| private class PredictableRandomTaintedMethodAccessSource extends PredictableRandomFlowSource { | ||
| PredictableRandomTaintedMethodAccessSource() { | ||
| this.asExpr() instanceof PredictableRandomMethodAccess or | ||
| this.asExpr() instanceof TaintedByteArrayWrite | ||
| } | ||
| } |
There was a problem hiding this comment.
This file should probably live somewhere else (and maybe be broken up), but leaving it here for now allows me to continue to iterate on the query against LGTM.com
| import java | ||
| | ||
| class UUIDCreationExp extends ClassInstanceExpr { | ||
| UUIDCreationExp() { this.getConstructedType() instanceof TypeUUID } | ||
| } | ||
| | ||
| class ApacheRandomStringUtilsType extends RefType { | ||
| ApacheRandomStringUtilsType() { | ||
| this.hasQualifiedName("org.apache.commons.lang", "RandomStringUtils") or | ||
| this.hasQualifiedName("org.apache.commons.lang3", "RandomStringUtils") | ||
| } | ||
| } | ||
| | ||
| class ApacheRandomStringGeneratorType extends RefType { | ||
| ApacheRandomStringGeneratorType() { | ||
| this.hasQualifiedName("org.apache.commons.text", "RandomStringGenerator") | ||
| } | ||
| | ||
| Method getGenerateMethod() { | ||
| exists(Method m | | ||
| m.getDeclaringType() = this and | ||
| m.hasName("generate") and | ||
| m = result | ||
| ) | ||
| } | ||
| } | ||
| | ||
| class ApacheRandomStringGeneratorBuilderType extends RefType { | ||
| ApacheRandomStringGeneratorBuilderType() { | ||
| this.hasQualifiedName("org.apache.commons.text", "RandomStringGenerator$Builder") | ||
| } | ||
| } | ||
| | ||
| class ApacheRandomStringGeneratorBuilderUsingRandomMethod extends Method { | ||
| ApacheRandomStringGeneratorBuilderUsingRandomMethod() { | ||
| this.getDeclaringType() instanceof ApacheRandomStringGeneratorBuilderType and | ||
| this.hasName("usingRandom") | ||
| } | ||
| } | ||
| | ||
| class ApacheTextRandomProviderType extends RefType { | ||
| ApacheTextRandomProviderType() { | ||
| this.hasQualifiedName("org.apache.commons.text", "TextRandomProvider") | ||
| } | ||
| } | ||
| | ||
| class JavaStdlibInsecureRandomType extends RefType { | ||
| JavaStdlibInsecureRandomType() { | ||
| this.hasQualifiedName("java.util.concurrent", "ThreadLocalRandom") or | ||
| this.hasQualifiedName("java.util", "Random") | ||
| } | ||
| } |
There was a problem hiding this comment.
Another file that should get moved.
There was a problem hiding this comment.
Add java.util.SplittableRandom?
| /** | ||
| * A byte array tainted by insecure RNG. | ||
| * | ||
| * Examples: | ||
| * | ||
| * ``` | ||
| * byte[] array = new byte[]; | ||
| * new Random().nextBytes(array); | ||
| * ``` | ||
| */ | ||
| private class TaintedByteArrayWrite extends VarAccess { | ||
| TaintedByteArrayWrite() { | ||
| exists(PredictableJavaStdlibRandomMethodAcccess insecureMethodAccess, MethodAccess ma | | ||
| ma.getMethod().hasName("nextBytes") and | ||
| ma = insecureMethodAccess and | ||
| ma.getArgument(0) = this | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Just discovered that this has a false positive on:
Random random = new SecureRandom(); byte[] array = new byte[]; random.nextBytes(array); 
We have the |
| There are a couple of similar "insecure randomness" queries for other languages. Prior to merging, we should consider whether to give this query the same ID ( |
| @aschackmull The Got anything for this? I'm running into this case still with Elasticsearch: |
| Your example code snippet should work with |
| In the ElasticSearch example, the fact that |
| If the relevant FP in ElasticSearch is only in |
| An alternative approach is to perhaps make method calls like |
| ApacheRandomStringUtilsType() { | ||
| this.hasQualifiedName("org.apache.commons.lang", "RandomStringUtils") or | ||
| this.hasQualifiedName("org.apache.commons.lang3", "RandomStringUtils") | ||
| } |
There was a problem hiding this comment.
Have you tried non-string variants (eg: org.apache.commons.lang3.RandomUtils)? do they generate a high rate of FPs?
There was a problem hiding this comment.
Can you elaborate on what you mean?
There was a problem hiding this comment.
You are accounting for org.apache.commons.lang3.RandomStringUtils, but not for org.apache.commons.lang3.RandomUtils. Did you miss that class or did you discard it because it was generating too many false positives?
There was a problem hiding this comment.
Huh, nope, I just didn't know it existed, thanks!
| This is still a WIP, I'll be coming back to it soon. |
…neration * master: (2719 commits) C++: Fix QLDoc on `FormattingFunction` library Update docs/language/ql-handbook/name-resolution.rst Mention `libraryPathDependencies` JS: Update another test JS: Autoformat JS: Update test QL reference: Update process for name resolution JS: Propagate locally returned functions out of calls JS: Add test Add link to Java regex Pattern documentation to language.rst Fix typo C++: Fix reference to `Block` JS: Fix a misleading javadoc comment CodeQL for Go: Edit AST reference C#: Rename a class C#: Recognize more calls to `IHtmlHelper.Raw` Give general syntax instead of examples for exprs Add more links to java AST class reference Move explicit hyperlink targets to the bottom Add references to the AST class reference for go ...
Hey @JLLeitschuh do you intend to come back to this PR? |
Very much so. It's been on the back-burner of my mind for the past few months. I probably won't have time/energy to finish it until after August. |
felicitymay left a comment
felicitymay left a comment There was a problem hiding this comment.
FWIW, I'm waiting to review the text until this PR is nearly ready to be merged.
| I will get back to this eventually |
| @egregius313 rather old query, but you may find some things valuable, and worth including in the current variant of this query that ended up being merged. |
6 participants
GitHub Security Lab BB Submission
The goal of this query is to detect the use of a PRNG like
java.util.Random,org.apache.commons.lang.RandomStringUtils,org.apache.commons.text.RandomStringGenerator, orjava.util.concurrent.ThreadLocalRandomin a security sensitive context.This vulnerability can have up to a CVSSv3 score of 9.8/10 depending upon the use of the insecure data generated.
Design
The design of the query is to find strings that are generated with some sort of insecure randomess component. Then use taint tracking to see if that string ever taints a value that has some sort of security sensitive indicator.
Currently the indicators I'm using are variables/methods that (when lowercase) match a name with the following predicate.
I've debated adding
%accnt%,%account%, and%trusted%as well.Query Current Status
I'm currently not totally happy with the false-positive rate that this query produces and I'm looking for support/suggestions on how to increase its accuracy.
Additionally, because my 'sink' locations are inherently fuzzy you end up with multiple sink locations for the same source.
Determining whether or not a
Randominstance is actually aSecureRandomtype at runtime is also inherently difficult. Currently, I'm only considering aRandomvariable 'safe' if at any point it's assigned as a final variable.For example, these are 'safe' random values:
If there's a better way to guess the type of a
Randomexpression, I'd love to use it.Known False Positives
Eclipse/Jetty
The way that Jetty sets it's random number generator is inherently 'safe', in probably 99% of cases.
https://github.com/eclipse/jetty.project/blob/69808d3851de31ffffb343115030d7bcf826ed01/jetty-server/src/main/java/org/eclipse/jetty/server/session/DefaultSessionIdManager.java#L367-L384
Query Results: https://lgtm.com/query/1859599740827953942/
Apache/shiro
https://github.com/apache/shiro/blob/cb3a1b3c0dcd8b89cc8d51ee9ede086882921faa/core/src/main/java/org/apache/shiro/session/mgt/eis/RandomSessionIdGenerator.java
Random randomtype is ambiguous in my query because there's asetRandommethod that could be called by the user.Apache/cloudstack
https://github.com/apache/cloudstack/blob/ac202639a5108d126ecea585c7e793696679dbe4/utils/src/main/java/com/cloud/utils/PasswordGenerator.java#L53-L107
Can't track that the
generateLowercaseCharwill always be passed aSecureRandominstance.Elasticsearch/Elasticsearch
https://github.com/elastic/elasticsearch/blob/a5c40b3d828fe6cc5d722ec134305864d31c6515/server/src/main/java/org/elasticsearch/common/RandomBasedUUIDGenerator.java
Can't track that the
Randominstance passed togetUUIDByteswill always be an instance ofSecureRandom.More... TODO
Test Repository
I've been testing this query against the code I've been uploading to this repository. Ideally, this repository would get contributed as a part of the tests for this PR when it's complete.
See: https://github.com/JLLeitschuh/bad-random-examples