mirror of
https://github.com/github/codeql.git
synced 2026-04-18 13:34:02 +02:00
Go: Improve QHelp for go/unvalidated-url-redirection.
The example showed a different (and better) fix from what the help claimed, but the suggestion also had a subtle bug that I fixed at the same time.
This commit is contained in:
@@ -16,6 +16,10 @@ To guard against untrusted URL redirection, it is advisable to avoid putting use
|
||||
a redirect URL. Instead, maintain a list of authorized redirects on the server; then choose from
|
||||
that list based on the user input provided.
|
||||
</p>
|
||||
<p>
|
||||
If this is not possible, then the user input should be validated in some other way,
|
||||
for example, by verifying that the target URL is local and does not redirect to a different host.
|
||||
</p>
|
||||
</recommendation>
|
||||
|
||||
<example>
|
||||
@@ -27,11 +31,17 @@ validating the input, which facilitates phishing attacks:
|
||||
<sample src="OpenUrlRedirect.go"/>
|
||||
|
||||
<p>
|
||||
One way to remedy the problem is to validate the user input against a known fixed string
|
||||
before doing the redirection:
|
||||
One way to remedy the problem is to parse the target URL and check that its hostname is empty,
|
||||
which means that it is a relative URL:
|
||||
</p>
|
||||
|
||||
<sample src="OpenUrlRedirectGood.go"/>
|
||||
|
||||
<p>
|
||||
Note that some browsers treat backslashes in URLs as forward slashes. To account for this,
|
||||
we replace all backslashes with forward slashes before parsing the URL and checking its hostname.
|
||||
</p>
|
||||
|
||||
</example>
|
||||
|
||||
<references>
|
||||
|
||||
@@ -3,21 +3,26 @@ package main
|
||||
import (
|
||||
"net/http"
|
||||
"net/url"
|
||||
"strings"
|
||||
)
|
||||
|
||||
func serve() {
|
||||
func serve1() {
|
||||
http.HandleFunc("/redir", func(w http.ResponseWriter, r *http.Request) {
|
||||
r.ParseForm()
|
||||
target, err := url.Parse(r.Form.Get("target"))
|
||||
targetUrl := r.Form.Get("target")
|
||||
// replace all backslashes with forward slashes before parsing the URL
|
||||
targetUrl = strings.ReplaceAll(targetUrl, "\\", "/")
|
||||
|
||||
target, err := url.Parse(targetUrl)
|
||||
if err != nil {
|
||||
// ...
|
||||
}
|
||||
|
||||
if target.Hostname() == "semmle.com" {
|
||||
// GOOD: checking hostname
|
||||
if target.Hostname() == "" {
|
||||
// GOOD: check that it is a local redirect
|
||||
http.Redirect(w, r, target.String(), 302)
|
||||
} else {
|
||||
http.WriteHeader(400)
|
||||
w.WriteHeader(400)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
@@ -3,17 +3,23 @@ package main
|
||||
import (
|
||||
"net/http"
|
||||
"net/url"
|
||||
"strings"
|
||||
)
|
||||
|
||||
func serve1() {
|
||||
http.HandleFunc("/redir", func(w http.ResponseWriter, r *http.Request) {
|
||||
r.ParseForm()
|
||||
target, err := url.Parse(r.Form.Get("target"))
|
||||
targetUrl := r.Form.Get("target")
|
||||
// replace all backslashes with forward slashes before parsing the URL
|
||||
targetUrl = strings.ReplaceAll(targetUrl, "\\", "/")
|
||||
|
||||
target, err := url.Parse(targetUrl)
|
||||
if err != nil {
|
||||
// ...
|
||||
}
|
||||
if target.Hostname() == "semmle.com" {
|
||||
// GOOD: checking hostname
|
||||
|
||||
if target.Hostname() == "" {
|
||||
// GOOD: check that it is a local redirect
|
||||
http.Redirect(w, r, target.String(), 302)
|
||||
} else {
|
||||
w.WriteHeader(400)
|
||||
|
||||
Reference in New Issue
Block a user