Java: Improve NonConstantTimeCheckOnSignatureQuery.qll

[Marcono1234]

Follow-up for some issues raised during the review of #6006.

• Asymmetric array check in existsFailFastCheck (#6006 (comment))
  The following lines should probably either both call getArray():
  

  codeql/java/ql/src/experimental/Security/CWE/CWE-208/NonConstantTimeCheckOnSignatureQuery.qll

  Lines 284 to 286 in 4a02505

  	firstArrayAccess.getArray() = firstArray and secondArray = secondArrayAccess
  	or
  	secondArrayAccess.getArray() = firstArray and secondArray = firstArrayAccess

  -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 (#6006 (comment))
  The following line considers any final variable to be likely a constant:
  

  codeql/java/ql/src/experimental/Security/CWE/CWE-208/NonConstantTimeCheckOnSignatureQuery.qll

  Line 235 in 4a02505

  expr.(VarAccess).getVariable().isFinal() and expr.getType() instanceof TypeString

  
  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¹ is not flagged when the parameter is made final:

  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 (?).

Footnotes

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:

   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();
   }

   ↩