From babad0941a211859225ff980b79b3f0ee63f1144 Mon Sep 17 00:00:00 2001 From: Sean Smith Date: Mon, 19 Aug 2024 12:36:30 -0500 Subject: [PATCH 1/4] Fixes a few issues with redirects First, this prevents a DNS lookup from happening when we encounter a redirect, *even if we don't intend to follow it*. This likely addresses some part of #452 Second, if we aren't following redirects, don't have the scan fail in an 'application-error'. We are succeeding in what we intended to do, which is to scan without following redirects --- modules/http/scanner.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/modules/http/scanner.go b/modules/http/scanner.go index a124b306..b63aba3c 100644 --- a/modules/http/scanner.go +++ b/modules/http/scanner.go @@ -388,6 +388,12 @@ func redirectsToLocalhost(host string) bool { // the redirectToLocalhost and MaxRedirects config func (scan *scan) getCheckRedirect() func(*http.Request, *http.Response, []*http.Request) error { return func(req *http.Request, res *http.Response, via []*http.Request) error { + if scan.scanner.config.MaxRedirects == 0 { + return nil + } + if len(via) > scan.scanner.config.MaxRedirects { + return ErrTooManyRedirects + } if !scan.scanner.config.FollowLocalhostRedirects && redirectsToLocalhost(req.URL.Hostname()) { return ErrRedirLocalhost } @@ -413,10 +419,6 @@ func (scan *scan) getCheckRedirect() func(*http.Request, *http.Response, []*http } } - if len(via) > scan.scanner.config.MaxRedirects { - return ErrTooManyRedirects - } - return nil } } From 04c99a37cd487c9be3e7a0933bdbcea779468fda Mon Sep 17 00:00:00 2001 From: Sean Smith Date: Mon, 19 Aug 2024 13:58:46 -0500 Subject: [PATCH 2/4] Make sure to check redirects before we loop through and parse the host Properly handle no redirects wanted to return success --- lib/http/client.go | 11 +++++++++++ modules/http/scanner.go | 6 +++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/lib/http/client.go b/lib/http/client.go index f7b05c51..b9340b61 100644 --- a/lib/http/client.go +++ b/lib/http/client.go @@ -614,6 +614,17 @@ func (c *Client) Do(req *Request) (resp *Response, err error) { } return nil, uerr(err) } + err = c.checkRedirect(req, resp, reqs) + + // Sentinel error to let users select the + // previous response, without closing its + // body. See Issue 10069. + if err == ErrUseLastResponse { + return resp, nil + } + if err != nil { + return resp, err + } var shouldRedirect bool redirectMethod, shouldRedirect, includeBody = redirectBehavior(req.Method, resp, reqs[0]) diff --git a/modules/http/scanner.go b/modules/http/scanner.go index b63aba3c..14452813 100644 --- a/modules/http/scanner.go +++ b/modules/http/scanner.go @@ -37,6 +37,8 @@ var ( // ErrTooManyRedirects is returned when the number of HTTP redirects exceeds // MaxRedirects. ErrTooManyRedirects = errors.New("Too many redirects") + + ErrDoNotRedirect = errors.New("No redirects configured") ) // Flags holds the command-line configuration for the HTTP scan module. @@ -389,7 +391,7 @@ func redirectsToLocalhost(host string) bool { func (scan *scan) getCheckRedirect() func(*http.Request, *http.Response, []*http.Request) error { return func(req *http.Request, res *http.Response, via []*http.Request) error { if scan.scanner.config.MaxRedirects == 0 { - return nil + return ErrDoNotRedirect } if len(via) > scan.scanner.config.MaxRedirects { return ErrTooManyRedirects @@ -531,6 +533,8 @@ func (scan *scan) Grab() *zgrab2.ScanError { } if err != nil { switch err { + case ErrDoNotRedirect: + break case ErrRedirLocalhost: break case ErrTooManyRedirects: From ff0bf062f526f8f4799b60b6a9cc48f8d6b91b74 Mon Sep 17 00:00:00 2001 From: Sean Smith Date: Mon, 19 Aug 2024 14:12:14 -0500 Subject: [PATCH 3/4] Handle 0 indexing --- modules/http/scanner.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/http/scanner.go b/modules/http/scanner.go index 14452813..6ebf5ed8 100644 --- a/modules/http/scanner.go +++ b/modules/http/scanner.go @@ -393,7 +393,8 @@ func (scan *scan) getCheckRedirect() func(*http.Request, *http.Response, []*http if scan.scanner.config.MaxRedirects == 0 { return ErrDoNotRedirect } - if len(via) > scan.scanner.config.MaxRedirects { + //len-1 because otherwise we'll return a failure on 1 redirect when we specify only 1 redirect. I.e. we are 0 + if len(via)-1 > scan.scanner.config.MaxRedirects { return ErrTooManyRedirects } if !scan.scanner.config.FollowLocalhostRedirects && redirectsToLocalhost(req.URL.Hostname()) { From 5d0e6c96300870fb541dc9da10c4ef0312e2c5ab Mon Sep 17 00:00:00 2001 From: Sean Smith Date: Mon, 19 Aug 2024 14:12:52 -0500 Subject: [PATCH 4/4] Pull out the original checkRedirectCode so we deal with consistently --- lib/http/client.go | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/lib/http/client.go b/lib/http/client.go index b9340b61..283ecbf8 100644 --- a/lib/http/client.go +++ b/lib/http/client.go @@ -534,8 +534,9 @@ func (c *Client) Do(req *Request) (resp *Response, err error) { for { // For all but the first request, create the next // request hop and replace req. + loc := req.URL.String() if len(reqs) > 0 { - loc := resp.Header.Get("Location") + loc = resp.Header.Get("Location") if loc == "" { return nil, uerr(fmt.Errorf("%d response missing Location header", resp.StatusCode)) } @@ -571,14 +572,6 @@ func (c *Client) Do(req *Request) (resp *Response, err error) { if ref := refererForURL(reqs[len(reqs)-1].URL, req.URL); ref != "" { req.Header.Set("Referer", ref) } - err = c.checkRedirect(req, resp, reqs) - - // Sentinel error to let users select the - // previous response, without closing its - // body. See Issue 10069. - if err == ErrUseLastResponse { - return resp, nil - } // Close the previous response's body. But // read at least some of the body so if it's @@ -590,16 +583,6 @@ func (c *Client) Do(req *Request) (resp *Response, err error) { io.CopyN(ioutil.Discard, resp.Body, maxBodySlurpSize) } resp.Body.Close() - - if err != nil { - // Special case for Go 1 compatibility: return both the response - // and an error if the CheckRedirect function failed. - // See https://golang.org/issue/3795 - // The resp.Body has already been closed. - ue := uerr(err) - ue.(*url.Error).URL = loc - return resp, ue - } } reqs = append(reqs, req) @@ -623,6 +606,11 @@ func (c *Client) Do(req *Request) (resp *Response, err error) { return resp, nil } if err != nil { + // Special case for Go 1 compatibility: return both the response + // and an error if the CheckRedirect function failed. + // See https://golang.org/issue/3795 + ue := uerr(err) + ue.(*url.Error).URL = loc return resp, err }