Inconsistent results in loop

[draftyfrog]

Bug

Bug description
Please consider the following code:

for(int i = 0; i < 10; i++){
    try{
        Boolean trigger = false; // If removed or changed to something else than a wrapper type, no issue is reported
    }catch(Exception e){
    }finally{
        List<String> list = new ArrayList<String>();
        sink(list); // Is reported as issue
        list.add(source());
    }
}

where public String source() and public void sink(List<?> param) are defined as source and sink respectively in Marian Trenchs config.

Mariana Trench reports the sink()-call in the finally block as an issue. But if we remove the Boolean instantiation in the try block, MT doesn't report the issue anymore. Also if we just replace it with another statement like a String instantiation String trigger = "false"; MT doesn't report an issue.

Probably this issue is related to the wrapper types for Java primitives, as changing the statement to boolean trigger = false; also leads to no reported issue, but replacing it with Integer trigger = 1; makes MT report an issue.

Expected behavior
No reported issue independent from the unrelated statement in the try block.

[arthaud]

Hi @draftyfrog, thanks for reaching out.

There are multiple problems here which makes MT results inconsistent.
Depending on the body of the try..catch, the Dalvik bytecode will be different.

Trivial try/catch body

If the body only uses scalar types like boolean, then the compiler detects that no exception can be thrown, so the code is compiled into:

for(int i = 0; i < 10; i++) {
  List<String> list = new ArrayList<String>();
  sink(list);
  list.add(source());
}

From that code, MT can indeed detect that list is not tainted in sink(list) since it's always re-initialized right before.

Regular try/catch

If the try/catch can throw, then it is compiled in something much more complex:

List<String> list;
for(int i = 0; i < 10; i++) {
  try {
    Boolean trigger = Boolean.valueOf(false); // can throw (in theory)
    list = new ArrayList<String>(); // A
  } catch(RuntimeException e) { // RuntimeException cannot be caught, but the finally is still executed
    List<String> y = new ArrayList<String>();
    sink(y); // no issue
    y.add(source());
    throw e;
  } catch (Exception e) {
    list = new ArrayList<String>(); // B
  }
  sink(list); // issue reported here
  list.add(source());
}

While this is correct, this leads to a false positive in MT due to our alias analysis.
We treat both list = new ArrayList<String>(); // A and list = new ArrayList<String>(); // B as different memory locations that list can point to. If the first loop iteration leads to an exception, that will taint B, and if the second iteration does not throw an exception, we would reset the taint for A but not B. sink(list) can read from both A or B, therefore we find an issue.

There are a few ways to solve this, but it seems unlikely that we would prioritize that work, unless we get false positives from this pattern internally.

[draftyfrog]

@arthaud thank you very much for the detailed response!