Skip to content

Commit 0a35fee

Browse files
luchua-bcluchua-bc
committed
Exclude CSRF cookies to reduce FPs
1 parent a0a1dde commit 0a35fee

File tree

5 files changed

+113
-37
lines changed

5 files changed

+113
-37
lines changed

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

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,18 @@ import DataFlow::PathGraph
1616
/** Gets a regular expression for matching common names of sensitive cookies. */
1717
string getSensitiveCookieNameRegex() { result = "(?i).*(auth|session|token|key|credential).*" }
1818

19-
/** Holds if a string is concatenated with the name of a sensitive cookie. */
19+
/** Gets a regular expression for matching CSRF cookies. */
20+
string getCsrfCookieNameRegex() { result = "(?i).*(csrf).*" }
21+
22+
/**
23+
* Holds if a string is concatenated with the name of a sensitive cookie. Excludes CSRF cookies since
24+
* they are special cookies implementing the Synchronizer Token Pattern that can be used in JavaScript.
25+
*/
2026
predicate isSensitiveCookieNameExpr(Expr expr) {
21-
expr.(CompileTimeConstantExpr)
22-
.getStringValue()
23-
.toLowerCase()
24-
.regexpMatch(getSensitiveCookieNameRegex()) or
27+
exists(string s | s = expr.(CompileTimeConstantExpr).getStringValue().toLowerCase() |
28+
s.regexpMatch(getSensitiveCookieNameRegex()) and not s.regexpMatch(getCsrfCookieNameRegex())
29+
)
30+
or
2531
isSensitiveCookieNameExpr(expr.(AddExpr).getAnOperand())
2632
}
2733

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

Lines changed: 30 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,33 +1,33 @@
11
edges
2-
| SensitiveCookieNotHttpOnly.java:22:33:22:43 | "jwt_token" : String | SensitiveCookieNotHttpOnly.java:29:28:29:36 | jwtCookie |
3-
| SensitiveCookieNotHttpOnly.java:40:42:40:49 | "token=" : String | SensitiveCookieNotHttpOnly.java:40:42:40:69 | ... + ... |
4-
| SensitiveCookieNotHttpOnly.java:40:42:40:57 | ... + ... : String | SensitiveCookieNotHttpOnly.java:40:42:40:69 | ... + ... |
5-
| SensitiveCookieNotHttpOnly.java:50:56:50:75 | "session-access-key" : String | SensitiveCookieNotHttpOnly.java:50:42:50:124 | toString(...) |
6-
| SensitiveCookieNotHttpOnly.java:61:51:61:70 | "session-access-key" : String | SensitiveCookieNotHttpOnly.java:63:42:63:47 | keyStr |
7-
| SensitiveCookieNotHttpOnly.java:68:28:68:35 | "token=" : String | SensitiveCookieNotHttpOnly.java:69:42:69:50 | secString |
8-
| SensitiveCookieNotHttpOnly.java:68:28:68:43 | ... + ... : String | SensitiveCookieNotHttpOnly.java:69:42:69:50 | secString |
9-
| SensitiveCookieNotHttpOnly.java:68:28:68:55 | ... + ... : String | SensitiveCookieNotHttpOnly.java:69:42:69:50 | secString |
2+
| SensitiveCookieNotHttpOnly.java:24:33:24:43 | "jwt_token" : String | SensitiveCookieNotHttpOnly.java:31:28:31:36 | jwtCookie |
3+
| SensitiveCookieNotHttpOnly.java:42:42:42:49 | "token=" : String | SensitiveCookieNotHttpOnly.java:42:42:42:69 | ... + ... |
4+
| SensitiveCookieNotHttpOnly.java:42:42:42:57 | ... + ... : String | SensitiveCookieNotHttpOnly.java:42:42:42:69 | ... + ... |
5+
| SensitiveCookieNotHttpOnly.java:52:56:52:75 | "session-access-key" : String | SensitiveCookieNotHttpOnly.java:52:42:52:124 | toString(...) |
6+
| SensitiveCookieNotHttpOnly.java:63:51:63:70 | "session-access-key" : String | SensitiveCookieNotHttpOnly.java:65:42:65:47 | keyStr |
7+
| SensitiveCookieNotHttpOnly.java:70:28:70:35 | "token=" : String | SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString |
8+
| SensitiveCookieNotHttpOnly.java:70:28:70:43 | ... + ... : String | SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString |
9+
| SensitiveCookieNotHttpOnly.java:70:28:70:55 | ... + ... : String | SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString |
1010
nodes
11-
| SensitiveCookieNotHttpOnly.java:22:33:22:43 | "jwt_token" : String | semmle.label | "jwt_token" : String |
12-
| SensitiveCookieNotHttpOnly.java:29:28:29:36 | jwtCookie | semmle.label | jwtCookie |
13-
| SensitiveCookieNotHttpOnly.java:40:42:40:49 | "token=" : String | semmle.label | "token=" : String |
14-
| SensitiveCookieNotHttpOnly.java:40:42:40:57 | ... + ... : String | semmle.label | ... + ... : String |
15-
| SensitiveCookieNotHttpOnly.java:40:42:40:69 | ... + ... | semmle.label | ... + ... |
16-
| SensitiveCookieNotHttpOnly.java:50:42:50:124 | toString(...) | semmle.label | toString(...) |
17-
| SensitiveCookieNotHttpOnly.java:50:56:50:75 | "session-access-key" : String | semmle.label | "session-access-key" : String |
18-
| SensitiveCookieNotHttpOnly.java:61:51:61:70 | "session-access-key" : String | semmle.label | "session-access-key" : String |
19-
| SensitiveCookieNotHttpOnly.java:63:42:63:47 | keyStr | semmle.label | keyStr |
20-
| SensitiveCookieNotHttpOnly.java:68:28:68:35 | "token=" : String | semmle.label | "token=" : String |
21-
| SensitiveCookieNotHttpOnly.java:68:28:68:43 | ... + ... : String | semmle.label | ... + ... : String |
22-
| SensitiveCookieNotHttpOnly.java:68:28:68:55 | ... + ... : String | semmle.label | ... + ... : String |
23-
| SensitiveCookieNotHttpOnly.java:69:42:69:50 | secString | semmle.label | secString |
11+
| SensitiveCookieNotHttpOnly.java:24:33:24:43 | "jwt_token" : String | semmle.label | "jwt_token" : String |
12+
| SensitiveCookieNotHttpOnly.java:31:28:31:36 | jwtCookie | semmle.label | jwtCookie |
13+
| SensitiveCookieNotHttpOnly.java:42:42:42:49 | "token=" : String | semmle.label | "token=" : String |
14+
| SensitiveCookieNotHttpOnly.java:42:42:42:57 | ... + ... : String | semmle.label | ... + ... : String |
15+
| SensitiveCookieNotHttpOnly.java:42:42:42:69 | ... + ... | semmle.label | ... + ... |
16+
| SensitiveCookieNotHttpOnly.java:52:42:52:124 | toString(...) | semmle.label | toString(...) |
17+
| SensitiveCookieNotHttpOnly.java:52:56:52:75 | "session-access-key" : String | semmle.label | "session-access-key" : String |
18+
| SensitiveCookieNotHttpOnly.java:63:51:63:70 | "session-access-key" : String | semmle.label | "session-access-key" : String |
19+
| SensitiveCookieNotHttpOnly.java:65:42:65:47 | keyStr | semmle.label | keyStr |
20+
| SensitiveCookieNotHttpOnly.java:70:28:70:35 | "token=" : String | semmle.label | "token=" : String |
21+
| SensitiveCookieNotHttpOnly.java:70:28:70:43 | ... + ... : String | semmle.label | ... + ... : String |
22+
| SensitiveCookieNotHttpOnly.java:70:28:70:55 | ... + ... : String | semmle.label | ... + ... : String |
23+
| SensitiveCookieNotHttpOnly.java:71:42:71:50 | secString | semmle.label | secString |
2424
#select
25-
| SensitiveCookieNotHttpOnly.java:29:28:29:36 | jwtCookie | SensitiveCookieNotHttpOnly.java:22:33:22:43 | "jwt_token" : String | SensitiveCookieNotHttpOnly.java:29:28:29:36 | jwtCookie | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:22:33:22:43 | "jwt_token" | This sensitive cookie |
26-
| SensitiveCookieNotHttpOnly.java:40:42:40:69 | ... + ... | SensitiveCookieNotHttpOnly.java:40:42:40:49 | "token=" : String | SensitiveCookieNotHttpOnly.java:40:42:40:69 | ... + ... | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:40:42:40:49 | "token=" | This sensitive cookie |
27-
| SensitiveCookieNotHttpOnly.java:40:42:40:69 | ... + ... | SensitiveCookieNotHttpOnly.java:40:42:40:57 | ... + ... : String | SensitiveCookieNotHttpOnly.java:40:42:40:69 | ... + ... | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:40:42:40:57 | ... + ... | This sensitive cookie |
28-
| SensitiveCookieNotHttpOnly.java:40:42:40:69 | ... + ... | SensitiveCookieNotHttpOnly.java:40:42:40:69 | ... + ... | SensitiveCookieNotHttpOnly.java:40:42:40:69 | ... + ... | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:40:42:40:69 | ... + ... | This sensitive cookie |
29-
| SensitiveCookieNotHttpOnly.java:50:42:50:124 | toString(...) | SensitiveCookieNotHttpOnly.java:50:56:50:75 | "session-access-key" : String | SensitiveCookieNotHttpOnly.java:50:42:50:124 | toString(...) | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:50:56:50:75 | "session-access-key" | This sensitive cookie |
30-
| SensitiveCookieNotHttpOnly.java:63:42:63:47 | keyStr | SensitiveCookieNotHttpOnly.java:61:51:61:70 | "session-access-key" : String | SensitiveCookieNotHttpOnly.java:63:42:63:47 | keyStr | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:61:51:61:70 | "session-access-key" | This sensitive cookie |
31-
| SensitiveCookieNotHttpOnly.java:69:42:69:50 | secString | SensitiveCookieNotHttpOnly.java:68:28:68:35 | "token=" : String | SensitiveCookieNotHttpOnly.java:69:42:69:50 | secString | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:68:28:68:35 | "token=" | This sensitive cookie |
32-
| SensitiveCookieNotHttpOnly.java:69:42:69:50 | secString | SensitiveCookieNotHttpOnly.java:68:28:68:43 | ... + ... : String | SensitiveCookieNotHttpOnly.java:69:42:69:50 | secString | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:68:28:68:43 | ... + ... | This sensitive cookie |
33-
| SensitiveCookieNotHttpOnly.java:69:42:69:50 | secString | SensitiveCookieNotHttpOnly.java:68:28:68:55 | ... + ... : String | SensitiveCookieNotHttpOnly.java:69:42:69:50 | secString | $@ doesn't have the HttpOnly flag set. | SensitiveCookieNotHttpOnly.java:68:28:68:55 | ... + ... | This sensitive cookie |
25+
| 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 |
26+
| 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 |
27+
| 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 |
28+
| 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 |
29+
| 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 |
30+
| 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 |
31+
| 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 |
32+
| 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 |
33+
| 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 |

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

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77

88
import javax.ws.rs.core.NewCookie;
99

10+
import org.springframework.security.web.csrf.CsrfToken;
11+
1012
class SensitiveCookieNotHttpOnly {
1113
// GOOD - Tests adding a sensitive cookie with the `HttpOnly` flag set.
1214
public void addCookie(String jwt_token, HttpServletRequest request, HttpServletResponse response) {
@@ -67,5 +69,23 @@ public void addCookie8(String accessKey, HttpServletRequest request, HttpServlet
6769
public void addCookie9(String authId, HttpServletRequest request, HttpServletResponse response) {
6870
String secString = "token=" +authId + ";Secure";
6971
response.addHeader("Set-Cookie", secString);
70-
}
72+
}
73+
74+
// GOOD - CSRF token doesn't need to have the `HttpOnly` flag set.
75+
public void addCsrfCookie(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
76+
// Spring put the CSRF token in session attribute "_csrf"
77+
CsrfToken csrfToken = (CsrfToken) request.getAttribute("_csrf");
78+
79+
// Send the cookie only if the token has changed
80+
String actualToken = request.getHeader("X-CSRF-TOKEN");
81+
if (actualToken == null || !actualToken.equals(csrfToken.getToken())) {
82+
// Session cookie that can be used by AngularJS
83+
String pCookieName = "CSRF-TOKEN";
84+
Cookie cookie = new Cookie(pCookieName, csrfToken.getToken());
85+
cookie.setMaxAge(-1);
86+
cookie.setHttpOnly(false);
87+
cookie.setPath("/");
88+
response.addCookie(cookie);
89+
}
90+
}
7191
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
// semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/jsr311-api-1.1.1
1+
// semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/jsr311-api-1.1.1:${testdir}/../../../../stubs/springframework-5.2.3

‎java/ql/test/stubs/springframework-5.2.3/org/springframework/security/web/csrf/CsrfToken.java

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
/*
2+
* Copyright 2002-2013 the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.springframework.security.web.csrf;
18+
19+
import java.io.Serializable;
20+
21+
/**
22+
* Provides the information about an expected CSRF token.
23+
*
24+
* @author Rob Winch
25+
* @since 3.2
26+
* @see DefaultCsrfToken
27+
*/
28+
public interface CsrfToken extends Serializable {
29+
30+
/**
31+
* Gets the HTTP header that the CSRF is populated on the response and can be placed
32+
* on requests instead of the parameter. Cannot be null.
33+
* @return the HTTP header that the CSRF is populated on the response and can be
34+
* placed on requests instead of the parameter
35+
*/
36+
String getHeaderName();
37+
38+
/**
39+
* Gets the HTTP parameter name that should contain the token. Cannot be null.
40+
* @return the HTTP parameter name that should contain the token.
41+
*/
42+
String getParameterName();
43+
44+
/**
45+
* Gets the token value. Cannot be null.
46+
* @return the token value
47+
*/
48+
String getToken();
49+
50+
}

0 commit comments

Comments
 (0)