[Java] CWE-348: Using a client-supplied IP address in a security check #5631
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| import javax.servlet.http.HttpServletRequest; | ||
| import org.apache.commons.lang3.StringUtils; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.springframework.stereotype.Controller; | ||
| import org.springframework.web.bind.annotation.GetMapping; | ||
| import org.springframework.web.bind.annotation.ResponseBody; | ||
|
|
||
| @Controller | ||
| public class UseOfLessTrustedSource { | ||
|
|
||
| private static final Logger log = LoggerFactory.getLogger(UseOfLessTrustedSource.class); | ||
|
|
||
| @GetMapping(value = "bad1") | ||
| @ResponseBody | ||
| public String bad1(HttpServletRequest request) { | ||
| String remoteAddr = ""; | ||
| if (request != null) { | ||
| remoteAddr = request.getHeader("X-FORWARDED-FOR"); | ||
| remoteAddr = remoteAddr.split(",")[0]; | ||
| if (remoteAddr == null || "".equals(remoteAddr)) { | ||
| remoteAddr = request.getRemoteAddr(); | ||
| } | ||
| } | ||
| return remoteAddr; | ||
| } | ||
|
|
||
| @GetMapping(value = "bad2") | ||
| public void bad2(HttpServletRequest request) { | ||
| String ip = request.getHeader("X-Forwarded-For"); | ||
|
|
||
| log.debug("getClientIP header X-Forwarded-For:{}", ip); | ||
|
|
||
| if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { | ||
| ip = request.getHeader("Proxy-Client-IP"); | ||
| log.debug("getClientIP header Proxy-Client-IP:{}", ip); | ||
| } | ||
| if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { | ||
| ip = request.getHeader("WL-Proxy-Client-IP"); | ||
| log.debug("getClientIP header WL-Proxy-Client-IP:{}", ip); | ||
| } | ||
| if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { | ||
| ip = request.getHeader("HTTP_CLIENT_IP"); | ||
| log.debug("getClientIP header HTTP_CLIENT_IP:{}", ip); | ||
| } | ||
| if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { | ||
| ip = request.getHeader("HTTP_X_FORWARDED_FOR"); | ||
| log.debug("getClientIP header HTTP_X_FORWARDED_FOR:{}", ip); | ||
| } | ||
| if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { | ||
| ip = request.getHeader("X-Real-IP"); | ||
| log.debug("getClientIP header X-Real-IP:{}", ip); | ||
| } | ||
| if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { | ||
| ip = request.getRemoteAddr(); | ||
| log.debug("getRemoteAddr IP:{}", ip); | ||
| } | ||
| System.out.println("client ip is: " + ip); | ||
| } | ||
|
|
||
| @GetMapping(value = "good1") | ||
| @ResponseBody | ||
| public String good1(HttpServletRequest request) { | ||
| String remoteAddr = ""; | ||
| if (request != null) { | ||
| remoteAddr = request.getHeader("X-FORWARDED-FOR"); | ||
| remoteAddr = remoteAddr.split(",")[remoteAddr.split(",").length - 1]; // good | ||
| if (remoteAddr == null || "".equals(remoteAddr)) { | ||
| remoteAddr = request.getRemoteAddr(); | ||
| } | ||
| } | ||
| return remoteAddr; | ||
| } | ||
haby0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| <!DOCTYPE qhelp PUBLIC | ||
| "-//Semmle//qhelp//EN" | ||
| "qhelp.dtd"> | ||
| <qhelp> | ||
| <overview> | ||
| <p>The software obtains the original client IP address through the http header <code>X-Forwarded-For</code>, which is used to ensure | ||
| security or track it in the log for statistical or other reasons. Attackers can use <code>X-Forwarded-For </code> Spoofing software.</p> | ||
|
|
||
| </overview> | ||
| <recommendation> | ||
|
|
||
| <p>When the software is not using a proxy server, get the last ip.</p> | ||
haby0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| </recommendation> | ||
| <example> | ||
|
|
||
| <p>The following examples show the bad case and the good case respectively. Bad case, such as <code>bad1</code> to <code>bad2</code>. | ||
| In the <code>bad1</code> method, the value of <code>X-Forwarded-For</code> in <code>header</code> is split, and the first value of | ||
| the split array is obtained. Good case, such as <code>good1</code>, split the value of <code>X-Forwarded-For</code> in <code>header</code> | ||
| and get the last value of the split array.</p> | ||
|
|
||
| <sample src="UseOfLessTrustedSource.java" /> | ||
|
|
||
| </example> | ||
| <references> | ||
|
|
||
| <li>Prevent IP address spoofing with X-Forwarded-For header when using AWS ELB and Clojure Ring: | ||
| <a href="https://www.dennis-schneider.com/blog/prevent-ip-address-spoofing-with-x-forwarded-for-header-and-aws-elb-in-clojure-ring/"> | ||
| Prevent IP address spoofing with...</a> | ||
| </li> | ||
haby0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| <li>Security Rule Zero: A Warning about X-Forwarded-For: | ||
| <a href="https://www.f5.com/company/blog/security-rule-zero-a-warning-about-x-forwarded-for"> | ||
| Security Rule Zero: A Warning about X-Forwarded-For</a> | ||
| </li> | ||
haby0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| </references> | ||
| </qhelp> | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,44 @@ | ||||||
| /** | ||||||
| * @name X-Forwarded-For spoofing | ||||||
| * @description The software obtains the client ip through `X-Forwarded-For`, | ||||||
| * and the attacker can modify the value of `X-Forwarded-For` to forge the ip. | ||||||
| * @kind path-problem | ||||||
| * @problem.severity error | ||||||
| * @precision high | ||||||
| * @id java/use-of-less-trusted-source | ||||||
| * @tags security | ||||||
| * external/cwe/cwe-348 | ||||||
| */ | ||||||
|
|
||||||
| import java | ||||||
| import UseOfLessTrustedSourceLib | ||||||
| import semmle.code.java.dataflow.FlowSources | ||||||
| import DataFlow::PathGraph | ||||||
|
|
||||||
| class UseOfLessTrustedSourceConfig extends TaintTracking::Configuration { | ||||||
| UseOfLessTrustedSourceConfig() { this = "UseOfLessTrustedSourceConfig" } | ||||||
|
|
||||||
| override predicate isSource(DataFlow::Node source) { source instanceof UseOfLessTrustedSource } | ||||||
|
|
||||||
| override predicate isSink(DataFlow::Node sink) { sink instanceof UseOfLessTrustedSink } | ||||||
|
|
||||||
| /** | ||||||
| * When using `,` split request data and not taking the first value of | ||||||
| * the array, it is considered as `good`. | ||||||
| */ | ||||||
| override predicate isSanitizer(DataFlow::Node node) { | ||||||
| exists(ArrayAccess aa, MethodAccess ma | aa.getArray() = ma | | ||||||
| ma.getQualifier() = node.asExpr() and | ||||||
| ma.getMethod() instanceof SplitMethod and | ||||||
| not aa.getIndexExpr().toString() = "0" | ||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||
| ) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| from DataFlow::PathNode source, DataFlow::PathNode sink, UseOfLessTrustedSourceConfig conf | ||||||
| where | ||||||
| conf.hasFlowPath(source, sink) and | ||||||
| source.getNode().getEnclosingCallable() = sink.getNode().getEnclosingCallable() and | ||||||
| xffIsFirstGet(source.getNode()) | ||||||
| select sink.getNode(), source, sink, "X-Forwarded-For spoofing might include code from $@.", | ||||||
| source.getNode(), "this user input" | ||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| import java | ||
| import DataFlow | ||
| import semmle.code.java.dataflow.DataFlow | ||
| import experimental.semmle.code.java.Logging | ||
|
|
||
| /** A data flow source of the value of the `x-forwarded-for` field in the `header`. */ | ||
| class UseOfLessTrustedSource extends DataFlow::Node { | ||
| UseOfLessTrustedSource() { | ||
| exists(MethodAccess ma | | ||
| ma.getMethod().hasName("getHeader") and | ||
| ma.getArgument(0).toString().toLowerCase() = "\"x-forwarded-for\"" and | ||
| ma = this.asExpr() | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| /** A data flow sink of method return or log output or local print. */ | ||
| class UseOfLessTrustedSink extends DataFlow::Node { | ||
| UseOfLessTrustedSink() { | ||
| exists(ReturnStmt rs | rs.getResult() = this.asExpr()) | ||
|
There was a problem hiding this comment. If we're treating returning as a sink we might as well flag any use of X-Forwarded-For except splitting and taking the rightmost value. There was a problem hiding this comment. The result of There was a problem hiding this comment. Right, but you don't care about what is done with it subsequently? This is so general that there's almost no point checking how it is used at all. There was a problem hiding this comment. If so, I think there is a problem as long as there is a place to call this method. If you consider what to do with the result of the client ip method, I think there will be a false negative rate. What do you think? There was a problem hiding this comment. What uses do you actually consider dangerous, besides logging? There was a problem hiding this comment. In addition to local output or log records, there are also user authentication based on client ip, database storage, etc., but their use is not unique, so I think it is impossible to standardize the definition. There was a problem hiding this comment. Can we let the security-lab review it together? There was a problem hiding this comment. E.g:
There was a problem hiding this comment. I can, but as this stands I would have to warn them that the query will produce really strange results:
A user of this query would be very confused, because their expectation is that the way the value is used is the important thing, not trivial aspects of dataflow like is it returned from a function or used in function-local scope. I would recommend you find some e.g. rate-limiting utility that would constitute a dangerous way to use this identifier. Alternatively you can choose to send this to the security lab with the health warning attached. There was a problem hiding this comment. Thank you, I agree to let them know the current situation, and I also want to listen to their suggestions, so as to implement a changed query. |
||
| or | ||
| exists(MethodAccess ma | | ||
| ma.getMethod().getName() in ["print", "println"] and | ||
| ( | ||
| ma.getMethod().getDeclaringType().hasQualifiedName("java.io", "PrintWriter") or | ||
| ma.getMethod().getDeclaringType().hasQualifiedName("java.io", "PrintStream") | ||
haby0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ) and | ||
| ma.getAnArgument() = this.asExpr() | ||
| ) | ||
| or | ||
| exists(LoggingCall lc | lc.getAnArgument() = this.asExpr()) | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. I would recommend removing There was a problem hiding this comment. @kevinbackhouse I made changes, please review again, thank you! |
||
|
|
||
| /** A method that split string. */ | ||
| class SplitMethod extends Method { | ||
| SplitMethod() { | ||
| this.getNumberOfParameters() = 1 and | ||
| this.hasQualifiedName("java.lang", "String", "split") | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A call to the ServletRequest.getHeader method and the argument are | ||
haby0 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * `wl-proxy-client-ip`/`proxy-client-ip`/`http_client_ip`/`http_x_forwarded_for`/`x-real-ip`. | ||
| */ | ||
| class HeaderIpCall extends MethodAccess { | ||
| HeaderIpCall() { | ||
| this.getMethod().hasName("getHeader") and | ||
| this.getMethod() | ||
| .getDeclaringType() | ||
| .getASupertype*() | ||
| .hasQualifiedName("javax.servlet", "ServletRequest") and | ||
| this.getArgument(0).toString().toLowerCase() in [ | ||
| "\"wl-proxy-client-ip\"", "\"proxy-client-ip\"", "\"http_client_ip\"", | ||
|
There was a problem hiding this comment. Perhaps provide a link regarding how you chose this list of header values? There was a problem hiding this comment. Reference: |
||
| "\"http_x_forwarded_for\"", "\"x-real-ip\"" | ||
|
There was a problem hiding this comment. Explain why you're using these headers for There was a problem hiding this comment. This is related to the There was a problem hiding this comment. Right, but why are you checking for all these possibilities here, but only There was a problem hiding this comment. Yes, and There was a problem hiding this comment. Why are you only considering There was a problem hiding this comment.
There was a problem hiding this comment. Right, which is why you suppose that taking the right-most comma-separated piece of There was a problem hiding this comment. Thank you for reviewing my pr. You are right. But I still find it weird, regarding the questions here and above, I would like to hear the opinions of security-lab. Hope you can understand. |
||
| ] | ||
| } | ||
| } | ||
|
|
||
| /** A call to `ServletRequest.getRemoteAddr` method. */ | ||
| class RemoteAddrCall extends MethodAccess { | ||
| RemoteAddrCall() { | ||
| this.getMethod().hasName("getRemoteAddr") and | ||
| this.getMethod() | ||
| .getDeclaringType() | ||
| .getASupertype*() | ||
| .hasQualifiedName("javax.servlet", "ServletRequest") | ||
| } | ||
| } | ||
|
|
||
| /** The first one in the method to get the ip value through `x-forwarded-for`. */ | ||
| predicate xffIsFirstGet(Node node) { | ||
|
There was a problem hiding this comment. Explain what you're intending to do here? Should this suppress some warnings completely, or just prevent repeated warnings in the same function? There was a problem hiding this comment. Ensure that |
||
| exists(HeaderIpCall hic | | ||
| node.getEnclosingCallable() = hic.getEnclosingCallable() and | ||
| node.getLocation().getEndLine() < hic.getLocation().getEndLine() | ||
| ) | ||
| or | ||
| exists(RemoteAddrCall rac | | ||
| node.getEnclosingCallable() = rac.getEnclosingCallable() and | ||
| node.getLocation().getEndLine() < rac.getLocation().getEndLine() | ||
| ) | ||
| or | ||
| not exists(HeaderIpCall hic, RemoteAddrCall rac | | ||
| node.getEnclosingCallable() = hic.getEnclosingCallable() and | ||
| node.getEnclosingCallable() = rac.getEnclosingCallable() | ||
| ) | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| edges | ||
| | UseOfLessTrustedSource.java:19:26:19:61 | getHeader(...) : String | UseOfLessTrustedSource.java:25:16:25:25 | remoteAddr | | ||
| | UseOfLessTrustedSource.java:30:21:30:56 | getHeader(...) : String | UseOfLessTrustedSource.java:32:60:32:61 | ip | | ||
| | UseOfLessTrustedSource.java:30:21:30:56 | getHeader(...) : String | UseOfLessTrustedSource.java:58:28:58:48 | ... + ... | | ||
| nodes | ||
| | UseOfLessTrustedSource.java:19:26:19:61 | getHeader(...) : String | semmle.label | getHeader(...) : String | | ||
| | UseOfLessTrustedSource.java:25:16:25:25 | remoteAddr | semmle.label | remoteAddr | | ||
| | UseOfLessTrustedSource.java:30:21:30:56 | getHeader(...) : String | semmle.label | getHeader(...) : String | | ||
| | UseOfLessTrustedSource.java:32:60:32:61 | ip | semmle.label | ip | | ||
| | UseOfLessTrustedSource.java:58:28:58:48 | ... + ... | semmle.label | ... + ... | | ||
| #select | ||
| | UseOfLessTrustedSource.java:25:16:25:25 | remoteAddr | UseOfLessTrustedSource.java:19:26:19:61 | getHeader(...) : String | UseOfLessTrustedSource.java:25:16:25:25 | remoteAddr | X-Forwarded-For spoofing might include code from $@. | UseOfLessTrustedSource.java:19:26:19:61 | getHeader(...) | this user input | | ||
| | UseOfLessTrustedSource.java:32:60:32:61 | ip | UseOfLessTrustedSource.java:30:21:30:56 | getHeader(...) : String | UseOfLessTrustedSource.java:32:60:32:61 | ip | X-Forwarded-For spoofing might include code from $@. | UseOfLessTrustedSource.java:30:21:30:56 | getHeader(...) | this user input | | ||
| | UseOfLessTrustedSource.java:58:28:58:48 | ... + ... | UseOfLessTrustedSource.java:30:21:30:56 | getHeader(...) : String | UseOfLessTrustedSource.java:58:28:58:48 | ... + ... | X-Forwarded-For spoofing might include code from $@. | UseOfLessTrustedSource.java:30:21:30:56 | getHeader(...) | this user input | |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| import javax.servlet.http.HttpServletRequest; | ||
| import org.apache.commons.lang3.StringUtils; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.springframework.stereotype.Controller; | ||
| import org.springframework.web.bind.annotation.GetMapping; | ||
| import org.springframework.web.bind.annotation.ResponseBody; | ||
|
|
||
| @Controller | ||
| public class UseOfLessTrustedSource { | ||
|
|
||
| private static final Logger log = LoggerFactory.getLogger(UseOfLessTrustedSource.class); | ||
|
|
||
| @GetMapping(value = "bad1") | ||
| @ResponseBody | ||
| public String bad1(HttpServletRequest request) { | ||
| String remoteAddr = ""; | ||
| if (request != null) { | ||
| remoteAddr = request.getHeader("X-FORWARDED-FOR"); | ||
| remoteAddr = remoteAddr.split(",")[0]; | ||
| if (remoteAddr == null || "".equals(remoteAddr)) { | ||
| remoteAddr = request.getRemoteAddr(); | ||
| } | ||
| } | ||
| return remoteAddr; | ||
| } | ||
|
|
||
| @GetMapping(value = "bad2") | ||
| public void bad2(HttpServletRequest request) { | ||
| String ip = request.getHeader("X-Forwarded-For"); | ||
|
|
||
| log.debug("getClientIP header X-Forwarded-For:{}", ip); | ||
|
|
||
| if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { | ||
| ip = request.getHeader("Proxy-Client-IP"); | ||
| log.debug("getClientIP header Proxy-Client-IP:{}", ip); | ||
| } | ||
| if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { | ||
| ip = request.getHeader("WL-Proxy-Client-IP"); | ||
| log.debug("getClientIP header WL-Proxy-Client-IP:{}", ip); | ||
| } | ||
| if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { | ||
| ip = request.getHeader("HTTP_CLIENT_IP"); | ||
| log.debug("getClientIP header HTTP_CLIENT_IP:{}", ip); | ||
| } | ||
| if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { | ||
| ip = request.getHeader("HTTP_X_FORWARDED_FOR"); | ||
| log.debug("getClientIP header HTTP_X_FORWARDED_FOR:{}", ip); | ||
| } | ||
| if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { | ||
| ip = request.getHeader("X-Real-IP"); | ||
| log.debug("getClientIP header X-Real-IP:{}", ip); | ||
| } | ||
| if (StringUtils.isBlank(ip) || StringUtils.equalsIgnoreCase("unknown", ip)) { | ||
| ip = request.getRemoteAddr(); | ||
| log.debug("getRemoteAddr IP:{}", ip); | ||
| } | ||
| System.out.println("client ip is: " + ip); | ||
| } | ||
|
|
||
| @GetMapping(value = "good1") | ||
| @ResponseBody | ||
| public String good1(HttpServletRequest request) { | ||
| String remoteAddr = ""; | ||
| if (request != null) { | ||
| remoteAddr = request.getHeader("X-FORWARDED-FOR"); | ||
| remoteAddr = remoteAddr.split(",")[remoteAddr.split(",").length - 1]; // good | ||
| if (remoteAddr == null || "".equals(remoteAddr)) { | ||
| remoteAddr = request.getRemoteAddr(); | ||
| } | ||
| } | ||
| return remoteAddr; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| experimental/Security/CWE/CWE-348/UseOfLessTrustedSource.ql |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| //semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.2.3/:${tes | ||
| tdir}/../../../../stubs/slf4j-api-1.6.4/:${testdir}/../../../../stubs/apache-commons-lang3-3.7/ |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
IMHO Logging is a bad example of this vulnerability.
bad2is a better case because it shows logic-flow stopping logic being gated behind this value. Purely logging like this doesn't seem like the best demonstration of this for the CodeQL documentation.Are there ways that this can be demonstrates this but with a more explicit demonstration of why this is a bad idea?
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.
There will not be a direct security issue here. However, it is very bad to do a log investigation in the online environment. For example, when the online environment is attacked, when the log is checked, the ip is forged, so the attacker cannot be found ip or increase the investigation time.