Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ ql/java/ql/src/Security/CWE/CWE-094/JexlInjection.ql
ql/java/ql/src/Security/CWE/CWE-094/MvelInjection.ql
ql/java/ql/src/Security/CWE/CWE-094/SpelInjection.ql
ql/java/ql/src/Security/CWE/CWE-094/TemplateInjection.ql
ql/java/ql/src/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.ql
ql/java/ql/src/Security/CWE/CWE-1104/MavenPomDependsOnBintray.ql
ql/java/ql/src/Security/CWE/CWE-113/NettyResponseSplitting.ql
ql/java/ql/src/Security/CWE/CWE-113/ResponseSplitting.ql
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ ql/java/ql/src/Security/CWE/CWE-927/ImplicitPendingIntents.ql
ql/java/ql/src/Security/CWE/CWE-927/SensitiveCommunication.ql
ql/java/ql/src/Security/CWE/CWE-927/SensitiveResultReceiver.ql
ql/java/ql/src/Security/CWE/CWE-940/AndroidIntentRedirection.ql
ql/java/ql/src/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.ql
ql/java/ql/src/Telemetry/DatabaseQualityDiagnostics.ql
ql/java/ql/src/Telemetry/ExternalLibraryUsage.ql
ql/java/ql/src/Telemetry/ExtractorInformation.ql
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,6 @@ ql/java/ql/src/experimental/Security/CWE/CWE-094/ScriptInjection.ql
ql/java/ql/src/experimental/Security/CWE/CWE-094/SpringImplicitViewManipulation.ql
ql/java/ql/src/experimental/Security/CWE/CWE-094/SpringViewManipulation.ql
ql/java/ql/src/experimental/Security/CWE/CWE-1004/InsecureTomcatConfig.ql
ql/java/ql/src/experimental/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.ql
ql/java/ql/src/experimental/Security/CWE/CWE-200/InsecureWebResourceResponse.ql
ql/java/ql/src/experimental/Security/CWE/CWE-200/SensitiveAndroidFileLeak.ql
ql/java/ql/src/experimental/Security/CWE/CWE-208/PossibleTimingAttackAgainstSignature.ql
Expand Down
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little bit weird that for the last way of making a cookie we only have the good example and not the bad one. I mean the same thing but with false as the last argument. Is there a reason not to include that? I guess just that you wouldn't use that constructor, you'd use the one with one less argument. I guess there's no need to change it.

File renamed without changes.
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@
<qhelp>

<overview>
<p>Cross-Site Scripting (XSS) is categorized as one of the OWASP Top 10 Security Vulnerabilities. The <code>HttpOnly</code> flag directs compatible browsers to prevent client-side script from accessing cookies. Including the <code>HttpOnly</code> 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.</p>
<p>Cookies without the <code>HttpOnly</code> 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 <code>HttpOnly</code> flag should be set.</p>
</overview>

<recommendation>
<p>Use the <code>HttpOnly</code> flag when generating a cookie containing sensitive information to help mitigate the risk of client side script accessing the protected cookie.</p>
<p>Use the <code>HttpOnly</code> flag when generating a cookie containing sensitive information to help mitigate the risk of client-side scripts accessing the protected cookie.</p>
</recommendation>

<example>
Expand All @@ -23,5 +25,6 @@
OWASP:
<a href="https://owasp.org/www-community/HttpOnly">HttpOnly</a>
</li>
<li>MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Set-Cookie#httponly">Set-Cookie HttpOnly</a>.</li>
</references>
</qhelp>
Original file line number Diff line number Diff line change
Expand Up @@ -7,24 +7,22 @@
* @precision medium
* @id java/sensitive-cookie-not-httponly
* @tags security
* experimental
* external/cwe/cwe-1004
*/

/*
* 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
* `MatchesHttpOnlyToRawHeaderConfig` and `SetHttpOnlyInCookieConfig` 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.
* `MissingHttpOnlyConfig` establishes that a non-`httpOnly` cookie has a sensitive-seeming name.
*/

import java
import semmle.code.java.dataflow.FlowSteps
import semmle.code.java.frameworks.Servlets
import semmle.code.java.dataflow.TaintTracking
import MissingHttpOnlyFlow::PathGraph

/** Gets a regular expression for matching common names of sensitive cookies. */
string getSensitiveCookieNameRegex() { result = "(?i).*(auth|session|token|key|credential).*" }
Expand All @@ -50,8 +48,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
Expand All @@ -62,19 +60,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<MatchesHttpOnlyConfig>;
module MatchesHttpOnlyToRawHeaderFlow = TaintTracking::Global<MatchesHttpOnlyToRawHeaderConfig>;

/** A class descended from `javax.servlet.http.Cookie`. */
class CookieClass extends RefType {
Expand Down Expand Up @@ -102,30 +100,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()
Expand All @@ -137,25 +116,25 @@ module SetHttpOnlyOrRemovesCookieConfig implements DataFlow::ConfigSig {
}
}

module SetHttpOnlyOrRemovesCookieFlow = TaintTracking::Global<SetHttpOnlyOrRemovesCookieConfig>;
module SetHttpOnlyOrRemovesCookieToAddCookieFlow =
TaintTracking::Global<SetHttpOnlyOrRemovesCookieToAddCookieConfig>;

/**
* 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")
)
)
}
}
Expand All @@ -179,14 +158,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 (`SensitiveCookieNameExpr`), to an `addCookie` call (as a `Cookie` object)
* or an `addHeader` call (as a string) (`CookieResponseWithoutHttpOnlySink`).
* 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())
}

Expand All @@ -212,13 +195,8 @@ module MissingHttpOnlyConfig implements DataFlow::ConfigSig {

module MissingHttpOnlyFlow = TaintTracking::Global<MissingHttpOnlyConfig>;

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"
}
import MissingHttpOnlyFlow::PathGraph

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"
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: newQuery
---
* The `java/sensitive-cookie-not-httponly` query has been promoted from experimental to the main query pack.

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
#select
| SensitiveCookieNotHttpOnly.java:31:28:31:36 | jwtCookie | SensitiveCookieNotHttpOnly.java:24:33:24:43 | "jwt_token" : String | SensitiveCookieNotHttpOnly.java:31:28:31:36 | jwtCookie | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:24:33:24:43 | "jwt_token" : String | This sensitive cookie |
| SensitiveCookieNotHttpOnly.java:42:42:42:69 | ... + ... | SensitiveCookieNotHttpOnly.java:42:42:42:49 | "token=" : String | SensitiveCookieNotHttpOnly.java:42:42:42:69 | ... + ... | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:42:42:42:49 | "token=" : String | This sensitive cookie |
| SensitiveCookieNotHttpOnly.java:42:42:42:69 | ... + ... | SensitiveCookieNotHttpOnly.java:42:42:42:57 | ... + ... : String | SensitiveCookieNotHttpOnly.java:42:42:42:69 | ... + ... | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:42:42:42:57 | ... + ... : String | This sensitive cookie |
| SensitiveCookieNotHttpOnly.java:42:42:42:69 | ... + ... | SensitiveCookieNotHttpOnly.java:42:42:42:69 | ... + ... | SensitiveCookieNotHttpOnly.java:42:42:42:69 | ... + ... | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:42:42:42:69 | ... + ... | This sensitive cookie |
| SensitiveCookieNotHttpOnly.java:52:42:52:124 | toString(...) | SensitiveCookieNotHttpOnly.java:52:56:52:75 | "session-access-key" : String | SensitiveCookieNotHttpOnly.java:52:42:52:124 | toString(...) | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:52:56:52:75 | "session-access-key" : String | This sensitive cookie |
| SensitiveCookieNotHttpOnly.java:65:42:65:47 | keyStr | SensitiveCookieNotHttpOnly.java:63:51:63:70 | "session-access-key" : String | SensitiveCookieNotHttpOnly.java:65:42:65:47 | keyStr | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:63:51:63:70 | "session-access-key" : String | This sensitive cookie |
| SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString | SensitiveCookieNotHttpOnly.java:70:28:70:35 | "token=" : String | SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:70:28:70:35 | "token=" : String | This sensitive cookie |
| SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString | SensitiveCookieNotHttpOnly.java:70:28:70:43 | ... + ... : String | SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:70:28:70:43 | ... + ... : String | This sensitive cookie |
| SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString | SensitiveCookieNotHttpOnly.java:70:28:70:55 | ... + ... : String | SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:70:28:70:55 | ... + ... : String | This sensitive cookie |
| SensitiveCookieNotHttpOnly.java:111:28:111:33 | cookie | SensitiveCookieNotHttpOnly.java:88:35:88:51 | "Presto-UI-Token" : String | SensitiveCookieNotHttpOnly.java:111:28:111:33 | cookie | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:88:35:88:51 | "Presto-UI-Token" : String | This sensitive cookie |
edges
| SensitiveCookieNotHttpOnly.java:24:33:24:43 | "jwt_token" : String | SensitiveCookieNotHttpOnly.java:25:39:25:52 | tokenCookieStr : String | provenance | |
| SensitiveCookieNotHttpOnly.java:25:28:25:64 | new Cookie(...) : Cookie | SensitiveCookieNotHttpOnly.java:31:28:31:36 | jwtCookie | provenance | Sink:MaD:1 |
Expand Down Expand Up @@ -53,15 +64,4 @@ nodes
| SensitiveCookieNotHttpOnly.java:91:16:91:21 | cookie : Cookie | semmle.label | cookie : Cookie |
| SensitiveCookieNotHttpOnly.java:110:25:110:64 | createAuthenticationCookie(...) : Cookie | semmle.label | createAuthenticationCookie(...) : Cookie |
| SensitiveCookieNotHttpOnly.java:111:28:111:33 | cookie | semmle.label | cookie |
problems
| SensitiveCookieNotHttpOnly.java:31:28:31:36 | jwtCookie | SensitiveCookieNotHttpOnly.java:24:33:24:43 | "jwt_token" : String | SensitiveCookieNotHttpOnly.java:31:28:31:36 | jwtCookie | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:24:33:24:43 | "jwt_token" | This sensitive cookie |
| SensitiveCookieNotHttpOnly.java:42:42:42:69 | ... + ... | SensitiveCookieNotHttpOnly.java:42:42:42:49 | "token=" : String | SensitiveCookieNotHttpOnly.java:42:42:42:69 | ... + ... | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:42:42:42:49 | "token=" | This sensitive cookie |
| SensitiveCookieNotHttpOnly.java:42:42:42:69 | ... + ... | SensitiveCookieNotHttpOnly.java:42:42:42:57 | ... + ... : String | SensitiveCookieNotHttpOnly.java:42:42:42:69 | ... + ... | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:42:42:42:57 | ... + ... | This sensitive cookie |
| SensitiveCookieNotHttpOnly.java:42:42:42:69 | ... + ... | SensitiveCookieNotHttpOnly.java:42:42:42:69 | ... + ... | SensitiveCookieNotHttpOnly.java:42:42:42:69 | ... + ... | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:42:42:42:69 | ... + ... | This sensitive cookie |
| SensitiveCookieNotHttpOnly.java:52:42:52:124 | toString(...) | SensitiveCookieNotHttpOnly.java:52:56:52:75 | "session-access-key" : String | SensitiveCookieNotHttpOnly.java:52:42:52:124 | toString(...) | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:52:56:52:75 | "session-access-key" | This sensitive cookie |
| SensitiveCookieNotHttpOnly.java:65:42:65:47 | keyStr | SensitiveCookieNotHttpOnly.java:63:51:63:70 | "session-access-key" : String | SensitiveCookieNotHttpOnly.java:65:42:65:47 | keyStr | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:63:51:63:70 | "session-access-key" | This sensitive cookie |
| SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString | SensitiveCookieNotHttpOnly.java:70:28:70:35 | "token=" : String | SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:70:28:70:35 | "token=" | This sensitive cookie |
| SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString | SensitiveCookieNotHttpOnly.java:70:28:70:43 | ... + ... : String | SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:70:28:70:43 | ... + ... | This sensitive cookie |
| SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString | SensitiveCookieNotHttpOnly.java:70:28:70:55 | ... + ... : String | SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:70:28:70:55 | ... + ... | This sensitive cookie |
| SensitiveCookieNotHttpOnly.java:111:28:111:33 | cookie | SensitiveCookieNotHttpOnly.java:88:35:88:51 | "Presto-UI-Token" : String | SensitiveCookieNotHttpOnly.java:111:28:111:33 | cookie | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:88:35:88:51 | "Presto-UI-Token" | This sensitive cookie |
subpaths
Loading