Update Intent summary

[flankerhqd]

This fixes issue #520

[RichardHoOoOo]

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.

[flankerhqd]

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.

Yep correct, you can just go ahead and replace all of them :)

[StevenArzt]

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.

[flankerhqd]

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

[flankerhqd]

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?

[RichardHoOoOo]

@StevenArzt @flankerhqd I think commit1 and commit2 caused this problem. So if flankerhqd's proposed fix is adopted, commit1 is basically reverted. Does it have any side effects?

[flankerhqd]

@StevenArzt @flankerhqd I think commit1 and commit2 caused this problem. So if flankerhqd's proposed fix is adopted, commit1 is basically reverted. Does it have any side effects?

Yes, I observed serious performance regression after applying the aforementioned patch, which should not be possible.

[StevenArzt]

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.