From c79a3eb6aece78ec4d7a230d9e6ab303851f7a03 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Wed, 7 Feb 2024 16:37:40 +0000 Subject: [PATCH 1/7] Add query for insecure key generation --- .../java/security/AndroidLocalAuthQuery.qll | 19 +++++++++++++++++ .../CWE/CWE-287/AndroidInsecureKeys.ql | 21 +++++++++++++++++++ 2 files changed, 40 insertions(+) create mode 100644 java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.ql diff --git a/java/ql/lib/semmle/code/java/security/AndroidLocalAuthQuery.qll b/java/ql/lib/semmle/code/java/security/AndroidLocalAuthQuery.qll index ca725a041d5..14476e737d9 100644 --- a/java/ql/lib/semmle/code/java/security/AndroidLocalAuthQuery.qll +++ b/java/ql/lib/semmle/code/java/security/AndroidLocalAuthQuery.qll @@ -1,6 +1,7 @@ /** Definitions for the insecure local authentication query. */ import java +import semmle.code.java.dataflow.DataFlow /** A base class that is used as a callback for biometric authentication. */ private class AuthenticationCallbackClass extends Class { @@ -40,3 +41,21 @@ class AuthenticationSuccessCallback extends Method { not result = this.getASuperResultUse() } } + +/** A call that sets a parameter for key generation that is insecure for use with biometric authentication. */ +class InsecureBiometricKeyParam extends MethodCall { + InsecureBiometricKeyParam() { + exists(string name, CompileTimeConstantExpr val | + this.getMethod() + .hasQualifiedName("android.security.keystore", "KeyGenParameterSpec$Builder", name) and + DataFlow::localExprFlow(val, this.getArgument(0)) and + ( + name = ["setUserAuthenticationRequired", "setInvalidatedByBiometricEnrollment"] and + val.getBooleanValue() = false + or + name = "setUserAuthenticationValidityDurationSeconds" and + val.getIntValue() != -1 + ) + ) + } +} diff --git a/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.ql b/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.ql new file mode 100644 index 00000000000..00156283045 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.ql @@ -0,0 +1,21 @@ +/** + * @name Insecurely generated keys for local authentication + * @description Keys used for local biometric authentication should be generated with secure parameters. + * @kind problem + * @problem.severity warning + * @security-severity 9.3 + * @precision medium + * @id java/android/insecure-local-key-gen + * @tags security + * external/cwe/cwe-287 + */ + +import java +import semmle.code.java.security.AndroidLocalAuthQuery + +/** Holds if the application contains an instance of a key being used for local biometric authentication. */ +predicate usesLocalAuth() { exists(AuthenticationSuccessCallback cb | exists(cb.getAResultUse())) } + +from InsecureBiometricKeyParam call +where usesLocalAuth() +select call, "This key is not secure for biometric authentication." From d8985f9f5bc489f2c15049249e00a7d4ea34a945 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Thu, 8 Feb 2024 10:32:56 +0000 Subject: [PATCH 2/7] Move tests for local auth to a folder --- java/ql/lib/semmle/code/java/security/AndroidLocalAuthQuery.qll | 2 +- .../CWE-287/{ => InsecureLocalAuth}/InsecureLocalAuth.expected | 0 .../CWE-287/{ => InsecureLocalAuth}/InsecureLocalAuth.ql | 0 .../security/CWE-287/{ => InsecureLocalAuth}/Test.java | 0 .../security/CWE-287/{ => InsecureLocalAuth}/Test2.java | 0 .../security/CWE-287/{ => InsecureLocalAuth}/options | 2 +- 6 files changed, 2 insertions(+), 2 deletions(-) rename java/ql/test/query-tests/security/CWE-287/{ => InsecureLocalAuth}/InsecureLocalAuth.expected (100%) rename java/ql/test/query-tests/security/CWE-287/{ => InsecureLocalAuth}/InsecureLocalAuth.ql (100%) rename java/ql/test/query-tests/security/CWE-287/{ => InsecureLocalAuth}/Test.java (100%) rename java/ql/test/query-tests/security/CWE-287/{ => InsecureLocalAuth}/Test2.java (100%) rename java/ql/test/query-tests/security/CWE-287/{ => InsecureLocalAuth}/options (67%) diff --git a/java/ql/lib/semmle/code/java/security/AndroidLocalAuthQuery.qll b/java/ql/lib/semmle/code/java/security/AndroidLocalAuthQuery.qll index 14476e737d9..004741aabfc 100644 --- a/java/ql/lib/semmle/code/java/security/AndroidLocalAuthQuery.qll +++ b/java/ql/lib/semmle/code/java/security/AndroidLocalAuthQuery.qll @@ -1,7 +1,7 @@ /** Definitions for the insecure local authentication query. */ import java -import semmle.code.java.dataflow.DataFlow +private import semmle.code.java.dataflow.DataFlow /** A base class that is used as a callback for biometric authentication. */ private class AuthenticationCallbackClass extends Class { diff --git a/java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth.expected b/java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth/InsecureLocalAuth.expected similarity index 100% rename from java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth.expected rename to java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth/InsecureLocalAuth.expected diff --git a/java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth.ql b/java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth/InsecureLocalAuth.ql similarity index 100% rename from java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth.ql rename to java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth/InsecureLocalAuth.ql diff --git a/java/ql/test/query-tests/security/CWE-287/Test.java b/java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth/Test.java similarity index 100% rename from java/ql/test/query-tests/security/CWE-287/Test.java rename to java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth/Test.java diff --git a/java/ql/test/query-tests/security/CWE-287/Test2.java b/java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth/Test2.java similarity index 100% rename from java/ql/test/query-tests/security/CWE-287/Test2.java rename to java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth/Test2.java diff --git a/java/ql/test/query-tests/security/CWE-287/options b/java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth/options similarity index 67% rename from java/ql/test/query-tests/security/CWE-287/options rename to java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth/options index dacd3cb21df..33cdc1ea940 100644 --- a/java/ql/test/query-tests/security/CWE-287/options +++ b/java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth/options @@ -1 +1 @@ -//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/google-android-9.0.0 +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/google-android-9.0.0 From 2eb93b7a3b5875bc1186d686a7ab60aa94bd13f1 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Thu, 8 Feb 2024 11:11:48 +0000 Subject: [PATCH 3/7] Add unit tests --- .../java/security/AndroidLocalAuthQuery.qll | 7 +- .../CWE/CWE-287/AndroidInsecureKeys.ql | 5 +- .../InsecureKeys/Test1/InsecureKeys.expected | 2 + .../InsecureKeys/Test1/InsecureKeys.ql | 19 +++++ .../CWE-287/InsecureKeys/Test1/Test.java | 21 +++++ .../CWE-287/InsecureKeys/Test1/options | 1 + .../InsecureKeys/Test2/InsecureKeys.expected | 2 + .../InsecureKeys/Test2/InsecureKeys.ql | 19 +++++ .../CWE-287/InsecureKeys/Test2/Test.java | 13 ++++ .../CWE-287/InsecureKeys/Test2/options | 1 + .../keystore/KeyGenParameterSpec.java | 76 +++++++++++++++++++ .../security/keystore/KeyProperties.java | 54 +++++++++++++ 12 files changed, 214 insertions(+), 6 deletions(-) create mode 100644 java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/InsecureKeys.expected create mode 100644 java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/InsecureKeys.ql create mode 100644 java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/Test.java create mode 100644 java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/options create mode 100644 java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test2/InsecureKeys.expected create mode 100644 java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test2/InsecureKeys.ql create mode 100644 java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test2/Test.java create mode 100644 java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test2/options create mode 100644 java/ql/test/stubs/google-android-9.0.0/android/security/keystore/KeyGenParameterSpec.java create mode 100644 java/ql/test/stubs/google-android-9.0.0/android/security/keystore/KeyProperties.java diff --git a/java/ql/lib/semmle/code/java/security/AndroidLocalAuthQuery.qll b/java/ql/lib/semmle/code/java/security/AndroidLocalAuthQuery.qll index 004741aabfc..4a31dc2568d 100644 --- a/java/ql/lib/semmle/code/java/security/AndroidLocalAuthQuery.qll +++ b/java/ql/lib/semmle/code/java/security/AndroidLocalAuthQuery.qll @@ -43,8 +43,8 @@ class AuthenticationSuccessCallback extends Method { } /** A call that sets a parameter for key generation that is insecure for use with biometric authentication. */ -class InsecureBiometricKeyParam extends MethodCall { - InsecureBiometricKeyParam() { +class InsecureBiometricKeyParamCall extends MethodCall { + InsecureBiometricKeyParamCall() { exists(string name, CompileTimeConstantExpr val | this.getMethod() .hasQualifiedName("android.security.keystore", "KeyGenParameterSpec$Builder", name) and @@ -59,3 +59,6 @@ class InsecureBiometricKeyParam extends MethodCall { ) } } + +/** Holds if the application contains an instance of a key being used for local biometric authentication. */ +predicate usesLocalAuth() { exists(AuthenticationSuccessCallback cb | exists(cb.getAResultUse())) } diff --git a/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.ql b/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.ql index 00156283045..0e85229c29b 100644 --- a/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.ql +++ b/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.ql @@ -13,9 +13,6 @@ import java import semmle.code.java.security.AndroidLocalAuthQuery -/** Holds if the application contains an instance of a key being used for local biometric authentication. */ -predicate usesLocalAuth() { exists(AuthenticationSuccessCallback cb | exists(cb.getAResultUse())) } - -from InsecureBiometricKeyParam call +from InsecureBiometricKeyParamCall call where usesLocalAuth() select call, "This key is not secure for biometric authentication." diff --git a/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/InsecureKeys.expected b/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/InsecureKeys.expected new file mode 100644 index 00000000000..8ec8033d086 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/InsecureKeys.expected @@ -0,0 +1,2 @@ +testFailures +failures diff --git a/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/InsecureKeys.ql b/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/InsecureKeys.ql new file mode 100644 index 00000000000..eec3b62dfc2 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/InsecureKeys.ql @@ -0,0 +1,19 @@ +import java +import TestUtilities.InlineExpectationsTest +import semmle.code.java.dataflow.DataFlow +import semmle.code.java.security.AndroidLocalAuthQuery + +module InsecureKeysTest implements TestSig { + string getARelevantTag() { result = "insecure-key" } + + predicate hasActualResult(Location location, string element, string tag, string value) { + tag = "insecure-key" and + exists(InsecureBiometricKeyParamCall call | usesLocalAuth() | + call.getLocation() = location and + element = call.toString() and + value = "" + ) + } +} + +import MakeTest diff --git a/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/Test.java b/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/Test.java new file mode 100644 index 00000000000..5fc2c83eea9 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/Test.java @@ -0,0 +1,21 @@ +import android.security.keystore.KeyGenParameterSpec; +import android.hardware.biometrics.BiometricPrompt; +import android.security.keystore.KeyProperties; + +class Test { + void test() { + KeyGenParameterSpec.Builder builder = new KeyGenParameterSpec.Builder("MySecretKey", KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT); + builder.setUserAuthenticationRequired(false); // $insecure-key + builder.setInvalidatedByBiometricEnrollment(false); // $insecure-key + builder.setUserAuthenticationValidityDurationSeconds(30); // $insecure-key + } +} + +class Callback extends BiometricPrompt.AuthenticationCallback { + public static void useKey(BiometricPrompt.CryptoObject key) {} + + @Override + public void onAuthenticationSucceeded(BiometricPrompt.AuthenticationResult result) { + useKey(result.getCryptoObject()); + } +} \ No newline at end of file diff --git a/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/options b/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/options new file mode 100644 index 00000000000..7039c596a23 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/options @@ -0,0 +1 @@ +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../../stubs/google-android-9.0.0 diff --git a/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test2/InsecureKeys.expected b/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test2/InsecureKeys.expected new file mode 100644 index 00000000000..8ec8033d086 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test2/InsecureKeys.expected @@ -0,0 +1,2 @@ +testFailures +failures diff --git a/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test2/InsecureKeys.ql b/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test2/InsecureKeys.ql new file mode 100644 index 00000000000..eec3b62dfc2 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test2/InsecureKeys.ql @@ -0,0 +1,19 @@ +import java +import TestUtilities.InlineExpectationsTest +import semmle.code.java.dataflow.DataFlow +import semmle.code.java.security.AndroidLocalAuthQuery + +module InsecureKeysTest implements TestSig { + string getARelevantTag() { result = "insecure-key" } + + predicate hasActualResult(Location location, string element, string tag, string value) { + tag = "insecure-key" and + exists(InsecureBiometricKeyParamCall call | usesLocalAuth() | + call.getLocation() = location and + element = call.toString() and + value = "" + ) + } +} + +import MakeTest diff --git a/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test2/Test.java b/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test2/Test.java new file mode 100644 index 00000000000..e65b50da888 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test2/Test.java @@ -0,0 +1,13 @@ +import android.security.keystore.KeyGenParameterSpec; +import android.hardware.biometrics.BiometricPrompt; +import android.security.keystore.KeyProperties; + +class Test { + void test() { + KeyGenParameterSpec.Builder builder = new KeyGenParameterSpec.Builder("MySecretKey", KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT); + // No alert as there is no use of biometric authentication in this application. + builder.setUserAuthenticationRequired(false); + builder.setInvalidatedByBiometricEnrollment(false); + builder.setUserAuthenticationValidityDurationSeconds(30); + } +} \ No newline at end of file diff --git a/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test2/options b/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test2/options new file mode 100644 index 00000000000..7039c596a23 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test2/options @@ -0,0 +1 @@ +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../../stubs/google-android-9.0.0 diff --git a/java/ql/test/stubs/google-android-9.0.0/android/security/keystore/KeyGenParameterSpec.java b/java/ql/test/stubs/google-android-9.0.0/android/security/keystore/KeyGenParameterSpec.java new file mode 100644 index 00000000000..7998d48428e --- /dev/null +++ b/java/ql/test/stubs/google-android-9.0.0/android/security/keystore/KeyGenParameterSpec.java @@ -0,0 +1,76 @@ +// Generated automatically from android.security.keystore.KeyGenParameterSpec for testing purposes + +package android.security.keystore; + +import java.math.BigInteger; +import java.security.spec.AlgorithmParameterSpec; +import java.util.Date; +import javax.security.auth.x500.X500Principal; + +public class KeyGenParameterSpec implements AlgorithmParameterSpec +{ + public AlgorithmParameterSpec getAlgorithmParameterSpec(){ return null; } + public BigInteger getCertificateSerialNumber(){ return null; } + public Date getCertificateNotAfter(){ return null; } + public Date getCertificateNotBefore(){ return null; } + public Date getKeyValidityForConsumptionEnd(){ return null; } + public Date getKeyValidityForOriginationEnd(){ return null; } + public Date getKeyValidityStart(){ return null; } + public String getAttestKeyAlias(){ return null; } + public String getKeystoreAlias(){ return null; } + public String[] getBlockModes(){ return null; } + public String[] getDigests(){ return null; } + public String[] getEncryptionPaddings(){ return null; } + public String[] getSignaturePaddings(){ return null; } + public X500Principal getCertificateSubject(){ return null; } + public boolean isDevicePropertiesAttestationIncluded(){ return false; } + public boolean isDigestsSpecified(){ return false; } + public boolean isInvalidatedByBiometricEnrollment(){ return false; } + public boolean isRandomizedEncryptionRequired(){ return false; } + public boolean isStrongBoxBacked(){ return false; } + public boolean isUnlockedDeviceRequired(){ return false; } + public boolean isUserAuthenticationRequired(){ return false; } + public boolean isUserAuthenticationValidWhileOnBody(){ return false; } + public boolean isUserConfirmationRequired(){ return false; } + public boolean isUserPresenceRequired(){ return false; } + public byte[] getAttestationChallenge(){ return null; } + public int getKeySize(){ return 0; } + public int getMaxUsageCount(){ return 0; } + public int getPurposes(){ return 0; } + public int getUserAuthenticationType(){ return 0; } + public int getUserAuthenticationValidityDurationSeconds(){ return 0; } + static public class Builder + { + protected Builder() {} + public Builder(String p0, int p1){} + public KeyGenParameterSpec build(){ return null; } + public KeyGenParameterSpec.Builder setAlgorithmParameterSpec(AlgorithmParameterSpec p0){ return null; } + public KeyGenParameterSpec.Builder setAttestKeyAlias(String p0){ return null; } + public KeyGenParameterSpec.Builder setAttestationChallenge(byte[] p0){ return null; } + public KeyGenParameterSpec.Builder setBlockModes(String... p0){ return null; } + public KeyGenParameterSpec.Builder setCertificateNotAfter(Date p0){ return null; } + public KeyGenParameterSpec.Builder setCertificateNotBefore(Date p0){ return null; } + public KeyGenParameterSpec.Builder setCertificateSerialNumber(BigInteger p0){ return null; } + public KeyGenParameterSpec.Builder setCertificateSubject(X500Principal p0){ return null; } + public KeyGenParameterSpec.Builder setDevicePropertiesAttestationIncluded(boolean p0){ return null; } + public KeyGenParameterSpec.Builder setDigests(String... p0){ return null; } + public KeyGenParameterSpec.Builder setEncryptionPaddings(String... p0){ return null; } + public KeyGenParameterSpec.Builder setInvalidatedByBiometricEnrollment(boolean p0){ return null; } + public KeyGenParameterSpec.Builder setIsStrongBoxBacked(boolean p0){ return null; } + public KeyGenParameterSpec.Builder setKeySize(int p0){ return null; } + public KeyGenParameterSpec.Builder setKeyValidityEnd(Date p0){ return null; } + public KeyGenParameterSpec.Builder setKeyValidityForConsumptionEnd(Date p0){ return null; } + public KeyGenParameterSpec.Builder setKeyValidityForOriginationEnd(Date p0){ return null; } + public KeyGenParameterSpec.Builder setKeyValidityStart(Date p0){ return null; } + public KeyGenParameterSpec.Builder setMaxUsageCount(int p0){ return null; } + public KeyGenParameterSpec.Builder setRandomizedEncryptionRequired(boolean p0){ return null; } + public KeyGenParameterSpec.Builder setSignaturePaddings(String... p0){ return null; } + public KeyGenParameterSpec.Builder setUnlockedDeviceRequired(boolean p0){ return null; } + public KeyGenParameterSpec.Builder setUserAuthenticationParameters(int p0, int p1){ return null; } + public KeyGenParameterSpec.Builder setUserAuthenticationRequired(boolean p0){ return null; } + public KeyGenParameterSpec.Builder setUserAuthenticationValidWhileOnBody(boolean p0){ return null; } + public KeyGenParameterSpec.Builder setUserAuthenticationValidityDurationSeconds(int p0){ return null; } + public KeyGenParameterSpec.Builder setUserConfirmationRequired(boolean p0){ return null; } + public KeyGenParameterSpec.Builder setUserPresenceRequired(boolean p0){ return null; } + } +} diff --git a/java/ql/test/stubs/google-android-9.0.0/android/security/keystore/KeyProperties.java b/java/ql/test/stubs/google-android-9.0.0/android/security/keystore/KeyProperties.java new file mode 100644 index 00000000000..b9e7a912698 --- /dev/null +++ b/java/ql/test/stubs/google-android-9.0.0/android/security/keystore/KeyProperties.java @@ -0,0 +1,54 @@ +// Generated automatically from android.security.keystore.KeyProperties for testing purposes + +package android.security.keystore; + + +abstract public class KeyProperties +{ + protected KeyProperties() {} + public static String BLOCK_MODE_CBC = null; + public static String BLOCK_MODE_CTR = null; + public static String BLOCK_MODE_ECB = null; + public static String BLOCK_MODE_GCM = null; + public static String DIGEST_MD5 = null; + public static String DIGEST_NONE = null; + public static String DIGEST_SHA1 = null; + public static String DIGEST_SHA224 = null; + public static String DIGEST_SHA256 = null; + public static String DIGEST_SHA384 = null; + public static String DIGEST_SHA512 = null; + public static String ENCRYPTION_PADDING_NONE = null; + public static String ENCRYPTION_PADDING_PKCS7 = null; + public static String ENCRYPTION_PADDING_RSA_OAEP = null; + public static String ENCRYPTION_PADDING_RSA_PKCS1 = null; + public static String KEY_ALGORITHM_3DES = null; + public static String KEY_ALGORITHM_AES = null; + public static String KEY_ALGORITHM_EC = null; + public static String KEY_ALGORITHM_HMAC_SHA1 = null; + public static String KEY_ALGORITHM_HMAC_SHA224 = null; + public static String KEY_ALGORITHM_HMAC_SHA256 = null; + public static String KEY_ALGORITHM_HMAC_SHA384 = null; + public static String KEY_ALGORITHM_HMAC_SHA512 = null; + public static String KEY_ALGORITHM_RSA = null; + public static String SIGNATURE_PADDING_RSA_PKCS1 = null; + public static String SIGNATURE_PADDING_RSA_PSS = null; + public static int AUTH_BIOMETRIC_STRONG = 0; + public static int AUTH_DEVICE_CREDENTIAL = 0; + public static int ORIGIN_GENERATED = 0; + public static int ORIGIN_IMPORTED = 0; + public static int ORIGIN_SECURELY_IMPORTED = 0; + public static int ORIGIN_UNKNOWN = 0; + public static int PURPOSE_AGREE_KEY = 0; + public static int PURPOSE_ATTEST_KEY = 0; + public static int PURPOSE_DECRYPT = 0; + public static int PURPOSE_ENCRYPT = 0; + public static int PURPOSE_SIGN = 0; + public static int PURPOSE_VERIFY = 0; + public static int PURPOSE_WRAP_KEY = 0; + public static int SECURITY_LEVEL_SOFTWARE = 0; + public static int SECURITY_LEVEL_STRONGBOX = 0; + public static int SECURITY_LEVEL_TRUSTED_ENVIRONMENT = 0; + public static int SECURITY_LEVEL_UNKNOWN = 0; + public static int SECURITY_LEVEL_UNKNOWN_SECURE = 0; + public static int UNRESTRICTED_USAGE_COUNT = 0; +} From 16a7d68780180cf36835c5659c5506a5c31ba94b Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Mon, 12 Feb 2024 13:57:31 +0000 Subject: [PATCH 4/7] Add documentation --- .../CWE/CWE-287/AndroidInsecureKeys.qhelp | 40 +++++++++++++++++++ .../CWE/CWE-287/AndroidInsecureKeysGood.java | 16 ++++++++ 2 files changed, 56 insertions(+) create mode 100644 java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.qhelp create mode 100644 java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeysGood.java diff --git a/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.qhelp b/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.qhelp new file mode 100644 index 00000000000..7b7a86ca667 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.qhelp @@ -0,0 +1,40 @@ + + + + +

+Biometric authentication such as fingerprint recognition can be used alongside cryptographic keys stored in the Android KeyStore to protect sensitive parts of the application. However, +when a key generated for this purpose has certain parameters set insecurely, it can allow an attacker with physical access to bypass the +authentication check, using application hooking tools such as Frida. +

+ + + +

+When generating a key for use with biometric authentication, ensure that the following parameters of KeyGenParameterSpec.Builder are set: +

+
    +
  • setUserAuthenticationRequired should be set to true; otherwise the key can be used without user authentication.
    • +
  • setInvalidatedByBiometricEnrollment should be set to true (the default); otherwise an attacker can use the key by enrolling additional biometrics on the device.
    • +
  • setUserAuthenticationValidityDurationSeconds, if used, should be set to -1; otherwise non-biometric (less secure) credentials can be used to access the key. setUserAuthenticationParameters is instead recommended to explicitly set both the timeout and the types of credentials that may be used.
    • +
+ + + + +

The following example demonstrates a key that is configured with secure paramaters:

+ + + + +
  • +WithSecure: How Secure is your Android Keystore Authentication? +
    • +
  • +Android Developers: KeyGenParameterSpec.Builder +
    • + +     +     diff --git a/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeysGood.java b/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeysGood.java new file mode 100644 index 00000000000..843f020fdbe --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeysGood.java @@ -0,0 +1,16 @@ +private void generateSecretKey() { + KeyGenParameterSpec keyGenParameterSpec = new KeyGenParameterSpec.Builder( + "MySecretKey", + KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT) + .setBlockModes(KeyProperties.BLOCK_MODE_CBC) + .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_PKCS7) + // GOOD: Secure parameters are used to generate a key for biometric authentication. + .setUserAuthenticationRequired(true) + .setInvalidatedByBiometricEnrollment(true) + .setUserAuthenticationParamters(0, KeyProperties.AUTH_BIOMETRIC_STRONG) + .build(); + KeyGenerator keyGenerator = KeyGenerator.getInstance( + KeyProperties.KEY_ALGORITHM_AES, "AndroidKeyStore"); + keyGenerator.init(keyGenParameterSpec); + keyGenerator.generateKey(); +} \ No newline at end of file From 3a4a841844d66263adb051d07afb8a0ea0bb5041 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Mon, 12 Feb 2024 14:01:27 +0000 Subject: [PATCH 5/7] Add change note + update severity --- java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.ql | 2 +- java/ql/src/change-notes/2024-02-12-android-insecure-keys.md | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 java/ql/src/change-notes/2024-02-12-android-insecure-keys.md diff --git a/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.ql b/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.ql index 0e85229c29b..c8090f23c1d 100644 --- a/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.ql +++ b/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.ql @@ -3,7 +3,7 @@ * @description Keys used for local biometric authentication should be generated with secure parameters. * @kind problem * @problem.severity warning - * @security-severity 9.3 + * @security-severity 4.4 * @precision medium * @id java/android/insecure-local-key-gen * @tags security diff --git a/java/ql/src/change-notes/2024-02-12-android-insecure-keys.md b/java/ql/src/change-notes/2024-02-12-android-insecure-keys.md new file mode 100644 index 00000000000..1de07727796 --- /dev/null +++ b/java/ql/src/change-notes/2024-02-12-android-insecure-keys.md @@ -0,0 +1,4 @@ +--- +category: newQuery +--- +* Added a new query `java/android/insecure-local-key-gen` for finding instances of keys generated for biometric authentication in an insecure way. \ No newline at end of file From 9ad05fe51c08c0a84033fc956844b526d550c2cd Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Fri, 16 Feb 2024 12:00:51 +0000 Subject: [PATCH 6/7] Address reveiws - Add BAD example to doc, add doc example to tests and fix typo. --- .../CWE/CWE-287/AndroidInsecureKeys.qhelp | 3 ++ .../CWE/CWE-287/AndroidInsecureKeysBad.java | 47 +++++++++++++++++++ .../CWE/CWE-287/AndroidInsecureKeysGood.java | 2 +- .../CWE-287/InsecureKeys/Test1/Test.java | 18 +++++++ 4 files changed, 69 insertions(+), 1 deletion(-) create mode 100644 java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeysBad.java diff --git a/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.qhelp b/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.qhelp index 7b7a86ca667..95257fb020c 100644 --- a/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.qhelp +++ b/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.qhelp @@ -26,6 +26,9 @@ When generating a key for use with biometric authentication, ensure that the fol

    The following example demonstrates a key that is configured with secure paramaters:

    + +

    In each of the following cases, a parameter is set insecurely:

    +     diff --git a/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeysBad.java b/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeysBad.java new file mode 100644 index 00000000000..deb14d7a5e7 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeysBad.java @@ -0,0 +1,47 @@ +private void generateSecretKey() { + KeyGenParameterSpec keyGenParameterSpec = new KeyGenParameterSpec.Builder( + "MySecretKey", + KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT) + .setBlockModes(KeyProperties.BLOCK_MODE_CBC) + .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_PKCS7) + // BAD: User authentication is not required to use this key. + .setUserAuthenticationRequired(false) + .build(); + KeyGenerator keyGenerator = KeyGenerator.getInstance( + KeyProperties.KEY_ALGORITHM_AES, "AndroidKeyStore"); + keyGenerator.init(keyGenParameterSpec); + keyGenerator.generateKey(); +} + +private void generateSecretKey() { + KeyGenParameterSpec keyGenParameterSpec = new KeyGenParameterSpec.Builder( + "MySecretKey", + KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT) + .setBlockModes(KeyProperties.BLOCK_MODE_CBC) + .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_PKCS7) + .setUserAuthenticationRequired(true) + // BAD: An attacker can access this key by enrolling additional biometrics. + .setInvalidatedByBiometricEnrollment(false) + .build(); + KeyGenerator keyGenerator = KeyGenerator.getInstance( + KeyProperties.KEY_ALGORITHM_AES, "AndroidKeyStore"); + keyGenerator.init(keyGenParameterSpec); + keyGenerator.generateKey(); +} + +private void generateSecretKey() { + KeyGenParameterSpec keyGenParameterSpec = new KeyGenParameterSpec.Builder( + "MySecretKey", + KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT) + .setBlockModes(KeyProperties.BLOCK_MODE_CBC) + .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_PKCS7) + .setUserAuthenticationRequired(true) + .setInvalidatedByBiometricEnrollment(true) + // BAD: This key can be accessed using non-biometric credentials. + .setUserAuthenticationValidityDurationSeconds(30) + .build(); + KeyGenerator keyGenerator = KeyGenerator.getInstance( + KeyProperties.KEY_ALGORITHM_AES, "AndroidKeyStore"); + keyGenerator.init(keyGenParameterSpec); + keyGenerator.generateKey(); +} \ No newline at end of file diff --git a/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeysGood.java b/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeysGood.java index 843f020fdbe..64f9c94f9ee 100644 --- a/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeysGood.java +++ b/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeysGood.java @@ -7,7 +7,7 @@ private void generateSecretKey() { // GOOD: Secure parameters are used to generate a key for biometric authentication. .setUserAuthenticationRequired(true) .setInvalidatedByBiometricEnrollment(true) - .setUserAuthenticationParamters(0, KeyProperties.AUTH_BIOMETRIC_STRONG) + .setUserAuthenticationParameters(0, KeyProperties.AUTH_BIOMETRIC_STRONG) .build(); KeyGenerator keyGenerator = KeyGenerator.getInstance( KeyProperties.KEY_ALGORITHM_AES, "AndroidKeyStore"); diff --git a/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/Test.java b/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/Test.java index 5fc2c83eea9..87e973ab774 100644 --- a/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/Test.java +++ b/java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/Test.java @@ -1,6 +1,7 @@ import android.security.keystore.KeyGenParameterSpec; import android.hardware.biometrics.BiometricPrompt; import android.security.keystore.KeyProperties; +import javax.crypto.KeyGenerator; class Test { void test() { @@ -9,6 +10,23 @@ class Test { builder.setInvalidatedByBiometricEnrollment(false); // $insecure-key builder.setUserAuthenticationValidityDurationSeconds(30); // $insecure-key } + + private void generateSecretKey() throws Exception { + KeyGenParameterSpec keyGenParameterSpec = new KeyGenParameterSpec.Builder( + "MySecretKey", + KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT) + .setBlockModes(KeyProperties.BLOCK_MODE_CBC) + .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_PKCS7) + // GOOD: Secure parameters are used to generate a key for biometric authentication. + .setUserAuthenticationRequired(true) + .setInvalidatedByBiometricEnrollment(true) + .setUserAuthenticationParameters(0, KeyProperties.AUTH_BIOMETRIC_STRONG) + .build(); + KeyGenerator keyGenerator = KeyGenerator.getInstance( + KeyProperties.KEY_ALGORITHM_AES, "AndroidKeyStore"); + keyGenerator.init(keyGenParameterSpec); + keyGenerator.generateKey(); + } } class Callback extends BiometricPrompt.AuthenticationCallback { From ef124695a5d68472eb02f60741f7ab5773618bcf Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Thu, 22 Feb 2024 10:11:49 +0000 Subject: [PATCH 7/7] Apply suggestions from documentation review Co-authored-by: Sam Browning <106113886+sabrowning1@users.noreply.github.com> --- .../CWE/CWE-287/AndroidInsecureKeys.qhelp | 16 ++++++++-------- .../Security/CWE/CWE-287/AndroidInsecureKeys.ql | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.qhelp b/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.qhelp index 95257fb020c..6b3546f85f5 100644 --- a/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.qhelp +++ b/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.qhelp @@ -5,9 +5,9 @@

    -Biometric authentication such as fingerprint recognition can be used alongside cryptographic keys stored in the Android KeyStore to protect sensitive parts of the application. However, -when a key generated for this purpose has certain parameters set insecurely, it can allow an attacker with physical access to bypass the -authentication check, using application hooking tools such as Frida. +Biometric authentication, such as fingerprint recognition, can be used alongside cryptographic keys stored in the Android KeyStore to protect sensitive parts of the application. However, +when a key generated for this purpose has certain parameters set insecurely, an attacker with physical access can bypass the +authentication check using application hooking tools such as Frida.

    @@ -16,9 +16,9 @@ authentication check, using application hooking tools such as Frida. When generating a key for use with biometric authentication, ensure that the following parameters of KeyGenParameterSpec.Builder are set:

      -
    • setUserAuthenticationRequired should be set to true; otherwise the key can be used without user authentication.
      • -
    • setInvalidatedByBiometricEnrollment should be set to true (the default); otherwise an attacker can use the key by enrolling additional biometrics on the device.
      • -
    • setUserAuthenticationValidityDurationSeconds, if used, should be set to -1; otherwise non-biometric (less secure) credentials can be used to access the key. setUserAuthenticationParameters is instead recommended to explicitly set both the timeout and the types of credentials that may be used.
      • +
    • setUserAuthenticationRequired should be set to true; otherwise, the key can be used without user authentication.
      • +
    • setInvalidatedByBiometricEnrollment should be set to true (the default); otherwise, an attacker can use the key by enrolling additional biometrics on the device.
      • +
    • setUserAuthenticationValidityDurationSeconds, if used, should be set to -1; otherwise, non-biometric (less secure) credentials can be used to access the key. We recommend using setUserAuthenticationParameters instead to explicitly set both the timeout and the types of credentials that may be used.
    @@ -33,10 +33,10 @@ When generating a key for use with biometric authentication, ensure that the fol
  • -WithSecure: How Secure is your Android Keystore Authentication? +WithSecure: How Secure is your Android Keystore Authentication?.
  • -Android Developers: KeyGenParameterSpec.Builder +Android Developers: KeyGenParameterSpec.Builder.
    • diff --git a/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.ql b/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.ql index c8090f23c1d..52aaf8d885d 100644 --- a/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.ql +++ b/java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.ql @@ -1,6 +1,6 @@ /** * @name Insecurely generated keys for local authentication - * @description Keys used for local biometric authentication should be generated with secure parameters. + * @description Generation of keys with insecure parameters for local biometric authentication can allow attackers with physical access to bypass authentication checks. * @kind problem * @problem.severity warning * @security-severity 4.4