-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Go: Add tests for SafeUrlFlow, and fix a latent bug #20556
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.
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
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
b5fda88
Remove duplication of `UnsafeFieldReadSanitizer`
owen-mc 5b07e8c
Fix bug in `UnsafeFieldReadSanitizer`
owen-mc a2a9575
Add tests for safe URL flow
owen-mc dd3f754
Add change note.
owen-mc c93852d
Improve comments in test file
owen-mc 8983ac9
Phrase test in terms of safe URLs
owen-mc File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
112 changes: 112 additions & 0 deletions
112
go/ql/test/library-tests/semmle/go/security/SafeUrlFlow/SafeUrlFlow.expected
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,112 @@ | ||
| #select | ||
| | SafeUrlFlow.go:11:24:11:46 | ...+... | SafeUrlFlow.go:10:10:10:17 | selection of Host | SafeUrlFlow.go:11:24:11:46 | ...+... | A safe URL flows here from $@. | SafeUrlFlow.go:10:10:10:17 | selection of Host | here | | ||
| | SafeUrlFlow.go:14:29:14:44 | call to String | SafeUrlFlow.go:13:13:13:19 | selection of URL | SafeUrlFlow.go:14:29:14:44 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:13:13:13:19 | selection of URL | here | | ||
| | SafeUrlFlow.go:18:11:18:28 | call to String | SafeUrlFlow.go:10:10:10:17 | selection of Host | SafeUrlFlow.go:18:11:18:28 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:10:10:10:17 | selection of Host | here | | ||
| | SafeUrlFlow.go:49:24:49:57 | ...+... | SafeUrlFlow.go:39:13:39:19 | selection of URL | SafeUrlFlow.go:49:24:49:57 | ...+... | A safe URL flows here from $@. | SafeUrlFlow.go:39:13:39:19 | selection of URL | here | | ||
| | SafeUrlFlow.go:50:29:50:51 | ...+... | SafeUrlFlow.go:39:13:39:19 | selection of URL | SafeUrlFlow.go:50:29:50:51 | ...+... | A safe URL flows here from $@. | SafeUrlFlow.go:39:13:39:19 | selection of URL | here | | ||
| | SafeUrlFlow.go:51:11:51:38 | ...+... | SafeUrlFlow.go:39:13:39:19 | selection of URL | SafeUrlFlow.go:51:11:51:38 | ...+... | A safe URL flows here from $@. | SafeUrlFlow.go:39:13:39:19 | selection of URL | here | | ||
| | SafeUrlFlow.go:60:11:60:26 | call to String | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:60:11:60:26 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:57:13:57:19 | selection of URL | here | | ||
| | SafeUrlFlow.go:61:12:61:27 | call to String | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:61:12:61:27 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:57:13:57:19 | selection of URL | here | | ||
| | SafeUrlFlow.go:62:16:62:31 | call to String | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:62:16:62:31 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:57:13:57:19 | selection of URL | here | | ||
| | SafeUrlFlow.go:63:12:63:27 | call to String | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:63:12:63:27 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:57:13:57:19 | selection of URL | here | | ||
| | SafeUrlFlow.go:67:13:67:28 | call to String | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:67:13:67:28 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:57:13:57:19 | selection of URL | here | | ||
| | SafeUrlFlow.go:68:14:68:29 | call to String | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:68:14:68:29 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:57:13:57:19 | selection of URL | here | | ||
| | SafeUrlFlow.go:69:18:69:33 | call to String | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:69:18:69:33 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:57:13:57:19 | selection of URL | here | | ||
| | SafeUrlFlow.go:70:14:70:29 | call to String | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:70:14:70:29 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:57:13:57:19 | selection of URL | here | | ||
| | SafeUrlFlow.go:73:39:73:54 | call to String | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:73:39:73:54 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:57:13:57:19 | selection of URL | here | | ||
| | SafeUrlFlow.go:77:70:77:85 | call to String | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:77:70:77:85 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:57:13:57:19 | selection of URL | here | | ||
| | SafeUrlFlow.go:81:40:81:55 | call to String | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:81:40:81:55 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:57:13:57:19 | selection of URL | here | | ||
| | SafeUrlFlow.go:94:24:94:41 | call to String | SafeUrlFlow.go:87:14:87:21 | selection of Host | SafeUrlFlow.go:94:24:94:41 | call to String | A safe URL flows here from $@. | SafeUrlFlow.go:87:14:87:21 | selection of Host | here | | ||
| | SafeUrlFlow.go:116:11:116:23 | reconstructed | SafeUrlFlow.go:106:13:106:19 | selection of URL | SafeUrlFlow.go:116:11:116:23 | reconstructed | A safe URL flows here from $@. | SafeUrlFlow.go:106:13:106:19 | selection of URL | here | | ||
| | SafeUrlFlow.go:119:24:119:46 | ...+... | SafeUrlFlow.go:106:13:106:19 | selection of URL | SafeUrlFlow.go:119:24:119:46 | ...+... | A safe URL flows here from $@. | SafeUrlFlow.go:106:13:106:19 | selection of URL | here | | ||
| | SafeUrlFlow.go:120:29:120:54 | ...+... | SafeUrlFlow.go:106:13:106:19 | selection of URL | SafeUrlFlow.go:120:29:120:54 | ...+... | A safe URL flows here from $@. | SafeUrlFlow.go:106:13:106:19 | selection of URL | here | | ||
| | SafeUrlFlow.go:121:12:121:38 | ...+... | SafeUrlFlow.go:106:13:106:19 | selection of URL | SafeUrlFlow.go:121:12:121:38 | ...+... | A safe URL flows here from $@. | SafeUrlFlow.go:106:13:106:19 | selection of URL | here | | ||
| edges | ||
| | SafeUrlFlow.go:10:10:10:17 | selection of Host | SafeUrlFlow.go:11:24:11:46 | ...+... | provenance | Sink:MaD:1 | | ||
| | SafeUrlFlow.go:10:10:10:17 | selection of Host | SafeUrlFlow.go:17:19:17:22 | host | provenance | | | ||
| | SafeUrlFlow.go:13:13:13:19 | selection of URL | SafeUrlFlow.go:14:29:14:35 | baseURL | provenance | Src:MaD:2 | | ||
| | SafeUrlFlow.go:14:29:14:35 | baseURL | SafeUrlFlow.go:14:29:14:44 | call to String | provenance | MaD:3 | | ||
| | SafeUrlFlow.go:17:19:17:22 | host | SafeUrlFlow.go:18:11:18:19 | targetURL | provenance | Config | | ||
| | SafeUrlFlow.go:18:11:18:19 | targetURL | SafeUrlFlow.go:18:11:18:28 | call to String | provenance | MaD:3 | | ||
| | SafeUrlFlow.go:39:13:39:19 | selection of URL | SafeUrlFlow.go:49:24:49:57 | ...+... | provenance | Src:MaD:2 Sink:MaD:1 | | ||
| | SafeUrlFlow.go:39:13:39:19 | selection of URL | SafeUrlFlow.go:50:29:50:51 | ...+... | provenance | Src:MaD:2 | | ||
| | SafeUrlFlow.go:39:13:39:19 | selection of URL | SafeUrlFlow.go:51:11:51:38 | ...+... | provenance | Src:MaD:2 | | ||
| | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:60:11:60:17 | baseURL | provenance | Src:MaD:2 | | ||
| | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:61:12:61:18 | baseURL | provenance | Src:MaD:2 | | ||
| | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:62:16:62:22 | baseURL | provenance | Src:MaD:2 | | ||
| | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:63:12:63:18 | baseURL | provenance | Src:MaD:2 | | ||
| | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:67:13:67:19 | baseURL | provenance | Src:MaD:2 | | ||
| | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:68:14:68:20 | baseURL | provenance | Src:MaD:2 | | ||
| | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:69:18:69:24 | baseURL | provenance | Src:MaD:2 | | ||
| | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:70:14:70:20 | baseURL | provenance | Src:MaD:2 | | ||
| | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:73:39:73:45 | baseURL | provenance | Src:MaD:2 | | ||
| | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:77:70:77:76 | baseURL | provenance | Src:MaD:2 | | ||
| | SafeUrlFlow.go:57:13:57:19 | selection of URL | SafeUrlFlow.go:81:40:81:46 | baseURL | provenance | Src:MaD:2 | | ||
| | SafeUrlFlow.go:60:11:60:17 | baseURL | SafeUrlFlow.go:60:11:60:26 | call to String | provenance | MaD:3 | | ||
| | SafeUrlFlow.go:61:12:61:18 | baseURL | SafeUrlFlow.go:61:12:61:27 | call to String | provenance | MaD:3 | | ||
| | SafeUrlFlow.go:62:16:62:22 | baseURL | SafeUrlFlow.go:62:16:62:31 | call to String | provenance | MaD:3 | | ||
| | SafeUrlFlow.go:63:12:63:18 | baseURL | SafeUrlFlow.go:63:12:63:27 | call to String | provenance | MaD:3 | | ||
| | SafeUrlFlow.go:67:13:67:19 | baseURL | SafeUrlFlow.go:67:13:67:28 | call to String | provenance | MaD:3 | | ||
| | SafeUrlFlow.go:68:14:68:20 | baseURL | SafeUrlFlow.go:68:14:68:29 | call to String | provenance | MaD:3 | | ||
| | SafeUrlFlow.go:69:18:69:24 | baseURL | SafeUrlFlow.go:69:18:69:33 | call to String | provenance | MaD:3 | | ||
| | SafeUrlFlow.go:70:14:70:20 | baseURL | SafeUrlFlow.go:70:14:70:29 | call to String | provenance | MaD:3 | | ||
| | SafeUrlFlow.go:73:39:73:45 | baseURL | SafeUrlFlow.go:73:39:73:54 | call to String | provenance | MaD:3 | | ||
| | SafeUrlFlow.go:77:70:77:76 | baseURL | SafeUrlFlow.go:77:70:77:85 | call to String | provenance | MaD:3 | | ||
| | SafeUrlFlow.go:81:40:81:46 | baseURL | SafeUrlFlow.go:81:40:81:55 | call to String | provenance | MaD:3 | | ||
| | SafeUrlFlow.go:87:14:87:21 | selection of Host | SafeUrlFlow.go:91:19:91:26 | safeHost | provenance | | | ||
| | SafeUrlFlow.go:91:19:91:26 | safeHost | SafeUrlFlow.go:94:24:94:32 | targetURL | provenance | Config | | ||
| | SafeUrlFlow.go:94:24:94:32 | targetURL | SafeUrlFlow.go:94:24:94:41 | call to String | provenance | MaD:3 Sink:MaD:1 | | ||
| | SafeUrlFlow.go:106:13:106:19 | selection of URL | SafeUrlFlow.go:116:11:116:23 | reconstructed | provenance | Src:MaD:2 | | ||
| | SafeUrlFlow.go:106:13:106:19 | selection of URL | SafeUrlFlow.go:119:24:119:46 | ...+... | provenance | Src:MaD:2 Sink:MaD:1 | | ||
| | SafeUrlFlow.go:106:13:106:19 | selection of URL | SafeUrlFlow.go:120:29:120:54 | ...+... | provenance | Src:MaD:2 | | ||
| | SafeUrlFlow.go:106:13:106:19 | selection of URL | SafeUrlFlow.go:121:12:121:38 | ...+... | provenance | Src:MaD:2 | | ||
| models | ||
| | 1 | Sink: net/http; ; false; Redirect; ; ; Argument[2]; url-redirection[0]; manual | | ||
| | 2 | Source: net/http; Request; true; URL; ; ; ; remote; manual | | ||
| | 3 | Summary: fmt; Stringer; true; String; ; ; Argument[receiver]; ReturnValue; taint; manual | | ||
| nodes | ||
| | SafeUrlFlow.go:10:10:10:17 | selection of Host | semmle.label | selection of Host | | ||
| | SafeUrlFlow.go:11:24:11:46 | ...+... | semmle.label | ...+... | | ||
| | SafeUrlFlow.go:13:13:13:19 | selection of URL | semmle.label | selection of URL | | ||
| | SafeUrlFlow.go:14:29:14:35 | baseURL | semmle.label | baseURL | | ||
| | SafeUrlFlow.go:14:29:14:44 | call to String | semmle.label | call to String | | ||
| | SafeUrlFlow.go:17:19:17:22 | host | semmle.label | host | | ||
| | SafeUrlFlow.go:18:11:18:19 | targetURL | semmle.label | targetURL | | ||
| | SafeUrlFlow.go:18:11:18:28 | call to String | semmle.label | call to String | | ||
| | SafeUrlFlow.go:39:13:39:19 | selection of URL | semmle.label | selection of URL | | ||
| | SafeUrlFlow.go:49:24:49:57 | ...+... | semmle.label | ...+... | | ||
| | SafeUrlFlow.go:50:29:50:51 | ...+... | semmle.label | ...+... | | ||
| | SafeUrlFlow.go:51:11:51:38 | ...+... | semmle.label | ...+... | | ||
| | SafeUrlFlow.go:57:13:57:19 | selection of URL | semmle.label | selection of URL | | ||
| | SafeUrlFlow.go:60:11:60:17 | baseURL | semmle.label | baseURL | | ||
| | SafeUrlFlow.go:60:11:60:26 | call to String | semmle.label | call to String | | ||
| | SafeUrlFlow.go:61:12:61:18 | baseURL | semmle.label | baseURL | | ||
| | SafeUrlFlow.go:61:12:61:27 | call to String | semmle.label | call to String | | ||
| | SafeUrlFlow.go:62:16:62:22 | baseURL | semmle.label | baseURL | | ||
| | SafeUrlFlow.go:62:16:62:31 | call to String | semmle.label | call to String | | ||
| | SafeUrlFlow.go:63:12:63:18 | baseURL | semmle.label | baseURL | | ||
| | SafeUrlFlow.go:63:12:63:27 | call to String | semmle.label | call to String | | ||
| | SafeUrlFlow.go:67:13:67:19 | baseURL | semmle.label | baseURL | | ||
| | SafeUrlFlow.go:67:13:67:28 | call to String | semmle.label | call to String | | ||
| | SafeUrlFlow.go:68:14:68:20 | baseURL | semmle.label | baseURL | | ||
| | SafeUrlFlow.go:68:14:68:29 | call to String | semmle.label | call to String | | ||
| | SafeUrlFlow.go:69:18:69:24 | baseURL | semmle.label | baseURL | | ||
| | SafeUrlFlow.go:69:18:69:33 | call to String | semmle.label | call to String | | ||
| | SafeUrlFlow.go:70:14:70:20 | baseURL | semmle.label | baseURL | | ||
| | SafeUrlFlow.go:70:14:70:29 | call to String | semmle.label | call to String | | ||
| | SafeUrlFlow.go:73:39:73:45 | baseURL | semmle.label | baseURL | | ||
| | SafeUrlFlow.go:73:39:73:54 | call to String | semmle.label | call to String | | ||
| | SafeUrlFlow.go:77:70:77:76 | baseURL | semmle.label | baseURL | | ||
| | SafeUrlFlow.go:77:70:77:85 | call to String | semmle.label | call to String | | ||
| | SafeUrlFlow.go:81:40:81:46 | baseURL | semmle.label | baseURL | | ||
| | SafeUrlFlow.go:81:40:81:55 | call to String | semmle.label | call to String | | ||
| | SafeUrlFlow.go:87:14:87:21 | selection of Host | semmle.label | selection of Host | | ||
| | SafeUrlFlow.go:91:19:91:26 | safeHost | semmle.label | safeHost | | ||
| | SafeUrlFlow.go:94:24:94:32 | targetURL | semmle.label | targetURL | | ||
| | SafeUrlFlow.go:94:24:94:41 | call to String | semmle.label | call to String | | ||
| | SafeUrlFlow.go:106:13:106:19 | selection of URL | semmle.label | selection of URL | | ||
| | SafeUrlFlow.go:116:11:116:23 | reconstructed | semmle.label | reconstructed | | ||
| | SafeUrlFlow.go:119:24:119:46 | ...+... | semmle.label | ...+... | | ||
| | SafeUrlFlow.go:120:29:120:54 | ...+... | semmle.label | ...+... | | ||
| | SafeUrlFlow.go:121:12:121:38 | ...+... | semmle.label | ...+... | | ||
| subpaths |
139 changes: 139 additions & 0 deletions
139
go/ql/test/library-tests/semmle/go/security/SafeUrlFlow/SafeUrlFlow.go
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,139 @@ | ||
| package main | ||
|
|
||
| import ( | ||
| "context" | ||
| "net/http" | ||
| "net/url" | ||
| ) | ||
|
|
||
| func testStdlibSources(w http.ResponseWriter, req *http.Request) { | ||
| host := req.Host // $ Source | ||
| http.Redirect(w, req, "https://"+host+"/safe", http.StatusFound) // $ Alert | ||
|
|
||
| baseURL := req.URL // $ Source | ||
| w.Header().Set("Location", baseURL.String()) // $ Alert | ||
|
|
||
| targetURL := url.URL{} | ||
| targetURL.Host = host // propagation to URL when Host is assigned | ||
| http.Get(targetURL.String()) // $ Alert | ||
| } | ||
|
|
||
| func testSanitizerEdge1(w http.ResponseWriter, req *http.Request) { | ||
| baseURL := req.URL | ||
|
|
||
| // SanitizerEdge: Query method call (unsafe URL method - breaks flow) | ||
| query := baseURL.Query() // sanitizer edge blocks flow here | ||
| http.Redirect(w, req, query.Get("redirect"), http.StatusFound) // no flow expected | ||
| } | ||
|
|
||
| func testSanitizerEdge2(w http.ResponseWriter, req *http.Request) { | ||
| baseURL := req.URL | ||
|
|
||
| // SanitizerEdge: String slicing (breaks flow) | ||
| urlString := baseURL.String() | ||
| sliced := urlString[0:10] // sanitizer edge blocks flow here | ||
| w.Header().Set("Location", sliced) // no flow expected | ||
| } | ||
|
|
||
| func testFieldReads(w http.ResponseWriter, req *http.Request) { | ||
| baseURL := req.URL // $ Source | ||
|
|
||
| // Test that other URL methods preserve flow | ||
| scheme := baseURL.Scheme // should preserve flow | ||
| host := baseURL.Host // should preserve flow | ||
| path := baseURL.Path // should preserve flow | ||
| fragment := baseURL.Fragment // should preserve flow | ||
| user := baseURL.User // should preserve flow (but unsafe field) | ||
|
|
||
| // These should still have flow (not sanitized) | ||
| http.Redirect(w, req, "https://"+scheme+"://example.com", http.StatusFound) // $ Alert | ||
| w.Header().Set("Location", "https://"+host+"/safe") // $ Alert | ||
| http.Get("https://example.com" + path) // $ Alert | ||
| http.Get(fragment) | ||
| http.Get(user.String()) | ||
| } | ||
|
|
||
| func testRequestForgerySinks(req *http.Request) { | ||
| baseURL := req.URL // $ Source | ||
|
|
||
| // Standard library HTTP functions (request-forgery sinks) | ||
| http.Get(baseURL.String()) // $ Alert | ||
| http.Post(baseURL.String(), "application/json", nil) // $ Alert | ||
| http.PostForm(baseURL.String(), nil) // $ Alert | ||
| http.Head(baseURL.String()) // $ Alert | ||
|
|
||
| // HTTP Client methods (request-forgery sinks) | ||
| client := &http.Client{} | ||
| client.Get(baseURL.String()) // $ Alert | ||
| client.Post(baseURL.String(), "application/json", nil) // $ Alert | ||
| client.PostForm(baseURL.String(), nil) // $ Alert | ||
| client.Head(baseURL.String()) // $ Alert | ||
|
|
||
| // NewRequest + Client.Do (request-forgery sinks) | ||
| request, _ := http.NewRequest("GET", baseURL.String(), nil) // $ Alert | ||
| client.Do(request) | ||
|
|
||
| // NewRequestWithContext + Client.Do (request-forgery sinks) | ||
| reqWithCtx, _ := http.NewRequestWithContext(context.TODO(), "POST", baseURL.String(), nil) // $ Alert | ||
| client.Do(reqWithCtx) | ||
|
|
||
| // RoundTrip method (request-forgery sink) | ||
| request2, _ := http.NewRequest("GET", baseURL.String(), nil) // $ Alert | ||
| transport := &http.Transport{} | ||
| transport.RoundTrip(request2) | ||
| } | ||
|
|
||
| func testHostFieldAssignmentFlow(w http.ResponseWriter, req *http.Request) { | ||
| safeHost := req.Host // $ Source | ||
|
|
||
| // Test additional flow step: propagation when Host field is assigned | ||
| targetURL, _ := url.Parse("http://example.com/data") | ||
| targetURL.Host = safeHost // additional flow step from SafeUrlFlow config | ||
|
|
||
| // Flow should propagate to the whole URL after Host assignment | ||
| http.Redirect(w, req, targetURL.String(), http.StatusFound) // $ Alert | ||
| } | ||
|
|
||
| func testHostFieldOverwritten(w http.ResponseWriter, req *http.Request) { | ||
| baseURL := req.URL | ||
|
|
||
| // Flow should be blocked when Host is overwritten | ||
| baseURL.Host = "something.else.com" | ||
| http.Get(baseURL.String()) | ||
| } | ||
|
|
||
| func testFieldAccess(w http.ResponseWriter, req *http.Request) { | ||
| baseURL := req.URL // $ Source | ||
|
|
||
| // Safe field accesses that should preserve flow | ||
| host := baseURL.Host | ||
| path := baseURL.Path | ||
| scheme := baseURL.Scheme | ||
| opaquePart := baseURL.Opaque | ||
|
|
||
| // Reconstruct URL - flow should be preserved through field access | ||
| reconstructed := scheme + "://" + host + path | ||
| http.Get(reconstructed) // $ Alert | ||
|
|
||
| // Test individual fields | ||
| http.Redirect(w, req, "https://"+host+"/path", http.StatusFound) // $ Alert | ||
| w.Header().Set("Location", "https://example.com"+path) // $ Alert | ||
| http.Post(scheme+"://example.com/api", "application/json", nil) // $ Alert | ||
| use(opaquePart) // avoid unused variable warning | ||
|
|
||
| // Unsafe field accesses that should be sanitized by UnsafeFieldReadSanitizer | ||
| // These read unsafe URL fields and should NOT have flow | ||
| unsafeUser := baseURL.User // sanitizer edge (User field) | ||
| unsafeQuery := baseURL.RawQuery // sanitizer edge (RawQuery field) | ||
| unsafeFragment := baseURL.Fragment // sanitizer edge (Fragment field) | ||
|
|
||
| // These should NOT have flow due to sanitizer edges | ||
| if unsafeUser != nil { | ||
| http.Redirect(w, req, unsafeUser.String(), http.StatusFound) // no flow expected | ||
| } | ||
| w.Header().Set("Location", "https://example.com/?"+unsafeQuery) // no flow expected | ||
| http.Get("https://example.com/#" + unsafeFragment) // no flow expected | ||
| } | ||
|
|
||
| // Helper function to avoid unused variable warnings | ||
| func use(vars ...interface{}) {} | ||
15 changes: 15 additions & 0 deletions
15
go/ql/test/library-tests/semmle/go/security/SafeUrlFlow/SafeUrlFlow.ql
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,15 @@ | ||
| /** | ||
| * @id go/test-safe-url-flow | ||
| * @kind path-problem | ||
| * @problem.severity recommendation | ||
| */ | ||
|
|
||
| import go | ||
| import semmle.go.security.RequestForgeryCustomizations | ||
| import semmle.go.security.OpenUrlRedirectCustomizations | ||
| import semmle.go.security.SafeUrlFlow | ||
| import SafeUrlFlow::Flow::PathGraph | ||
|
|
||
| from SafeUrlFlow::Flow::PathNode source, SafeUrlFlow::Flow::PathNode sink | ||
| where SafeUrlFlow::Flow::flowPath(source, sink) | ||
| select sink.getNode(), source, sink, "A safe URL flows here from $@.", source.getNode(), "here" |
4 changes: 4 additions & 0 deletions
4
go/ql/test/library-tests/semmle/go/security/SafeUrlFlow/SafeUrlFlow.qlref
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,4 @@ | ||
| query: SafeUrlFlow.ql | ||
| postprocess: | ||
| - utils/test/PrettyPrintModels.ql | ||
| - utils/test/InlineExpectationsTestQuery.ql |
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.
All the wording about flow and alerts in this file leaves me confused. We're testing flow of things known to be safe, right? I.e. stuff that's subtracted from the actual query results? But the wording in a bunch of these comments seem to mix things up - e.g. by talking about a sanitizer that blocks flow, which then means that we don't get "safe" flow, so the sanitizer makes things unsafe? Or what's going on?
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.
Hmm, yes the terminology is confusing, since we're so used to a flow config tracking things that are unsafe, but in this case it is tracking something which is safe. I've removed uses of "unsafe" and tried to stick to flow terminology like "source", "sink", "propagate" and "barrier". Also, the tests were written by copilot, which tends to add lots of comments. I had already removed many redundant ones, but I've removed some more where they don't add any clarity. (And one or two were wrong.)
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.
I still find the comments confusing. Perhaps we could name all the sources something like
safeURL. And then instead of talking about "preserving flow" we should say that something "remains safe" or similar. And instead of talking about "not preserving flow" or "barriers" then we should call out that now the thing is no longer guaranteed to be safe (and possibly why). For instance, it's somewhat counterintuitive that a string can be safe, but taking a substring makes it not safe.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.
How about now?
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.
Much better!