Skip to content

Commit 4e722da

Browse files
joebandenburgjoebandenburg
committed
OPENAM-7063
1 parent e6c3a72 commit 4e722da

File tree

4 files changed

+112
-23
lines changed

4 files changed

+112
-23
lines changed

‎openam-server-only/src/main/webapp/oauth2c/OAuthLogout.jsp

+18-13
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,11 @@
3838
<%@ page import="java.util.Locale" %>
3939
<%@ page import="java.util.MissingResourceException" %>
4040
<%@ page import="org.forgerock.openam.authentication.modules.oauth2.OAuthUtil" %>
41+
<%@ page import="com.iplanet.sso.SSOTokenManager" %>
42+
<%@ page import="com.iplanet.sso.SSOException" %>
43+
<%@ page import="com.iplanet.sso.SSOToken" %>
44+
<%@ page import="com.sun.identity.authentication.service.AuthD" %>
45+
<%@ page import="com.sun.identity.idm.AMIdentity" %>
4146
<%
4247
// Internationalization stuff. You can use any internationalization framework
4348
String lang = request.getParameter("lang");
@@ -76,19 +81,19 @@
7681
String gotoURLencAttr = "";
7782
String OAuth2IdP = "";
7883
79-
String ServiceURI = SystemProperties.get(Constants.AM_SERVICES_DEPLOYMENT_DESCRIPTOR);
80-
if (gotoURL == null || gotoURL.isEmpty() ) {
81-
gotoURL = ServiceURI + "/UI/Logout";
82-
} else {
83-
boolean isValidURL = ESAPI.validator().
84-
isValidInput("URLContext", gotoURL, "URL", 255, false);
85-
boolean isValidURI = ESAPI.validator().
86-
isValidInput("HTTP URI: " + gotoURL, gotoURL, "HTTPURI", 2000, false);
87-
if (!isValidURL && !isValidURI) {
88-
OAuthUtil.debugError("OAuthLogout: wrong goto URL attempted to be used "
89-
+ "in the Logout page: " + gotoURL);
90-
gotoURL = "wronggotoURL";
91-
}
84+
String ServiceURI = SystemProperties.get(Constants.AM_SERVICES_DEPLOYMENT_DESCRIPTOR);
85+
SSOTokenManager manager = SSOTokenManager.getInstance();
86+
try {
87+
SSOToken ssoToken = manager.createSSOToken(request);
88+
String realm = ssoToken.getProperty("Organization");
89+
boolean isValidGotoUrl = AuthD.getAuth().isGotoUrlValid(gotoURL, realm);
90+
if (!isValidGotoUrl) {
91+
OAuthUtil.debugError("OAuthLogout: invalid or empty goto URL ignored on Logout page: " + gotoURL);
92+
gotoURL = ServiceURI + "/UI/Logout";
93+
}
94+
} catch (SSOException e) {
95+
OAuthUtil.debugMessage("OAuthLogout: using default goto URL because we failed to get a session");
96+
gotoURL = ServiceURI + "/UI/Logout";
9297
}
9398
9499
String logoutURL = request.getParameter(PARAM_LOGOUT_URL);

‎openam-shared/pom.xml

+5
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,11 @@
157157
<artifactId>java-ipv6</artifactId>
158158
</dependency>
159159

160+
<dependency>
161+
<groupId>external</groupId>
162+
<artifactId>esapiport</artifactId>
163+
</dependency>
164+
160165
<dependency>
161166
<groupId>org.testng</groupId>
162167
<artifactId>testng</artifactId>

‎openam-shared/src/main/java/org/forgerock/openam/shared/security/whitelist/RedirectUrlValidator.java

+23-10
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,11 @@
2222
import java.net.MalformedURLException;
2323
import java.net.URI;
2424
import java.net.URISyntaxException;
25+
import java.net.URL;
2526
import java.util.Collection;
2627
import javax.servlet.http.HttpServletRequest;
2728
import org.forgerock.json.JsonValue;
29+
import org.owasp.esapi.ESAPI;
2830

2931
/**
3032
* Validates the provided redirect URL against the list of valid goto URL domains.
@@ -46,6 +48,9 @@ public class RedirectUrlValidator<T> {
4648
private static final Debug DEBUG = Debug.getInstance("patternMatching");
4749
private final ValidDomainExtractor<T> domainExtractor;
4850

51+
// See http://stackoverflow.com/a/417184
52+
private final static int MAX_URL_LENGTH = 2000;
53+
4954
/**
5055
* Constructs a new RedirectUrlValidator instance.
5156
*
@@ -72,21 +77,18 @@ public boolean isRedirectUrlValid(final String url, final T configInfo) {
7277
if (DEBUG.messageEnabled()) {
7378
DEBUG.message("Validating goto URL " + url + " against patterns:\n" + patterns);
7479
}
75-
if (patterns == null || patterns.isEmpty()) {
76-
if (DEBUG.messageEnabled()) {
77-
DEBUG.message("There are no patterns to validate the URL against, the goto URL is considered valid");
78-
}
79-
return true;
80+
81+
// JavaScript URIs are a common vector for XSS attacks.
82+
if (url.toLowerCase().startsWith("javascript:")) {
83+
return false;
8084
}
8185

8286
try {
8387
final URI uri = new URI(url);
84-
//Both Absolute and scheme relative URLs should be validated.
88+
// Both Absolute and scheme relative URLs should be validated.
8589
if (!uri.isAbsolute() && !url.startsWith("//")) {
86-
if (DEBUG.messageEnabled()) {
87-
DEBUG.message(url + " is a relative URI, the goto URL is considered valid");
88-
}
89-
return true;
90+
return ESAPI.validator().isValidInput("isRedirectUrlValid", url, "HTTPURI", MAX_URL_LENGTH,
91+
false);
9092
}
9193
} catch (final URISyntaxException urise) {
9294
if (DEBUG.messageEnabled()) {
@@ -95,6 +97,17 @@ public boolean isRedirectUrlValid(final String url, final T configInfo) {
9597
return false;
9698
}
9799

100+
if (!ESAPI.validator().isValidInput("isRedirectUrlValid", url, "URL", MAX_URL_LENGTH, false)) {
101+
return false;
102+
}
103+
104+
if (patterns == null || patterns.isEmpty()) {
105+
if (DEBUG.messageEnabled()) {
106+
DEBUG.message("There are no patterns to validate the URL against, the goto URL is considered valid");
107+
}
108+
return true;
109+
}
110+
98111
final URLPatternMatcher patternMatcher = new URLPatternMatcher();
99112
try {
100113
return patternMatcher.match(url, patterns, true);

‎openam-shared/src/test/java/org/forgerock/openam/shared/security/whitelist/RedirectUrlValidatorTest.java

+66
Original file line numberDiff line numberDiff line change
@@ -157,4 +157,70 @@ public Collection<String> extractValidDomains(String configInfo) {
157157
}
158158
});
159159
}
160+
161+
@DataProvider(name = "relative")
162+
public Object[][] getRelativeCases() {
163+
return new Object[][]{
164+
{"/foo", true},
165+
{"foo", true},
166+
{"foo?abc=123", true},
167+
{"foo/bar", true},
168+
{"areallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurl", true},
169+
{"areallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurlareallyreallylongurltoolong", false},
170+
};
171+
}
172+
173+
@Test(dataProvider = "relative")
174+
public void testRelativeUrlsWithWhitelist(String url, boolean result) {
175+
RedirectUrlValidator<String> validator = getValidator(asSet("http://example.com/*"));
176+
assertThat(validator.isRedirectUrlValid(url, null)).isEqualTo(result);
177+
}
178+
179+
@Test(dataProvider = "relative")
180+
public void testRelativeUrlsWithoutWhitelist(String url, boolean result) {
181+
RedirectUrlValidator<String> validator = getValidator(null);
182+
assertThat(validator.isRedirectUrlValid(url, null)).isEqualTo(result);
183+
}
184+
185+
@DataProvider(name = "javascript")
186+
public Object[][] getJavaScriptCases() {
187+
return new Object[][]{
188+
{"javascript:alert", false},
189+
{"JavaSCRIpt:alert", false},
190+
{"/javascript:alert", true},
191+
};
192+
}
193+
194+
@Test(dataProvider = "javascript")
195+
public void testJavaScriptUrlsWithWhitelist(String url, boolean result) {
196+
RedirectUrlValidator<String> validator = getValidator(asSet("http://example.com/*"));
197+
assertThat(validator.isRedirectUrlValid(url, null)).isEqualTo(result);
198+
}
199+
200+
@Test(dataProvider = "javascript")
201+
public void testJavaScriptUrlsWithoutWhitelist(String url, boolean result) {
202+
RedirectUrlValidator<String> validator = getValidator(null);
203+
assertThat(validator.isRedirectUrlValid(url, null)).isEqualTo(result);
204+
}
205+
206+
@DataProvider(name = "malformed")
207+
public Object[][] getMalformedCases() {
208+
return new Object[][]{
209+
{"http:abc", false},
210+
{"http:/abc", false},
211+
{"/a$bc", false}
212+
};
213+
}
214+
215+
@Test(dataProvider = "malformed")
216+
public void testMalformedUrlsWithWhitelist(String url, boolean result) {
217+
RedirectUrlValidator<String> validator = getValidator(asSet("http://example.com/*"));
218+
assertThat(validator.isRedirectUrlValid(url, null)).isEqualTo(result);
219+
}
220+
221+
@Test(dataProvider = "malformed")
222+
public void testMalformedUrlsWithoutWhitelist(String url, boolean result) {
223+
RedirectUrlValidator<String> validator = getValidator(null);
224+
assertThat(validator.isRedirectUrlValid(url, null)).isEqualTo(result);
225+
}
160226
}

0 commit comments

Comments
 (0)