EndSessionCallbackResult.GetHtml() does not safely escape URIs #5184
Description
The current latest version of IdentityServer4 uses only string concatenation to build the <iframe> list for the EndSessionCallbackResult in the GetHtml method - this will result in incorrect rendering of ampersands in the src="" attribute and could potentially be an XSS vulnerability if there's a way for user-provided content to make its way into the FrontChannelLogoutUrls collection.
I recommend changing this:
private string GetHtml()
{
string framesHtml = null;
if (_result.FrontChannelLogoutUrls != null && _result.FrontChannelLogoutUrls.Any())
{
var frameUrls = _result.FrontChannelLogoutUrls.Select(url => $"<iframe src='{url}'></iframe>");
framesHtml = frameUrls.Aggregate((x, y) => x + y);
}
return $"<!DOCTYPE html><html><style>iframe{{display:none;width:0;height:0;}}</style><body>{framesHtml}</body></html>";
}
to this:
private string GetHtml()
{
var sb = new StringBuilder();
_ = sb.Append(@"<!DOCTYPE html><html><style>iframe{display:none;width:0;height:0;}</style><body>");
_ = sb.AppendLine();
foreach (var iframeUrl in _result.FrontChannelLogoutUrls ?? Array.Empty<string>())
{
_ = sb.AppendFormat(@"<iframe loading=""eager"" src=""{0}""></iframe>", WebUtility.HtmlEncode(iframeUrl));
_ = sb.AppendLine();
}
_ = sb.Append(@"</body></html>");
var html = sb.ToString();
return html;
}
I note that the iframe { display: none; } style rule does mean that UAs could, at their discretion, defer loading the iframe because it isn't visible when the callback iframe loads (thus breaking the signout process). I understand that UAs presently do not defer loading hidden iframes, but it's a possibility in future, but the good news is that we can pre-empt this by adding the loading="eager" attribute (which I have included in my above suggested fix).
I also think it would be prudent to also add the following functionality (not included in my suggested fix as it requires more thought and review):
- Add the
<iframe allow=""attribute to set a Feature Policy to mitigate the risk of a malicious client signout page from interfering with IdentityServer. - Ditto
<iframe csp=""andsandbox. - Adding an
onloadandonerrorhandler function which could be used to notify the user that the signout process failed - or to confirm to the user that the signout process completed for all client iframes - right now the stock logged-out page always informs the user that they signed out successfully, even when an iframe fails to load.