Prevent SSO Redirects to other origins #33853

jentfoo · 2023-10-23T23:11:39Z

This change fixes https://github.com/gravitational/security-findings/issues/69

Through the url.Parse and then .RequestURI() it is expected to be a path which will redirect to the same origin. However there are potential patterns that customers could be phished to inject which would result in a redirect to another domain. This is done after the auth process, as such this would NOT result in secrets being sent to the attacker controlled domain.

In this change I removed the unused SafeRedirect function and instead added a new function OriginLocalRedirectURI which will ensure that the provided URI's will remain Origin local after being interpreted by the browser.

github-actions · 2023-10-23T23:12:12Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

github-actions · 2023-10-23T23:12:16Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

github-actions · 2023-10-23T23:12:18Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

github-actions · 2023-10-23T23:12:19Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

zmb3 · 2023-10-24T16:04:46Z

@jentfoo this one seems worthy of a changelog entry don't you think? It's a behavior change and a security improvement, both of which are things I would think users want to know about.

lib/httplib/httplib_test.go

zmb3 · 2023-10-24T16:10:14Z

lib/httplib/httplib.go

-// SafeRedirect performs a relative redirect to the URI part of the provided redirect URL
-func SafeRedirect(w http.ResponseWriter, r *http.Request, redirectURL string) error {
+// OriginLocalRedirectURI will take an incoming URL including optionally the host and schema and provide the URI
+// associated with the URL.  Additionally, it will ensure that the URI does not include any techniques potentially


On first read I was very confused by what "the URI associated with the URL" meant.

Would it make more sense to call this function something like ExtractPath?

Unclear to me if ExtractPath is a better name. This specifically is trying to extract the path in a way that is safe for redirects when the origin must remain consistent. So I tried to have the word Redirect in it (since that's the only known use case), and I tried to make sure that the Origin concept was also in the name.

ExtractPath sounds too generic IMO, and does not indicate the additional checks put here.

ExtractOriginLocalSafePath I think could be a good option, but I don't feel strongly about it. Do you like that naming at all?

Hmm, yeah I see that it's doing quite a bit more than that.

I wonder if httplib is the right place for this - it seems to be a pretty specialized function written specifically to handle SSO redirects. Should it be located closer to the code that handles SSO?

No strong preference on my end. I put it in httplib because theoretically this is useful for any origin local redirects (though SSO was the only place I could find where we currently need this). I suspect this fairly generic quality is why the prior (unused) SafeRedirect also lived in httplib

lib/httplib/httplib.go

jentfoo · 2023-10-24T17:35:29Z

this one seems worthy of a changelog entry don't you think? It's a behavior change and a security improvement, both of which are things I would think users want to know about.

@zmb3 I am not sure, I would like to hear your perspective. Here is why I was thinking we would not want a changelog:

The behavior change requires manual effort from the user to experience, so nothing that should be experienced in a normal case
You are correct that there is a low severity phishing risk here, but I was not sure that the improvement was worthy for a changelog. I view these other recent security improvements as being more significant (since they don't require a phishing attack):
Enforce body size limits for http responses #33768
Secure File Removal Improvements #32260
Careful handling when loading files #31721
MFA Fuzzing and Json.Unmarshal Panic fix #30854

My concern is that these improvements are very slight, and I am not sure that customers would be interested in them. However I have no objection to adding them to the changelog. I just wanted to highlight that there are a range of low severity fixes we have not been elevating yet. Let me know your thoughts.

camscale

I'm still trying to grok where the opaque part of the URL comes in, from the comment on the linked issue. I see how an opaque element is turned into a URL with a different hostname, but I don't see how the example exploit has an opaque part. But I'll keep thinking on it and publish these comments for now...

lib/httplib/httplib.go

lib/httplib/httplib_test.go

camscale · 2023-10-23T23:49:11Z

lib/httplib/httplib_test.go

+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			result, err := OriginLocalRedirectURI(tc.input)
+			require.Equal(t, tc.expected, result)


it is not usual for the non-error parameter to be relevant when there is an error result. I'd suggest moving this into the non-error if block, and remove the expected: "" fields for the error cases in the table (both since it is the zero value so does not need to be initialised but also because it is not relevant).

if you want the returned URI to be the empty string in error cases, I'd document that in the function's doc comment.

camscale · 2023-10-23T23:55:08Z

lib/httplib/httplib.go

 	parsedURL, err := url.Parse(redirectURL)
 	if err != nil {
-		return trace.Wrap(err)
+		return "", trace.Wrap(err)
+	} else if parsedURL.IsAbs() && (parsedURL.Scheme != "http" && parsedURL.Scheme != "https") {


nit: the parsedURL.IsAbs() check is redundant since the empty string for Scheme will fail the other checks.

This is actually not redundant. It's possible for a non-absolute url to be provided, one which has no scheme. Removing this will cause some of the existing test cases to fail.

yeah my mistake

camscale

LGTM. I now understand what's happening - the link to the opaque URL handling is a misdirection. The exploit works by having Teleport send an HTML meta redirect containing:

<meta http-equiv="refresh" content="0;URL='//example.com/'" />

and it is the browser that is redirecting to example.com. Nowhere in the Teleport source is it hitting https://github.com/golang/go/blob/c65f74d339169a5597c64a0076c17905c85b37d8/src/net/url/url.go#L1136 as described in the linked ticket.

As an aside, I could find no way to have the Chrome inspector show the HTML body of a page that does a meta refresh redirect - it's just gone. I could only verify exactly what Teleport was returning to the browser by adding some logging to the source.

However, I have reproduced the exploit locally and re-run it with the changes in this PR and verified that it does stop the exploit. So that's a LGTM from me.

camscale · 2023-10-30T07:05:01Z

lib/httplib/httplib.go

 	parsedURL, err := url.Parse(redirectURL)
 	if err != nil {
-		return trace.Wrap(err)
+		return "", trace.Wrap(err)
+	} else if parsedURL.IsAbs() && (parsedURL.Scheme != "http" && parsedURL.Scheme != "https") {


yeah my mistake

public-teleport-github-review-bot · 2023-10-31T15:47:42Z

@jentfoo See the table below for backport results.

Branch	Result
branch/v12	Create PR
branch/v13	Create PR
branch/v14	Create PR

Prevent SSO Redirects to other origins

78e6039

jentfoo added backport/branch/v12 backport/branch/v13 backport/branch/v14 labels Oct 23, 2023

jentfoo requested review from zmb3, ibeckermayer, camscale, rosstimothy and kimlisa October 23, 2023 23:11

jentfoo self-assigned this Oct 23, 2023

github-actions bot requested a review from jimbishopp October 23, 2023 23:12

github-actions bot added the size/sm label Oct 23, 2023

jentfoo added the no-changelog Indicates that a PR does not require a changelog entry label Oct 23, 2023

zmb3 reviewed Oct 24, 2023

View reviewed changes

Apply PR feedback

028652b

ibeckermayer approved these changes Oct 25, 2023

View reviewed changes

camscale reviewed Oct 25, 2023

View reviewed changes

Apply PR feedback on httplib redirect parsing

e148126

camscale approved these changes Oct 30, 2023

View reviewed changes

zmb3 approved these changes Oct 31, 2023

View reviewed changes

public-teleport-github-review-bot bot removed request for jimbishopp, rosstimothy and kimlisa October 31, 2023 15:24

jentfoo added this pull request to the merge queue Oct 31, 2023

Merged via the queue into master with commit f6cde21 Oct 31, 2023
33 checks passed

jentfoo deleted the jent/sso_redirect_check branch October 31, 2023 15:45

This was referenced Oct 31, 2023

[v12] Prevent SSO Redirects to other origins #34075

Merged

[v13] Prevent SSO Redirects to other origins #34076

Merged

[v14] Prevent SSO Redirects to other origins #34077

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent SSO Redirects to other origins #33853

Prevent SSO Redirects to other origins #33853

jentfoo commented Oct 23, 2023

github-actions bot commented Oct 23, 2023

github-actions bot commented Oct 23, 2023

github-actions bot commented Oct 23, 2023

github-actions bot commented Oct 23, 2023

zmb3 commented Oct 24, 2023

zmb3 Oct 24, 2023

jentfoo Oct 24, 2023

zmb3 Oct 24, 2023

jentfoo Oct 24, 2023

jentfoo commented Oct 24, 2023 •

edited

camscale left a comment

camscale Oct 23, 2023

camscale Oct 23, 2023

jentfoo Oct 25, 2023

camscale Oct 30, 2023

camscale left a comment

camscale Oct 30, 2023

public-teleport-github-review-bot bot commented Oct 31, 2023

Prevent SSO Redirects to other origins #33853

Prevent SSO Redirects to other origins #33853

Conversation

jentfoo commented Oct 23, 2023

github-actions bot commented Oct 23, 2023

github-actions bot commented Oct 23, 2023

github-actions bot commented Oct 23, 2023

github-actions bot commented Oct 23, 2023

zmb3 commented Oct 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jentfoo commented Oct 24, 2023 • edited

camscale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camscale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

public-teleport-github-review-bot bot commented Oct 31, 2023

jentfoo commented Oct 24, 2023 •

edited