Go: Promote non-httponly cookie query, and add insecure cookie query #20762
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| /** | ||
| * Provides classes for modeling the `github.com/gin-gonic/gin` package. | ||
| */ | ||
|
|
||
| import go | ||
| import semmle.go.concepts.HTTP | ||
|
|
||
| /** Provides models for the `gin-gonic/gin` package. */ | ||
| module Gin { | ||
| /** Gets the package name `github.com/gin-gonic/gin`. */ | ||
| string packagePath() { result = package("github.com/gin-gonic/gin", "") } | ||
|
|
||
| private class GinCookieWrite extends Http::CookieWrite::Range, DataFlow::MethodCallNode { | ||
| GinCookieWrite() { this.getTarget().hasQualifiedName(packagePath(), "Context", "SetCookie") } | ||
|
|
||
| override DataFlow::Node getName() { result = this.getArgument(0) } | ||
|
|
||
| override DataFlow::Node getValue() { result = this.getArgument(1) } | ||
|
|
||
| override DataFlow::Node getSecure() { result = this.getArgument(5) } | ||
|
|
||
| override DataFlow::Node getHttpOnly() { result = this.getArgument(6) } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -293,4 +293,38 @@ module NetHttp { | |
|
|
||
| override DataFlow::Node getAPathArgument() { result = this.getArgument(2) } | ||
| } | ||
|
|
||
| private class CookieWrite extends Http::CookieWrite::Range, DataFlow::CallNode { | ||
| CookieWrite() { this.getTarget().hasQualifiedName(package("net/http", ""), "SetCookie") } | ||
|
|
||
| override DataFlow::Node getName() { result = this.getArgument(1) } | ||
|
|
||
| override DataFlow::Node getValue() { result = this.getArgument(1) } | ||
|
|
||
| override DataFlow::Node getSecure() { result = this.getArgument(1) } | ||
|
|
||
| override DataFlow::Node getHttpOnly() { result = this.getArgument(1) } | ||
| } | ||
|
|
||
| private class CookieFieldWrite extends Http::CookieOptions::Range { | ||
| Write w; | ||
| Field f; | ||
|
Comment on lines
+310
to
+311
Contributor
There was a problem hiding this comment. Rather than storing these as fields, they should be introduced in the characteristic predicate using |
||
| DataFlow::Node written; | ||
| string fieldName; | ||
|
|
||
| CookieFieldWrite() { | ||
| f.hasQualifiedName(package("net/http", ""), "Cookie", fieldName) and | ||
| w.writesField(this, f, written) | ||
| } | ||
|
|
||
| override DataFlow::Node getCookieOutput() { result = this } | ||
|
|
||
| override DataFlow::Node getName() { fieldName = "Name" and result = written } | ||
|
|
||
| override DataFlow::Node getValue() { fieldName = "Value" and result = written } | ||
|
|
||
| override DataFlow::Node getSecure() { fieldName = "Secure" and result = written } | ||
|
|
||
| override DataFlow::Node getHttpOnly() { fieldName = "HttpOnly" and result = written } | ||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. It's confusing having predicates for two different queries in this one file (which is then only named after one of the predicates). Unless there is a good reason to keep them together, please put them in two separate files, named |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,108 @@ | ||||||
| /** Provides classes and predicates for identifying HTTP cookies with insecure attributes. */ | ||||||
|
|
||||||
| import go | ||||||
| import semmle.go.concepts.HTTP | ||||||
| import semmle.go.dataflow.DataFlow | ||||||
|
|
||||||
| /** | ||||||
| * Holds if the expression or its value has a sensitive name | ||||||
|
Contributor
There was a problem hiding this comment.
Suggested change
|
||||||
| */ | ||||||
| private predicate isSensitiveExpr(Expr expr, string val) { | ||||||
|
Contributor
There was a problem hiding this comment. The predicate name is confusing because in |
||||||
| ( | ||||||
| val = expr.getStringValue() or | ||||||
| val = expr.(Name).getTarget().getName() | ||||||
| ) and | ||||||
| val.regexpMatch("(?i).*(session|login|token|user|auth|credential).*") and | ||||||
| not val.regexpMatch("(?i).*(xsrf|csrf|forgery).*") | ||||||
| } | ||||||
|
|
||||||
| private module SensitiveCookieNameConfig implements DataFlow::ConfigSig { | ||||||
| predicate isSource(DataFlow::Node source) { isSensitiveExpr(source.asExpr(), _) } | ||||||
|
|
||||||
| predicate isSink(DataFlow::Node sink) { exists(Http::CookieWrite cw | sink = cw.getName()) } | ||||||
|
Contributor
There was a problem hiding this comment. I think this would be cleaner with an additional predicate |
||||||
|
|
||||||
| predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) { | ||||||
| exists(Http::CookieOptions co | co.getName() = pred and co.getCookieOutput() = succ) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** Tracks flow from sensitive names to HTTP cookie writes. */ | ||||||
| module SensitiveCookieNameFlow = TaintTracking::Global<SensitiveCookieNameConfig>; | ||||||
|
|
||||||
| private module BooleanCookieSecureConfig implements DataFlow::ConfigSig { | ||||||
| predicate isSource(DataFlow::Node source) { | ||||||
| source.getType().getUnderlyingType() instanceof BoolType | ||||||
| } | ||||||
|
|
||||||
| predicate isSink(DataFlow::Node sink) { exists(Http::CookieWrite cw | sink = cw.getSecure()) } | ||||||
|
|
||||||
| predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) { | ||||||
| exists(Http::CookieOptions co | co.getSecure() = pred and co.getCookieOutput() = succ) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** Tracks flow from boolean expressions to the `Secure` attribute of HTTP cookie writes. */ | ||||||
| module BooleanCookieSecureFlow = TaintTracking::Global<BooleanCookieSecureConfig>; | ||||||
|
|
||||||
| private module BooleanCookieHttpOnlyConfig implements DataFlow::ConfigSig { | ||||||
| predicate isSource(DataFlow::Node source) { | ||||||
| source.getType().getUnderlyingType() instanceof BoolType | ||||||
| } | ||||||
|
|
||||||
| predicate isSink(DataFlow::Node sink) { exists(Http::CookieWrite cw | sink = cw.getHttpOnly()) } | ||||||
|
|
||||||
| predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) { | ||||||
| exists(Http::CookieOptions co | co.getHttpOnly() = pred and co.getCookieOutput() = succ) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /** Tracks flow from boolean expressions to the `HttpOnly` attribute of HTTP cookie writes. */ | ||||||
| module BooleanCookieHttpOnlyFlow = TaintTracking::Global<BooleanCookieHttpOnlyConfig>; | ||||||
|
|
||||||
| /** Holds if `cw` has the `Secure` attribute left at its default value of `false`. */ | ||||||
| predicate isInsecureDefault(Http::CookieWrite cw) { | ||||||
| not BooleanCookieSecureFlow::flow(_, cw.getSecure()) | ||||||
| } | ||||||
|
|
||||||
| /** Holds if `cw` has the `Secure` attribute explicitly set to `false`, from the expression `boolFalse`. */ | ||||||
| predicate isInsecureDirect(Http::CookieWrite cw, Expr boolFalse) { | ||||||
| BooleanCookieSecureFlow::flow(DataFlow::exprNode(boolFalse), cw.getSecure()) and | ||||||
| boolFalse.getBoolValue() = false | ||||||
| } | ||||||
|
|
||||||
| /** Holds if `cw` has the `Secure` attribute set to `false`, either explicitly or by default. */ | ||||||
| predicate isInsecureCookie(Http::CookieWrite cw) { | ||||||
| isInsecureDefault(cw) or | ||||||
| isInsecureDirect(cw, _) | ||||||
| } | ||||||
|
|
||||||
| /** Holds if `cw` has the `HttpOnly` attribute left at its default value of `false`. */ | ||||||
| predicate isNonHttpOnlyDefault(Http::CookieWrite cw) { | ||||||
| not BooleanCookieHttpOnlyFlow::flow(_, cw.getHttpOnly()) | ||||||
| } | ||||||
|
|
||||||
| /** Holds if `cw` has the `HttpOnly` attribute explicitly set to `false`, from the expression `boolFalse`. */ | ||||||
| predicate isNonHttpOnlyDirect(Http::CookieWrite cw, Expr boolFalse) { | ||||||
| BooleanCookieHttpOnlyFlow::flow(DataFlow::exprNode(boolFalse), cw.getHttpOnly()) and | ||||||
| boolFalse.getBoolValue() = false | ||||||
| } | ||||||
|
|
||||||
| /** Holds if `cw` has the `HttpOnly` attribute set to `false`, either explicitly or by default. */ | ||||||
| predicate isNonHttpOnlyCookie(Http::CookieWrite cw) { | ||||||
| isNonHttpOnlyDefault(cw) or | ||||||
| isNonHttpOnlyDirect(cw, _) | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Holds if `cw` has the sensitive name `name`, from the expression `nameExpr`. | ||||||
| * `source` and `sink` represent the data flow path from the sensitive name expression to the cookie write. | ||||||
| */ | ||||||
| predicate isSensitiveCookie( | ||||||
| Http::CookieWrite cw, Expr nameExpr, string name, SensitiveCookieNameFlow::PathNode source, | ||||||
| SensitiveCookieNameFlow::PathNode sink | ||||||
| ) { | ||||||
| SensitiveCookieNameFlow::flowPath(source, sink) and | ||||||
| source.getNode().asExpr() = nameExpr and | ||||||
| sink.getNode() = cw.getName() and | ||||||
| isSensitiveExpr(nameExpr, name) | ||||||
| } | ||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| <!DOCTYPE qhelp PUBLIC | ||
| "-//Semmle//qhelp//EN" | ||
| "qhelp.dtd"> | ||
| <qhelp> | ||
|
|
||
| <overview> | ||
| <p>Cookies without the <code>HttpOnly</code> flag set are accessible to client-side scripts such as JavaScript running in the same origin. | ||
| In case of a Cross-Site Scripting (XSS) vulnerability, the cookie can be stolen by a malicious script. | ||
| If a sensitive cookie does not need to be accessed directly by client-side JS, the <code>HttpOnly</code> flag should be set.</p> | ||
| </overview> | ||
|
|
||
| <recommendation> | ||
| <p> | ||
| Set the <code>HttpOnly</code> flag to <code>true</code> for authentication cookies to ensure they are not accessible to client-side scripts. | ||
| </p> | ||
| </recommendation> | ||
|
|
||
| <example> | ||
| <p> | ||
| In the following example, in the case marked BAD, the <code>HttpOnly</code> flag is not set, so the default value of <code>false</code> is used. | ||
| In the case marked GOOD, the <code>HttpOnly</code> flag is set to <code>true</code>. | ||
| </p> | ||
| <sample src="examples/CookieWithoutHttpOnly.go"/> | ||
|
|
||
|
|
||
| </example> | ||
|
|
||
| <references> | ||
|
|
||
| <li>MDN: <a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie">Set-Cookie</a> Header.</li> | ||
| <li>PortSwigger: <a href="https://portswigger.net/kb/issues/00500600_cookie-without-httponly-flag-set">Cookie without HttpOnly flag set</a></li> | ||
|
|
||
| </references> | ||
| </qhelp> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| /** | ||
| * @name Cookie 'HttpOnly' attribute is not set to true | ||
| * @description Sensitive cookies without the `HttpOnly` property set are accessible by client-side scripts such as JavaScript. | ||
| * This makes them more vulnerable to being stolen by an XSS attack. | ||
| * @kind path-problem | ||
| * @problem.severity warning | ||
| * @precision high | ||
| * @security-severity 5.0 | ||
| * @id go/cookie-httponly-not-set | ||
| * @tags security | ||
| * external/cwe/cwe-1004 | ||
| */ | ||
|
|
||
| import go | ||
| import semmle.go.security.SecureCookies | ||
| import SensitiveCookieNameFlow::PathGraph | ||
|
|
||
| from | ||
| Http::CookieWrite cw, Expr sensitiveNameExpr, string name, | ||
| SensitiveCookieNameFlow::PathNode source, SensitiveCookieNameFlow::PathNode sink | ||
| where | ||
| isSensitiveCookie(cw, sensitiveNameExpr, name, source, sink) and | ||
| isNonHttpOnlyCookie(cw) | ||
| select cw, source, sink, "Sensitive cookie $@ does not set HttpOnly attribute to true.", | ||
| sensitiveNameExpr, name |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.