From 1c1ba7734fba62dc77f8063fba2d115aebb149d4 Mon Sep 17 00:00:00 2001 From: Daniel Winther Petersen Date: Thu, 25 Jul 2024 18:02:54 +0200 Subject: [PATCH] Now alerts about exposing `exception.getMessage()` in servlet responses are split out of `java/stack-trace-exposure` into its own alert `java/error-message-exposure` because this is a better fit. --- .../semmle/code/java/dataflow/ApiSources.qll | 1 + ...veDataExposureThroughErrorMessageQuery.qll | 34 +++++++++++++++ .../java/security/StackTraceExposureQuery.qll | 29 ------------- ...sitiveDataExposureThroughErrorMessage.java | 22 ++++++++++ ...itiveDataExposureThroughErrorMessage.qhelp | 41 +++++++++++++++++++ ...ensitiveDataExposureThroughErrorMessage.ql | 22 ++++++++++ .../CWE/CWE-209/StackTraceExposure.qhelp | 6 +-- .../CWE/CWE-209/StackTraceExposure.ql | 3 +- .../2024-07-25-java-error-message-exposure.md | 4 ++ ...veDataExposureThroughErrorMessage.expected | 1 + ...itiveDataExposureThroughErrorMessage.qlref | 1 + .../semmle/tests/StackTraceExposure.expected | 1 - 12 files changed, 128 insertions(+), 37 deletions(-) create mode 100644 java/ql/lib/semmle/code/java/security/SensitiveDataExposureThroughErrorMessageQuery.qll create mode 100644 java/ql/src/Security/CWE/CWE-209/SensitiveDataExposureThroughErrorMessage.java create mode 100644 java/ql/src/Security/CWE/CWE-209/SensitiveDataExposureThroughErrorMessage.qhelp create mode 100644 java/ql/src/Security/CWE/CWE-209/SensitiveDataExposureThroughErrorMessage.ql create mode 100644 java/ql/src/change-notes/2024-07-25-java-error-message-exposure.md create mode 100644 java/ql/test/query-tests/security/CWE-209/semmle/tests/SensitiveDataExposureThroughErrorMessage.expected create mode 100644 java/ql/test/query-tests/security/CWE-209/semmle/tests/SensitiveDataExposureThroughErrorMessage.qlref diff --git a/java/ql/lib/semmle/code/java/dataflow/ApiSources.qll b/java/ql/lib/semmle/code/java/dataflow/ApiSources.qll index 5f825ad5445..8649b5cf830 100644 --- a/java/ql/lib/semmle/code/java/dataflow/ApiSources.qll +++ b/java/ql/lib/semmle/code/java/dataflow/ApiSources.qll @@ -19,5 +19,6 @@ private module AllApiSources { private import semmle.code.java.security.InsecureTrustManager private import semmle.code.java.security.JWT private import semmle.code.java.security.StackTraceExposureQuery + private import semmle.code.java.security.SensitiveDataExposureThroughErrorMessageQuery private import semmle.code.java.security.ZipSlipQuery } diff --git a/java/ql/lib/semmle/code/java/security/SensitiveDataExposureThroughErrorMessageQuery.qll b/java/ql/lib/semmle/code/java/security/SensitiveDataExposureThroughErrorMessageQuery.qll new file mode 100644 index 00000000000..8644ef415b3 --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/SensitiveDataExposureThroughErrorMessageQuery.qll @@ -0,0 +1,34 @@ +/** Provides predicates to reason about exposure of error messages. */ + +import java +private import semmle.code.java.dataflow.FlowSources +private import semmle.code.java.security.InformationLeak + +/** + * A get message source node. + */ +private class GetMessageFlowSource extends ApiSourceNode { + GetMessageFlowSource() { + exists(Method method | this.asExpr().(MethodCall).getMethod() = method | + method.hasName("getMessage") and + method.hasNoParameters() and + method.getDeclaringType().hasQualifiedName("java.lang", "Throwable") + ) + } +} + +private module GetMessageFlowSourceToHttpResponseSinkFlowConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node src) { src instanceof GetMessageFlowSource } + + predicate isSink(DataFlow::Node sink) { sink instanceof InformationLeakSink } +} + +private module GetMessageFlowSourceToHttpResponseSinkFlow = + TaintTracking::Global; + +/** + * Holds if there is a call to `getMessage()` that then flows to a servlet response. + */ +predicate getMessageFlowsExternally(DataFlow::Node externalExpr, GetMessageFlowSource getMessage) { + GetMessageFlowSourceToHttpResponseSinkFlow::flow(getMessage, externalExpr) +} diff --git a/java/ql/lib/semmle/code/java/security/StackTraceExposureQuery.qll b/java/ql/lib/semmle/code/java/security/StackTraceExposureQuery.qll index 1a2fe31e879..0eb069a06c2 100644 --- a/java/ql/lib/semmle/code/java/security/StackTraceExposureQuery.qll +++ b/java/ql/lib/semmle/code/java/security/StackTraceExposureQuery.qll @@ -93,32 +93,3 @@ predicate stringifiedStackFlowsExternally(DataFlow::Node externalExpr, Expr stac StackTraceStringToHttpResponseSinkFlow::flow(DataFlow::exprNode(stackTraceString), externalExpr) ) } - -/** - * A get message source node. - */ -private class GetMessageFlowSource extends ApiSourceNode { - GetMessageFlowSource() { - exists(Method method | this.asExpr().(MethodCall).getMethod() = method | - method.hasName("getMessage") and - method.hasNoParameters() and - method.getDeclaringType().hasQualifiedName("java.lang", "Throwable") - ) - } -} - -private module GetMessageFlowSourceToHttpResponseSinkFlowConfig implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node src) { src instanceof GetMessageFlowSource } - - predicate isSink(DataFlow::Node sink) { sink instanceof InformationLeakSink } -} - -private module GetMessageFlowSourceToHttpResponseSinkFlow = - TaintTracking::Global; - -/** - * Holds if there is a call to `getMessage()` that then flows to a servlet response. - */ -predicate getMessageFlowsExternally(DataFlow::Node externalExpr, GetMessageFlowSource getMessage) { - GetMessageFlowSourceToHttpResponseSinkFlow::flow(getMessage, externalExpr) -} diff --git a/java/ql/src/Security/CWE/CWE-209/SensitiveDataExposureThroughErrorMessage.java b/java/ql/src/Security/CWE/CWE-209/SensitiveDataExposureThroughErrorMessage.java new file mode 100644 index 00000000000..9b91e3ccd54 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-209/SensitiveDataExposureThroughErrorMessage.java @@ -0,0 +1,22 @@ +protected void doGet(HttpServletRequest request, HttpServletResponse response) { + try { + doSomeWork(); + } catch (NullPointerException ex) { + // BAD: printing a exception message back to the response + response.sendError( + HttpServletResponse.SC_INTERNAL_SERVER_ERROR, + ex.getMessage()); + return; + } + + try { + doSomeWork(); + } catch (NullPointerException ex) { + // GOOD: log the stack trace, and send back a non-revealing response + log("Exception occurred", ex.getMessage); + response.sendError( + HttpServletResponse.SC_INTERNAL_SERVER_ERROR, + "Exception occurred"); + return; + } +} diff --git a/java/ql/src/Security/CWE/CWE-209/SensitiveDataExposureThroughErrorMessage.qhelp b/java/ql/src/Security/CWE/CWE-209/SensitiveDataExposureThroughErrorMessage.qhelp new file mode 100644 index 00000000000..ba597651267 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-209/SensitiveDataExposureThroughErrorMessage.qhelp @@ -0,0 +1,41 @@ + + + +

The error message at the top of a stack trace can include +information such as server-side file names and SQL code that the +application relies on, allowing an attacker to fine-tune a subsequent +injection attack.

+ + + +

Send the user a more generic error message that reveals less information. +Either suppress the error message entirely, or log it only on the server.

+ + + +

In the following example, an exception is handled in two different +ways. In the first version, labeled BAD, the exception is sent back to +the remote user using the getMessage() method. As such, +the user is able to see a detailed error message, which may contain +sensitive information. In the second version, the error message is +logged only on the server. That way, the developers can still access +and use the error log, but remote users will not see the +information.

+ + + + + +
  • OWASP: Improper Error Handling.
    • + +
  • CERT Java Coding Standard: +ERR01-J. +Do not allow exceptions to expose sensitive information.
    • + +
  • + CWE-209: Information Exposure Through an Error Message. +
    • +     +     diff --git a/java/ql/src/Security/CWE/CWE-209/SensitiveDataExposureThroughErrorMessage.ql b/java/ql/src/Security/CWE/CWE-209/SensitiveDataExposureThroughErrorMessage.ql new file mode 100644 index 00000000000..eec728f4c17 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-209/SensitiveDataExposureThroughErrorMessage.ql @@ -0,0 +1,22 @@ +/** + * @name Information exposure through a error message + * @description Information from a error message propagates to an external user. + * Error messages can unintentionally reveal implementation details + * that are useful to an attacker for developing a subsequent exploit. + * @kind problem + * @problem.severity error + * @security-severity 5.4 + * @precision high + * @id java/error-message-exposure + * @tags security + * external/cwe/cwe-209 + */ + +import java +import semmle.code.java.dataflow.DataFlow +import semmle.code.java.security.SensitiveDataExposureThroughErrorMessageQuery + +from Expr externalExpr, Expr errorInformation +where + getMessageFlowsExternally(DataFlow::exprNode(externalExpr), DataFlow::exprNode(errorInformation)) +select externalExpr, "$@ can be exposed to an external user.", errorInformation, "Error information" diff --git a/java/ql/src/Security/CWE/CWE-209/StackTraceExposure.qhelp b/java/ql/src/Security/CWE/CWE-209/StackTraceExposure.qhelp index d0d7e1b0e50..b3ec873546b 100644 --- a/java/ql/src/Security/CWE/CWE-209/StackTraceExposure.qhelp +++ b/java/ql/src/Security/CWE/CWE-209/StackTraceExposure.qhelp @@ -12,11 +12,7 @@ final state of the software when the error occurred.

    Unfortunately, the same information can be useful to an attacker. The sequence of class names in a stack trace can reveal the structure -of the application as well as any internal components it relies on. -Furthermore, the error message at the top of a stack trace can include -information such as server-side file names and SQL code that the -application relies on, allowing an attacker to fine-tune a subsequent -injection attack.

    +of the application as well as any internal components it relies on.

    diff --git a/java/ql/src/Security/CWE/CWE-209/StackTraceExposure.ql b/java/ql/src/Security/CWE/CWE-209/StackTraceExposure.ql index fd72e595cdd..a52a65e95c4 100644 --- a/java/ql/src/Security/CWE/CWE-209/StackTraceExposure.ql +++ b/java/ql/src/Security/CWE/CWE-209/StackTraceExposure.ql @@ -20,6 +20,5 @@ import semmle.code.java.security.StackTraceExposureQuery from Expr externalExpr, Expr errorInformation where printsStackExternally(externalExpr, errorInformation) or - stringifiedStackFlowsExternally(DataFlow::exprNode(externalExpr), errorInformation) or - getMessageFlowsExternally(DataFlow::exprNode(externalExpr), DataFlow::exprNode(errorInformation)) + stringifiedStackFlowsExternally(DataFlow::exprNode(externalExpr), errorInformation) select externalExpr, "$@ can be exposed to an external user.", errorInformation, "Error information" diff --git a/java/ql/src/change-notes/2024-07-25-java-error-message-exposure.md b/java/ql/src/change-notes/2024-07-25-java-error-message-exposure.md new file mode 100644 index 00000000000..67cb0425b11 --- /dev/null +++ b/java/ql/src/change-notes/2024-07-25-java-error-message-exposure.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Now alerts about exposing `exception.getMessage()` in servlet responses are split out of `java/stack-trace-exposure` into its own alert `java/error-message-exposure` because this is a better fit. \ No newline at end of file diff --git a/java/ql/test/query-tests/security/CWE-209/semmle/tests/SensitiveDataExposureThroughErrorMessage.expected b/java/ql/test/query-tests/security/CWE-209/semmle/tests/SensitiveDataExposureThroughErrorMessage.expected new file mode 100644 index 00000000000..89fd00cc4b1 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-209/semmle/tests/SensitiveDataExposureThroughErrorMessage.expected @@ -0,0 +1 @@ +| Test.java:55:5:55:19 | getMessage(...) | $@ can be exposed to an external user. | Test.java:55:5:55:19 | getMessage(...) | Error information | diff --git a/java/ql/test/query-tests/security/CWE-209/semmle/tests/SensitiveDataExposureThroughErrorMessage.qlref b/java/ql/test/query-tests/security/CWE-209/semmle/tests/SensitiveDataExposureThroughErrorMessage.qlref new file mode 100644 index 00000000000..25d68a7fcef --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-209/semmle/tests/SensitiveDataExposureThroughErrorMessage.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-209/SensitiveDataExposureThroughErrorMessage.ql diff --git a/java/ql/test/query-tests/security/CWE-209/semmle/tests/StackTraceExposure.expected b/java/ql/test/query-tests/security/CWE-209/semmle/tests/StackTraceExposure.expected index 94e56634a6d..b212b07a9ce 100644 --- a/java/ql/test/query-tests/security/CWE-209/semmle/tests/StackTraceExposure.expected +++ b/java/ql/test/query-tests/security/CWE-209/semmle/tests/StackTraceExposure.expected @@ -1,4 +1,3 @@ | Test.java:25:4:25:43 | printStackTrace(...) | $@ can be exposed to an external user. | Test.java:25:4:25:5 | ex | Error information | | Test.java:35:5:35:18 | printTrace(...) | $@ can be exposed to an external user. | Test.java:65:3:65:4 | ex | Error information | | Test.java:45:5:45:19 | printTrace2(...) | $@ can be exposed to an external user. | Test.java:71:3:71:4 | ex | Error information | -| Test.java:55:5:55:19 | getMessage(...) | $@ can be exposed to an external user. | Test.java:55:5:55:19 | getMessage(...) | Error information |