Skip to content

Go: GoAdd Cookie Sanitizer to Reflected XSS #14608

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Oct 27, 2023
Merged

Go: GoAdd Cookie Sanitizer to Reflected XSS #14608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
21e4a5b
Add Cookie Sanitizer
Kwstubbs Oct 26, 2023
e6e87a4
Add change note
Kwstubbs Oct 26, 2023
135923c
Change XNetHtmltest
Kwstubbs Oct 27, 2023
52a0fda
formatting
Kwstubbs Oct 27, 2023
ce01047
Fix minor issues
Kwstubbs Oct 27, 2023
57cbacb
test.go change
Kwstubbs Oct 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions go/ql/lib/change-notes/2023-10-25-reflectedxss-cookie-sanitizer.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Added [Request.Cookie](https://pkg.go.dev/net/http#Request.Cookie) to reflected XSS sanitizers.
12 changes: 12 additions & 0 deletions go/ql/lib/semmle/go/security/ReflectedXssCustomizations.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,18 @@ module ReflectedXss {
/** A shared XSS sanitizer as a sanitizer for reflected XSS. */
private class SharedXssSanitizer extends Sanitizer instanceof SharedXss::Sanitizer { }

/**
* A request.Cookie method returns the request cookie, which is not user controlled in reflected XSS context.
*/
class CookieSanitizer extends Sanitizer {
CookieSanitizer() {
exists(Method m, DataFlow::CallNode call | call = m.getACall() |
m.hasQualifiedName("net/http", "Request", "Cookie") and
this = call.getResult(0)
)
}
}

/**
* A third-party controllable input, considered as a flow source for reflected XSS.
*/
Expand Down
240 changes: 122 additions & 118 deletions go/ql/test/library-tests/semmle/go/frameworks/XNetHtml/ReflectedXss.expected
View file
Open in desktop

Large diffs are not rendered by default.

12 changes: 6 additions & 6 deletions go/ql/test/library-tests/semmle/go/frameworks/XNetHtml/SqlInjection.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
edges
| test.go:55:2:55:42 | ... := ...[0] | test.go:56:29:56:40 | selection of Value |
| test.go:56:29:56:40 | selection of Value | test.go:56:11:56:41 | call to EscapeString |
| test.go:56:2:56:42 | ... := ...[0] | test.go:57:29:57:40 | selection of Value |
| test.go:57:29:57:40 | selection of Value | test.go:57:11:57:41 | call to EscapeString |
nodes
| test.go:55:2:55:42 | ... := ...[0] | semmle.label | ... := ...[0] |
| test.go:56:11:56:41 | call to EscapeString | semmle.label | call to EscapeString |
| test.go:56:29:56:40 | selection of Value | semmle.label | selection of Value |
| test.go:56:2:56:42 | ... := ...[0] | semmle.label | ... := ...[0] |
| test.go:57:11:57:41 | call to EscapeString | semmle.label | call to EscapeString |
| test.go:57:29:57:40 | selection of Value | semmle.label | selection of Value |
subpaths
#select
| test.go:56:11:56:41 | call to EscapeString | test.go:55:2:55:42 | ... := ...[0] | test.go:56:11:56:41 | call to EscapeString | This query depends on a $@. | test.go:55:2:55:42 | ... := ...[0] | user-provided value |
| test.go:57:11:57:41 | call to EscapeString | test.go:56:2:56:42 | ... := ...[0] | test.go:57:11:57:41 | call to EscapeString | This query depends on a $@. | test.go:56:2:56:42 | ... := ...[0] | user-provided value |
9 changes: 5 additions & 4 deletions go/ql/test/library-tests/semmle/go/frameworks/XNetHtml/test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,17 @@ package test

import (
"database/sql"
"golang.org/x/net/html"
"net/http"

"golang.org/x/net/html"
)

func test(request *http.Request, writer http.ResponseWriter) {

cookie, _ := request.Cookie("SomeCookie")
writer.Write([]byte(html.EscapeString(cookie.Value))) // GOOD: escaped.
param1 := request.URL.Query().Get("param1")
writer.Write([]byte(html.EscapeString(param1))) // GOOD: escaped.

writer.Write([]byte(html.UnescapeString(cookie.Value))) // BAD: unescaped.
writer.Write([]byte(html.UnescapeString(param1))) // BAD: unescaped.

node, _ := html.Parse(request.Body)
writer.Write([]byte(node.Data)) // BAD: writing unescaped HTML data
Expand Down
8 changes: 0 additions & 8 deletions go/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,6 @@ edges
| contenttype.go:73:10:73:28 | call to FormValue | contenttype.go:79:11:79:14 | data |
| contenttype.go:88:10:88:28 | call to FormValue | contenttype.go:91:4:91:7 | data |
| contenttype.go:113:10:113:28 | call to FormValue | contenttype.go:114:50:114:53 | data |
| reflectedxsstest.go:27:2:27:38 | ... := ...[0] | reflectedxsstest.go:28:50:28:55 | cookie |
| reflectedxsstest.go:28:17:28:56 | call to Sprintf | reflectedxsstest.go:28:10:28:57 | type conversion |
| reflectedxsstest.go:28:50:28:55 | cookie | reflectedxsstest.go:28:17:28:56 | call to Sprintf |
| reflectedxsstest.go:31:2:31:44 | ... := ...[0] | reflectedxsstest.go:32:34:32:37 | file |
| reflectedxsstest.go:31:2:31:44 | ... := ...[1] | reflectedxsstest.go:34:46:34:60 | selection of Filename |
| reflectedxsstest.go:32:2:32:38 | ... := ...[0] | reflectedxsstest.go:33:49:33:55 | content |
Expand Down Expand Up @@ -62,10 +59,6 @@ nodes
| contenttype.go:91:4:91:7 | data | semmle.label | data |
| contenttype.go:113:10:113:28 | call to FormValue | semmle.label | call to FormValue |
| contenttype.go:114:50:114:53 | data | semmle.label | data |
| reflectedxsstest.go:27:2:27:38 | ... := ...[0] | semmle.label | ... := ...[0] |
| reflectedxsstest.go:28:10:28:57 | type conversion | semmle.label | type conversion |
| reflectedxsstest.go:28:17:28:56 | call to Sprintf | semmle.label | call to Sprintf |
| reflectedxsstest.go:28:50:28:55 | cookie | semmle.label | cookie |
| reflectedxsstest.go:31:2:31:44 | ... := ...[0] | semmle.label | ... := ...[0] |
| reflectedxsstest.go:31:2:31:44 | ... := ...[1] | semmle.label | ... := ...[1] |
| reflectedxsstest.go:32:2:32:38 | ... := ...[0] | semmle.label | ... := ...[0] |
Expand Down Expand Up @@ -119,7 +112,6 @@ subpaths
| contenttype.go:79:11:79:14 | data | contenttype.go:73:10:73:28 | call to FormValue | contenttype.go:79:11:79:14 | data | Cross-site scripting vulnerability due to $@. | contenttype.go:73:10:73:28 | call to FormValue | user-provided value | contenttype.go:0:0:0:0 | contenttype.go | |
| contenttype.go:91:4:91:7 | data | contenttype.go:88:10:88:28 | call to FormValue | contenttype.go:91:4:91:7 | data | Cross-site scripting vulnerability due to $@. | contenttype.go:88:10:88:28 | call to FormValue | user-provided value | contenttype.go:0:0:0:0 | contenttype.go | |
| contenttype.go:114:50:114:53 | data | contenttype.go:113:10:113:28 | call to FormValue | contenttype.go:114:50:114:53 | data | Cross-site scripting vulnerability due to $@. | contenttype.go:113:10:113:28 | call to FormValue | user-provided value | contenttype.go:0:0:0:0 | contenttype.go | |
| reflectedxsstest.go:28:10:28:57 | type conversion | reflectedxsstest.go:27:2:27:38 | ... := ...[0] | reflectedxsstest.go:28:10:28:57 | type conversion | Cross-site scripting vulnerability due to $@. | reflectedxsstest.go:27:2:27:38 | ... := ...[0] | user-provided value | reflectedxsstest.go:0:0:0:0 | reflectedxsstest.go | |
| reflectedxsstest.go:33:10:33:57 | type conversion | reflectedxsstest.go:31:2:31:44 | ... := ...[0] | reflectedxsstest.go:33:10:33:57 | type conversion | Cross-site scripting vulnerability due to $@. | reflectedxsstest.go:31:2:31:44 | ... := ...[0] | user-provided value | reflectedxsstest.go:0:0:0:0 | reflectedxsstest.go | |
| reflectedxsstest.go:34:10:34:62 | type conversion | reflectedxsstest.go:31:2:31:44 | ... := ...[1] | reflectedxsstest.go:34:10:34:62 | type conversion | Cross-site scripting vulnerability due to $@. | reflectedxsstest.go:31:2:31:44 | ... := ...[1] | user-provided value | reflectedxsstest.go:0:0:0:0 | reflectedxsstest.go | |
| reflectedxsstest.go:44:10:44:55 | type conversion | reflectedxsstest.go:38:2:38:35 | ... := ...[0] | reflectedxsstest.go:44:10:44:55 | type conversion | Cross-site scripting vulnerability due to $@. | reflectedxsstest.go:38:2:38:35 | ... := ...[0] | user-provided value | reflectedxsstest.go:0:0:0:0 | reflectedxsstest.go | |
Expand Down
2 changes: 1 addition & 1 deletion go/ql/test/query-tests/Security/CWE-079/reflectedxsstest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func ServeJsonDirect(w http.ResponseWriter, r http.Request) {

func ErrTest(w http.ResponseWriter, r http.Request) {
cookie, err := r.Cookie("somecookie")
w.Write([]byte(fmt.Sprintf("Cookie result: %v", cookie))) // BAD: Cookie's value is user-controlled
w.Write([]byte(fmt.Sprintf("Cookie result: %v", cookie))) // GOOD: Cookie's value is not user-controlled in reflected xss.
w.Write([]byte(fmt.Sprintf("Cookie check error: %v", err))) // GOOD: Cookie's err return is harmless
http.Error(w, fmt.Sprintf("Cookie result: %v", cookie), 500) // Good: only plain text is written.
file, header, err := r.FormFile("someFile")
Expand Down