Skip to content

Commit

Permalink
Fix session leak via SAML RelayState
Browse files Browse the repository at this point in the history
Motivation:
This is a follow-up for 8edcf91
The `RelayState` must be url encoded instead of HTML escaping because it's used in the URL.
https://cheatsheetseries.owasp.org/cheatsheets/Cross_Site_Scripting_Prevention_Cheat_Sheet.html#output-encoding-for-url-contexts

Modification:
- URL-encode `RelayState`
Result:
- Improved security by mitigating the risk of XSS attacks through URL encoding of SAML `RelayState`.
  • Loading branch information
minwoox committed Feb 5, 2024
1 parent 9ecb84d commit 9e42e8b
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import static com.linecorp.centraldogma.server.auth.saml.HtmlUtil.getHtmlWithOnload;
import static java.util.Objects.requireNonNull;

import java.io.UnsupportedEncodingException;
import java.net.URLEncoder;
import java.time.Duration;
import java.util.List;
import java.util.Optional;
Expand All @@ -38,7 +40,6 @@
import org.opensaml.saml.saml2.core.Response;

import com.google.common.base.Strings;
import com.google.common.html.HtmlEscapers;

import com.linecorp.armeria.common.AggregatedHttpRequest;
import com.linecorp.armeria.common.HttpRequest;
Expand Down Expand Up @@ -126,8 +127,12 @@ public HttpResponse loginSucceeded(ServiceRequestContext ctx, AggregatedHttpRequ

final String redirectionScript;
if (!Strings.isNullOrEmpty(relayState)) {
redirectionScript = "window.location.href='/#" +
HtmlEscapers.htmlEscaper().escape(relayState) + '\'';
try {
redirectionScript = "window.location.href='/#" + URLEncoder.encode(relayState, "UTF-8") + '\'';
} catch (UnsupportedEncodingException e) {
// Should never reach here.
throw new Error();
}
} else {
redirectionScript = "window.location.href='/'";
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,6 @@ void relayStateIsHtmlEscaped() {
samlAuthSsoHandler.loginSucceeded(ctx, req, messageContext, null, relayState);
assertThat(httpResponse.aggregate().join().contentUtf8()).isEqualTo(getHtmlWithOnload(
"localStorage.setItem('sessionId','id')",
"window.location.href='/#'.substr(0.1)'"&<>'"));
"window.location.href='/#%27.substr%280.1%29%27%22%26%3C%3E'"));
}
}

0 comments on commit 9e42e8b

Please sign in to comment.