Validate that the "/web/sso_confirm" redirect URL is only used for MFA ceremonies #56006
Conversation
…so/redirector.go * Replace binary ssoTestFlow boolean with ceremony type to incorporate MFA ceremonies * Validate that "/web/sso_confirm" is only used for MFA ceremonies
Joerger
added
the
no-changelog
| // For non test ceremonies, If the "insecure_allowed_cidr_ranges" list is non-empty, URLs in both the | ||
| // "http" and "https" schema are allowed if the hostname is an IP address that is contained in a | ||
| // specified CIDR range on any port. | ||
| func ValidateClientRedirect(clientRedirect string, ceremonyType CeremonyType, settings *types.SSOClientRedirectSettings) error { |
There was a problem hiding this comment.
This is a security-critical function. Can we add more comments (maybe inside the body of the function) to warn future devs that this function is our main protection against open redirect vulnerabilities?
There was a problem hiding this comment.
Sure, adding this comment at the top of the function:
// Warning to developers and reviewers: this validation function is critical to sso security
// and any changes to it should be carefully considered from a vulnerability point of view.
| } | ||
| if q, err := url.ParseQuery(u.RawQuery); err != nil { | ||
| return trace.Wrap(err, "parsing query in client redirect URL") | ||
| } else if len(q) != 1 || len(q["secret_key"]) != 1 { |
There was a problem hiding this comment.
Is it correct that the secret_key query parameter doesn't need to be validated for SSO MFA?
There was a problem hiding this comment.
Yes, the secret_key is not used for the web SSO MFA flow. For non web SSO flows, the secret_key is required on the server side to encode the response:
Lines 2333 to 2348 in e71690a
If a non-web client tried to abuse this by setting the path to /web/sso_confirm and not setting a secret_key, it would fail thanks to the previous fix enforcing the use of a relative URL on the server side.
lib/client/sso/redirector.go
Outdated
| return trace.BadParameter("the web mfa redirect path, \"/web/sso_confirm\", cannot be used for non-MFA flows") | ||
| } | ||
| if u.IsAbs() { | ||
| return trace.BadParameter("invalid scheme in client redirect URL for SSO MFA") |
There was a problem hiding this comment.
| return trace.BadParameter("invalid scheme in client redirect URL for SSO MFA") | |
| return trace.BadParameter("redirects for SSO MFA must be relative") |
Or something similar?
There was a problem hiding this comment.
Looks like I failed to address this comment, taking that suggestion instead.
Follow up to #55398
This change does not have any known security impact, but it adds some better partitioning between SSO ceremony types to ensure that using the Web MFA specific redirect URL "/web/sso_confirm" can not even be attempted outside of MFA flows.
I also moved the function out of
lib/auth/github.gotolib/client/sso/redirector.gosince it is not github specific, and updated the comment for readability.