Open Relay in SLO

[Bandes]

Hi - our pentesters found what they describe as an Open Relay in our SLO process, which is handled by omniauth-saml.

On our application this can be reproduced like so:

https://[OUR_APPLICATION]/users/auth/saml/spslo?RelayState=//attacker_domain.com

A url like that will redirect to attacher_domain.com

They describe the issue like this

An open redirect vulnerability occurs when a web application improperly
handles user-supplied input, allowing attackers to manipulate redirection to an
external, untrusted URL. This vulnerability typically arises when an application
accepts a user-provided URL as a parameter without proper validation before
redirecting the user to it.

[gerardo-navarro]

@Bandes Thanks for reporting this. I've been looking into this issue and can confirm the vulnerability exists.

The RelayState parameter is indeed accepted without any validation and forwarded through the SLO flow, potentially allowing redirects to arbitrary external (potential malicious) domains.

I was able to reproduce this locally with the following test:

curl -I "https://example.app/users/auth/saml/spslo?RelayState=//attacker.com"
# Results in Location header containing the unvalidated RelayState

Security Considerations

This could be considered a HIGH severity issue because:

• It enables credential phishing attacks
• The gem provides no validation by default
• Protection depends entirely on IdP configuration (which can widely vary)
• It violates the defense-in-depth principle

Different SAML IdPs have varying RelayState validation behaviors - some validate strictly, others don't validate at all. The gem shouldn't rely on downstream validation.

Root Analysis

The vulnerability stems from the slo_relay_state method which returns request.params["RelayState"] directly without any validation. This unvalidated value is then used in:

• other_phase_for_spslo - SP-initiated logout
• handle_logout_response - Redirects to the RelayState after IdP logout

Proposed Solutions

Looking at the codebase, I see three potential approaches to address this:

Option 1: Add Built-in RelayState Validation (Recommended)

Add validation directly in the slo_relay_state method to only allow relative paths. This would:

• Only allow paths starting with / (but not //)
• Reject any URLs with host or scheme components
• Return nil for invalid RelayState values
• Parse URIs safely with proper error handling

Option 2: Configurable RelayState Validation

Make RelayState validation configurable with an allowlist approach:

• Add validate_relay_state option (default: true for security)
• Add allowed_relay_state_domains option for specific domain allowlisting
• Maintain backward compatibility while defaulting to secure behavior
• Allow applications with specific requirements to configure as needed

Option 3: Opt-in SLO with Security Documentation

Given that not all applications require SLO functionality:

• Make SLO endpoints opt-in via enable_slo option (default: false)
• Add slo_security_callback for custom validation logic
• Provide clear security warnings in documentation
• Include example code for implementing secure RelayState validation

Next Steps

I believe Option 1 provides the best balance of security and simplicity - adding built-in validation that only allows relative paths by default. This would protect all users immediately while maintaining backward compatibility for legitimate use cases.

Would love to hear thoughts from maintainers and the community on the preferred approach.

[md5]

I think a combination of your Option 1 and Option 3 makes sense.

Allow a Proc to be passed via an option like relay_state_validator. The default Proc for that option could default to allowing the relative path RelayState values you mention in your Option 1. It might make sense to allow full URLs with matching host, scheme, and port as well as relative URLs since in some cases it might be more convenient to use full URLs.

If a user passes a false, nil, or another falsey value for relay_state_validator, the gem could have the current behavior of allowing any RelayState. If someone wants to do some kind of "allowed hosts" thing like your Option 2, they can provide a Proc that does that. The docs could provide some guidance on that (or not).

[gerardo-navarro]

@md5 Thanks for the reply and your feedback. Here are my thoughts.

Allow a Proc to be passed via an option like relay_state_validator. The default Proc for that option could default to allowing the relative path RelayState values you mention in your Option 1. It might make sense to allow full URLs with matching host, scheme, and port as well as relative URLs since in some cases it might be more convenient to use full URLs.

I like your idea regarding the relay_state_validator option that receives a Proc. This provides users with the flexibility to implement custom validation logic for their specific needs. 👍 I also agree that we can define a default Proc for the saml option relay_state_validator. 👍

Here is the current PR: #245

---

It might make sense to allow full URLs with matching host, scheme, and port as well as relative URLs since in some cases it might be more convenient to use full URLs.

That’s a good point. However, I’d recommend keeping the default behavior restricted to relative paths only (e.g. /dashboard) to ensure security by default.

Even though allowing full URLs with matching host, scheme, and port can be convenient, it also increases the risk of misconfiguration. With LLM-generated code and copy-paste setups becoming more common, a secure default reduces the likelihood of vulnerable configurations spreading unnoticed.

This way, developers who fully understand their environment can still relax the restriction by providing their own validator Proc, but new or unaware users are protected by default.

---

If a user passes a false, nil, or another falsey value for relay_state_validator, the gem could have the current behavior of allowing any RelayState

I get the idea. But, I think we should invert the logic for clarity and consistency:

• When relay_state_validator option is true, then the gem will allow any RelayState (current behavior).
• When relay_state_validator option is false or another falsy value, then the gem will consider every given RelayState.
• When relay_state_validator option is a Proc, then the proc is evaluated and the result (true or false) determines if the given RelayState is valid or invalid.

IMO This will be more consistent to the actual behavior of the relay_state_validator Proc. When the Proc returns false, then the given RelayState is invalid <==> When the option relay_state_validator is false, then any given RelayState should be invalid.

---

Regarding Option 3: Opt-in SLO with Security Documentation

I decided To flip the logic here as well and add an opt-out of the single logout mechanism. Because the logout endpoint itself is not the problem. The problem was that the relay state is not validated.

This already merged in this PR: #243

[gerardo-navarro]

#245 was merged.

The default behavior has changed to block external redirects given via the RelayState param. Developers can customize the validation behavior by passing a proc to the slo_relay_state_validator option, see https://github.com/omniauth/omniauth-saml?tab=readme-ov-file#options

I think that we can close this issue.

[bufferoverflow]

@gerardo-navarro I suggest we get #242 in and maybe you can review #215 (I guess this can be closed). And then I would create a new release and the we can close this. ok?

/cc @fh1ch

[gerardo-navarro]

@gerardo-navarro I suggest we get #242 in and maybe you can review #215 (I guess this can be closed).

Regarding #242: I closed this #242 because the changes in this PR are included in #245.

Regarding #215: I reviewed the PR and it looks good to me. 📓 Note: The PR #215 itself is not directly related to this issue (Open Relay in SLO) because it deals with request phase of the SAML flow (and not the logout phase). Having that said, #215 adds a good default parameter mapping for generating the redirect to the IdP as part of the initial SAML request step.

And then I would create a new release and the we can close this. ok?

@bufferoverflow 👍 💯 Thanks. I agree. We can wait for #215