Java: Add query to detect Apache Struts enabled Devmode

[ghost]

This query detects cases where the development mode is enabled for a struts configuration. I can't find a CVE per se but, at present, Github's fuzzy search returns more than 44000 results. Some of them look like they are classroom projects, so they may be ineligible for a CVE. But we should be flagging them anyways as setting the development on in a production system is a very bad practice and can often lead to remote code execution. So these should be fixed anyways.

[Marcono1234]

Maybe it would be good to include "Apache Struts" in some way in the file name devMode.qhelp (and similar) since this query only applies to that?

[luchua-bc]

This is an interesting area. As all development frameworks and application servers offer a dev or debug mode, is it possible for the query to detect production deployment only to reduce false positives? Thanks.

[ghost]

Hi @luchua-bc,

Yes, some of the frameworks offer a flag to turn developer/debug mode on. As you can see, I haven't adddressed @Marcono1234's comments on this PR. I am adding support for some other frameworks like Vaadin too here. Let me know if you have a particular framework in mind. I will include that one too.

is it possible for the query to detect production deployment only to reduce false positives

IMO, development mode should only be turned on when you are developing the code. No decent project actually allows you to knowingly push bad code into master. It is expected of the developer to clean the code for any debug symbols/bugs before pushing the changes. So, prod or no prod, if debug mode is turned on in a public repo, it's something which codeql should alert on.

As for cases where debug mode is enabled in test files, codeql, if I remember correctly, would by default ignore all test files while running the query. So, I don't have to skip any test files in my analysis as such.

[ghost]

Also, please do note that errors for this sorts have actually seeped into actual codebases multiple times. The most common one being the Flask Werkzeug debugger RCE.

A similar bug is possible for Apache Struts, you may see the exploitation steps here. Also, this presentation looks like it is about this very issue. However, the presentation is not in a language I understand so I can't say for sure.

[smowton]

I wouldn't necessarily worry about the initial PR covering many different frameworks. You might want to look at excluding common false-positives though -- for example, are there common signs that a web.xml file is an example that isn't enabled by default, such as its filename or directory?

Are you intending a bounty application for this? Either way, let me know when you consider it ready to evaluate.

[ghost]

@smowton @Marcono1234 I have included the changes you proposed and included a small heuristic to check for a bad path.

Also, @smowton I am opening the GHSL issue for htis one now.

[Marcono1234]

Thanks! When there are already review comments, could you please refrain from force-pushing? It makes it difficult (in GitHub) to see the differences. If the commit history of the pull request becomes too long the commits can be squashed when merging, if necessary.

I have added some more review comments, hopefully they are helpful. Note that I am not a member of this project so these comments are not mandatory (unless the maintainers agree with the them).

[ghost]

include suggestions from review
--- EDIT ---
Please ignore this comment I posted it by mistake.

[ghost]

@smowton The tests for this one would fail as I mark all files with test in their file path as FP's. The output included here is for when you invert the isBadPath filter.

[pwntester]

Also, please do note that errors for this sorts have actually seeped into actual codebases multiple times. The most common one being the Flask Werkzeug debugger RCE.

A similar bug is possible for Apache Struts, you may see the exploitation steps here. Also, this presentation looks like it is about this very issue. However, the presentation is not in a language I understand so I can't say for sure.

or in my 6 years old post :)
https://www.pwntester.com/blog/2014/01/21/struts-2-devmode-an-ognl-backdoor/

[ghost]

While this is pending for review, I am merging the latest main here.

[smowton]

Apologies, I hadn't realised this one was back in my court. Marked to review after the Christmas break.

Also, please rebase rather than merge, it makes the resulting history simpler

[ghost]

@smowton No worries. Happy holidays'.

I plan on working on my PR's during this time though so you may have a lot of spam headed your way. Apologies in advance.

[smowton]

/me braces

[smowton]

Please also rebase this PR so we're not committing merges out of main.

[smowton]

Suggested change

	predicate isBadPath(string path) { path.regexpMatch("(?i).*(demo|test|example).*") }
	predicate isLikelyDemoProject(string path) { path.regexpMatch("(?i).*(demo|test|example).*") }

[smowton]

@porcupineyhairs this currently produces zero results. Could you re-check whether you're able to get any results against some of the projects you turned up using fuzzy search?

[ghost]

@smowton The query fails to detect any bugs as the XML extractor does not parse files located in theresources directory. As all struts.xml files are usually stored in that directory, none of them are actually included in the final CodeQL database rendering the run useless.

Is the Java-XML extractor source public? I can't seem to find it.

[smowton]

No, they're still in the private repo. I've asked around about this, will get back to you shortly.

[smowton]

Looks like you can add --xml-indexing-mode all to the command in $CODEQL_HOME/codeql/java/tools/autobuild.sh for a quick hack -- will let you know if I find a better way to do that!

[smowton]

Better alternative, set environment variable LGTM_INDEX_XML_MODE=smart, which as you can see in https://github.com/github/semmle-code/blob/main/language-packs/java/tools/pre-finalize.sh selects by the top-level tag name including struts. If for any reason that doesn't work you could also try the value LGTM_INDEX_XML_MODE=all.

[intrigus-lgtm]

Better alternative, set environment variable LGTM_INDEX_XML_MODE=smart, which as you can see in https://github.com/github/semmle-code/blob/main/language-packs/java/tools/pre-finalize.sh selects by the top-level tag name including struts. If for any reason that doesn't work you could also try the value LGTM_INDEX_XML_MODE=all.

https://github.com/github/semmle-code/blob/main/language-packs/java/tools/pre-finalize.sh is not public :)

[smowton]

Hah, whoops. In any case the environment variable settings can be tried from the command-line, and you should be able to see the script distributed as $CODEQL_HOME/codeql/java/tools/pre-finalize.sh.

[ghost]

@smowton exporting LGTM_INDEX_XML_MODE=all or LGTM_INDEX_XML_MODE=smart works well for me. I can confirm the fuzzy search results are now being detected.

[smowton]

We're going to add struts.xml to our routine XML import filter, so that we can test this against a broad swathe of projects. We should be in a position to do that in roughly 2 weeks. Prod me if you don't hear anything by then.

[smowton]

Looks like the change to routinely import struts files missed this merge window, so I'll try this again in 2 weeks.

[smowton]

This is now producing results on lgtm.com

[ghost]

I have rebased it to the latest main while this is pending merge.

[aschackmull]

StrutsXML.qll needs autoformatting

[ERROR] Input file ql/java/ql/src/experimental/semmle/code/xml/StrutsXML.qll is not correctly formatted

[ghost]

@aschackmull Changes done!

[aschackmull]

Looks like the qltest is failing.

[3/8]  [18/42] ql/java/ql/test/experimental/query-tests/security/CWE-489/devMode.qlref: FAILED (compilation: 1.9s, execution: 42ms, total: 2s)
Expected results:
| StrutsBad.xml:8:5:8:52 | constant | Enabling development mode in production environments is dangerous |

Actual results:

Diff:
--- expected
+++ actual
@@ -1,1 +1,1 @@
-| StrutsBad.xml:8:5:8:52 | constant | Enabling development mode in production environments is dangerous |
+

[ghost]

@aschackmull The tests were failing due to the isLikelyDemoProject check applied on all results. Since that check blocks all results which have *test* in their relative file paths, results in ql test directory are excluded as they contain query-tests in their relative path.

To fix this, I have removed the tests in question from this PR. I don't know if there is another way possible which does not involve duplicating the entire query.