|
| 1 | +/** |
| 2 | + * @name Sensitive cookies without the HttpOnly response header set |
| 3 | + * @description Sensitive cookies without 'HttpOnly' leaves session cookies vulnerable to an XSS attack. |
| 4 | + * @kind path-problem |
| 5 | + * @id java/sensitive-cookie-not-httponly |
| 6 | + * @tags security |
| 7 | + * external/cwe/cwe-1004 |
| 8 | + */ |
| 9 | + |
| 10 | +import java |
| 11 | +import semmle.code.java.frameworks.Servlets |
| 12 | +import semmle.code.java.dataflow.FlowSources |
| 13 | +import semmle.code.java.dataflow.TaintTracking |
| 14 | +import DataFlow::PathGraph |
| 15 | + |
| 16 | +/** Gets a regular expression for matching common names of sensitive cookies. */ |
| 17 | +string getSensitiveCookieNameRegex() { result = "(?i).*(auth|session|token|key|credential).*" } |
| 18 | + |
| 19 | +/** Holds if a string is concatenated with the name of a sensitive cookie. */ |
| 20 | +predicate isSensitiveCookieNameExpr(Expr expr) { |
| 21 | + expr.(StringLiteral) |
| 22 | + .getRepresentedString() |
| 23 | + .toLowerCase() |
| 24 | + .regexpMatch(getSensitiveCookieNameRegex()) or |
| 25 | + isSensitiveCookieNameExpr(expr.(AddExpr).getAnOperand()) |
| 26 | +} |
| 27 | + |
| 28 | +/** Holds if a string is concatenated with the `HttpOnly` flag. */ |
| 29 | +predicate hasHttpOnlyExpr(Expr expr) { |
| 30 | + expr.(StringLiteral).getRepresentedString().toLowerCase().matches("%httponly%") or |
| 31 | + hasHttpOnlyExpr(expr.(AddExpr).getAnOperand()) |
| 32 | +} |
| 33 | + |
| 34 | +/** The method call `Set-Cookie` of `addHeader` or `setHeader`. */ |
| 35 | +class SetCookieMethodAccess extends MethodAccess { |
| 36 | + SetCookieMethodAccess() { |
| 37 | + ( |
| 38 | + this.getMethod() instanceof ResponseAddHeaderMethod or |
| 39 | + this.getMethod() instanceof ResponseSetHeaderMethod |
| 40 | + ) and |
| 41 | + this.getArgument(0).(StringLiteral).getRepresentedString().toLowerCase() = "set-cookie" |
| 42 | + } |
| 43 | +} |
| 44 | + |
| 45 | +/** Sensitive cookie name used in a `Cookie` constructor or a `Set-Cookie` call. */ |
| 46 | +class SensitiveCookieNameExpr extends Expr { |
| 47 | + SensitiveCookieNameExpr() { |
| 48 | + isSensitiveCookieNameExpr(this) and |
| 49 | + ( |
| 50 | + exists( |
| 51 | + ClassInstanceExpr cie // new Cookie("jwt_token", token) |
| 52 | + | |
| 53 | + ( |
| 54 | + cie.getConstructor().getDeclaringType().hasQualifiedName("javax.servlet.http", "Cookie") or |
| 55 | + cie.getConstructor() |
| 56 | + .getDeclaringType() |
| 57 | + .getASupertype*() |
| 58 | + .hasQualifiedName("javax.ws.rs.core", "Cookie") or |
| 59 | + cie.getConstructor() |
| 60 | + .getDeclaringType() |
| 61 | + .getASupertype*() |
| 62 | + .hasQualifiedName("jakarta.ws.rs.core", "Cookie") |
| 63 | + ) and |
| 64 | + DataFlow::localExprFlow(this, cie.getArgument(0)) |
| 65 | + ) |
| 66 | + or |
| 67 | + exists( |
| 68 | + SetCookieMethodAccess ma // response.addHeader("Set-Cookie: token=" +authId + ";HttpOnly;Secure") |
| 69 | + | |
| 70 | + DataFlow::localExprFlow(this, ma.getArgument(1)) |
| 71 | + ) |
| 72 | + ) |
| 73 | + } |
| 74 | +} |
| 75 | + |
| 76 | +/** Sink of adding a cookie to the HTTP response. */ |
| 77 | +class CookieResponseSink extends DataFlow::ExprNode { |
| 78 | + CookieResponseSink() { |
| 79 | + exists(MethodAccess ma | |
| 80 | + ( |
| 81 | + ma.getMethod() instanceof ResponseAddCookieMethod or |
| 82 | + ma instanceof SetCookieMethodAccess |
| 83 | + ) and |
| 84 | + ma.getAnArgument() = this.getExpr() |
| 85 | + ) |
| 86 | + } |
| 87 | +} |
| 88 | + |
| 89 | +/** Holds if the `node` is a method call of `setHttpOnly(true)` on a cookie. */ |
| 90 | +predicate setHttpOnlyMethodAccess(DataFlow::Node node) { |
| 91 | + exists( |
| 92 | + MethodAccess addCookie, Variable cookie, MethodAccess m // jwtCookie.setHttpOnly(true) |
| 93 | + | |
| 94 | + addCookie.getMethod() instanceof ResponseAddCookieMethod and |
| 95 | + addCookie.getArgument(0) = cookie.getAnAccess() and |
| 96 | + m.getMethod().getName() = "setHttpOnly" and |
| 97 | + m.getArgument(0).(BooleanLiteral).getBooleanValue() = true and |
| 98 | + m.getQualifier() = cookie.getAnAccess() and |
| 99 | + node.asExpr() = cookie.getAnAccess() |
| 100 | + ) |
| 101 | +} |
| 102 | + |
| 103 | +/** Holds if the `node` is a method call of `Set-Cookie` header with the `HttpOnly` flag whose cookie name is sensitive. */ |
| 104 | +predicate setHttpOnlyInSetCookie(DataFlow::Node node) { |
| 105 | + exists(SetCookieMethodAccess sa | |
| 106 | + hasHttpOnlyExpr(node.asExpr()) and |
| 107 | + DataFlow::localExprFlow(node.asExpr(), sa.getArgument(1)) |
| 108 | + ) |
| 109 | +} |
| 110 | + |
| 111 | +/** Holds if the `node` is an invocation of a JAX-RS `NewCookie` constructor that sets `HttpOnly` to true. */ |
| 112 | +predicate setHttpOnlyInNewCookie(DataFlow::Node node) { |
| 113 | + exists(ClassInstanceExpr cie | |
| 114 | + cie.getConstructor().getDeclaringType().hasName("NewCookie") and |
| 115 | + DataFlow::localExprFlow(node.asExpr(), cie.getArgument(0)) and |
| 116 | + ( |
| 117 | + cie.getNumArgument() = 6 and cie.getArgument(5).(BooleanLiteral).getBooleanValue() = true // NewCookie(Cookie cookie, String comment, int maxAge, Date expiry, boolean secure, boolean httpOnly) |
| 118 | + or |
| 119 | + cie.getNumArgument() = 8 and |
| 120 | + cie.getArgument(6).getType() instanceof BooleanType and |
| 121 | + cie.getArgument(7).(BooleanLiteral).getBooleanValue() = true // NewCookie(String name, String value, String path, String domain, String comment, int maxAge, boolean secure, boolean httpOnly) |
| 122 | + or |
| 123 | + 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) |
| 124 | + ) |
| 125 | + ) |
| 126 | +} |
| 127 | + |
| 128 | +/** |
| 129 | + * Holds if the node is a test method indicated by: |
| 130 | + * a) in a test directory such as `src/test/java` |
| 131 | + * b) in a test package whose name has the word `test` |
| 132 | + * c) in a test class whose name has the word `test` |
| 133 | + * d) in a test class implementing a test framework such as JUnit or TestNG |
| 134 | + */ |
| 135 | +predicate isTestMethod(DataFlow::Node node) { |
| 136 | + exists(MethodAccess ma, Method m | |
| 137 | + node.asExpr() = ma.getAnArgument() and |
| 138 | + m = ma.getEnclosingCallable() and |
| 139 | + ( |
| 140 | + m.getDeclaringType().getName().toLowerCase().matches("%test%") or // Simple check to exclude test classes to reduce FPs |
| 141 | + m.getDeclaringType().getPackage().getName().toLowerCase().matches("%test%") or // Simple check to exclude classes in test packages to reduce FPs |
| 142 | + exists(m.getLocation().getFile().getAbsolutePath().indexOf("/src/test/java")) or // Match test directory structure of build tools like maven |
| 143 | + m instanceof TestMethod // Test method of a test case implementing a test framework such as JUnit or TestNG |
| 144 | + ) |
| 145 | + ) |
| 146 | +} |
| 147 | + |
| 148 | +/** A taint configuration tracking flow from a sensitive cookie without HttpOnly flag set to its HTTP response. */ |
| 149 | +class MissingHttpOnlyConfiguration extends TaintTracking::Configuration { |
| 150 | + MissingHttpOnlyConfiguration() { this = "MissingHttpOnlyConfiguration" } |
| 151 | + |
| 152 | + override predicate isSource(DataFlow::Node source) { |
| 153 | + source.asExpr() instanceof SensitiveCookieNameExpr |
| 154 | + } |
| 155 | + |
| 156 | + override predicate isSink(DataFlow::Node sink) { sink instanceof CookieResponseSink } |
| 157 | + |
| 158 | + override predicate isSanitizer(DataFlow::Node node) { |
| 159 | + // cookie.setHttpOnly(true) |
| 160 | + setHttpOnlyMethodAccess(node) |
| 161 | + or |
| 162 | + // response.addHeader("Set-Cookie: token=" +authId + ";HttpOnly;Secure") |
| 163 | + setHttpOnlyInSetCookie(node) |
| 164 | + or |
| 165 | + // new NewCookie("session-access-key", accessKey, "/", null, null, 0, true, true) |
| 166 | + setHttpOnlyInNewCookie(node) |
| 167 | + or |
| 168 | + // Test class or method |
| 169 | + isTestMethod(node) |
| 170 | + } |
| 171 | + |
| 172 | + override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) { |
| 173 | + exists( |
| 174 | + ClassInstanceExpr cie // `NewCookie` constructor |
| 175 | + | |
| 176 | + cie.getAnArgument() = pred.asExpr() and |
| 177 | + cie = succ.asExpr() and |
| 178 | + cie.getConstructor().getDeclaringType().hasName("NewCookie") |
| 179 | + ) |
| 180 | + or |
| 181 | + exists( |
| 182 | + MethodAccess ma // `toString` call on a cookie object |
| 183 | + | |
| 184 | + ma.getQualifier() = pred.asExpr() and |
| 185 | + ma.getMethod().hasName("toString") and |
| 186 | + ma = succ.asExpr() |
| 187 | + ) |
| 188 | + } |
| 189 | +} |
| 190 | + |
| 191 | +from DataFlow::PathNode source, DataFlow::PathNode sink, MissingHttpOnlyConfiguration c |
| 192 | +where c.hasFlowPath(source, sink) |
| 193 | +select sink.getNode(), source, sink, "$@ doesn't have the HttpOnly flag set.", source.getNode(), |
| 194 | + "This sensitive cookie" |
0 commit comments