Java: Add query for insecure local authentication

[joefarebrother]

This query finds instances of local authentication callbacks that don't use a CryptoObject.

[github-actions]

QHelp previews:

java/ql/src/Security/CWE/CWE-287/AndroidInsecureLocalAuthentication.qhelp

Insecure local authentication

Biometric local authentication such as fingerprint recognition can be used to protect sensitive data or actions within an application. However, if this authentication does not use a KeyStore-backed key, it can be bypassed by a privileged malicious application, or by an attacker with physical access using application hooking tools such as Frida.

Recommendation

Generate a secure key in the Android KeyStore. Ensure that the onAuthenticationSuccess callback for a biometric prompt uses it in a way that is required for the sensitive parts of the application to function, such as by using it to decrypt sensitive data or credentials.

Example

In the following (bad) case, no CryptoObject is required for the biometric prompt to grant access, so it can be bypassed.

biometricPrompt.authenticate(
    cancellationSignal,
    executor,
    new BiometricPrompt.AuthenticationCallback {
        @Override
        // BAD: This authentication callback does not make use of a `CryptoObject` from the `result`.
        public void onAuthenticationSucceeded(BiometricPrompt.AuthenticationResult result) {
            grantAccess()
        }
    }
)

In the following (good) case, a secret key is generated in the Android KeyStore. The application requires this secret key for access, using it to decrypt data.

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)
        .build();
    KeyGenerator keyGenerator = KeyGenerator.getInstance(
            KeyProperties.KEY_ALGORITHM_AES, "AndroidKeyStore");
    keyGenerator.init(keyGenParameterSpec);
    keyGenerator.generateKey();
}


private SecretKey getSecretKey() {
    KeyStore keyStore = KeyStore.getInstance("AndroidKeyStore");
    keyStore.load(null);
    return ((SecretKey)keyStore.getKey("MySecretKey", null));
}

private Cipher getCipher() {
    return Cipher.getInstance(KeyProperties.KEY_ALGORITHM_AES + "/"
            + KeyProperties.BLOCK_MODE_CBC + "/"
            + KeyProperties.ENCRYPTION_PADDING_PKCS7);
}

public prompt(byte[] encryptedData) {
    Cipher cipher = getCipher();
    SecretKey secretKey = getSecretKey();
    cipher.init(Cipher.DECRYPT_MODE, secretKey);

    biometricPrompt.authenticate(
        new BiometricPrompt.CryptoObject(cipher),
        cancellationSignal,
        executor,
        new BiometricPrompt.AuthenticationCallback() {
            @Override
            // GOOD: This authentication callback uses the result to decrypt some data.
            public void onAuthenticationSucceeded(BiometricPrompt.AuthenticationResult result) {
                Cipher cipher = result.getCryptoObject().getCipher();
                byte[] decryptedData = cipher.doFinal(encryptedData);
                grantAccessWithData(decryptedData);
            }
        }
    );
}

References

• OWASP Mobile Application Security: Android Local Authentication
• OWASP Mobile Application Security: Testing Biometric Authentication
• WithSecure: How Secure is your Android Keystore Authentication?
• Android Developers: Biometric Authentication
• Common Weakness Enumeration: CWE-287.

[atorralba]

Code LGTM. Just a handful of comments/questions. After resolving them, and assuming DCA and MRVA look good, feel free to go ahead with the docs review.

[atorralba]

Should we specify that the risk we're trying to defend against is application hooking (with e.g. Frida)?

[atorralba]

I think we should reduce this score. See this CVE for instance, it's the same issue and it has a 6.7 CVSS.

IMHO, I think we could get it even lower. The CVSS vector that makes sense for me is AV:L/AC:L/PR:H/UI:N/S:U/C:H/I:H/A:H, which has a 4.4 score.

Suggested change

	* @security-severity 9.3
	* @security-severity 4.4

[joefarebrother]

Is the 4.4 obtained by taking the average of the base/impact/exploitability metrics?
This page appears to output 6.7 as an overall CVSS score.

[atorralba]

I copied the wrong link 😅 It's 4.4 when you reduce the Integrity and Availability scores to None (since we're only considering the Confidentiality impact of accessing data protected by biometrics): https://nvd.nist.gov/vuln-metrics/cvss/v3-calculator?vector=AV:L/AC:L/PR:H/UI:N/S:U/C:H/I:N/A:N&version=3.1

[subatoi]

Thanks! Just the usual minor suggestions

[subatoi]

Thank you!