Skip to content

Commit 4d023f1

Browse files
aschackmullaschackmull
authored
Merge pull request #17075 from RobbingDaHood/17052-second-try-do-not-expose-error-message
Java: 17052 Second try: do not expose error message
2 parents c23938d + 1cb5892 commit 4d023f1

12 files changed

+125
-37
lines changed

‎java/ql/lib/semmle/code/java/dataflow/ApiSources.qll‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,5 +19,6 @@ private module AllApiSources {
1919
private import semmle.code.java.security.InsecureTrustManager
2020
private import semmle.code.java.security.JWT
2121
private import semmle.code.java.security.StackTraceExposureQuery
22+
private import semmle.code.java.security.SensitiveDataExposureThroughErrorMessageQuery
2223
private import semmle.code.java.security.ZipSlipQuery
2324
}

‎java/ql/lib/semmle/code/java/security/SensitiveDataExposureThroughErrorMessageQuery.qll‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
/** Provides predicates to reason about exposure of error messages. */
2+
3+
import java
4+
private import semmle.code.java.dataflow.FlowSources
5+
private import semmle.code.java.security.InformationLeak
6+
7+
/**
8+
* A get message source node.
9+
*/
10+
private class GetMessageFlowSource extends ApiSourceNode {
11+
GetMessageFlowSource() {
12+
exists(Method method | this.asExpr().(MethodCall).getMethod() = method |
13+
method.hasName("getMessage") and
14+
method.hasNoParameters() and
15+
method.getDeclaringType().hasQualifiedName("java.lang", "Throwable")
16+
)
17+
}
18+
}
19+
20+
private module GetMessageFlowSourceToHttpResponseSinkFlowConfig implements DataFlow::ConfigSig {
21+
predicate isSource(DataFlow::Node src) { src instanceof GetMessageFlowSource }
22+
23+
predicate isSink(DataFlow::Node sink) { sink instanceof InformationLeakSink }
24+
}
25+
26+
private module GetMessageFlowSourceToHttpResponseSinkFlow =
27+
TaintTracking::Global<GetMessageFlowSourceToHttpResponseSinkFlowConfig>;
28+
29+
/**
30+
* Holds if there is a call to `getMessage()` that then flows to a servlet response.
31+
*/
32+
predicate getMessageFlowsExternally(DataFlow::Node externalExpr, GetMessageFlowSource getMessage) {
33+
GetMessageFlowSourceToHttpResponseSinkFlow::flow(getMessage, externalExpr)
34+
}

‎java/ql/lib/semmle/code/java/security/StackTraceExposureQuery.qll‎

Lines changed: 0 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -93,32 +93,3 @@ predicate stringifiedStackFlowsExternally(DataFlow::Node externalExpr, Expr stac
9393
StackTraceStringToHttpResponseSinkFlow::flow(DataFlow::exprNode(stackTraceString), externalExpr)
9494
)
9595
}
96-
97-
/**
98-
* A get message source node.
99-
*/
100-
private class GetMessageFlowSource extends ApiSourceNode {
101-
GetMessageFlowSource() {
102-
exists(Method method | this.asExpr().(MethodCall).getMethod() = method |
103-
method.hasName("getMessage") and
104-
method.hasNoParameters() and
105-
method.getDeclaringType().hasQualifiedName("java.lang", "Throwable")
106-
)
107-
}
108-
}
109-
110-
private module GetMessageFlowSourceToHttpResponseSinkFlowConfig implements DataFlow::ConfigSig {
111-
predicate isSource(DataFlow::Node src) { src instanceof GetMessageFlowSource }
112-
113-
predicate isSink(DataFlow::Node sink) { sink instanceof InformationLeakSink }
114-
}
115-
116-
private module GetMessageFlowSourceToHttpResponseSinkFlow =
117-
TaintTracking::Global<GetMessageFlowSourceToHttpResponseSinkFlowConfig>;
118-
119-
/**
120-
* Holds if there is a call to `getMessage()` that then flows to a servlet response.
121-
*/
122-
predicate getMessageFlowsExternally(DataFlow::Node externalExpr, GetMessageFlowSource getMessage) {
123-
GetMessageFlowSourceToHttpResponseSinkFlow::flow(getMessage, externalExpr)
124-
}

‎java/ql/src/Security/CWE/CWE-209/SensitiveDataExposureThroughErrorMessage.java‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
protected void doGet(HttpServletRequest request, HttpServletResponse response) {
2+
try {
3+
doSomeWork();
4+
} catch (NullPointerException ex) {
5+
// BAD: printing a exception message back to the response
6+
response.sendError(
7+
HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
8+
ex.getMessage());
9+
return;
10+
}
11+
12+
try {
13+
doSomeWork();
14+
} catch (NullPointerException ex) {
15+
// GOOD: log the exception message, and send back a non-revealing response
16+
log("Exception occurred", ex.getMessage);
17+
response.sendError(
18+
HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
19+
"Exception occurred");
20+
return;
21+
}
22+
}

‎java/ql/src/Security/CWE/CWE-209/SensitiveDataExposureThroughErrorMessage.qhelp‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>The error message at the top of a stack trace can include
7+
information such as server-side file names and SQL code that the
8+
application relies on, allowing an attacker to fine-tune a subsequent
9+
injection attack.</p>
10+
</overview>
11+
12+
<recommendation>
13+
<p>Send the user a more generic error message that reveals less information.
14+
Either suppress the error message entirely, or log it only on the server.</p>
15+
</recommendation>
16+
17+
<example>
18+
<p>In the following example, an exception is handled in two different
19+
ways. In the first version, labeled BAD, the exception is sent back to
20+
the remote user using the <code>getMessage()</code> method. As such,
21+
the user is able to see a detailed error message, which may contain
22+
sensitive information. In the second version, the error message is
23+
logged only on the server. That way, the developers can still access
24+
and use the error log, but remote users will not see the
25+
information.</p>
26+
27+
<sample src="SensitiveDataExposureThroughErrorMessage.java" />
28+
</example>
29+
30+
<references>
31+
<li>OWASP: <a href="https://owasp.org/www-community/Improper_Error_Handling">Improper Error Handling</a>.</li>
32+
33+
<li>CERT Java Coding Standard:
34+
<a href="https://www.securecoding.cert.org/confluence/display/java/ERR01-J.+Do+not+allow+exceptions+to+expose+sensitive+information">ERR01-J.
35+
Do not allow exceptions to expose sensitive information</a>.</li>
36+
37+
</references>
38+
</qhelp>

‎java/ql/src/Security/CWE/CWE-209/SensitiveDataExposureThroughErrorMessage.ql‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/**
2+
* @name Information exposure through an error message
3+
* @description Information from an error message propagates to an external user.
4+
* Error messages can unintentionally reveal implementation details
5+
* that are useful to an attacker for developing a subsequent exploit.
6+
* @kind problem
7+
* @problem.severity error
8+
* @security-severity 5.4
9+
* @precision high
10+
* @id java/error-message-exposure
11+
* @tags security
12+
* external/cwe/cwe-209
13+
*/
14+
15+
import java
16+
import semmle.code.java.dataflow.DataFlow
17+
import semmle.code.java.security.SensitiveDataExposureThroughErrorMessageQuery
18+
19+
from Expr externalExpr, Expr errorInformation
20+
where
21+
getMessageFlowsExternally(DataFlow::exprNode(externalExpr), DataFlow::exprNode(errorInformation))
22+
select externalExpr, "$@ can be exposed to an external user.", errorInformation, "Error information"

‎java/ql/src/Security/CWE/CWE-209/StackTraceExposure.qhelp‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,7 @@ final state of the software when the error occurred.</p>
1212

1313
<p>Unfortunately, the same information can be useful to an attacker.
1414
The sequence of class names in a stack trace can reveal the structure
15-
of the application as well as any internal components it relies on.
16-
Furthermore, the error message at the top of a stack trace can include
17-
information such as server-side file names and SQL code that the
18-
application relies on, allowing an attacker to fine-tune a subsequent
19-
injection attack.</p>
15+
of the application as well as any internal components it relies on.</p>
2016
</overview>
2117

2218
<recommendation>

‎java/ql/src/Security/CWE/CWE-209/StackTraceExposure.ql‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,5 @@ import semmle.code.java.security.StackTraceExposureQuery
2020
from Expr externalExpr, Expr errorInformation
2121
where
2222
printsStackExternally(externalExpr, errorInformation) or
23-
stringifiedStackFlowsExternally(DataFlow::exprNode(externalExpr), errorInformation) or
24-
getMessageFlowsExternally(DataFlow::exprNode(externalExpr), DataFlow::exprNode(errorInformation))
23+
stringifiedStackFlowsExternally(DataFlow::exprNode(externalExpr), errorInformation)
2524
select externalExpr, "$@ can be exposed to an external user.", errorInformation, "Error information"

‎java/ql/src/change-notes/2024-07-25-java-error-message-exposure.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Alerts about exposing `exception.getMessage()` in servlet responses are now split out of `java/stack-trace-exposure` into its own query `java/error-message-exposure`.

‎java/ql/test/query-tests/security/CWE-209/semmle/tests/SensitiveDataExposureThroughErrorMessage.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
| Test.java:55:5:55:19 | getMessage(...) | $@ can be exposed to an external user. | Test.java:55:5:55:19 | getMessage(...) | Error information |

0 commit comments

Comments
 (0)