Skip to content

Conversation

aibaars
Copy link
Contributor

@aibaars aibaars commented Jul 3, 2020

This pull request model taint propagation for the methods of the java.util.Arrays class.

@aibaars aibaars requested a review from a team as a code owner July 3, 2020 15:17
or
method.getDeclaringType().hasQualifiedName("java.util", "Arrays") and
(
method.hasName(["copyOf", "copyOfRange", "deepToString", "spliterator", "stream", "toString"]) and
Copy link
Contributor

@Marcono1234 Marcono1234 Jul 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the general convention regarding Object.toString() of this project? It could be a taint step if it is overridden and includes the values of the fields of the object, but if it is not overridden or does not ouput the value of all fields or their values do not override toString() this would cause false positives.

So if this considers deepToString and toString as taint steps, Object.toString() should probably be considered as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's exclude "deepToString" and "toString" for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

or
method.getDeclaringType().hasQualifiedName("java.util", "Arrays") and
(
method.hasName(["fill", "parallelPrefix", "parallelSetAll", "setAll"]) and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"parallelPrefix", "parallelSetAll", and "setAll" won't work like this. We have yet to set up a framework for data and taint flow in library methods designed to accept lambdas.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove them for now.

@aschackmull aschackmull merged commit c166fee into github:master Jul 8, 2020
@lcartey
Copy link
Contributor

lcartey commented Jul 9, 2020

@aibaars I think we're missing Arrays.toString from this list.

@aschackmull
Copy link
Contributor

@aibaars I think we're missing Arrays.toString from this list.

Has this sort of step come up? Including this suggests that we also include Object.toString() for all types, and I wasn't certain that was desirable? We currently only include this step for certain types, e.g. StringBuilder.toString().

@aibaars
Copy link
Contributor Author

aibaars commented Jul 9, 2020

@lcartey I got a little push back on toString in other places. Perhaps we should make an issue to track how/if we'd like to support toString without introducing too many false positives. Apart from Arrays.toString and deepToString there is also Object.toString, any map or collection toString, String.valueOf and probably many more.

@lcartey
Copy link
Contributor

lcartey commented Jul 9, 2020

@aschackmull I've seen this only in synthetic benchmarks so far. I do actually think Object.toString() is desirable, with certain caveats, but here we're talking about Arrays.toString(Object[] arg), which I think is a completely different API (these are static methods). We should be able to add this one without really introducing false positives.

@lcartey
Copy link
Contributor

lcartey commented Jul 9, 2020

Actually, on reflection, you're right - the cases are inextricably linked for arrays of Objects other than String, because it will call Object.toString. @aibaars suggested special casing String arrays, which I think would work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants