Java: Improve `NonConstantTimeCheckOnSignatureQuery.qll`
- Dominant language
- CodeQL
- Stars
- 10.1k
- Forks
- 2.1k
- Avg merge
- 2d 15h
- Merged PRs (30d)
- 141
Description
Follow-up for some issues raised during the review of https://github.com/github/codeql/pull/6006.
- Asymmetric array check in `existsFailFastCheck` (https://github.com/github/codeql/pull/6006#discussion_r688978139)
The following lines should probably either both call `getArray()`:
https://github.com/github/codeql/blob/4a025053cc9acdda596565ff30f2e3ff36301b04/java/ql/src/experimental/Security/CWE/CWE-208/NonConstantTimeCheckOnSignatureQuery.qll#L284-L286
```diff
-firstArrayAccess.getArray() = firstArray and secondArray = secondArrayAccess
+firstArrayAccess.getArray() = firstArray and secondArray = secondArrayAccess.getArray()
or
-secondArrayAccess.getArray() = firstArray and secondArray = firstArrayAccess
+secondArrayAccess.getArray() = firstArray and secondArray = firstArrayAccess.getArray()
```
Most likely it is however not needed to call `getArray()` at all because the enclosing predicate `existsFailFastCheck` is used as part of taint tracking and I assume the standard taint tracking already considers flow from an array to an access to one of its elements.
- False negatives for `final` variables (https://github.com/github/codeql/pull/6006#discussion_r688978802)
The following line considers any `final` variable to be likely a constant:
https://github.com/github/codeql/blob/4a025053cc9acdda596565ff30f2e3ff36301b04/java/ql/src/experimental/Security/CWE/CWE-208/NonConstantTimeCheckOnSignatureQuery.qll#L235
This leads to false negatives because merely making a parameter or a local variable with a non-constant value `final` causes the predicate to consider it constant. For example the following Java code[^1] is not flagged when the parameter is made `final`:
```java
public boolean unsafeCheckCustomMac(/* final */ String expected, byte[] plaintext, Key key) throws Exception {
Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
cipher.init(Cipher.ENCRYPT_MODE, key);
String tag = new String(cipher.doFinal(plaintext));
return tag.equals(expected);
}
```
The question is whether that `looksLikeConstant` predicate is really needed in the first place (@artem-smotrakov). While checking for hardcoded credentials is covered by a different query, checking for hardcoded credentials in a non-constant time way seems like an additional vulnerability because it might even allow extracting the hardcoded credential. If the intention was only to ignore Java test classes, then maybe those should be ignored by file path of the compilation unit or by checking if the enclosing class is a test class (similar to how other queries do that), or to rely on GitHub code scanning classifying the code as test code and not adding any checks (?).
[^1]: This is not actually realistic code because `new String(...)` does not produce reasonable output in this situation. Instead it is more likely that user code converts the bytes to a hex string. Unfortunately taint does not seem to propagate through such manually written code properly, for example using a `hexString(...)` call with the following method instead of a `new String(...)` call does not seem to be detected:
```java
private static String hexString(byte[] bytes) {
StringBuilder sb = new StringBuilder(bytes.length * 2);
for (byte b : bytes) {
String hexChars = Integer.toHexString(b & 0xFF);
if (hexChars.length() == 1) {
sb.append('0');
}
sb.append(hexChars);
}
return sb.toString();
}
```
Contributor guide
Assessment
This issue has not been assessed yet.