-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Java: Add query to detect Apache Struts enabled Devmode #3945
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
java/ql/test/experimental/query-tests/security/CWE-489/Test.java
Outdated
Show resolved
Hide resolved
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. |
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
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
93a9abb
to
af2db33
Compare
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include suggestions from review
--- EDIT ---
Please ignore this comment I posted it by mistake.
@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 |
or in my 6 years old post :) |
While this is pending for review, I am merging the latest main here. |
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 |
@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. |
/me braces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also rebase this PR so we're not committing merges out of main
.
import experimental.semmle.code.xml.StrutsXML | ||
|
||
bindingset[path] | ||
predicate isBadPath(string path) { path.regexpMatch("(?i).*(demo|test|example).*") } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
predicate isBadPath(string path) { path.regexpMatch("(?i).*(demo|test|example).*") } | |
predicate isLikelyDemoProject(string path) { path.regexpMatch("(?i).*(demo|test|example).*") } |
@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? |
@smowton The query fails to detect any bugs as the XML extractor does not parse files located in the Is the |
No, they're still in the private repo. I've asked around about this, will get back to you shortly. |
Looks like you can add |
Better alternative, set environment variable |
https://github.com/github/semmle-code/blob/main/language-packs/java/tools/pre-finalize.sh is not public :) |
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 |
0f9aa58
to
f1b5f57
Compare
@smowton exporting |
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. |
Looks like the change to routinely import struts files missed this merge window, so I'll try this again in 2 weeks. |
This is now producing results on lgtm.com |
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](https://github.com/search?q=%3Cconstant+name%3D%22struts.devMode%22+value%3D%22true%22+%2F%3E+language%3Axml&type=Code) 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.
f1b5f57
to
42a84a1
Compare
I have rebased it to the latest main while this is pending merge. |
|
@aschackmull Changes done! |
Looks like the qltest is failing.
|
@aschackmull The tests were failing due to the 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. |
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.