Java: QL Query to Detect Security Sensitive non-CSPRNG usage #2694

JLLeitschuh · 2020-01-24T21:24:16Z

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, or java.util.concurrent.ThreadLocalRandom in 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.

/**
 * A 'Sensitive Name' is something that indicates that the name has some sort of security
 * implication associated with it's usage.
 */
bindingset[name]
private predicate isSensitiveName(string name) {
  name.matches("%pass%") // also matches password
  or
  name.matches("%tok%") // also matches token
  or
  name.matches("%secret%")
  or
  name.matches("%reset%key%") and not name.matches("%value%") // Reduce false positive from 'keyValue' type of methods from maps
  or
  name.matches("%cred%") // also matches credentials
  or
  name.matches("%auth%") // also matches authentication
  or
  name.matches("%sess%id%") // also matches sessionid
}

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 Random instance is actually a SecureRandom type at runtime is also inherently difficult. Currently, I'm only considering a Random variable 'safe' if at any point it's assigned as a final variable.

For example, these are 'safe' random values:

class Tester {
    final Random random = new SecureRandom();
    final Random random2;

    Tester() {
        this.random2 = new SecureRandom();
    }
}

If there's a better way to guess the type of a Random expression, 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 random type is ambiguous in my query because there's a setRandom method 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 generateLowercaseChar will always be passed a SecureRandom instance.

Elasticsearch/Elasticsearch

https://github.com/elastic/elasticsearch/blob/a5c40b3d828fe6cc5d722ec134305864d31c6515/server/src/main/java/org/elasticsearch/common/RandomBasedUUIDGenerator.java

Can't track that the Random instance passed to getUUIDBytes will always be an instance of SecureRandom.

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

JLLeitschuh

Just some initial thoughts and self-review

java/ql/src/Security/CWE/CWE-338/PredictableRandomNumberGenerator.java

java/ql/src/Security/CWE/CWE-338/PredictableRandomNumberGenerator.qhelp

JLLeitschuh · 2020-01-24T21:56:48Z

java/ql/src/Security/CWE/CWE-338/PredictableRandomNumberGeneratorSource.qll

+  )
+}
+
+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
+  }
+}


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

JLLeitschuh · 2020-01-24T21:57:07Z

java/ql/src/Security/CWE/CWE-338/types.qll

+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")
+  }
+}


Another file that should get moved.

Add java.util.SplittableRandom?

JLLeitschuh · 2020-01-24T22:48:13Z

java/ql/src/Security/CWE/CWE-338/PredictableRandomNumberGeneratorSource.qll

+/**
+ * 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
+    )
+  }
+}


Just discovered that this has a false positive on:

Random random = new SecureRandom(); byte[] array = new byte[]; random.nextBytes(array);

aschackmull · 2020-01-28T13:28:45Z

If there's a better way to guess the type of a Random expression, I'd love to use it.

We have the TypeFlow library, which can do a bit in terms of giving improved type bounds for expressions and fields. It's currently mostly used as part of the virtual dispatch resolution, but it should also be a fine stand-alone library. Just import semmle.code.java.dataflow.TypeFlow and try out the predicate exprTypeFlow(Expr e, RefType t, boolean exact) and predicate fieldTypeFlow(Field f, RefType t, boolean exact) predicates. Note that those predicates only have results for expressions and fields for which the library has determined a type bound that's likely to be better than the static type.

yo-h · 2020-01-28T16:32:40Z

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 (java/insecure-randomness), or whether there is a good reason to differentiate this query from those in the other languages.

JLLeitschuh · 2020-01-29T02:22:03Z

@aschackmull The exprTypeFlow was a good tip, thanks.

Got anything for this?

private static final SecureRandom RANDOM_INSTANCE = new SecureRandom();
private String generateString(Random random) {
 byte[] array = new byte[];
 random.nextBytes(array);
 return new String(array);
}

public String generateSecure() { // Thinks this method is tainted
  return generateString(RANDOM_INSTANCE);
}

I'm running into this case still with Elasticsearch:
https://github.com/elastic/elasticsearch/blob/a5c40b3d828fe6cc5d722ec134305864d31c6515/server/src/main/java/org/elasticsearch/common/RandomBasedUUIDGenerator.java

aschackmull · 2020-01-29T13:11:22Z

Your example code snippet should work with exprTypeFlow. However, a prerequisite for inferring a type bound for the parameter is that generateString is private - otherwise it might be called from someplace else with something that isn't a SecureRandom.
Abstracting a bit, the typeflow library is designed for universal flow, i.e. merge points in the flow graph are treated with universal quantification, whereas the general dataflow library essentially uses existential quantification.
If you're generating too many FPs, it might be worth it to consider whether a result should be ruled out by the existence of a SecureRandom that reaches the relevant node, instead of trying to universally prove that all Randoms that get to that point are in fact SecureRandom. This would be slightly more heuristic, but might work better if you're seeing too many FPs.

aschackmull · 2020-01-29T13:14:33Z

In the ElasticSearch example, the fact that getBase64UUID is public means that we cannot give a tighter bound on the type of the parameter in getUUIDBytes.

aschackmull · 2020-01-29T13:21:14Z

If the relevant FP in ElasticSearch is only in getBase64UUIDSecureString via a data flow return from getUUIDBytes then it is theoretically possible to combine the type flow analysis with the subgraph of data flow related to the source-sink pair and get a better type bound that way. I have plans to do this in order to improve virtual dispatch in data flow, but for this use case it would probably need to be exposed in a slightly different way. These sorts of extensions to data flow are, however, quite non-trivial.

aschackmull · 2020-01-29T13:28:29Z

An alternative approach is to perhaps make method calls like nextBytes into additional steps instead of sources, and trace the source further back to a constructor call that definitely isn't new SecureRandom, but rather e.g. new Random. That would deal with the ElasticSearch FP, I think.

pwntester · 2020-02-11T12:27:24Z

java/ql/src/Security/CWE/CWE-338/types.qll

+  ApacheRandomStringUtilsType() {
+    this.hasQualifiedName("org.apache.commons.lang", "RandomStringUtils") or
+    this.hasQualifiedName("org.apache.commons.lang3", "RandomStringUtils")
+  }


Have you tried non-string variants (eg: org.apache.commons.lang3.RandomUtils)? do they generate a high rate of FPs?

Can you elaborate on what you mean?

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?

Huh, nope, I just didn't know it existed, thanks!

JLLeitschuh · 2020-02-11T12:31:44Z

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

xcorail · 2020-07-08T23:18:45Z

This is still a WIP, I'll be coming back to it soon.

Hey @JLLeitschuh do you intend to come back to this PR?

JLLeitschuh · 2020-07-08T23:24:05Z

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

FWIW, I'm waiting to review the text until this PR is nearly ready to be merged.

JLLeitschuh · 2021-01-21T13:49:17Z

I will get back to this eventually

JLLeitschuh · 2024-02-06T23:55:54Z

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

Java: Initial add of Security Sensitive non-CSPRNG usage

4e8ea28

JLLeitschuh requested review from felicitymay and a team as code owners January 24, 2020 21:24

JLLeitschuh commented Jan 24, 2020

View reviewed changes

JLLeitschuh mentioned this pull request Jan 24, 2020

Java: PRNG used when CSPRNG is required github/securitylab#30

Closed

1 task

Apply suggestions from self code review

c2bfa22

felicitymay added the Java label Feb 5, 2020

pwntester reviewed Feb 11, 2020

View reviewed changes

Rewrite random query for stricter checks

cd1b8a7

JLLeitschuh requested a review from a team as a code owner June 16, 2020 01:37

felicitymay reviewed Jul 9, 2020

View reviewed changes

adityasharad changed the base branch from master to main August 14, 2020 18:34

JLLeitschuh mentioned this pull request Oct 2, 2020

Java: QL Query Detector for JHipster Generated CVE-2019-16303 #4312

Merged

ghost approved these changes Dec 22, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java: QL Query to Detect Security Sensitive non-CSPRNG usage #2694

Java: QL Query to Detect Security Sensitive non-CSPRNG usage #2694

JLLeitschuh commented Jan 24, 2020 •

edited

Loading

JLLeitschuh left a comment

JLLeitschuh Jan 24, 2020

JLLeitschuh Jan 24, 2020

pwntester Feb 11, 2020

JLLeitschuh Jan 24, 2020 •

edited

Loading

aschackmull commented Jan 28, 2020

yo-h commented Jan 28, 2020

JLLeitschuh commented Jan 29, 2020

aschackmull commented Jan 29, 2020

aschackmull commented Jan 29, 2020

aschackmull commented Jan 29, 2020

aschackmull commented Jan 29, 2020

pwntester Feb 11, 2020 •

edited

Loading

JLLeitschuh Feb 11, 2020

pwntester Feb 11, 2020

JLLeitschuh Feb 11, 2020

JLLeitschuh commented Feb 11, 2020

xcorail commented Jul 8, 2020

JLLeitschuh commented Jul 8, 2020

felicitymay left a comment

JLLeitschuh commented Jan 21, 2021

JLLeitschuh commented Feb 6, 2024 •

edited

Loading

Java: QL Query to Detect Security Sensitive non-CSPRNG usage #2694

Are you sure you want to change the base?

Java: QL Query to Detect Security Sensitive non-CSPRNG usage #2694

Conversation

JLLeitschuh commented Jan 24, 2020 • edited Loading

GitHub Security Lab BB Submission

Design

Query Current Status

Known False Positives

Eclipse/Jetty

Apache/shiro

Apache/cloudstack

Elasticsearch/Elasticsearch

More... TODO

Test Repository

JLLeitschuh left a comment

Choose a reason for hiding this comment

JLLeitschuh Jan 24, 2020

Choose a reason for hiding this comment

JLLeitschuh Jan 24, 2020

Choose a reason for hiding this comment

pwntester Feb 11, 2020

Choose a reason for hiding this comment

JLLeitschuh Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

aschackmull commented Jan 28, 2020

yo-h commented Jan 28, 2020

JLLeitschuh commented Jan 29, 2020

aschackmull commented Jan 29, 2020

aschackmull commented Jan 29, 2020

aschackmull commented Jan 29, 2020

aschackmull commented Jan 29, 2020

pwntester Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

JLLeitschuh Feb 11, 2020

Choose a reason for hiding this comment

pwntester Feb 11, 2020

Choose a reason for hiding this comment

JLLeitschuh Feb 11, 2020

Choose a reason for hiding this comment

JLLeitschuh commented Feb 11, 2020

xcorail commented Jul 8, 2020

JLLeitschuh commented Jul 8, 2020

felicitymay left a comment

Choose a reason for hiding this comment

JLLeitschuh commented Jan 21, 2021

JLLeitschuh commented Feb 6, 2024 • edited Loading

JLLeitschuh commented Jan 24, 2020 •

edited

Loading

JLLeitschuh Jan 24, 2020 •

edited

Loading

pwntester Feb 11, 2020 •

edited

Loading

JLLeitschuh commented Feb 6, 2024 •

edited

Loading