Skip to content
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.

Sign up for GitHub

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

fix(sso): respect X-Forwarded-* headers for redirect URL. Fixes #13031 #13609

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix(sso): respect X-Forwarded-* headers for redirect URL. Fixes #13031 #13609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
649d561
fix(sso): respect X-Forwarded-* headers for redirect URL. Fixes #13031
MasonM Sep 16, 2024
a6effd5
Merge branch 'main' into fix-13031
MasonM Jan 5, 2025
11844f8
Merge branch 'main' into fix-13031
MasonM Jan 7, 2025
bd66afd
Merge branch 'main' into fix-13031
MasonM Jan 9, 2025
c4fb5f3
Merge branch 'main' into fix-13031
MasonM Jan 10, 2025
85530de
Merge branch 'main' into fix-13031
MasonM Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions server/auth/sso/sso.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,11 +342,7 @@ func (s *sso) HandleCallback(w http.ResponseWriter, r *http.Request) {
})
redirect := s.baseHRef

proto := "http"
if s.secure {
proto = "https"
}
prefix := fmt.Sprintf("%s://%s%s", proto, r.Host, s.baseHRef)
prefix := s.getRedirectUrlPrefix(r)

if strings.HasPrefix(cookie.Value, prefix) {
redirect = cookie.Value
Expand Down Expand Up @@ -376,14 +372,26 @@ func (s *sso) getRedirectUrl(r *http.Request) string {
if s.config.RedirectURL != "" {
return s.config.RedirectURL
}
return fmt.Sprintf("%soauth2/callback", s.getRedirectUrlPrefix(r))
}

func (s *sso) getRedirectUrlPrefix(r *http.Request) string {
proto := "http"

if r.URL.Scheme != "" {
// Get protocol from the X-Forwarded-Proto header if set to support running Argo behind a TLS termination proxy
if r.Header.Get("X-Forwarded-Proto") != "" {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my main concern with changing configuration based on proxy is that it becomes susceptible to MITMs or similar -- anyone can send such a header.

Whereas redirectURL is configured by an admin

Copy link

@agilgur5 agilgur5 Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this was previously using the request URL (r.URL.Scheme), which I believe could be similarly spoofed 🤔 .
HandleCallback wasn't using that though, but was using r.Host, although that's technically not exactly the same as the host header in Go (related #12691)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but that was already an inherent problem with this setup, because if you're running a TLS termination in front of Argo, you have to trust everything in between you and the proxy (including the proxy). The only way of protecting against a malicious proxy is through E2E encryption.

Also, if someone does MITM the connection and has the ability to inject arbitrary headers, then they almost certainly have the ability to completely rewrite the response body too, in which case there's no need for them to redirect the user to a malicious page. They can just directly serve up whatever content they want,.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the specific setup of the issue, yes, but this potentially extends it further too. Even with E2EE, a maliciously crafted request could potentially send someone to an uncontrolled/malicious URL, similar to an open redirect.

Copy link
Member Author

@MasonM MasonM Oct 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even with E2EE, a maliciously crafted request could potentially send someone to an uncontrolled/malicious URL, similar to an open redirect.

I can't think of any attack vector where that can happen that wasn't already a viable attack vector. If E2E encryption is used, the only means for an attacker to inject arbitrary headers is to either compromise the Argo server or the end user's device, and in both cases the attacker isn't going to bother with open redirect vulnerabilities because they can just rewrite the page to be whatever they want.

Regardless, I entered #13747 with an alternative approach: use relative redirect URLs. That sidesteps the whole issue.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If E2E encryption is used, the only means for an attacker to inject arbitrary headers is to either compromise the Argo server or the end user's device

The simplest and one of the most common variants is to, for instance, write an informative blog post that sends requests to the API, and one of those requests is maliciously crafted. The user then copy+pastes it unknowingly.

In CVSS, this kind of exploit requires "User Interaction", but does not require compromising the user's device

Copy link
Member Author

@MasonM MasonM Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm still not seeing how an attacker could inject arbitrary headers. I know an attacker can control the ?redirect= parameter via something like https://localhost:8080/oauth2/redirect?redirect=https://evil.com, but that's useless if they can't control the request headers too. Here's what would happen if an attacker were to try to pull this off:

  1. Attacker tricks user into visiting https://localhost:8080/oauth2/redirect?redirect=https://evil.com
  2. Argo stores https://evil.com in the cookie associated with the state parameter and redirects the user to the OAuth2 provider
  3. User logs in
  4. OAuth2 provider redirects user to https://localhost:8080/oauth2/callback
  5. Argo reads the cookie and rejects https://evil.com because it doesn't match request headers, so it redirects them to https://localhost:8080/

Even if the attacker can execute Javascript and send AJAX requests, the CORS policy won't allow it. Try running this in a DevTools console at https://example.com :

fetch('https://localhost:8080/oauth2/redirect?redirect=https://evil.com', {headers: {"X-Forwarded-Host": "evil.com"}})

You should see an error like this:

Access to fetch at 'https://localhost:8080/oauth2/redirect?redirect=https://evil.com' from origin 'https://example.com' has been blocked by CORS policy: Response to preflight request doesn't pass access control check: Redirect is not allowed for a preflight request.

Can you give a concrete example of the kind of attack you're thinking of?

proto = r.Header.Get("X-Forwarded-Proto")
} else if r.URL != nil && r.URL.Scheme != "" {
proto = r.URL.Scheme
} else if s.secure {
proto = "https"
}

return fmt.Sprintf("%s://%s%soauth2/callback", proto, r.Host, s.baseHRef)
// As above, get host from X-Forwarded-Host if set to support proxies
host := r.Header.Get("X-Forwarded-Host")
if host == "" {
host = r.Host
}

return fmt.Sprintf("%s://%s%s", proto, host, s.baseHRef)
}
80 changes: 80 additions & 0 deletions server/auth/sso/sso_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package sso

import (
"context"
"net/http"
"testing"
"time"

Expand Down Expand Up @@ -154,3 +155,82 @@ func TestGetSessionExpiry(t *testing.T) {
}
assert.Equal(t, 5*time.Hour, config.GetSessionExpiry())
}

func TestGetRedirectUrlPrefix(t *testing.T) {
tests := []struct {
name string
secure bool
baseHRef string
request *http.Request
expectedPrefix string
}{
{
name: "secure with X-Forwarded-Proto and X-Forwarded-Host",
secure: true,
baseHRef: "/",
request: &http.Request{
Header: http.Header{
"X-Forwarded-Proto": []string{"https"},
"X-Forwarded-Host": []string{"example.com"},
},
Host: "example.com",
},
expectedPrefix: "https://example.com/",
},
{
name: "insecure with X-Forwarded-Proto and X-Forwarded-Host",
secure: false,
baseHRef: "/",
request: &http.Request{
Header: http.Header{
"X-Forwarded-Proto": []string{"http"},
"X-Forwarded-Host": []string{"example.com"},
},
Host: "example.com",
},
expectedPrefix: "http://example.com/",
},
{
name: "secure without X-Forwarded-*",
secure: true,
baseHRef: "/",
request: &http.Request{
Host: "example.com",
},
expectedPrefix: "https://example.com/",
},
{
name: "insecure without X-Forwarded-*",
secure: false,
baseHRef: "/",
request: &http.Request{
Host: "example.com",
},
expectedPrefix: "http://example.com/",
},
{
name: "with baseHRef",
secure: true,
baseHRef: "/base/",
request: &http.Request{
Header: http.Header{
"X-Forwarded-Proto": []string{"https"},
"X-Forwarded-Host": []string{"example.com"},
},
Host: "example.com",
},
expectedPrefix: "https://example.com/base/",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
s := &sso{
secure: tt.secure,
baseHRef: tt.baseHRef,
}
prefix := s.getRedirectUrlPrefix(tt.request)
assert.Equal(t, tt.expectedPrefix, prefix)
})
}
}