From 168fc4170db536c72aa660eafc1696bbb423b9a2 Mon Sep 17 00:00:00 2001 From: f1v3 Date: Mon, 24 May 2021 17:02:06 +0800 Subject: [PATCH] Apply suggestions from code review --- ...1-05-24-hardcoded-shiro-key-in-api-call.md | 3 ++ .../code/java/dataflow/ExternalFlow.qll | 2 + .../CWE-798/HardcodedCredentialsApiCall.ql | 8 ++-- .../src/Security/CWE/CWE-798/SensitiveApi.qll | 6 +-- .../HardcodedCredentialsApiCall.expected | 20 +++++---- .../semmle/tests/HardcodedShiroKey.java | 42 +++++++++---------- .../org/apache/shiro/codec/Base64.java | 36 ++++++++++++++++ 7 files changed, 81 insertions(+), 36 deletions(-) create mode 100644 java/change-notes/2021-05-24-hardcoded-shiro-key-in-api-call.md create mode 100644 java/ql/test/stubs/shiro-core-1.4.0/org/apache/shiro/codec/Base64.java diff --git a/java/change-notes/2021-05-24-hardcoded-shiro-key-in-api-call.md b/java/change-notes/2021-05-24-hardcoded-shiro-key-in-api-call.md new file mode 100644 index 00000000000..929f6ee9aa4 --- /dev/null +++ b/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. \ No newline at end of file diff --git a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll index 7f46f2d79b6..f9b1c22c1f0 100644 --- a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll @@ -309,6 +309,8 @@ private predicate summaryModelCsv(string row) { "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", "org.apache.commons.codec;Encoder;true;encode;(Object);;Argument[0];ReturnValue;taint", "org.apache.commons.codec;Decoder;true;decode;(Object);;Argument[0];ReturnValue;taint", "org.apache.commons.codec;BinaryEncoder;true;encode;(byte[]);;Argument[0];ReturnValue;taint", diff --git a/java/ql/src/Security/CWE/CWE-798/HardcodedCredentialsApiCall.ql b/java/ql/src/Security/CWE/CWE-798/HardcodedCredentialsApiCall.ql index 223eacede87..3b2f20f228a 100644 --- a/java/ql/src/Security/CWE/CWE-798/HardcodedCredentialsApiCall.ql +++ b/java/ql/src/Security/CWE/CWE-798/HardcodedCredentialsApiCall.ql @@ -14,6 +14,8 @@ import java import semmle.code.java.dataflow.DataFlow import HardcodedCredentials import DataFlow::PathGraph +import semmle.code.java.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl + class HardcodedCredentialApiCallConfiguration extends DataFlow::Configuration { HardcodedCredentialApiCallConfiguration() { this = "HardcodedCredentialApiCallConfiguration" } @@ -27,10 +29,10 @@ class HardcodedCredentialApiCallConfiguration extends DataFlow::Configuration { override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { node1.asExpr().getType() instanceof TypeString and - exists(MethodAccess ma | ma.getMethod().hasName(["getBytes", "toCharArray","decode"]) | + (exists(MethodAccess ma | ma.getMethod().hasName(["getBytes", "toCharArray"]) | node2.asExpr() = ma and - (ma.getQualifier() = node1.asExpr() or ma.getAnArgument() = node1.asExpr()) - ) + ma.getQualifier() = node1.asExpr()) or FlowSummaryImpl::Private::Steps::summaryThroughStep(node1, node2, false)) + } override predicate isBarrier(DataFlow::Node n) { diff --git a/java/ql/src/Security/CWE/CWE-798/SensitiveApi.qll b/java/ql/src/Security/CWE/CWE-798/SensitiveApi.qll index 60a62d8831e..63b1ffd1ad8 100644 --- a/java/ql/src/Security/CWE/CWE-798/SensitiveApi.qll +++ b/java/ql/src/Security/CWE/CWE-798/SensitiveApi.qll @@ -490,8 +490,7 @@ private predicate javaApiCallableCryptoKeyParam(string s) { s = "sun.security.provider.JavaKeyStore;engineSetKeyEntry(String, byte[], Certificate[]);1" or s = "sun.security.tools.keytool.Main;recoverKey(String, char[], char[]);2" or s = "sun.security.tools.keytool.Main;getKeyPasswd(String, String, char[]);2" or - s = "sun.security.x509.X509Key;decode(byte[]);0" or - s = "org.apache.shiro.mgt.AbstractRememberMeManager;setCipherKey(byte[]);0" + s = "sun.security.x509.X509Key;decode(byte[]);0" } /** @@ -514,5 +513,6 @@ private predicate otherApiCallableCredentialParam(string s) { s = "com.amazonaws.auth.BasicAWSCredentials;BasicAWSCredentials(String, String);1" or s = "com.azure.identity.UsernamePasswordCredentialBuilder;username(String);0" or s = "com.azure.identity.UsernamePasswordCredentialBuilder;password(String);0" or - s = "com.azure.identity.ClientSecretCredentialBuilder;clientSecret(String);0" + s = "com.azure.identity.ClientSecretCredentialBuilder;clientSecret(String);0" or + s = "org.apache.shiro.mgt.AbstractRememberMeManager;setCipherKey(byte[]);0" } diff --git a/java/ql/test/query-tests/security/CWE-798/semmle/tests/HardcodedCredentialsApiCall.expected b/java/ql/test/query-tests/security/CWE-798/semmle/tests/HardcodedCredentialsApiCall.expected index b50796315d5..33b8879d9e5 100644 --- a/java/ql/test/query-tests/security/CWE-798/semmle/tests/HardcodedCredentialsApiCall.expected +++ b/java/ql/test/query-tests/security/CWE-798/semmle/tests/HardcodedCredentialsApiCall.expected @@ -26,8 +26,9 @@ edges | HardcodedAzureCredentials.java:61:3:61:33 | new HardcodedAzureCredentials(...) [clientSecret] : String | HardcodedAzureCredentials.java:15:14:15:42 | parameter this [clientSecret] : String | | HardcodedAzureCredentials.java:61:3:61:33 | new HardcodedAzureCredentials(...) [username] : String | HardcodedAzureCredentials.java:15:14:15:42 | parameter this [username] : String | | HardcodedAzureCredentials.java:63:3:63:33 | new HardcodedAzureCredentials(...) [clientSecret] : String | HardcodedAzureCredentials.java:43:14:43:38 | parameter this [clientSecret] : String | -| HardcodedShiroKey.java:8:46:8:54 | "TEST123" : String | HardcodedShiroKey.java:8:46:8:65 | getBytes(...) | -| HardcodedShiroKey.java:16:60:16:85 | "4AvVhmFLUs0KTA3Kprsdag==" : String | HardcodedShiroKey.java:16:46:16:86 | decode(...) | +| HardcodedShiroKey.java:9:46:9:54 | "TEST123" : String | HardcodedShiroKey.java:9:46:9:65 | getBytes(...) | +| HardcodedShiroKey.java:18:61:18:86 | "4AvVhmFLUs0KTA3Kprsdag==" : String | HardcodedShiroKey.java:18:46:18:87 | decode(...) | +| HardcodedShiroKey.java:26:83:26:108 | "6ZmI6I2j5Y+R5aSn5ZOlAA==" : String | HardcodedShiroKey.java:26:46:26:109 | decode(...) | | Test.java:9:16:9:22 | "admin" : String | Test.java:12:13:12:15 | usr : String | | Test.java:9:16:9:22 | "admin" : String | Test.java:15:36:15:38 | usr | | Test.java:9:16:9:22 | "admin" : String | Test.java:17:39:17:41 | usr | @@ -78,10 +79,12 @@ nodes | HardcodedAzureCredentials.java:61:3:61:33 | new HardcodedAzureCredentials(...) [clientSecret] : String | semmle.label | new HardcodedAzureCredentials(...) [clientSecret] : String | | HardcodedAzureCredentials.java:61:3:61:33 | new HardcodedAzureCredentials(...) [username] : String | semmle.label | new HardcodedAzureCredentials(...) [username] : String | | HardcodedAzureCredentials.java:63:3:63:33 | new HardcodedAzureCredentials(...) [clientSecret] : String | semmle.label | new HardcodedAzureCredentials(...) [clientSecret] : String | -| HardcodedShiroKey.java:8:46:8:54 | "TEST123" : String | semmle.label | "TEST123" : String | -| HardcodedShiroKey.java:8:46:8:65 | getBytes(...) | semmle.label | getBytes(...) | -| HardcodedShiroKey.java:16:46:16:86 | decode(...) | semmle.label | decode(...) | -| HardcodedShiroKey.java:16:60:16:85 | "4AvVhmFLUs0KTA3Kprsdag==" : String | semmle.label | "4AvVhmFLUs0KTA3Kprsdag==" : String | +| HardcodedShiroKey.java:9:46:9:54 | "TEST123" : String | semmle.label | "TEST123" : String | +| HardcodedShiroKey.java:9:46:9:65 | getBytes(...) | semmle.label | getBytes(...) | +| HardcodedShiroKey.java:18:46:18:87 | decode(...) | semmle.label | decode(...) | +| HardcodedShiroKey.java:18:61:18:86 | "4AvVhmFLUs0KTA3Kprsdag==" : String | semmle.label | "4AvVhmFLUs0KTA3Kprsdag==" : String | +| HardcodedShiroKey.java:26:46:26:109 | decode(...) | semmle.label | decode(...) | +| HardcodedShiroKey.java:26:83:26:108 | "6ZmI6I2j5Y+R5aSn5ZOlAA==" : String | semmle.label | "6ZmI6I2j5Y+R5aSn5ZOlAA==" : String | | Test.java:9:16:9:22 | "admin" : String | semmle.label | "admin" : String | | Test.java:10:17:10:24 | "123456" : String | semmle.label | "123456" : String | | Test.java:12:13:12:15 | usr : String | semmle.label | usr : String | @@ -116,8 +119,9 @@ subpaths | HardcodedAzureCredentials.java:10:34:10:67 | "username@example.onmicrosoft.com" | HardcodedAzureCredentials.java:10:34:10:67 | "username@example.onmicrosoft.com" : String | HardcodedAzureCredentials.java:18:13:18:20 | username | Hard-coded value flows to $@. | HardcodedAzureCredentials.java:18:13:18:20 | username | sensitive API call | | HardcodedAzureCredentials.java:11:38:11:73 | "1n1.qAc~3Q-1t38aF79Xzv5AUEfR5-ct3_" | HardcodedAzureCredentials.java:11:38:11:73 | "1n1.qAc~3Q-1t38aF79Xzv5AUEfR5-ct3_" : String | HardcodedAzureCredentials.java:19:13:19:24 | clientSecret | Hard-coded value flows to $@. | HardcodedAzureCredentials.java:19:13:19:24 | clientSecret | sensitive API call | | HardcodedAzureCredentials.java:11:38:11:73 | "1n1.qAc~3Q-1t38aF79Xzv5AUEfR5-ct3_" | HardcodedAzureCredentials.java:11:38:11:73 | "1n1.qAc~3Q-1t38aF79Xzv5AUEfR5-ct3_" : String | HardcodedAzureCredentials.java:46:17:46:28 | clientSecret | Hard-coded value flows to $@. | HardcodedAzureCredentials.java:46:17:46:28 | clientSecret | sensitive API call | -| HardcodedShiroKey.java:8:46:8:54 | "TEST123" | HardcodedShiroKey.java:8:46:8:54 | "TEST123" : String | HardcodedShiroKey.java:8:46:8:65 | getBytes(...) | Hard-coded value flows to $@. | HardcodedShiroKey.java:8:46:8:65 | getBytes(...) | sensitive API call | -| HardcodedShiroKey.java:16:60:16:85 | "4AvVhmFLUs0KTA3Kprsdag==" | HardcodedShiroKey.java:16:60:16:85 | "4AvVhmFLUs0KTA3Kprsdag==" : String | HardcodedShiroKey.java:16:46:16:86 | decode(...) | Hard-coded value flows to $@. | HardcodedShiroKey.java:16:46:16:86 | decode(...) | sensitive API call | +| HardcodedShiroKey.java:9:46:9:54 | "TEST123" | HardcodedShiroKey.java:9:46:9:54 | "TEST123" : String | HardcodedShiroKey.java:9:46:9:65 | getBytes(...) | Hard-coded value flows to $@. | HardcodedShiroKey.java:9:46:9:65 | getBytes(...) | sensitive API call | +| HardcodedShiroKey.java:18:61:18:86 | "4AvVhmFLUs0KTA3Kprsdag==" | HardcodedShiroKey.java:18:61:18:86 | "4AvVhmFLUs0KTA3Kprsdag==" : String | HardcodedShiroKey.java:18:46:18:87 | decode(...) | Hard-coded value flows to $@. | HardcodedShiroKey.java:18:46:18:87 | decode(...) | sensitive API call | +| HardcodedShiroKey.java:26:83:26:108 | "6ZmI6I2j5Y+R5aSn5ZOlAA==" | HardcodedShiroKey.java:26:83:26:108 | "6ZmI6I2j5Y+R5aSn5ZOlAA==" : String | HardcodedShiroKey.java:26:46:26:109 | decode(...) | Hard-coded value flows to $@. | HardcodedShiroKey.java:26:46:26:109 | decode(...) | sensitive API call | | Test.java:9:16:9:22 | "admin" | Test.java:9:16:9:22 | "admin" : String | Test.java:15:36:15:38 | usr | Hard-coded value flows to $@. | Test.java:15:36:15:38 | usr | sensitive API call | | Test.java:9:16:9:22 | "admin" | Test.java:9:16:9:22 | "admin" : String | Test.java:17:39:17:41 | usr | Hard-coded value flows to $@. | Test.java:17:39:17:41 | usr | sensitive API call | | Test.java:9:16:9:22 | "admin" | Test.java:9:16:9:22 | "admin" : String | Test.java:18:39:18:41 | usr | Hard-coded value flows to $@. | Test.java:18:39:18:41 | usr | sensitive API call | diff --git a/java/ql/test/query-tests/security/CWE-798/semmle/tests/HardcodedShiroKey.java b/java/ql/test/query-tests/security/CWE-798/semmle/tests/HardcodedShiroKey.java index ddb0ede6e5f..3647af01ed1 100644 --- a/java/ql/test/query-tests/security/CWE-798/semmle/tests/HardcodedShiroKey.java +++ b/java/ql/test/query-tests/security/CWE-798/semmle/tests/HardcodedShiroKey.java @@ -1,40 +1,38 @@ import org.apache.shiro.web.mgt.CookieRememberMeManager; + public class HardcodedShiroKey { //BAD: hard-coded shiro key - public void testHardcodedShiroKey(String input) { - CookieRememberMeManager cookieRememberMeManager = new CookieRememberMeManager(); + public void testHardcodedShiroKey(String input) { + CookieRememberMeManager cookieRememberMeManager = new CookieRememberMeManager(); cookieRememberMeManager.setCipherKey("TEST123".getBytes()); - } - - - //BAD: hard-coded shiro key - public void testHardcodedbase64ShiroKey(String input) { - CookieRememberMeManager cookieRememberMeManager = new CookieRememberMeManager(); - cookieRememberMeManager.setCipherKey(Base64.decode("4AvVhmFLUs0KTA3Kprsdag==")); - - } - - //GOOD: random shiro key - public void testRandomShiroKey(String input) { - CookieRememberMeManager cookieRememberMeManager = new CookieRememberMeManager(); - } + } + //BAD: hard-coded shiro key encoded by java.util.Base64 + public void testHardcodedbase64ShiroKey1(String input) { + CookieRememberMeManager cookieRememberMeManager = new CookieRememberMeManager(); + java.util.Base64.Decoder decoder = java.util.Base64.getDecoder(); + cookieRememberMeManager.setCipherKey(decoder.decode("4AvVhmFLUs0KTA3Kprsdag==")); - static class Base64 { + } - static byte[] decode(String str){ - byte[] x = new byte[1024]; + //BAD: hard-coded shiro key encoded by org.apache.shiro.codec.Base64 + public void testHardcodedbase64ShiroKey2(String input) { + CookieRememberMeManager cookieRememberMeManager = new CookieRememberMeManager(); + cookieRememberMeManager.setCipherKey(org.apache.shiro.codec.Base64.decode("6ZmI6I2j5Y+R5aSn5ZOlAA==")); - return x; + } + + //GOOD: random shiro key + public void testRandomShiroKey(String input) { + CookieRememberMeManager cookieRememberMeManager = new CookieRememberMeManager(); + } - } - } diff --git a/java/ql/test/stubs/shiro-core-1.4.0/org/apache/shiro/codec/Base64.java b/java/ql/test/stubs/shiro-core-1.4.0/org/apache/shiro/codec/Base64.java new file mode 100644 index 00000000000..d1599df6abe --- /dev/null +++ b/java/ql/test/stubs/shiro-core-1.4.0/org/apache/shiro/codec/Base64.java @@ -0,0 +1,36 @@ +// +// Source code recreated from a .class file by IntelliJ IDEA +// (powered by Fernflower decompiler) +// + +package org.apache.shiro.codec; + +public class Base64 { + static final int CHUNK_SIZE = 76; + static final byte[] CHUNK_SEPARATOR = "\r\n".getBytes(); + private static final int BASELENGTH = 255; + private static final int LOOKUPLENGTH = 64; + private static final int EIGHTBIT = 8; + private static final int SIXTEENBIT = 16; + private static final int TWENTYFOURBITGROUP = 24; + private static final int FOURBYTE = 4; + private static final int SIGN = -128; + private static final byte PAD = 61; + private static final byte[] base64Alphabet = new byte[255]; + private static final byte[] lookUpBase64Alphabet = new byte[64]; + + public Base64() { + } + + + public static byte[] decode(String base64Encoded) { + byte[] bytes = new byte[1024]; + return decode(bytes); + } + + public static byte[] decode(byte[] base64Data) { + return base64Data; + } + } + +