Skip to content

Commit 2ebb80b

Browse files
joefarebrotherjoefarebrother
authored
Merge pull request #15548 from joefarebrother/android-local-auth-keys
Java: Add query for insecurely generated keys for local authentication.
2 parents 67e8f17 + ef12469 commit 2ebb80b

File tree

21 files changed

+377
-1
lines changed

21 files changed

+377
-1
lines changed

‎java/ql/lib/semmle/code/java/security/AndroidLocalAuthQuery.qll

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/** Definitions for the insecure local authentication query. */
22

33
import java
4+
private import semmle.code.java.dataflow.DataFlow
45

56
/** A base class that is used as a callback for biometric authentication. */
67
private class AuthenticationCallbackClass extends Class {
@@ -40,3 +41,24 @@ class AuthenticationSuccessCallback extends Method {
4041
not result = this.getASuperResultUse()
4142
}
4243
}
44+
45+
/** A call that sets a parameter for key generation that is insecure for use with biometric authentication. */
46+
class InsecureBiometricKeyParamCall extends MethodCall {
47+
InsecureBiometricKeyParamCall() {
48+
exists(string name, CompileTimeConstantExpr val |
49+
this.getMethod()
50+
.hasQualifiedName("android.security.keystore", "KeyGenParameterSpec$Builder", name) and
51+
DataFlow::localExprFlow(val, this.getArgument(0)) and
52+
(
53+
name = ["setUserAuthenticationRequired", "setInvalidatedByBiometricEnrollment"] and
54+
val.getBooleanValue() = false
55+
or
56+
name = "setUserAuthenticationValidityDurationSeconds" and
57+
val.getIntValue() != -1
58+
)
59+
)
60+
}
61+
}
62+
63+
/** Holds if the application contains an instance of a key being used for local biometric authentication. */
64+
predicate usesLocalAuth() { exists(AuthenticationSuccessCallback cb | exists(cb.getAResultUse())) }

‎java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.qhelp

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
Biometric authentication, such as fingerprint recognition, can be used alongside cryptographic keys stored in the Android <code>KeyStore</code> to protect sensitive parts of the application. However,
9+
when a key generated for this purpose has certain parameters set insecurely, an attacker with physical access can bypass the
10+
authentication check using application hooking tools such as Frida.
11+
</p>
12+
</overview>
13+
14+
<recommendation>
15+
<p>
16+
When generating a key for use with biometric authentication, ensure that the following parameters of <code>KeyGenParameterSpec.Builder</code> are set:
17+
</p>
18+
<ul>
19+
<li><code>setUserAuthenticationRequired</code> should be set to <code>true</code>; otherwise, the key can be used without user authentication.</li>
20+
<li><code>setInvalidatedByBiometricEnrollment</code> should be set to <code>true</code> (the default); otherwise, an attacker can use the key by enrolling additional biometrics on the device.</li>
21+
<li><code>setUserAuthenticationValidityDurationSeconds</code>, if used, should be set to <code>-1</code>; otherwise, non-biometric (less secure) credentials can be used to access the key. We recommend using <code>setUserAuthenticationParameters</code> instead to explicitly set both the timeout and the types of credentials that may be used.</li>
22+
</ul>
23+
24+
</recommendation>
25+
26+
<example>
27+
<p>The following example demonstrates a key that is configured with secure paramaters:</p>
28+
<sample src="AndroidInsecureKeysGood.java"/>
29+
30+
<p>In each of the following cases, a parameter is set insecurely:</p>
31+
<sample src="AndroidInsecureKeysBad.java"/>
32+
</example>
33+
34+
<references>
35+
<li>
36+
WithSecure: <a href="https://labs.withsecure.com/publications/how-secure-is-your-android-keystore-authentication">How Secure is your Android Keystore Authentication?</a>.
37+
</li>
38+
<li>
39+
Android Developers: <a href="https://developer.android.com/reference/android/security/keystore/KeyGenParameterSpec.Builder">KeyGenParameterSpec.Builder</a>.
40+
</li>
41+
42+
</references>
43+
</qhelp>

‎java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeys.ql

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
/**
2+
* @name Insecurely generated keys for local authentication
3+
* @description Generation of keys with insecure parameters for local biometric authentication can allow attackers with physical access to bypass authentication checks.
4+
* @kind problem
5+
* @problem.severity warning
6+
* @security-severity 4.4
7+
* @precision medium
8+
* @id java/android/insecure-local-key-gen
9+
* @tags security
10+
* external/cwe/cwe-287
11+
*/
12+
13+
import java
14+
import semmle.code.java.security.AndroidLocalAuthQuery
15+
16+
from InsecureBiometricKeyParamCall call
17+
where usesLocalAuth()
18+
select call, "This key is not secure for biometric authentication."

‎java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeysBad.java

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
private void generateSecretKey() {
2+
KeyGenParameterSpec keyGenParameterSpec = new KeyGenParameterSpec.Builder(
3+
"MySecretKey",
4+
KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT)
5+
.setBlockModes(KeyProperties.BLOCK_MODE_CBC)
6+
.setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_PKCS7)
7+
// BAD: User authentication is not required to use this key.
8+
.setUserAuthenticationRequired(false)
9+
.build();
10+
KeyGenerator keyGenerator = KeyGenerator.getInstance(
11+
KeyProperties.KEY_ALGORITHM_AES, "AndroidKeyStore");
12+
keyGenerator.init(keyGenParameterSpec);
13+
keyGenerator.generateKey();
14+
}
15+
16+
private void generateSecretKey() {
17+
KeyGenParameterSpec keyGenParameterSpec = new KeyGenParameterSpec.Builder(
18+
"MySecretKey",
19+
KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT)
20+
.setBlockModes(KeyProperties.BLOCK_MODE_CBC)
21+
.setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_PKCS7)
22+
.setUserAuthenticationRequired(true)
23+
// BAD: An attacker can access this key by enrolling additional biometrics.
24+
.setInvalidatedByBiometricEnrollment(false)
25+
.build();
26+
KeyGenerator keyGenerator = KeyGenerator.getInstance(
27+
KeyProperties.KEY_ALGORITHM_AES, "AndroidKeyStore");
28+
keyGenerator.init(keyGenParameterSpec);
29+
keyGenerator.generateKey();
30+
}
31+
32+
private void generateSecretKey() {
33+
KeyGenParameterSpec keyGenParameterSpec = new KeyGenParameterSpec.Builder(
34+
"MySecretKey",
35+
KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT)
36+
.setBlockModes(KeyProperties.BLOCK_MODE_CBC)
37+
.setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_PKCS7)
38+
.setUserAuthenticationRequired(true)
39+
.setInvalidatedByBiometricEnrollment(true)
40+
// BAD: This key can be accessed using non-biometric credentials.
41+
.setUserAuthenticationValidityDurationSeconds(30)
42+
.build();
43+
KeyGenerator keyGenerator = KeyGenerator.getInstance(
44+
KeyProperties.KEY_ALGORITHM_AES, "AndroidKeyStore");
45+
keyGenerator.init(keyGenParameterSpec);
46+
keyGenerator.generateKey();
47+
}

‎java/ql/src/Security/CWE/CWE-287/AndroidInsecureKeysGood.java

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
private void generateSecretKey() {
2+
KeyGenParameterSpec keyGenParameterSpec = new KeyGenParameterSpec.Builder(
3+
"MySecretKey",
4+
KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT)
5+
.setBlockModes(KeyProperties.BLOCK_MODE_CBC)
6+
.setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_PKCS7)
7+
// GOOD: Secure parameters are used to generate a key for biometric authentication.
8+
.setUserAuthenticationRequired(true)
9+
.setInvalidatedByBiometricEnrollment(true)
10+
.setUserAuthenticationParameters(0, KeyProperties.AUTH_BIOMETRIC_STRONG)
11+
.build();
12+
KeyGenerator keyGenerator = KeyGenerator.getInstance(
13+
KeyProperties.KEY_ALGORITHM_AES, "AndroidKeyStore");
14+
keyGenerator.init(keyGenParameterSpec);
15+
keyGenerator.generateKey();
16+
}

‎java/ql/src/change-notes/2024-02-12-android-insecure-keys.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* Added a new query `java/android/insecure-local-key-gen` for finding instances of keys generated for biometric authentication in an insecure way.

‎java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth.expected renamed to ‎java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/InsecureKeys.expected

File renamed without changes.

‎java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/InsecureKeys.ql

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
import java
2+
import TestUtilities.InlineExpectationsTest
3+
import semmle.code.java.dataflow.DataFlow
4+
import semmle.code.java.security.AndroidLocalAuthQuery
5+
6+
module InsecureKeysTest implements TestSig {
7+
string getARelevantTag() { result = "insecure-key" }
8+
9+
predicate hasActualResult(Location location, string element, string tag, string value) {
10+
tag = "insecure-key" and
11+
exists(InsecureBiometricKeyParamCall call | usesLocalAuth() |
12+
call.getLocation() = location and
13+
element = call.toString() and
14+
value = ""
15+
)
16+
}
17+
}
18+
19+
import MakeTest<InsecureKeysTest>

‎java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/Test.java

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
import android.security.keystore.KeyGenParameterSpec;
2+
import android.hardware.biometrics.BiometricPrompt;
3+
import android.security.keystore.KeyProperties;
4+
import javax.crypto.KeyGenerator;
5+
6+
class Test {
7+
void test() {
8+
KeyGenParameterSpec.Builder builder = new KeyGenParameterSpec.Builder("MySecretKey", KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT);
9+
builder.setUserAuthenticationRequired(false); // $insecure-key
10+
builder.setInvalidatedByBiometricEnrollment(false); // $insecure-key
11+
builder.setUserAuthenticationValidityDurationSeconds(30); // $insecure-key
12+
}
13+
14+
private void generateSecretKey() throws Exception {
15+
KeyGenParameterSpec keyGenParameterSpec = new KeyGenParameterSpec.Builder(
16+
"MySecretKey",
17+
KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT)
18+
.setBlockModes(KeyProperties.BLOCK_MODE_CBC)
19+
.setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_PKCS7)
20+
// GOOD: Secure parameters are used to generate a key for biometric authentication.
21+
.setUserAuthenticationRequired(true)
22+
.setInvalidatedByBiometricEnrollment(true)
23+
.setUserAuthenticationParameters(0, KeyProperties.AUTH_BIOMETRIC_STRONG)
24+
.build();
25+
KeyGenerator keyGenerator = KeyGenerator.getInstance(
26+
KeyProperties.KEY_ALGORITHM_AES, "AndroidKeyStore");
27+
keyGenerator.init(keyGenParameterSpec);
28+
keyGenerator.generateKey();
29+
}
30+
}
31+
32+
class Callback extends BiometricPrompt.AuthenticationCallback {
33+
public static void useKey(BiometricPrompt.CryptoObject key) {}
34+
35+
@Override
36+
public void onAuthenticationSucceeded(BiometricPrompt.AuthenticationResult result) {
37+
useKey(result.getCryptoObject());
38+
}
39+
}

‎java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test1/options

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../../stubs/google-android-9.0.0

‎java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test2/InsecureKeys.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
testFailures
2+
failures

‎java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test2/InsecureKeys.ql

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
import java
2+
import TestUtilities.InlineExpectationsTest
3+
import semmle.code.java.dataflow.DataFlow
4+
import semmle.code.java.security.AndroidLocalAuthQuery
5+
6+
module InsecureKeysTest implements TestSig {
7+
string getARelevantTag() { result = "insecure-key" }
8+
9+
predicate hasActualResult(Location location, string element, string tag, string value) {
10+
tag = "insecure-key" and
11+
exists(InsecureBiometricKeyParamCall call | usesLocalAuth() |
12+
call.getLocation() = location and
13+
element = call.toString() and
14+
value = ""
15+
)
16+
}
17+
}
18+
19+
import MakeTest<InsecureKeysTest>

‎java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test2/Test.java

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
import android.security.keystore.KeyGenParameterSpec;
2+
import android.hardware.biometrics.BiometricPrompt;
3+
import android.security.keystore.KeyProperties;
4+
5+
class Test {
6+
void test() {
7+
KeyGenParameterSpec.Builder builder = new KeyGenParameterSpec.Builder("MySecretKey", KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT);
8+
// No alert as there is no use of biometric authentication in this application.
9+
builder.setUserAuthenticationRequired(false);
10+
builder.setInvalidatedByBiometricEnrollment(false);
11+
builder.setUserAuthenticationValidityDurationSeconds(30);
12+
}
13+
}

‎java/ql/test/query-tests/security/CWE-287/InsecureKeys/Test2/options

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../../stubs/google-android-9.0.0

‎java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth/InsecureLocalAuth.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
testFailures
2+
failures

‎java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth.ql renamed to ‎java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth/InsecureLocalAuth.ql

File renamed without changes.

‎java/ql/test/query-tests/security/CWE-287/Test.java renamed to ‎java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth/Test.java

File renamed without changes.

‎java/ql/test/query-tests/security/CWE-287/Test2.java renamed to ‎java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth/Test2.java

File renamed without changes.

‎java/ql/test/query-tests/security/CWE-287/options renamed to ‎java/ql/test/query-tests/security/CWE-287/InsecureLocalAuth/options

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/google-android-9.0.0
1+
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/google-android-9.0.0

‎java/ql/test/stubs/google-android-9.0.0/android/security/keystore/KeyGenParameterSpec.java

Lines changed: 76 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)