Skip to content

Java: Add support for data flow through thrown exceptions. #9914

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aschackmull
Copy link
Contributor

@aschackmull aschackmull commented Jul 28, 2022

This adds support for exception flow.

A test is included that shows some of what works and what doesn't.
Currently we get spurious flow due to imprecisions in type pruning. Mostly fixed now.
There's also some missing, since I've ignored MaD-synthesised method bodies for now.

@github-actions github-actions bot added the Java label Jul 28, 2022
@intrigus-lgtm
Copy link
Contributor

Will this also fix #3710?

@aschackmull
Copy link
Contributor Author

aschackmull commented Aug 1, 2022

Will this also fix #3710?

Excellent question. I am indeed introducing dedicated data flow nodes corresponding to variable declarations in catch clauses along with (among others) the outgoing edges you ask for in the issue. However, I was not initially planning to expose those nodes, as I hadn't yet imagined use-cases. Do you think it is useful to be able to e.g. declare such a variable declaration as a source in the way that you do in the issue? With the changes in this PR I'd hoped that you didn't need to.

@aschackmull aschackmull force-pushed the java/exceptionflow branch 2 times, most recently from 3357eba to 50b9339 Compare August 2, 2022 11:16
@intrigus-lgtm
Copy link
Contributor

Will this also fix #3710?

Excellent question. I am indeed introducing dedicated data flow nodes corresponding to variable declarations in catch clauses along with (among others) the outgoing edges you ask for in the issue. However, I was not initially planning to expose those nodes, as I hadn't yet imagined use-cases. Do you think it is useful to be able to e.g. declare such a variable declaration as a source in the way that you do in the issue? With the changes in this PR I'd hoped that you didn't need to.

#3710 was originally created in response to the "Step 3: Errors and Exceptions" part of https://securitylab.github.com/ctf/codeql-and-chill/.

try {
    parse(tainted);
} catch (Exception e) {
    sink(e.getMessage())
}

The goal was to catch flow from parse to sink, but the code of parse is unavailable.
Will something like this be modelable with this PR?
(I still have to say that this feels like a really rare use case)

@aschackmull
Copy link
Contributor Author

#3710 was originally created in response to the "Step 3: Errors and Exceptions" part of https://securitylab.github.com/ctf/codeql-and-chill/.

try {
    parse(tainted);
} catch (Exception e) {
    sink(e.getMessage())
}

The goal was to catch flow from parse to sink, but the code of parse is unavailable. Will something like this be modelable with this PR? (I still have to say that this feels like a really rare use case)

Yes. In the context of this PR, the way to do that would be to make parse a taint step that transfers taint from its argument to its returned exception. No need to match up the parse call with the enclosing try-catch, that's all something that the feature in this PR will handle (including the case when parse would be nested inside some wrapper call).

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

Successfully merging this pull request may close these issues.

2 participants