Java: CWE-400 - Query to detect uncontrolled thread resource consumption

[luchua-bc]

Thread.sleep method is used to pause the execution of current thread for specified time. When it is used to keep several relevant tasks in synchronization and the sleep time is user-controlled data, especially in the web application context, it can be abused to cause all of a server's threads to sleep, leading to denial of service.

The query detects uncontrolled thread resource consumption in Java EE web applications, which checks:

• sources of servlets and JSF
• remote user input and local container configuration
• both the javax and jakarta packages

Please consider to merge the PR. Thanks.

[luchua-bc]

Thanks @Marcono1234 and @intrigus-lgtm for reviewing this PR. I've made requested changes.

[Marcono1234]

Thanks for considering the review comments!
I have had a closer look at the implementation of this query now; hopefully these comments are useful as well.

Though as usual, feel free to consider them only as suggestions because I am not a member of this project.

[luchua-bc]

I've also added a new test case to detect remote source from an HTTP request header such as Retry-After:

codeql/java/ql/test/experimental/query-tests/security/CWE-400/ThreadResourceAbuse.java

Lines 170 to 173 in 272e4f6

	protected void doHead2(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
	// Get thread pause time from request header
	String header = request.getHeader("Retry-After");
	int retryAfter = Integer.parseInt(header);

[luchua-bc]

I've updated the query to handle the scenario of Apache Commons File Upload, which is the case of the referenced gwtupload project. Although this is a special case, it may make sense to have it in the query since Apache common libraries are widely used. Please let me know if it shall be removed to make the query more general-purpose.

[JarLob]

I think there is something wrong with the additional taint step. Could you check why it flags https://github.com/apache/struts/blob/3352d65d6cbef54a1813c7c5d8a21a6f8d6897a1/apps/showcase/src/main/java/org/apache/struts2/showcase/wait/LongProcessAction.java#L35? The class is not derived from Runnable...

[luchua-bc]

I think there is something wrong with the additional taint step. Could you check why it flags https://github.com/apache/struts/blob/3352d65d6cbef54a1813c7c5d8a21a6f8d6897a1/apps/showcase/src/main/java/org/apache/struts2/showcase/wait/LongProcessAction.java#L35? The class is not derived from Runnable...

Thanks @JarLob for reviewing this PR. I think it has nothing to do with the additional taint step. Thread.sleep() is defined in the sink PauseThreadSink, which is the reason that it's shown.

[JarLob]

The query doesn't show any path in that case. Where is the taint source? Why does it think the time is tainted? It is in a method that is called execute. This is why I thought it is related to the additional taint step.

[luchua-bc]

The query doesn't show any path in that case. Where is the taint source? Why does it think the time is tainted? It is in a method that is called execute. This is why I thought it is related to the additional taint step.

The taint source is RemoteFlowSource, in this case it is the time instance variable of the Struts action LongProcessAction. Getter and setter methods in Struts actions allow the time variable to be accessed and manipulated from invoking pages/actions.

[atorralba]

I added a couple of suggestions to the qhelp and the QLDocs.

Also, now that a second query has been added there's a lot of duplicated code. I'd suggest moving the additional taint steps, the sanitizer, and the barrier guard to a shared library (maybe to ThreadPauseSink.qll after renaming it to something like ThreadResourceAbuse.qll?) and importing them in both queries.

[smowton]

Made a few inline edits. Over to you to add the missing local-input tests.

[luchua-bc]

Thanks @smowton a lot for all your help with this PR.