-
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 8 commits
54aefe0
e1cf3d3
c799f93
c478114
1c54296
696ec29
093b04f
9cb593b
d8b37d0
e95e1a0
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 | ||||||||||||
|
||||||||||||
* @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 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. It could be changed to match the js version, though I was hesitant because I prefer this ID as more descriptive. |
||||||||||||
* @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. | ||||||||||||
|
* A taint configuration tracking flow of a method that sets the `HttpOnly` flag, | |
* or one that removes a cookie, to a `ServletResponse.addCookie` call. | |
* A taint configuration tracking the flow of a cookie that has had the | |
* `HttpOnly` flag set, or has been removed, to a `ServletResponse.addCookie` | |
* call. |
Outdated
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.
* A cookie that is added to an HTTP response and which doesn't have `httpOnly` set, used as a sink | |
* in `MissingHttpOnlyConfiguration`. | |
* A cookie that is added to an HTTP response and which doesn't have `HttpOnly` set, used as a sink | |
* in `MissingHttpOnlyConfig`. |
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.
* Tracks string literals containing sensitive names (`SensitiveCookieNameExpr`), to an `addCookie` call (as a `Cookie` object) | |
* | |
* Tracks string literals containing sensitive names (`SensitiveCookieNameExpr`), to an `addCookie` call (as a `Cookie` object) |
Outdated
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.
The prose doesn't quite make sense. Here is a suggestion.
// 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. | |
// JAX-RS's `new NewCookie("session-access-key", accessKey, "/", null, null, 0, true, true)` and similar | |
// Cookie constructors are barriers to considering the flow of the sensitive name, as the `HttpOnly` flag is set. |
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.
Quick question: since ClassInstanceExpr
is a subclass of ConstructorCall
, we only have to make these things barriers because we included them in isAdditionalFlowStep
, right? If so, I think it would be slightly simpler to exclude them from isAdditionalFlowStep
instead.
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.
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.
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.