Open flankerhqd opened 2 years ago
Hi @flankerhqd , thanks for your fix. But I guess the summary xml file needs a big refactor if #520 can be fixed in this way. Because not only setType
, but also all other chain calls like setClass
, setClassName
, setComponent
have this problem. In addition, except Intent
, other classes may also have this problem.
Hi @flankerhqd , thanks for your fix. But I guess the summary xml file needs a big refactor if #520 can be fixed in this way. Because not only
setType
, but also all other chain calls likesetClass
,setClassName
,setComponent
have this problem. In addition, exceptIntent
, other classes may also have this problem.
Yep correct, you can just go ahead and replace all of them :)
This fix doesn't solve the underlying problem. In the example, the incorrect propagation happens to fail the type check, so enforcing type checks avoids the problem in this case. In general, it might even cause problems.
We have a different attribute on summaries. Excerpt from the XSD:
<!--
True if no further fields shall be appended to the "to" element. Imagine the "from"
element references a.b and copies it to d, but a.b.c is tainted. Without this option,
the resulting taint would be on d.c. With this option, it would only be on d.
-->
<xs:attribute name="cutSubfields" type="xs:boolean" use="optional" default="false" />
The attribute defaults to false
, so FlowDroid should not taint the entire return value, but rather copy over the access path. We need to find out where the spurious additional taint comes from.
This fix doesn't solve the underlying problem. In the example, the incorrect propagation happens to fail the type check, so enforcing type checks avoids the problem in this case. In general, it might even cause problems.
We have a different attribute on summaries. Excerpt from the XSD:
<!-- True if no further fields shall be appended to the "to" element. Imagine the "from" element references a.b and copies it to d, but a.b.c is tainted. Without this option, the resulting taint would be on d.c. With this option, it would only be on d. --> <xs:attribute name="cutSubfields" type="xs:boolean" use="optional" default="false" />
The attribute defaults to
false
, so FlowDroid should not taint the entire return value, but rather copy over the access path. We need to find out where the spurious additional taint comes from.
Interesting, I'll take a look
This fix doesn't solve the underlying problem. In the example, the incorrect propagation happens to fail the type check, so enforcing type checks avoids the problem in this case. In general, it might even cause problems.
We have a different attribute on summaries. Excerpt from the XSD:
<!-- True if no further fields shall be appended to the "to" element. Imagine the "from" element references a.b and copies it to d, but a.b.c is tainted. Without this option, the resulting taint would be on d.c. With this option, it would only be on d. --> <xs:attribute name="cutSubfields" type="xs:boolean" use="optional" default="false" />
The attribute defaults to
false
, so FlowDroid should not taint the entire return value, but rather copy over the access path. We need to find out where the spurious additional taint comes from.
Hi Steven:
Currently the implementation of cutSubFields
depends on typeChecking
flag, see the following code:
SummaryTaintWrapper
/**
* Checks whether the following fields shall be deleted when applying the given
* flow specification
*
* @param flow The flow specification to check
* @return true if the following fields shall be deleted, false otherwise
*/
protected boolean isCutSubFields(MethodFlow flow) {
Boolean cut = flow.getCutSubFields();
Boolean typeChecking = flow.getTypeChecking();
if (cut == null) {
if (typeChecking != null)
return !typeChecking.booleanValue();
return false;
}
return cut.booleanValue();
}
The flow.getCutSubFields
default to null, so if typeChecking
is set to false, isCutSubFields
is actually disabled.
So the fix here might be by default assign all getCutSubFields to false instead of null, or ignore the typeChecking
flag here.
Which do you prefer?
Strange. The commits you mention just allowed us to not configure the setting and use the default value. Instead of pushing the default to the summary reader, I decided to just keep the variable null
and handle the proper default in the code that knows the semantics. It shouldn't change any performance numbers, though.
Concerning the logic behind the current code: You can explicitly instruct FlowDroid to cut subfields or not (non-null value for cut
). If you don't do this, we assume that if you disable type checking, you do this because the taint is transferred to an object of another type. We need to remove the subfields in this case, because they are wrong. Example:
A a = ?
B b = a.get();
Assume that after the first line, a.foo.x
is tainted. The get
method transfers a taint from this.foo
to the return value (which is sensible, because there might be many possible subfields of this.foo
depending on how the taint was created). Classes A and B are not cast-compatible, so the summary author disabled type checking. In this case, we also need to drop the subfield x
, because we would otherwise taint b.x
, where Class B doesn't even have a field x
.
This fixes issue https://github.com/secure-software-engineering/FlowDroid/issues/520