codeql icon indicating copy to clipboard operation
codeql copied to clipboard

Java: Add support for data flow through thrown exceptions.

Open aschackmull opened this issue 2 years ago • 4 comments

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. There's also some missing, since I've ignored MaD-synthesised method bodies for now.

aschackmull avatar Jul 28 '22 14:07 aschackmull

Will this also fix https://github.com/github/codeql/issues/3710?

intrigus-lgtm avatar Aug 01 '22 11:08 intrigus-lgtm

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 avatar Aug 01 '22 11:08 aschackmull

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)

intrigus-lgtm avatar Aug 07 '22 22:08 intrigus-lgtm

#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).

aschackmull avatar Aug 08 '22 07:08 aschackmull