Skip to content

Commit 08c3bf2

Browse files
luchua-bcluchua-bc
committed
Update the query to accommodate more cases
1 parent 0a35fee commit 08c3bf2

File tree

3 files changed

+130
-8
lines changed

3 files changed

+130
-8
lines changed

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

Lines changed: 65 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,23 @@ predicate isSensitiveCookieNameExpr(Expr expr) {
3333

3434
/** Holds if a string is concatenated with the `HttpOnly` flag. */
3535
predicate hasHttpOnlyExpr(Expr expr) {
36-
expr.(CompileTimeConstantExpr).getStringValue().toLowerCase().matches("%httponly%") or
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
3753
hasHttpOnlyExpr(expr.(AddExpr).getAnOperand())
3854
}
3955

@@ -56,6 +72,40 @@ class CookieClass extends RefType {
5672
}
5773
}
5874

75+
/** Holds if the `Expr` expr is evaluated to boolean true. */
76+
predicate isBooleanTrue(Expr expr) {
77+
expr.(CompileTimeConstantExpr).getBooleanValue() = true or
78+
expr.(VarAccess).getVariable().getAnAssignedValue().(CompileTimeConstantExpr).getBooleanValue() =
79+
true
80+
}
81+
82+
/** Holds if the method or a wrapper method sets the `HttpOnly` flag. */
83+
predicate setHttpOnlyInCookie(MethodAccess ma) {
84+
ma.getMethod().getName() = "setHttpOnly" and
85+
(
86+
isBooleanTrue(ma.getArgument(0)) // boolean literal true
87+
or
88+
exists(
89+
MethodAccess mpa, int i // runtime assignment of boolean value true
90+
|
91+
TaintTracking::localTaint(DataFlow::parameterNode(mpa.getMethod().getParameter(i)),
92+
DataFlow::exprNode(ma.getArgument(0))) and
93+
isBooleanTrue(mpa.getArgument(i))
94+
)
95+
)
96+
or
97+
exists(MethodAccess mca |
98+
ma.getMethod().calls(mca.getMethod()) and
99+
setHttpOnlyInCookie(mca)
100+
)
101+
}
102+
103+
/** Holds if the method or a wrapper method removes a cookie. */
104+
predicate removeCookie(MethodAccess ma) {
105+
ma.getMethod().getName() = "setMaxAge" and
106+
ma.getArgument(0).(IntegerLiteral).getIntValue() = 0
107+
}
108+
59109
/** Sensitive cookie name used in a `Cookie` constructor or a `Set-Cookie` call. */
60110
class SensitiveCookieNameExpr extends Expr {
61111
SensitiveCookieNameExpr() { isSensitiveCookieNameExpr(this) }
@@ -69,11 +119,16 @@ class CookieResponseSink extends DataFlow::ExprNode {
69119
ma.getMethod() instanceof ResponseAddCookieMethod and
70120
this.getExpr() = ma.getArgument(0) and
71121
not exists(
72-
MethodAccess ma2 // cookie.setHttpOnly(true)
122+
MethodAccess ma2 // a method or wrapper method that invokes cookie.setHttpOnly(true)
73123
|
74-
ma2.getMethod().getName() = "setHttpOnly" and
75-
ma2.getArgument(0).(BooleanLiteral).getBooleanValue() = true and
76-
DataFlow::localExprFlow(ma2.getQualifier(), this.getExpr())
124+
(
125+
setHttpOnlyInCookie(ma2) or
126+
removeCookie(ma2)
127+
) and
128+
(
129+
DataFlow::localExprFlow(ma2.getQualifier(), this.getExpr()) or
130+
DataFlow::localExprFlow(ma2, this.getExpr())
131+
)
77132
)
78133
or
79134
ma instanceof SetCookieMethodAccess and
@@ -92,13 +147,15 @@ class CookieResponseSink extends DataFlow::ExprNode {
92147
predicate setHttpOnlyInNewCookie(ClassInstanceExpr cie) {
93148
cie.getConstructedType().hasQualifiedName(["javax.ws.rs.core", "jakarta.ws.rs.core"], "NewCookie") and
94149
(
95-
cie.getNumArgument() = 6 and cie.getArgument(5).(BooleanLiteral).getBooleanValue() = true // NewCookie(Cookie cookie, String comment, int maxAge, Date expiry, boolean secure, boolean httpOnly)
150+
cie.getNumArgument() = 6 and
151+
isBooleanTrue(cie.getArgument(5)) // NewCookie(Cookie cookie, String comment, int maxAge, Date expiry, boolean secure, boolean httpOnly)
96152
or
97153
cie.getNumArgument() = 8 and
98154
cie.getArgument(6).getType() instanceof BooleanType and
99-
cie.getArgument(7).(BooleanLiteral).getBooleanValue() = true // NewCookie(String name, String value, String path, String domain, String comment, int maxAge, boolean secure, boolean httpOnly)
155+
isBooleanTrue(cie.getArgument(7)) // NewCookie(String name, String value, String path, String domain, String comment, int maxAge, boolean secure, boolean httpOnly)
100156
or
101-
cie.getNumArgument() = 10 and cie.getArgument(9).(BooleanLiteral).getBooleanValue() = true // NewCookie(String name, String value, String path, String domain, int version, String comment, int maxAge, Date expiry, boolean secure, boolean httpOnly)
157+
cie.getNumArgument() = 10 and
158+
isBooleanTrue(cie.getArgument(9)) // NewCookie(String name, String value, String path, String domain, int version, String comment, int maxAge, Date expiry, boolean secure, boolean httpOnly)
102159
)
103160
}
104161

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,9 @@ edges
77
| SensitiveCookieNotHttpOnly.java:70:28:70:35 | "token=" : String | SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString |
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 |
10+
| 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 |
1013
nodes
1114
| SensitiveCookieNotHttpOnly.java:24:33:24:43 | "jwt_token" : String | semmle.label | "jwt_token" : String |
1215
| SensitiveCookieNotHttpOnly.java:31:28:31:36 | jwtCookie | semmle.label | jwtCookie |
@@ -21,6 +24,10 @@ nodes
2124
| SensitiveCookieNotHttpOnly.java:70:28:70:43 | ... + ... : String | semmle.label | ... + ... : String |
2225
| SensitiveCookieNotHttpOnly.java:70:28:70:55 | ... + ... : String | semmle.label | ... + ... : String |
2326
| SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString | semmle.label | secString |
27+
| SensitiveCookieNotHttpOnly.java:88:35:88:51 | "Presto-UI-Token" : String | semmle.label | "Presto-UI-Token" : String |
28+
| 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 |
2431
#select
2532
| 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 |
2633
| 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 |
@@ -31,3 +38,4 @@ nodes
3138
| 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 |
3239
| 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 |
3340
| 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 |

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

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,63 @@ public void addCookie9(String authId, HttpServletRequest request, HttpServletRes
7171
response.addHeader("Set-Cookie", secString);
7272
}
7373

74+
// GOOD - Tests set a sensitive cookie header with the `HttpOnly` flag set using `String.format(...)`.
75+
public void addCookie10(HttpServletRequest request, HttpServletResponse response) {
76+
response.addHeader("SET-COOKIE", String.format("%s=%s;HttpOnly", "sessionkey", request.getSession().getAttribute("sessionkey")));
77+
}
78+
79+
public Cookie createHttpOnlyAuthenticationCookie(HttpServletRequest request, String jwt) {
80+
String PRESTO_UI_COOKIE = "Presto-UI-Token";
81+
Cookie cookie = new Cookie(PRESTO_UI_COOKIE, jwt);
82+
cookie.setHttpOnly(true);
83+
cookie.setPath("/ui");
84+
return cookie;
85+
}
86+
87+
public Cookie createAuthenticationCookie(HttpServletRequest request, String jwt) {
88+
String PRESTO_UI_COOKIE = "Presto-UI-Token";
89+
Cookie cookie = new Cookie(PRESTO_UI_COOKIE, jwt);
90+
cookie.setPath("/ui");
91+
return cookie;
92+
}
93+
94+
// GOOD - Tests set a sensitive cookie header with the `HttpOnly` flag set using a wrapper method.
95+
public void addCookie11(HttpServletRequest request, HttpServletResponse response, String jwt) {
96+
Cookie cookie = createHttpOnlyAuthenticationCookie(request, jwt);
97+
response.addCookie(cookie);
98+
}
99+
100+
// BAD - Tests set a sensitive cookie header without the `HttpOnly` flag set using a wrapper method.
101+
public void addCookie12(HttpServletRequest request, HttpServletResponse response, String jwt) {
102+
Cookie cookie = createAuthenticationCookie(request, jwt);
103+
response.addCookie(cookie);
104+
}
105+
106+
private Cookie createCookie(String name, String value, Boolean httpOnly){
107+
Cookie cookie = null;
108+
cookie = new Cookie(name, value);
109+
cookie.setDomain("/");
110+
cookie.setHttpOnly(httpOnly);
111+
112+
//for production https
113+
cookie.setSecure(true);
114+
115+
cookie.setMaxAge(60*60*24*30);
116+
cookie.setPath("/");
117+
118+
return cookie;
119+
}
120+
121+
// 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) {
123+
response.addCookie(createCookie("refresh_token", refreshToken, true));
124+
}
125+
126+
// 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) {
128+
response.addCookie(createCookie("refresh_token", refreshToken, false));
129+
}
130+
74131
// GOOD - CSRF token doesn't need to have the `HttpOnly` flag set.
75132
public void addCsrfCookie(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
76133
// Spring put the CSRF token in session attribute "_csrf"

0 commit comments

Comments
 (0)