-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Java: Promote Sensitive Cookie without HttpOnly query from experimental #20572
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
54aefe0
e1cf3d3
c799f93
c478114
1c54296
696ec29
093b04f
9cb593b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -7,24 +7,22 @@ | |||||||||||
* @precision medium | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you check the precision? |
||||||||||||
* @id java/sensitive-cookie-not-httponly | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you considered changing the id to match |
||||||||||||
* @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).*" } | ||||||||||||
|
@@ -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 | ||||||||||||
|
@@ -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 { | ||||||||||||
|
@@ -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. | ||||||||||||
Comment on lines
104
to
105
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
*/ | ||||||||||||
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() | ||||||||||||
|
@@ -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`. | ||||||||||||
Comment on lines
123
to
124
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
*/ | ||||||||||||
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") | ||||||||||||
) | ||||||||||||
) | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
@@ -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) | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
* 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. | ||||||||||||
Comment on lines
171
to
+172
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The prose doesn't quite make sense. Here is a suggestion.
Suggested change
|
||||||||||||
setsHttpOnlyInNewCookie(node.asExpr()) | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Quick question: since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually I just realised we also have MaD models for them, so we do need this as a barrier. So you can ignore this comment. |
||||||||||||
} | ||||||||||||
|
||||||||||||
|
@@ -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.
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.
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.