diff --git a/CHANGELOG.md b/CHANGELOG.md index ee00fd21..9b3ccea0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ - [#2803](https://github.com/oauth2-proxy/oauth2-proxy/pull/2803) fix: self signed certificate handling in v7.7.0 (@tuunit) - [#2619](https://github.com/oauth2-proxy/oauth2-proxy/pull/2619) fix: unable to use hyphen in JSON path for oidc-groups-claim option (@rd-danny-fleer) +- [#2311](https://github.com/oauth2-proxy/oauth2-proxy/pull/2311) fix: runtime error: index out of range (0) with length 0 (@miguelborges99 / @tuunit) # V7.7.0 diff --git a/oauthproxy.go b/oauthproxy.go index 2aec2078..c5a5928b 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -870,9 +870,22 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { return } - csrf, err := cookies.LoadCSRFCookie(req, p.CookieOptions) + nonce, appRedirect, err := decodeState(req.Form.Get("state"), p.encodeState) if err != nil { - logger.Println(req, logger.AuthFailure, "Invalid authentication via OAuth2. Error while loading CSRF cookie:", err.Error()) + logger.Errorf("Error while parsing OAuth2 state: %v", err) + p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) + return + } + + // calculate the cookie name + cookieName := cookies.GenerateCookieName(p.CookieOptions, nonce) + // Try to find the CSRF cookie and decode it + csrf, err := cookies.LoadCSRFCookie(req, cookieName, p.CookieOptions) + if err != nil { + // There are a lot of issues opened complaining about missing CSRF cookies. + // Try to log the INs and OUTs of OAuthProxy, to be easier to analyse these issues. + LoggingCSRFCookiesInOAuthCallback(req, cookieName) + logger.Println(req, logger.AuthFailure, "Invalid authentication via OAuth2: unable to obtain CSRF cookie: %s (state=%s)", err, nonce) p.ErrorPage(rw, req, http.StatusForbidden, err.Error(), "Login Failed: Unable to find a valid CSRF token. Please try again.") return } @@ -893,13 +906,6 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { csrf.ClearCookie(rw, req) - nonce, appRedirect, err := decodeState(req.Form.Get("state"), p.encodeState) - if err != nil { - logger.Errorf("Error while parsing OAuth2 state: %v", err) - p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error()) - return - } - if !csrf.CheckOAuthState(nonce) { logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: CSRF token mismatch, potential attack") p.ErrorPage(rw, req, http.StatusForbidden, "CSRF token mismatch, potential attack", "Login Failed: Unable to find a valid CSRF token. Please try again.") @@ -1297,3 +1303,27 @@ func (p *OAuthProxy) errorJSON(rw http.ResponseWriter, code int) { // application/json rw.Write([]byte("{}")) } + +// LoggingCSRFCookiesInOAuthCallback Log all CSRF cookies found in HTTP request OAuth callback, +// which were successfully parsed +func LoggingCSRFCookiesInOAuthCallback(req *http.Request, cookieName string) { + cookies := req.Cookies() + if len(cookies) == 0 { + logger.Println(req, logger.AuthFailure, "No cookies were found in OAuth callback.") + return + } + + for _, c := range cookies { + if cookieName == c.Name { + logger.Println(req, logger.AuthFailure, "CSRF cookie %s was found in OAuth callback.", c.Name) + return + } + + if strings.HasSuffix(c.Name, "_csrf") { + logger.Println(req, logger.AuthFailure, "CSRF cookie %s was found in OAuth callback, but it is not the expected one (%s).", c.Name, cookieName) + return + } + } + + logger.Println(req, logger.AuthFailure, "Cookies were found in OAuth callback, but none was a CSRF cookie.") +} diff --git a/pkg/cookies/csrf.go b/pkg/cookies/csrf.go index 284717dd..b1f95763 100644 --- a/pkg/cookies/csrf.go +++ b/pkg/cookies/csrf.go @@ -72,9 +72,7 @@ func NewCSRF(opts *options.Cookie, codeVerifier string) (CSRF, error) { } // LoadCSRFCookie loads a CSRF object from a request's CSRF cookie -func LoadCSRFCookie(req *http.Request, opts *options.Cookie) (CSRF, error) { - cookieName := GenerateCookieName(req, opts) - +func LoadCSRFCookie(req *http.Request, cookieName string, opts *options.Cookie) (CSRF, error) { cookies := req.Cookies() for _, cookie := range cookies { if cookie.Name != cookieName { @@ -89,17 +87,17 @@ func LoadCSRFCookie(req *http.Request, opts *options.Cookie) (CSRF, error) { return csrf, nil } - return nil, errors.New("CSRF cookie not found") + return nil, fmt.Errorf("CSRF cookie with name '%v' was not found", cookieName) } // GenerateCookieName in case cookie options state that CSRF cookie has fixed name then set fixed name, otherwise // build name based on the state -func GenerateCookieName(req *http.Request, opts *options.Cookie) string { +func GenerateCookieName(opts *options.Cookie, state string) string { stateSubstring := "" if opts.CSRFPerRequest { // csrfCookieName will include a substring of the state to enable multiple csrf cookies // in case of parallel requests - stateSubstring = ExtractStateSubstring(req) + stateSubstring = ExtractStateSubstring(state) } return csrfCookieName(opts, stateSubstring) } @@ -218,20 +216,15 @@ func csrfCookieName(opts *options.Cookie, stateSubstring string) string { if stateSubstring == "" { return fmt.Sprintf("%v_csrf", opts.Name) } - return fmt.Sprintf("%v_csrf_%v", opts.Name, stateSubstring) + return fmt.Sprintf("%v_%v_csrf", opts.Name, stateSubstring) } // ExtractStateSubstring extract the initial state characters, to add it to the CSRF cookie name -func ExtractStateSubstring(req *http.Request) string { +func ExtractStateSubstring(state string) string { lastChar := csrfStateLength - 1 stateSubstring := "" - - state := req.URL.Query()["state"] - if state[0] != "" { - state := state[0] - if lastChar <= len(state) { - stateSubstring = state[0:lastChar] - } + if lastChar <= len(state) { + stateSubstring = state[0:lastChar] } return stateSubstring } diff --git a/pkg/cookies/csrf_test.go b/pkg/cookies/csrf_test.go index 09d0d45a..8b22e408 100644 --- a/pkg/cookies/csrf_test.go +++ b/pkg/cookies/csrf_test.go @@ -19,6 +19,7 @@ var _ = Describe("CSRF Cookie Tests", func() { cookieOpts *options.Cookie publicCSRF CSRF privateCSRF *csrf + csrfName string ) BeforeEach(func() { @@ -39,6 +40,7 @@ var _ = Describe("CSRF Cookie Tests", func() { Expect(err).ToNot(HaveOccurred()) privateCSRF = publicCSRF.(*csrf) + csrfName = GenerateCookieName(cookieOpts, csrfNonce) }) Context("NewCSRF", func() { @@ -175,7 +177,7 @@ var _ = Describe("CSRF Cookie Tests", func() { }) It("should return error when no cookie is set", func() { - csrf, err := LoadCSRFCookie(req, cookieOpts) + csrf, err := LoadCSRFCookie(req, csrfName, cookieOpts) Expect(err).To(HaveOccurred()) Expect(csrf).To(BeNil()) }) @@ -191,7 +193,7 @@ var _ = Describe("CSRF Cookie Tests", func() { Value: encoded, }) - csrf, err := LoadCSRFCookie(req, cookieOpts) + csrf, err := LoadCSRFCookie(req, csrfName, cookieOpts) Expect(err).ToNot(HaveOccurred()) Expect(csrf).ToNot(BeNil()) }) @@ -202,7 +204,7 @@ var _ = Describe("CSRF Cookie Tests", func() { Value: "invalid", }) - csrf, err := LoadCSRFCookie(req, cookieOpts) + csrf, err := LoadCSRFCookie(req, csrfName, cookieOpts) Expect(err).To(HaveOccurred()) Expect(csrf).To(BeNil()) }) @@ -223,7 +225,7 @@ var _ = Describe("CSRF Cookie Tests", func() { Value: encoded, }) - csrf, err := LoadCSRFCookie(req, cookieOpts) + csrf, err := LoadCSRFCookie(req, csrfName, cookieOpts) Expect(err).ToNot(HaveOccurred()) Expect(csrf).ToNot(BeNil()) })