-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Java: Diff-informed queries: phase 3 (non-trivial locations) #20077
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
base: main
Are you sure you want to change the base?
Changes from all commits
5c2cf79
7aced48
49e03b4
94386f0
6134518
44bb5e7
b33058c
8353fdd
54546f6
0bcdb42
0cf1195
1c6ecf1
919fea5
19e5c3d
74b37e7
2d73405
b688df9
bc0b383
45b627d
b3b139b
3785dbe
7888dcb
ea4af83
24c28ed
05df1d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,10 @@ module ExecTaintedEnvironmentConfig implements DataFlow::ConfigSig { | |
ProcessBuilderEnvironmentFlow::flowToExpr(mm.getQualifier()) | ||
) | ||
} | ||
|
||
predicate observeDiffInformedIncrementalMode() { any() } | ||
|
||
Location getASelectedSourceLocation(DataFlow::Node source) { none() } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should exclude sources here, as the path is fully reported. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean path-problems always report the location of source and sink? That seems to contradict the example of WebviewDebuggingEnabled.ql/WebviewDebuggingEnabledQuery.qll mentioned in the incremental codeql docs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does seem to contradict that example, yes. I'm not completely sure here, but both ends of the path are a part of the result tuple. From the QL point of view, it's a bit arbitrary whether the second end-point is included in the message or not - a lot of queries do this, but from the query writers perspective it's perhaps a bit of a coin-toss, since the path includes both end-points regardless. Now if this coin-toss decision affects whether or not a result is included in a PR, then of course it shouldn't be made arbitrarily, and if that's the case then perhaps we should institute a rule (e.g. in ql-for-ql) to always include the second end-point in the message. Now, as mentioned, I'm unsure about the filtering semantics of the downstream consumption in PRs, but considering the two possible cases then either these kind of results aren't filtered in which case we shouldn't exclude sources here, or they are filtered in which case I think we also shouldn't filter here, since then I'd argue we'd want to modify the query to include the source in the message to prevent the filtering. We should move this conversation to slack and figure it out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the documentation we only want to report alerts on locations that are pertaining to either the primary location (location of the first element in the select) or an alert pertaining to a related location - locations for elements used in placeholders There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah ok, you are questioning the design (saw the thread in slack). |
||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -145,6 +145,10 @@ module TempDirSystemGetPropertyToCreateConfig implements DataFlow::ConfigSig { | |
or | ||
sanitizer instanceof WindowsOsSanitizer | ||
} | ||
|
||
predicate observeDiffInformedIncrementalMode() { any() } | ||
|
||
Location getASelectedSinkLocation(DataFlow::Node sink) { none() } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same. The query reports the full path, so we shouldn't exclude sinks like this. |
||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
import javax.servlet.http.HttpServlet; | ||
import javax.servlet.http.HttpServletRequest; | ||
import javax.servlet.http.HttpServletResponse; | ||
import javax.servlet.ServletException; | ||
import java.io.IOException; | ||
|
||
public class ExternalAPISinkExample extends HttpServlet { | ||
protected void doGet(HttpServletRequest request, HttpServletResponse response) | ||
throws ServletException, IOException { | ||
// BAD: a request parameter is written directly to an error response page | ||
response.sendError(HttpServletResponse.SC_NOT_FOUND, | ||
"The page \"" + request.getParameter("page") + "\" was not found."); // $ Alert | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
import javax.servlet.http.HttpServlet; | ||
import javax.servlet.http.HttpServletRequest; | ||
import javax.servlet.http.HttpServletResponse; | ||
import javax.servlet.ServletException; | ||
import java.io.IOException; | ||
|
||
public class ExternalAPITaintStepExample extends HttpServlet { | ||
protected void doGet(HttpServletRequest request, HttpServletResponse response) | ||
throws ServletException, IOException { | ||
|
||
StringBuilder sqlQueryBuilder = new StringBuilder(); | ||
sqlQueryBuilder.append("SELECT * FROM user WHERE user_id='"); | ||
// BAD: a request parameter is concatenated directly into a SQL query | ||
sqlQueryBuilder.append(request.getParameter("user_id")); | ||
sqlQueryBuilder.append("'"); | ||
|
||
// ... | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
| javax.servlet.http.HttpServletResponse.sendError(int,java.lang.String) [param 1] | 1 | 1 | |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Security/CWE/CWE-020/ExternalAPIsUsedWithUntrustedData.ql |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
#select | ||
| ExternalAPISinkExample.java:12:5:12:70 | ... + ... | ExternalAPISinkExample.java:12:21:12:48 | getParameter(...) : String | ExternalAPISinkExample.java:12:5:12:70 | ... + ... | Call to javax.servlet.http.HttpServletResponse.sendError with untrusted data from $@. | ExternalAPISinkExample.java:12:21:12:48 | getParameter(...) : String | getParameter(...) : String | | ||
edges | ||
| ExternalAPISinkExample.java:12:21:12:48 | getParameter(...) : String | ExternalAPISinkExample.java:12:5:12:70 | ... + ... | provenance | Src:MaD:2 Sink:MaD:1 | | ||
models | ||
| 1 | Sink: javax.servlet.http; HttpServletResponse; false; sendError; (int,String); ; Argument[1]; information-leak; manual | | ||
| 2 | Source: javax.servlet; ServletRequest; false; getParameter; (String); ; ReturnValue; remote; manual | | ||
nodes | ||
| ExternalAPISinkExample.java:12:5:12:70 | ... + ... | semmle.label | ... + ... | | ||
| ExternalAPISinkExample.java:12:21:12:48 | getParameter(...) : String | semmle.label | getParameter(...) : String | | ||
subpaths |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
query: Security/CWE/CWE-020/UntrustedDataToExternalAPI.ql | ||
postprocess: | ||
- utils/test/PrettyPrintModels.ql | ||
- utils/test/InlineExpectationsTestQuery.ql |
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.
The comment mentions OOMs under
--check-diff-informed
but doesn't provide sufficient context about the issue or potential solutions. Consider adding more details about the specific test case causing the problem and any planned follow-up actions.Copilot uses AI. Check for mistakes.