Skip to content

Commit fe0e7f5

Browse files
luchua-bcluchua-bc
committed
Change method check to taint flow
1 parent 08c3bf2 commit fe0e7f5

File tree

3 files changed

+58
-40
lines changed

3 files changed

+58
-40
lines changed

‎java/ql/src/experimental/Security/CWE/CWE-1004/SensitiveCookieNotHttpOnly.ql

Lines changed: 37 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import java
1111
import semmle.code.java.dataflow.FlowSteps
1212
import semmle.code.java.frameworks.Servlets
1313
import semmle.code.java.dataflow.TaintTracking
14+
import semmle.code.java.dataflow.TaintTracking2
1415
import DataFlow::PathGraph
1516

1617
/** Gets a regular expression for matching common names of sensitive cookies. */
@@ -31,28 +32,6 @@ predicate isSensitiveCookieNameExpr(Expr expr) {
3132
isSensitiveCookieNameExpr(expr.(AddExpr).getAnOperand())
3233
}
3334

34-
/** Holds if a string is concatenated with the `HttpOnly` flag. */
35-
predicate hasHttpOnlyExpr(Expr expr) {
36-
(
37-
expr.(CompileTimeConstantExpr).getStringValue().toLowerCase().matches("%httponly%")
38-
or
39-
exists(
40-
StaticMethodAccess ma // String.format("%s=%s;HttpOnly", "sessionkey", sessionKey)
41-
|
42-
ma.getType().getName() = "String" and
43-
ma.getMethod().getName() = "format" and
44-
ma.getArgument(0)
45-
.(CompileTimeConstantExpr)
46-
.getStringValue()
47-
.toLowerCase()
48-
.matches("%httponly%") and
49-
expr = ma
50-
)
51-
)
52-
or
53-
hasHttpOnlyExpr(expr.(AddExpr).getAnOperand())
54-
}
55-
5635
/** The method call `Set-Cookie` of `addHeader` or `setHeader`. */
5736
class SetCookieMethodAccess extends MethodAccess {
5837
SetCookieMethodAccess() {
@@ -64,6 +43,22 @@ class SetCookieMethodAccess extends MethodAccess {
6443
}
6544
}
6645

46+
/**
47+
* A taint configuration tracking flow from the text `httponly` to argument 1 of
48+
* `SetCookieMethodAccess`.
49+
*/
50+
class MatchesHttpOnlyConfiguration extends TaintTracking2::Configuration {
51+
MatchesHttpOnlyConfiguration() { this = "MatchesHttpOnlyConfiguration" }
52+
53+
override predicate isSource(DataFlow::Node source) {
54+
source.asExpr().(CompileTimeConstantExpr).getStringValue().toLowerCase().matches("%httponly%")
55+
}
56+
57+
override predicate isSink(DataFlow::Node sink) {
58+
sink.asExpr() = any(SetCookieMethodAccess ma).getArgument(1)
59+
}
60+
}
61+
6762
/** The cookie class of Java EE. */
6863
class CookieClass extends RefType {
6964
CookieClass() {
@@ -79,7 +74,7 @@ predicate isBooleanTrue(Expr expr) {
7974
true
8075
}
8176

82-
/** Holds if the method or a wrapper method sets the `HttpOnly` flag. */
77+
/** Holds if the method call sets the `HttpOnly` flag. */
8378
predicate setHttpOnlyInCookie(MethodAccess ma) {
8479
ma.getMethod().getName() = "setHttpOnly" and
8580
(
@@ -93,14 +88,24 @@ predicate setHttpOnlyInCookie(MethodAccess ma) {
9388
isBooleanTrue(mpa.getArgument(i))
9489
)
9590
)
96-
or
97-
exists(MethodAccess mca |
98-
ma.getMethod().calls(mca.getMethod()) and
99-
setHttpOnlyInCookie(mca)
100-
)
10191
}
10292

103-
/** Holds if the method or a wrapper method removes a cookie. */
93+
/**
94+
* A taint configuration tracking flow of a method or a wrapper method that sets
95+
* the `HttpOnly` flag.
96+
*/
97+
class SetHttpOnlyInCookieConfiguration extends TaintTracking2::Configuration {
98+
SetHttpOnlyInCookieConfiguration() { this = "SetHttpOnlyInCookieConfiguration" }
99+
100+
override predicate isSource(DataFlow::Node source) { any() }
101+
102+
override predicate isSink(DataFlow::Node sink) {
103+
sink.asExpr() =
104+
any(MethodAccess ma | ma.getMethod() instanceof ResponseAddCookieMethod).getArgument(0)
105+
}
106+
}
107+
108+
/** Holds if the method call removes a cookie. */
104109
predicate removeCookie(MethodAccess ma) {
105110
ma.getMethod().getName() = "setMaxAge" and
106111
ma.getArgument(0).(IntegerLiteral).getIntValue() = 0
@@ -125,15 +130,14 @@ class CookieResponseSink extends DataFlow::ExprNode {
125130
setHttpOnlyInCookie(ma2) or
126131
removeCookie(ma2)
127132
) and
128-
(
129-
DataFlow::localExprFlow(ma2.getQualifier(), this.getExpr()) or
130-
DataFlow::localExprFlow(ma2, this.getExpr())
133+
exists(SetHttpOnlyInCookieConfiguration cc |
134+
cc.hasFlow(DataFlow::exprNode(ma2.getQualifier()), this)
131135
)
132136
)
133137
or
134138
ma instanceof SetCookieMethodAccess and
135139
this.getExpr() = ma.getArgument(1) and
136-
not hasHttpOnlyExpr(this.getExpr()) // response.addHeader("Set-Cookie", "token=" +authId + ";HttpOnly;Secure")
140+
not exists(MatchesHttpOnlyConfiguration cc | cc.hasFlowToExpr(ma.getArgument(1))) // response.addHeader("Set-Cookie", "token=" +authId + ";HttpOnly;Secure")
137141
) and
138142
not isTestMethod(ma) // Test class or method
139143
)

‎java/ql/test/experimental/query-tests/security/CWE-1004/SensitiveCookieNotHttpOnly.expected

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@ edges
88
| SensitiveCookieNotHttpOnly.java:70:28:70:43 | ... + ... : String | SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString |
99
| SensitiveCookieNotHttpOnly.java:70:28:70:55 | ... + ... : String | SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString |
1010
| SensitiveCookieNotHttpOnly.java:88:35:88:51 | "Presto-UI-Token" : String | SensitiveCookieNotHttpOnly.java:91:16:91:21 | cookie : Cookie |
11-
| SensitiveCookieNotHttpOnly.java:91:16:91:21 | cookie : Cookie | SensitiveCookieNotHttpOnly.java:102:25:102:64 | createAuthenticationCookie(...) : Cookie |
12-
| SensitiveCookieNotHttpOnly.java:102:25:102:64 | createAuthenticationCookie(...) : Cookie | SensitiveCookieNotHttpOnly.java:103:28:103:33 | cookie |
11+
| SensitiveCookieNotHttpOnly.java:91:16:91:21 | cookie : Cookie | SensitiveCookieNotHttpOnly.java:110:25:110:64 | createAuthenticationCookie(...) : Cookie |
12+
| SensitiveCookieNotHttpOnly.java:110:25:110:64 | createAuthenticationCookie(...) : Cookie | SensitiveCookieNotHttpOnly.java:111:28:111:33 | cookie |
1313
nodes
1414
| SensitiveCookieNotHttpOnly.java:24:33:24:43 | "jwt_token" : String | semmle.label | "jwt_token" : String |
1515
| SensitiveCookieNotHttpOnly.java:31:28:31:36 | jwtCookie | semmle.label | jwtCookie |
@@ -26,8 +26,8 @@ nodes
2626
| SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString | semmle.label | secString |
2727
| SensitiveCookieNotHttpOnly.java:88:35:88:51 | "Presto-UI-Token" : String | semmle.label | "Presto-UI-Token" : String |
2828
| SensitiveCookieNotHttpOnly.java:91:16:91:21 | cookie : Cookie | semmle.label | cookie : Cookie |
29-
| SensitiveCookieNotHttpOnly.java:102:25:102:64 | createAuthenticationCookie(...) : Cookie | semmle.label | createAuthenticationCookie(...) : Cookie |
30-
| SensitiveCookieNotHttpOnly.java:103:28:103:33 | cookie | semmle.label | cookie |
29+
| SensitiveCookieNotHttpOnly.java:110:25:110:64 | createAuthenticationCookie(...) : Cookie | semmle.label | createAuthenticationCookie(...) : Cookie |
30+
| SensitiveCookieNotHttpOnly.java:111:28:111:33 | cookie | semmle.label | cookie |
3131
#select
3232
| 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 |
3333
| 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 |
@@ -38,4 +38,4 @@ nodes
3838
| 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 |
3939
| 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 |
4040
| 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 |
41-
| SensitiveCookieNotHttpOnly.java:103:28:103:33 | cookie | SensitiveCookieNotHttpOnly.java:88:35:88:51 | "Presto-UI-Token" : String | SensitiveCookieNotHttpOnly.java:103:28:103:33 | cookie | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:88:35:88:51 | "Presto-UI-Token" | This sensitive cookie |
41+
| 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 |

‎java/ql/test/experimental/query-tests/security/CWE-1004/SensitiveCookieNotHttpOnly.java

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,14 @@ public Cookie createAuthenticationCookie(HttpServletRequest request, String jwt)
9191
return cookie;
9292
}
9393

94+
public Cookie removeAuthenticationCookie(HttpServletRequest request, String jwt) {
95+
String PRESTO_UI_COOKIE = "Presto-UI-Token";
96+
Cookie cookie = new Cookie(PRESTO_UI_COOKIE, jwt);
97+
cookie.setPath("/ui");
98+
cookie.setMaxAge(0);
99+
return cookie;
100+
}
101+
94102
// GOOD - Tests set a sensitive cookie header with the `HttpOnly` flag set using a wrapper method.
95103
public void addCookie11(HttpServletRequest request, HttpServletResponse response, String jwt) {
96104
Cookie cookie = createHttpOnlyAuthenticationCookie(request, jwt);
@@ -103,6 +111,12 @@ public void addCookie12(HttpServletRequest request, HttpServletResponse response
103111
response.addCookie(cookie);
104112
}
105113

114+
// GOOD - Tests remove a sensitive cookie header without the `HttpOnly` flag set using a wrapper method.
115+
public void addCookie13(HttpServletRequest request, HttpServletResponse response, String jwt) {
116+
Cookie cookie = removeAuthenticationCookie(request, jwt);
117+
response.addCookie(cookie);
118+
}
119+
106120
private Cookie createCookie(String name, String value, Boolean httpOnly){
107121
Cookie cookie = null;
108122
cookie = new Cookie(name, value);
@@ -119,12 +133,12 @@ private Cookie createCookie(String name, String value, Boolean httpOnly){
119133
}
120134

121135
// GOOD - Tests set a sensitive cookie header with the `HttpOnly` flag set through a boolean variable using a wrapper method.
122-
public void addCookie13(HttpServletRequest request, HttpServletResponse response, String refreshToken) {
136+
public void addCookie14(HttpServletRequest request, HttpServletResponse response, String refreshToken) {
123137
response.addCookie(createCookie("refresh_token", refreshToken, true));
124138
}
125139

126140
// BAD - Tests set a sensitive cookie header with the `HttpOnly` flag not set through a boolean variable using a wrapper method.
127-
public void addCookie14(HttpServletRequest request, HttpServletResponse response, String refreshToken) {
141+
public void addCookie15(HttpServletRequest request, HttpServletResponse response, String refreshToken) {
128142
response.addCookie(createCookie("refresh_token", refreshToken, false));
129143
}
130144

0 commit comments

Comments
 (0)