From 11f77a77eaae785bda3957d8ca34260a36346394 Mon Sep 17 00:00:00 2001 From: Benjamin Wenzel Date: Fri, 26 Jan 2024 18:40:57 +0000 Subject: [PATCH 1/2] Fix for SECURITY-2979 / CVE-2023-50771 Sanitize redirect url to make sure it cannot point wherever the from query parameter shows. --- .../plugins/oic/OicSecurityRealm.java | 37 ++++++++++++------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index 6d13a600..d6dd305c 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -730,6 +730,29 @@ protected AuthorizationCodeFlow buildAuthorizationCodeFlow() { return builder.build(); } + private String getValidRedirectUrl(String url) { + if (url != null && !url.isEmpty()) { + // Check if the URL is relative and starts with a slash + if (url.startsWith("/")) { + return getRootUrl() + url; // Convert to absolute URL + } + // If not relative, then check if it's a valid absolute URL + try { + URL parsedUrl = new URL(url); + String host = parsedUrl.getHost(); + String expectedHost = new URL(getRootUrl()).getHost(); + // Check if the host matches the Jenkins domain + if (host.equals(expectedHost)) { + return url; // The URL is absolute and valid + } + } catch (MalformedURLException e) { + // Invalid absolute URL, will return root URL + } + } + // If the URL is null, empty, or invalid, return the root URL + return getRootUrl(); + } + /** * Handles the the securityRealm/commenceLogin resource and sends the user off to the IdP * @param from the relative URL to the page that the user has just come from @@ -741,7 +764,7 @@ public HttpResponse doCommenceLogin(@QueryParameter String from, @Header("Refere // reload config if needed loadWellKnownOpenIDConfigurationUrl(); - final String redirectOnFinish = determineRedirectTarget(from, referer); + final String redirectOnFinish = getValidRedirectUrl(from != null ? from : referer); final AuthorizationCodeFlow flow = this.buildAuthorizationCodeFlow(); @@ -1034,18 +1057,6 @@ private String getRootUrl() { } } - private String determineRedirectTarget(@QueryParameter String from, @Header("Referer") String referer) { - String target; - if (from != null) { - target = from; - } else if (referer != null) { - target = referer; - } else { - target = getRootUrl(); - } - return target; - } - private String buildOAuthRedirectUrl() throws NullPointerException { String rootUrl = getRootUrl(); if (rootUrl == null) { From f4d98ec49b4d3928620db890941dd70e0080977d Mon Sep 17 00:00:00 2001 From: Benjamin Wenzel Date: Wed, 31 Jan 2024 11:14:15 +0000 Subject: [PATCH 2/2] Add test for redirect url validation --- .../jenkinsci/plugins/oic/OicSecurityRealm.java | 2 +- .../plugins/oic/OicSecurityRealmTest.java | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java index d6dd305c..4db90324 100644 --- a/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java +++ b/src/main/java/org/jenkinsci/plugins/oic/OicSecurityRealm.java @@ -730,7 +730,7 @@ protected AuthorizationCodeFlow buildAuthorizationCodeFlow() { return builder.build(); } - private String getValidRedirectUrl(String url) { + protected String getValidRedirectUrl(String url) { if (url != null && !url.isEmpty()) { // Check if the URL is relative and starts with a slash if (url.startsWith("/")) { diff --git a/src/test/java/org/jenkinsci/plugins/oic/OicSecurityRealmTest.java b/src/test/java/org/jenkinsci/plugins/oic/OicSecurityRealmTest.java index cefe94fb..58d6996e 100644 --- a/src/test/java/org/jenkinsci/plugins/oic/OicSecurityRealmTest.java +++ b/src/test/java/org/jenkinsci/plugins/oic/OicSecurityRealmTest.java @@ -4,6 +4,8 @@ import com.github.tomakehurst.wiremock.junit.WireMockRule; import hudson.util.Secret; import java.io.IOException; +import java.net.MalformedURLException; + import org.acegisecurity.AuthenticationManager; import org.acegisecurity.BadCredentialsException; import org.acegisecurity.GrantedAuthority; @@ -16,6 +18,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThrows; public class OicSecurityRealmTest { @@ -83,4 +86,17 @@ public void testShouldSetNullClientSecretWhenSecretIsNone() throws IOException { .build(); assertEquals("none", Secret.toString(realm.getClientSecret())); } + + @Test + public void testGetValidRedirectUrl() throws IOException { + String rootUrl = "http://localhost:" + wireMockRule.port() + "/jenkins/"; + + TestRealm realm = new TestRealm.Builder(wireMockRule) + .WithMinimalDefaults().build(); + assertEquals(rootUrl + "foo", realm.getValidRedirectUrl("/foo")); + assertEquals(rootUrl + "bar", realm.getValidRedirectUrl(rootUrl + "bar")); + assertEquals(rootUrl, realm.getValidRedirectUrl(null)); + assertEquals(rootUrl, realm.getValidRedirectUrl("")); + assertThrows(MalformedURLException.class, () -> realm.getValidRedirectUrl("foobar")); + } }