Skip to content

Commit c1ecd5a

Browse files
owen-mcowen-mc
authored
Merge pull request github#14608 from Kwstubbs/golang-cookie-reflectedxss-sanitizer
Go: GoAdd Cookie Sanitizer to Reflected XSS
2 parents cb1cd5e + 57cbacb commit c1ecd5a

File tree

7 files changed

+150
-137
lines changed

7 files changed

+150
-137
lines changed

‎go/ql/lib/change-notes/2023-10-25-reflectedxss-cookie-sanitizer.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Added [Request.Cookie](https://pkg.go.dev/net/http#Request.Cookie) to reflected XSS sanitizers.

‎go/ql/lib/semmle/go/security/ReflectedXssCustomizations.qll

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,18 @@ module ReflectedXss {
2222
/** A shared XSS sanitizer as a sanitizer for reflected XSS. */
2323
private class SharedXssSanitizer extends Sanitizer instanceof SharedXss::Sanitizer { }
2424

25+
/**
26+
* A request.Cookie method returns the request cookie, which is not user controlled in reflected XSS context.
27+
*/
28+
class CookieSanitizer extends Sanitizer {
29+
CookieSanitizer() {
30+
exists(Method m, DataFlow::CallNode call | call = m.getACall() |
31+
m.hasQualifiedName("net/http", "Request", "Cookie") and
32+
this = call.getResult(0)
33+
)
34+
}
35+
}
36+
2537
/**
2638
* A third-party controllable input, considered as a flow source for reflected XSS.
2739
*/

‎go/ql/test/library-tests/semmle/go/frameworks/XNetHtml/ReflectedXss.expected

Lines changed: 122 additions & 118 deletions
Large diffs are not rendered by default.

‎go/ql/test/library-tests/semmle/go/frameworks/XNetHtml/SqlInjection.expected

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
edges
2-
| test.go:55:2:55:42 | ... := ...[0] | test.go:56:29:56:40 | selection of Value |
3-
| test.go:56:29:56:40 | selection of Value | test.go:56:11:56:41 | call to EscapeString |
2+
| test.go:56:2:56:42 | ... := ...[0] | test.go:57:29:57:40 | selection of Value |
3+
| test.go:57:29:57:40 | selection of Value | test.go:57:11:57:41 | call to EscapeString |
44
nodes
5-
| test.go:55:2:55:42 | ... := ...[0] | semmle.label | ... := ...[0] |
6-
| test.go:56:11:56:41 | call to EscapeString | semmle.label | call to EscapeString |
7-
| test.go:56:29:56:40 | selection of Value | semmle.label | selection of Value |
5+
| test.go:56:2:56:42 | ... := ...[0] | semmle.label | ... := ...[0] |
6+
| test.go:57:11:57:41 | call to EscapeString | semmle.label | call to EscapeString |
7+
| test.go:57:29:57:40 | selection of Value | semmle.label | selection of Value |
88
subpaths
99
#select
10-
| 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 |
10+
| 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 |

‎go/ql/test/library-tests/semmle/go/frameworks/XNetHtml/test.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,16 +2,17 @@ package test
22

33
import (
44
"database/sql"
5-
"golang.org/x/net/html"
65
"net/http"
6+
7+
"golang.org/x/net/html"
78
)
89

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

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

14-
writer.Write([]byte(html.UnescapeString(cookie.Value))) // BAD: unescaped.
15+
writer.Write([]byte(html.UnescapeString(param1))) // BAD: unescaped.
1516

1617
node, _ := html.Parse(request.Body)
1718
writer.Write([]byte(node.Data)) // BAD: writing unescaped HTML data

‎go/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,6 @@ edges
99
| contenttype.go:73:10:73:28 | call to FormValue | contenttype.go:79:11:79:14 | data |
1010
| contenttype.go:88:10:88:28 | call to FormValue | contenttype.go:91:4:91:7 | data |
1111
| contenttype.go:113:10:113:28 | call to FormValue | contenttype.go:114:50:114:53 | data |
12-
| reflectedxsstest.go:27:2:27:38 | ... := ...[0] | reflectedxsstest.go:28:50:28:55 | cookie |
13-
| reflectedxsstest.go:28:17:28:56 | call to Sprintf | reflectedxsstest.go:28:10:28:57 | type conversion |
14-
| reflectedxsstest.go:28:50:28:55 | cookie | reflectedxsstest.go:28:17:28:56 | call to Sprintf |
1512
| reflectedxsstest.go:31:2:31:44 | ... := ...[0] | reflectedxsstest.go:32:34:32:37 | file |
1613
| reflectedxsstest.go:31:2:31:44 | ... := ...[1] | reflectedxsstest.go:34:46:34:60 | selection of Filename |
1714
| reflectedxsstest.go:32:2:32:38 | ... := ...[0] | reflectedxsstest.go:33:49:33:55 | content |
@@ -62,10 +59,6 @@ nodes
6259
| contenttype.go:91:4:91:7 | data | semmle.label | data |
6360
| contenttype.go:113:10:113:28 | call to FormValue | semmle.label | call to FormValue |
6461
| contenttype.go:114:50:114:53 | data | semmle.label | data |
65-
| reflectedxsstest.go:27:2:27:38 | ... := ...[0] | semmle.label | ... := ...[0] |
66-
| reflectedxsstest.go:28:10:28:57 | type conversion | semmle.label | type conversion |
67-
| reflectedxsstest.go:28:17:28:56 | call to Sprintf | semmle.label | call to Sprintf |
68-
| reflectedxsstest.go:28:50:28:55 | cookie | semmle.label | cookie |
6962
| reflectedxsstest.go:31:2:31:44 | ... := ...[0] | semmle.label | ... := ...[0] |
7063
| reflectedxsstest.go:31:2:31:44 | ... := ...[1] | semmle.label | ... := ...[1] |
7164
| reflectedxsstest.go:32:2:32:38 | ... := ...[0] | semmle.label | ... := ...[0] |
@@ -119,7 +112,6 @@ subpaths
119112
| 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 | |
120113
| 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 | |
121114
| 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 | |
122-
| 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 | |
123115
| 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 | |
124116
| 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 | |
125117
| 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 | |

‎go/ql/test/query-tests/Security/CWE-079/reflectedxsstest.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ func ServeJsonDirect(w http.ResponseWriter, r http.Request) {
2525

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

0 commit comments

Comments
 (0)