Java: Uncaught servlet exception

[luchua-bc]

This PR is to address CWE-600 "Uncaught Exception in Servlet".

Failure to catch exceptions in a servlet could leave a system in a vulnerable state, possibly resulting in denial-of-service attacks, or the exposure of sensitive information because when a servlet throws an exception, the servlet container typically sends debugging information back to the user. And that information could be very valuable to an attacker.

The CodeQL query in this PR detects method calls within servlet methods that can throw IOException and RuntimeException but those exceptions are not caught then processed.

I've tested the query against some popular open-source software including Apache ActiveMQ. Details and screenshots are attached in the associated all-for-one issue #143.

Please also note I added a stub class GenericServlet, which implements the Servlet interface, and added extends GenericServlet to the stub class HttpServlet, which was missing in the repository. Without this, the check of instanceof ServletClass against the popular javax.servlet.http.HttpServlet class will fail, which should not in my opinion.

Please consider to merge the PR. Thanks.

[intrigus-lgtm]

This PR seems to contain unrelated commits?

[luchua-bc]

Thanks @intrigus-lgtm . I've removed those three unrelated commits.

[aschackmull]

I think for this case a force push to clean up the commits is fine - especially since review hasn't started.

[luchua-bc]

Thanks @aschackmull for the advice. I will do a force push next time before review starts.

[luchua-bc]

Improved the query by checking the web.xml file and making sure there is no defined in order to prevent FPs.

[smowton]

Your commit "Add error-page check" contains accidental changes to the Servlet stubs, and also an unrelated (but good) improvement adding // BAD and // GOOD tags to a different test (move those to a different commit)

[smowton]

Ah perhaps those changes were deliberate? In that case my complaint is just that the commit message disagrees with what the commit does -- try to (a) keep one commit doing one thing, and (b) make sure the message matches what is done, otherwise it looks like a mistake

[luchua-bc]

Those changes were deliberate but are for two loosely coupled things - one is the error-page check and the other is updated test cases although test results don't change since the error-page configuration in web.xml is disabled so that we can see what the query can detect without that configuration.

I've split the commit into two commits.

[smowton]

Looks good to me, will ping the Java team for final review

[smowton]

As with all PRs, if not done already make sure you've used codeql query format

[luchua-bc]

I used the "Format Document" tool of the CodeQL plugin within VS Code to do formatting and both query files (one query and one modified query library) were formatted already.

Thanks @smowton for the quick turn-around and all your help with this PR.

[Marcono1234]

Should this query also detect throw statements (whose exceptions are not caught) directly within the servlet methods?
The stack trace would not reveal that much information, but it still reveals implementation details. Though this contradicts with one of the test cases where rethrowing the exception without stack trace is considered good behavior.

[luchua-bc]

Hi @Marcono1234,

The purpose of this query is to detect a common programming error that Java EE developers don't handle IOExceptions because the Servlet method signatures declare those. And developers may not be aware that uncaught runtime exceptions and IO exceptions could leak valuable stack traces.

I've added the code to detect a throw statement containing the same exception variable directly under a catch statement e.g. catch (UnknownHostException uhex) {throw new IOException(uhex);}. This helps one of the test cases.

I think this change is probably safe but I don't want to catch every throw statement, which may introduce many false positives. The rational is that developers know what they are doing if they intentionally throw an exception with a message containing only some information e.g. ex.getMessage() or a custom message.

[Marcono1234]

Would it make sense to also detect calls to initCause(...) and addSuppressed(...)? For both the exception will be displayed in the stack trace as well, e.g.:

catch (Exception e) {
    IOException ioException = new IOException();
    ioException.initCause(e);
    // Or
    ioException.addSuppressed(e);
    throw ioException;
}

[luchua-bc]

I've modified the query to handle initCause(...) and addSuppressed(...). As there are only three simple cases, I modelled them without using a data flow.

[Marcono1234]

Looks like this should cover all common cases, thanks!

Also feel free to dismiss my review comments. After all only the comments of the maintainers of this repository matter I guess.

[luchua-bc]

Thanks @aschackmull for all your help with this PR.