Java: CWE-798: Query to detect hard-coded SHIRO key by x-f1v3 · Pull Request #5907 · github/codeql · GitHub

x-f1v3 · 2021-05-17T08:26:32Z

Apache Shiro is a powerful and easy-to-use Java security framework that performs authentication, authorization, cryptography, and session management.

shiro allows user to set default cipher key for the “remember me” feature，
when a cipher key can be guessed , allows remote attackers to execute arbitrary code or bypass intended access restrictions via an unspecified request parameter.

The query detects a hard-coded cipher key for shiro.

smowton · 2021-05-17T10:48:08Z

java/ql/src/Security/CWE/CWE-798/HardcodedCredentialsApiCall.ql


  override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
    node1.asExpr().getType() instanceof TypeString and
-    exists(MethodAccess ma | ma.getMethod().hasName(["getBytes", "toCharArray"]) |


Autoformat (https://codeql.github.com/docs/codeql-cli/manual/query-format/)

smowton · 2021-05-17T10:51:44Z

java/ql/src/Security/CWE/CWE-798/HardcodedCredentialsApiCall.ql

  override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
    node1.asExpr().getType() instanceof TypeString and
-    exists(MethodAccess ma | ma.getMethod().hasName(["getBytes", "toCharArray"]) |
+    exists(MethodAccess ma | ma.getMethod().hasName(["getBytes", "toCharArray","decode"]) |


Be specific: what type's decode method are you referring to here? (the existing getBytes and toCharArray should be specific too in fairness, though no need to fiddle with that in this PR)

when i test cookieRememberMeManager.setCipherKey(Base64.decode("1234")) by the query without decode , it do not work. so i add 'decode' method as additional data flow. Other queries used to detect hard-coded may also encounter similar situations.

OK, in that case you should check if ma.getMethod().hasQualifiedName("java.util", "Base64$Decoder", "decode")

The existing getBytes and toCharArray methods should also be qualified that way; it isn't mandatory for you to fix that here though.

Shouldn't this work out-of-the-box?

codeql/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll

Lines 244 to 251 in 9b0e3b1

"java.util;Base64$Encoder;false;encode;(byte[]);;Argument[0];ReturnValue;taint",

"java.util;Base64$Encoder;false;encode;(ByteBuffer);;Argument[0];ReturnValue;taint",

"java.util;Base64$Encoder;false;encodeToString;(byte[]);;Argument[0];ReturnValue;taint",

"java.util;Base64$Encoder;false;wrap;(OutputStream);;Argument[0];ReturnValue;taint",

"java.util;Base64$Decoder;false;decode;(byte[]);;Argument[0];ReturnValue;taint",

"java.util;Base64$Decoder;false;decode;(ByteBuffer);;Argument[0];ReturnValue;taint",

"java.util;Base64$Decoder;false;decode;(String);;Argument[0];ReturnValue;taint",

"java.util;Base64$Decoder;false;wrap;(InputStream);;Argument[0];ReturnValue;taint",

Good spot, yes it should. Can you show a test case that requires this extra decode method to be modelled this way?

nice. but in some case, java.util.Base64$Decoder is not be used. for instance, org.apache.shiro.codec.Base64 in https://github.com/yinna/wjproject/blob/master/wj/src/main/java/com/example/wj/config/ShiroConfiguration.java,cn.hutool.core.codec.Base64 in https://github.com/ph-sir/usermg/blob/master/src/main/java/com/pengh/usermg/shiro/ShiroConfig.java, how to detect that?

Either we would welcome a PR to model Shiro and/or Hutool, providing model specifications similar to those that @intrigus-lgtm linked to, or if you just want to provide enough support for this query, write something like

ma.getMethod().hasQualifiedName(["org.apache.shiro.codec", "cn.hutool.core.codec"], "Base64", "decode")

The important thing is to specify particular methods you want to model, rather than guessing that any future method encountered that is named decode will behave the same way.

Got it.thanks for your advice

intrigus-lgtm · 2021-05-18T10:55:58Z

java/ql/src/Security/CWE/CWE-798/SensitiveApi.qll

  s = "sun.security.tools.keytool.Main;getKeyPasswd(String, String, char[]);2" or
-  s = "sun.security.x509.X509Key;decode(byte[]);0"
+  s = "sun.security.x509.X509Key;decode(byte[]);0" or
+  s = "org.apache.shiro.mgt.AbstractRememberMeManager;setCipherKey(byte[]);0"


This should get moved to the otherApiCallableCredentialParam method, because the javaApiCallableCryptoKeyParam method is auto-generated.

smowton · 2021-05-18T11:11:55Z

You should also add a change note, similar to #5852 which is making very similar changes.

intrigus-lgtm

Some questions.

intrigus-lgtm · 2021-05-24T11:13:43Z

java/ql/src/Security/CWE/CWE-798/HardcodedCredentialsApiCall.ql

+        ma.getQualifier() = node1.asExpr()
+      )
+      or
+      FlowSummaryImpl::Private::Steps::summaryThroughStep(node1, node2, false)


OOI: Why is this needed?

I modelled the decode methods of shiro and Hutool in ExternalFlow.qll and FlowSummaryImpl::Private::Steps::summaryThroughStep(node1, node2, false) is for using that

As far as I know this should work by default, so you don't have to manually call it.

I tried but it didn't work

This doesn't work by default, because

codeql/java/ql/src/semmle/code/java/dataflow/ExternalFlow.qll

Lines 251 to 252 in 4ac91ea

"cn.hutool.core.codec;Base64;true;decode;;;Argument[0];ReturnValue;taint",

"org.apache.shiro.codec;Base64;false;decode;(String);;Argument[0];ReturnValue;taint",

only applies to TaintTracking::Configurations but not DataFlow::Configurations.

I'm not sure whether changing this to a TaintTracking::Configuration would be the right action.

I think it would: dataflow configs track things with the same object identity, while taint tracking also allows related but different objects. Base64 decoding clearly produces a different object but is relevant to the query. Therefore taint tracking seems appropriate (cc @aschackmull to double check you're happy with this)

It depends on whether you want the complete set of default taint steps to apply to your configuration or whether you just want object identity tracking plus a few select steps. In the former case, use a TaintTracking::Configuration, in the latter case, use a DataFlow::Configuration and add steps as needed. In either case, you shouldn't reference internal libraries directly (e.g. FlowSummaryImpl).
In this case, as we're likely mostly dealing with strings, consider whether steps like string concatenation are valid taint steps, if they are, then a TaintTracking::Configuration is likely the right choice, but if string concatenation doesn't make sense for this flow config, then a DataFlow::Configuration must be used.

intrigus-lgtm · 2021-05-24T11:15:10Z

java/change-notes/2021-05-24-hardcoded-shiro-key-in-api-call.md

@@ -0,0 +1,3 @@
+lgtm,codescanning
+* The query "Hard-coded credential in API call" (`java/hardcoded-credential-api-call`)
+  This query finds hard-coded cipher key for shiro. This vulnerability can lead to arbitrary code execution.


How does this lead to arbitrary code execution?

if you know the cipher key for the remember me feature,you can deserialize any class on the server environment by the key to achieve Remote Code Execution.
actually,This vulnerability is very similar to CVE-2016-4437. The difference is that CVE-2016-4437 is hard-coded by the shiro framework, this vulnerability is hard-coded by the developer，
(In order to fix CVE-2016-4437, shiro used a random key, but still retains "setCipherKey" method to set the key)

smowton · 2021-06-01T09:26:30Z

java/change-notes/2021-05-24-hardcoded-shiro-key-in-api-call.md

+* The query "Hard-coded credential in API call" (`java/hardcoded-credential-api-call`)
+  This query finds hard-coded cipher key for shiro. This vulnerability can lead to arbitrary code execution.


Suggested change

* The query "Hard-coded credential in API call" (`java/hardcoded-credential-api-call`)

This query finds hard-coded cipher key for shiro. This vulnerability can lead to arbitrary code execution.

* The query "Hard-coded credential in API call" (`java/hardcoded-credential-api-call`) can now detect a hard-coded Apache Shiro cipher key.

smowton · 2021-06-01T09:30:14Z

java/ql/src/Security/CWE/CWE-798/HardcodedCredentialsApiCall.ql

+        ma.getQualifier() = node1.asExpr()
+      )
+      or
+      FlowSummaryImpl::Private::Steps::summaryThroughStep(node1, node2, false)


I think it would: dataflow configs track things with the same object identity, while taint tracking also allows related but different objects. Base64 decoding clearly produces a different object but is relevant to the query. Therefore taint tracking seems appropriate (cc @aschackmull to double check you're happy with this)

…ep for hardcoded credentials

smowton · 2021-09-30T14:01:57Z

Updated this and will merge on green

github-actions · 2021-09-30T14:02:03Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. The following differences were found:

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    Others,"``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.opensymphony.xwork2.ognl``, ``com.unboundid.ldap.sdk``, ``flexjson``, ``groovy.lang``, ``groovy.util``, ``jodd.json``, ``net.sf.saxon.s9api``, ``ognl``, ``org.apache.commons.codec``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.ognl``, ``org.apache.directory.ldap.client.api``, ``org.apache.ibatis.jdbc``, ``org.apache.shiro.jndi``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.hibernate``, ``org.jooq``, ``org.mvel2``, ``org.xml.sax``, ``org.xmlpull.v1``, ``play.mvc``",7,26,151,,,,14,18,,
+    Others,"``cn.hutool.core.codec``, ``com.esotericsoftware.kryo.io``, ``com.esotericsoftware.kryo5.io``, ``com.fasterxml.jackson.core``, ``com.fasterxml.jackson.databind``, ``com.opensymphony.xwork2.ognl``, ``com.unboundid.ldap.sdk``, ``flexjson``, ``groovy.lang``, ``groovy.util``, ``jodd.json``, ``net.sf.saxon.s9api``, ``ognl``, ``org.apache.commons.codec``, ``org.apache.commons.jexl2``, ``org.apache.commons.jexl3``, ``org.apache.commons.ognl``, ``org.apache.directory.ldap.client.api``, ``org.apache.ibatis.jdbc``, ``org.apache.shiro.codec``, ``org.apache.shiro.jndi``, ``org.codehaus.groovy.control``, ``org.dom4j``, ``org.hibernate``, ``org.jooq``, ``org.mvel2``, ``org.xml.sax``, ``org.xmlpull.v1``, ``play.mvc``",7,28,151,,,,14,18,,
-    Totals,,143,4984,408,13,6,10,107,33,1,66
+    Totals,,143,4986,408,13,6,10,107,33,1,66

Changes to framework-coverage-java.csv:

+ cn.hutool.core.codec,,,1,,,,,,,,,,,,,,,,,,,,,1,
+ org.apache.shiro.codec,,,1,,,,,,,,,,,,,,,,,,,,,1,

x-f1v3 requested a review from a team as a code owner May 17, 2021 08:26

github-actions bot added the Java label May 17, 2021

This was referenced May 17, 2021

[Java] CWE-798: Query to detect hard-coded SHIRO key github/securitylab#369

Closed

[Java] CWE-798: Query to detect hard-coded SHIRO key github/securitylab#370

Closed

smowton reviewed May 17, 2021

View reviewed changes

intrigus-lgtm reviewed May 18, 2021

View reviewed changes

github-actions bot added the documentation label May 24, 2021

intrigus-lgtm reviewed May 24, 2021

View reviewed changes

smowton reviewed Jun 1, 2021

View reviewed changes

x-f1v3 and others added 5 commits September 30, 2021 14:22

detects a hard-coded cipher key for shiro

f3bde56

Apply suggestions from code review

168fc41

autoformat

24c9bb2

Amend change note

b57a58c

Specifically include Base64 encode/decode as a likely intermediate st…

b0983cb

…ep for hardcoded credentials

smowton force-pushed the java/hardcoded-shiro-key branch from 4ac91ea to b0983cb Compare September 30, 2021 13:59

Update change note; drop unnecessary import

cb4ce36

Fix typo

ec4cb7c

smowton approved these changes Sep 30, 2021

View reviewed changes

smowton merged commit f48c418 into github:main Sep 30, 2021

	"java.util;Base64$Encoder;false;encode;(byte[]);;Argument[0];ReturnValue;taint",
	"java.util;Base64$Encoder;false;encode;(ByteBuffer);;Argument[0];ReturnValue;taint",
	"java.util;Base64$Encoder;false;encodeToString;(byte[]);;Argument[0];ReturnValue;taint",
	"java.util;Base64$Encoder;false;wrap;(OutputStream);;Argument[0];ReturnValue;taint",
	"java.util;Base64$Decoder;false;decode;(byte[]);;Argument[0];ReturnValue;taint",
	"java.util;Base64$Decoder;false;decode;(ByteBuffer);;Argument[0];ReturnValue;taint",
	"java.util;Base64$Decoder;false;decode;(String);;Argument[0];ReturnValue;taint",
	"java.util;Base64$Decoder;false;wrap;(InputStream);;Argument[0];ReturnValue;taint",

	"cn.hutool.core.codec;Base64;true;decode;;;Argument[0];ReturnValue;taint",
	"org.apache.shiro.codec;Base64;false;decode;(String);;Argument[0];ReturnValue;taint",

		* The query "Hard-coded credential in API call" (`java/hardcoded-credential-api-call`)
		This query finds hard-coded cipher key for shiro. This vulnerability can lead to arbitrary code execution. No newline at end of file

Conversation

x-f1v3 commented May 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

intrigus-lgtm May 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smowton commented May 18, 2021

Uh oh!

intrigus-lgtm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

intrigus-lgtm May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smowton commented Sep 30, 2021

Uh oh!

github-actions bot commented Sep 30, 2021

java

Generated file changes for java

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

intrigus-lgtm May 18, 2021 •

edited

Loading

intrigus-lgtm May 24, 2021 •

edited

Loading