diff --git a/java/ql/src/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.qhelp b/java/ql/src/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.qhelp index ee3e8a4181a..71e016510e2 100644 --- a/java/ql/src/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.qhelp +++ b/java/ql/src/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.qhelp @@ -2,11 +2,13 @@ -

Cross-Site Scripting (XSS) is categorized as one of the OWASP Top 10 Security Vulnerabilities. The HttpOnly flag directs compatible browsers to prevent client-side script from accessing cookies. Including the HttpOnly flag in the Set-Cookie HTTP response header for a sensitive cookie helps mitigate the risk associated with XSS where an attacker's script code attempts to read the contents of a cookie and exfiltrate information obtained.

+

Cookies without the HttpOnly flag set are accessible to client-side scripts (such as JavaScript) running in the same origin. +In case of a Cross-Site Scripting (XSS) vulnerability, the cookie can be stolen by a malicious script. +If a sensitive cookie does not need to be accessed directly by client-side scripts, the HttpOnly flag should be set.

-

Use the HttpOnly flag when generating a cookie containing sensitive information to help mitigate the risk of client side script accessing the protected cookie.

+

Use the HttpOnly flag when generating a cookie containing sensitive information to help mitigate the risk of client-side scripts accessing the protected cookie.

@@ -23,5 +25,6 @@ OWASP: HttpOnly +
  • MDN: Set-Cookie HttpOnly.
  • diff --git a/java/ql/src/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.ql b/java/ql/src/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.ql index d2d596c23fa..41b2c95c870 100644 --- a/java/ql/src/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.ql +++ b/java/ql/src/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.ql @@ -14,7 +14,7 @@ * Sketch of the structure of this query: we track cookie names that appear to be sensitive * (e.g. `session` or `token`) to a `ServletResponse.addHeader(...)` or `.addCookie(...)` * method that does not set the `httpOnly` flag. Subsidiary configurations - * `MatchesHttpOnlyConfiguration` and `SetHttpOnlyInCookieConfiguration` are used to establish + * `MatchesHttpOnlyToRawHeaderConfiguration` and `SetHttpOnlyInCookieConfiguration` are used to establish * when the `httpOnly` flag is likely to have been set, before configuration * `MissingHttpOnlyConfiguration` establishes that a non-`httpOnly` cookie has a sensitive-seeming name. */ @@ -49,8 +49,8 @@ class SensitiveCookieNameExpr extends Expr { } /** A method call that sets a `Set-Cookie` header. */ -class SetCookieMethodCall extends MethodCall { - SetCookieMethodCall() { +class SetCookieRawHeaderMethodCall extends MethodCall { + SetCookieRawHeaderMethodCall() { ( this.getMethod() instanceof ResponseAddHeaderMethod or this.getMethod() instanceof ResponseSetHeaderMethod @@ -61,19 +61,19 @@ class SetCookieMethodCall extends MethodCall { /** * A taint configuration tracking flow from the text `httponly` to argument 1 of - * `SetCookieMethodCall`. + * `SetCookieRawHeaderMethodCall`. */ -module MatchesHttpOnlyConfig implements DataFlow::ConfigSig { +module MatchesHttpOnlyToRawHeaderConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { source.asExpr().(CompileTimeConstantExpr).getStringValue().toLowerCase().matches("%httponly%") } predicate isSink(DataFlow::Node sink) { - sink.asExpr() = any(SetCookieMethodCall ma).getArgument(1) + sink.asExpr() = any(SetCookieRawHeaderMethodCall ma).getArgument(1) } } -module MatchesHttpOnlyFlow = TaintTracking::Global; +module MatchesHttpOnlyToRawHeaderFlow = TaintTracking::Global; /** A class descended from `javax.servlet.http.Cookie`. */ class CookieClass extends RefType { @@ -101,30 +101,11 @@ predicate removesCookie(MethodCall ma) { ma.getArgument(0).(IntegerLiteral).getIntValue() = 0 } -/** - * Holds if the MethodCall `ma` is a test method call indicated by: - * a) in a test directory such as `src/test/java` - * b) in a test package whose name has the word `test` - * c) in a test class whose name has the word `test` - * d) in a test class implementing a test framework such as JUnit or TestNG - */ -predicate isTestMethod(MethodCall ma) { - exists(Method m | - m = ma.getEnclosingCallable() and - ( - m.getDeclaringType().getName().toLowerCase().matches("%test%") or // Simple check to exclude test classes to reduce FPs - m.getDeclaringType().getPackage().getName().toLowerCase().matches("%test%") or // Simple check to exclude classes in test packages to reduce FPs - exists(m.getLocation().getFile().getAbsolutePath().indexOf("/src/test/java")) or // Match test directory structure of build tools like maven - m instanceof TestMethod // Test method of a test case implementing a test framework such as JUnit or TestNG - ) - ) -} - /** * A taint configuration tracking flow of a method that sets the `HttpOnly` flag, * or one that removes a cookie, to a `ServletResponse.addCookie` call. */ -module SetHttpOnlyOrRemovesCookieConfig implements DataFlow::ConfigSig { +module SetHttpOnlyOrRemovesCookieToAddCookieConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { source.asExpr() = any(MethodCall ma | setsCookieHttpOnly(ma) or removesCookie(ma)).getQualifier() @@ -136,25 +117,25 @@ module SetHttpOnlyOrRemovesCookieConfig implements DataFlow::ConfigSig { } } -module SetHttpOnlyOrRemovesCookieFlow = TaintTracking::Global; +module SetHttpOnlyOrRemovesCookieToAddCookieFlow = + TaintTracking::Global; /** * A cookie that is added to an HTTP response and which doesn't have `httpOnly` set, used as a sink * in `MissingHttpOnlyConfiguration`. */ -class CookieResponseSink extends DataFlow::ExprNode { - CookieResponseSink() { +class CookieResponseWithoutHttpOnlySink extends DataFlow::ExprNode { + CookieResponseWithoutHttpOnlySink() { exists(MethodCall ma | ( ma.getMethod() instanceof ResponseAddCookieMethod and this.getExpr() = ma.getArgument(0) and - not SetHttpOnlyOrRemovesCookieFlow::flowTo(this) + not SetHttpOnlyOrRemovesCookieToAddCookieFlow::flowTo(this) or - ma instanceof SetCookieMethodCall and + ma instanceof SetCookieRawHeaderMethodCall and this.getExpr() = ma.getArgument(1) and - not MatchesHttpOnlyFlow::flowTo(this) // response.addHeader("Set-Cookie", "token=" +authId + ";HttpOnly;Secure") - ) and - not isTestMethod(ma) // Test class or method + not MatchesHttpOnlyToRawHeaderFlow::flowTo(this) // response.addHeader("Set-Cookie", "token=" +authId + ";HttpOnly;Secure") + ) ) } } @@ -178,14 +159,18 @@ predicate setsHttpOnlyInNewCookie(ClassInstanceExpr cie) { /** * A taint configuration tracking flow from a sensitive cookie without the `HttpOnly` flag * set to its HTTP response. + * Tracks string literals containing sensitive names (`SensitiveNameExpr`), to an `addCookie` call (as a `Cookie` object) + * or an `addHeader` call (as a string) (`CookieResponseWithoutHttpOnly`). + * Passes through `Cookie` constructors and `toString` calls. */ module MissingHttpOnlyConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { source.asExpr() instanceof SensitiveCookieNameExpr } - predicate isSink(DataFlow::Node sink) { sink instanceof CookieResponseSink } + predicate isSink(DataFlow::Node sink) { sink instanceof CookieResponseWithoutHttpOnlySink } predicate isBarrier(DataFlow::Node node) { // JAX-RS's `new NewCookie("session-access-key", accessKey, "/", null, null, 0, true, true)` and similar + // Cookie constructors, but barriers to considering the flow of the sensitive name, as httponly flag is set. setsHttpOnlyInNewCookie(node.asExpr()) } @@ -211,13 +196,6 @@ module MissingHttpOnlyConfig implements DataFlow::ConfigSig { module MissingHttpOnlyFlow = TaintTracking::Global; -deprecated query predicate problems( - DataFlow::Node sinkNode, MissingHttpOnlyFlow::PathNode source, MissingHttpOnlyFlow::PathNode sink, - string message1, DataFlow::Node sourceNode, string message2 -) { - MissingHttpOnlyFlow::flowPath(source, sink) and - sinkNode = sink.getNode() and - message1 = "$@ doesn't have the HttpOnly flag set." and - sourceNode = source.getNode() and - message2 = "This sensitive cookie" -} +from MissingHttpOnlyFlow::PathNode source, MissingHttpOnlyFlow::PathNode sink +where MissingHttpOnlyFlow::flowPath(source, sink) +select sink, source, sink, "$@ doesn't have the HttpOnly flag set.", source, "This sensitive cookie"