fix wrong arrayLocal in extractSootArray for HardcodedError

[panoob]

Fix extractSootArray for HardCodedError

1. Bug description:

There is a explicit bug in the implementation of crypto.constraints.ConstraintSolver.EvaluableConstraint#extractSootArray, which causes plentiful false positives of HardCodedError

protected Map<String, CallSiteWithExtractedValue> extractSootArray(CallSiteWithParamIndex callSite, ExtractedValue allocSite){
	Value arrayLocal = allocSite.getValue();	// here return right value in allocSite.stmt as arrayLocal 
	Body methodBody = allocSite.stmt().getMethod().getActiveBody();
	Map<String, CallSiteWithExtractedValue> arrVal = Maps.newHashMap();
		if (methodBody != null) {
			Iterator<Unit> unitIterator = methodBody.getUnits().snapshotIterator();
			while (unitIterator.hasNext()) {
				final Unit unit = unitIterator.next();
				if (unit instanceof AssignStmt) {
					AssignStmt uStmt = (AssignStmt) (unit);
					Value leftValue = uStmt.getLeftOp();
					Value rightValue = uStmt.getRightOp();
					if (leftValue.toString().contains(arrayLocal.toString()) && !rightValue.toString().contains("newarray")) {
						arrVal.put(retrieveConstantFromValue(rightValue), new CallSiteWithExtractedValue(callSite, allocSite));
					}
				}
			}
		}
	return arrVal;
		}

As the annotation shown, the arrayLocal return wrong value (right value in the stmt), which causes later condition judgment always fails leftValue.toString().contains(arrayLocal.toString()).

Then this extractSootArray() method always returns empty map, and causes isHardCodedArray() method always returns true.

As supposed, both following TP/FP examples cannot correctly extract array and output HardCodedError for PBEKeySpec class.

TP example:

public class Main {
    public static void main(String[] args) {
        char[] passwd = {'t','h','i','s'};
        byte[] salt = new byte[256];

        SecureRandom secureRandom = new SecureRandom();
        secureRandom.nextBytes(salt);

        byte[] key = getKey(passwd, salt, 10000, 256);
    }

    public static byte[] getKey(char[] pass, byte[] salt, int iterations, int size) {
        // generate a key via a PBEKeySpec
        try{
            PBEKeySpec spec = new PBEKeySpec(pass, salt, iterations, size);
            SecretKeyFactory skf = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256");
            byte[] key = skf.generateSecret(spec).getEncoded();
            spec.clearPassword();
            return key;
        } catch (Exception e) {
        }
        return null;
    }
}

FP example:

public class Main {
    public static void main(String[] args) {
        byte[] pass = new byte[256];
        byte[] salt = new byte[256];

        SecureRandom secureRandom = new SecureRandom();
        secureRandom.nextBytes(salt);
        secureRandom.nextBytes(pass);

        // convert byte array to char array
        char[] passwd = new char[pass.length];
        for(int i=0; i < pass.length; i++){
            passwd[i] = (char) (pass[i]&0xff);
        }

        byte[] key = getKey(passwd, salt, 10000, 256);
    }

    public static byte[] getKey(char[] pass, byte[] salt, int iterations, int size) {
        // generate a key via a PBEKeySpec
        try{
            PBEKeySpec spec = new PBEKeySpec(pass, salt, iterations, size);
            SecretKeyFactory skf = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256");
            byte[] key = skf.generateSecret(spec).getEncoded();
            spec.clearPassword();
            return key;
        } catch (Exception e) {
        }
        return null;
    }
}

This bug causes False Positive of HardCodedError.

2. Bug fix:

This pull request fix the bug in crypto.constraints.ConstraintSolver.EvaluableConstraint#extractSootArray, and assign the arrayLocal correct value (left value in stmt).

The fix can test correctly with above TP/FP examples.

[AnakinRaW]

Hey, thanks for creating this PR, would it be possible for you to add the TP and FP as test cases?

[panoob]

Hi, the test cases are added.

[svenfeld]

Hi @panoob, afer reviewing the build failure it appears that the suggested fix causes two problems. The first problem is that a IndexOutOfBoundsException is caused by the call allocSite.stmt().getUnit().get().getDefBoxes().get(0).getValue() in some test cases. For example in https://github.com/CROSSINGTUD/CryptoAnalysis/blob/develop/CryptoAnalysis/src/test/java/tests/headless/BouncyCastleHeadlessTest.java#L107. I added this if (allocSite.stmt().getUnit().get() instanceof AssignStmt) to fix the IndexOutOfBound. However, your change also causes False Negatives in the existing test cases. For example in this test case https://github.com/CROSSINGTUD/CryptoAnalysis/blob/develop/CryptoAnalysis/src/test/java/tests/headless/CryptoGuardTest.java#L233. With the corresponding code in https://github.com/CROSSINGTUD/CryptoAnalysis/blob/develop/CryptoAnalysisTargets/CryptoGuardExamples/predictablekeystorepassword/src/main/java/example/predictablekeystorepassword/PredictableKeyStorePasswordABICase1.java

Do you have an idea, what could cause the False Negatives due to your code change?