From fa4ec0a0e3665987df4aace51a9b5f76aef0375a Mon Sep 17 00:00:00 2001 From: Daniel Beck Date: Fri, 18 Oct 2019 21:19:51 +0200 Subject: [PATCH 1/2] [JENKINS-59849] --- .../security/ResourceDomainRootAction.java | 15 ++-- .../jenkins/security/ResourceDomainTest.java | 78 +++++++++++++++++++ 2 files changed, 88 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/jenkins/security/ResourceDomainRootAction.java b/core/src/main/java/jenkins/security/ResourceDomainRootAction.java index 5cb8d2b41e5e..0712aef1abf4 100644 --- a/core/src/main/java/jenkins/security/ResourceDomainRootAction.java +++ b/core/src/main/java/jenkins/security/ResourceDomainRootAction.java @@ -23,6 +23,7 @@ */ package jenkins.security; +import com.google.common.annotations.VisibleForTesting; import hudson.Extension; import hudson.ExtensionList; import hudson.Util; @@ -51,6 +52,7 @@ import java.util.Base64; import java.util.logging.Level; import java.util.logging.Logger; +import java.util.stream.Collectors; import static java.time.Instant.*; import static java.time.temporal.ChronoUnit.MINUTES; @@ -147,7 +149,7 @@ public String getRedirectUrl(@Nonnull Token token, @Nonnull String restOfPath) { // Unsure whether this can happen -- just be safe here restOfPath = "/" + restOfPath; } - return resourceRootUrl + getUrlName() + "/" + token.encode() + restOfPath; + return resourceRootUrl + getUrlName() + "/" + token.encode() + Arrays.stream(restOfPath.split("[/]")).map(Util::rawEncode).collect(Collectors.joining("/")); } private static String getResourceRootUrl() { @@ -165,7 +167,7 @@ private static String getResourceRootUrl() { @CheckForNull public Token getToken(@Nonnull DirectoryBrowserSupport dbs, @Nonnull StaplerRequest req) { // This is the "restOfPath" of the DirectoryBrowserSupport, i.e. the directory/file/pattern "inside" the DBS. - final String dbsFile = req.getRestOfPath(); + final String dbsFile = req.getOriginalRestOfPath(); // Now get the 'restOfUrl' after the top-level ancestor (which is the Jenkins singleton). // In other words, this is the complete URL after Jenkins handled the top-level request. @@ -222,7 +224,8 @@ public void doDynamic(StaplerRequest req, StaplerResponse rsp) throws IOExceptio try (ACLContext ignored = ACL.as(auth)) { try { - Stapler.getCurrent().invoke(req, rsp, Jenkins.get(), requestUrlSuffix + restOfPath); + String path = requestUrlSuffix + Arrays.stream(restOfPath.split("[/]")).map(Util::rawEncode).collect(Collectors.joining("/")); + Stapler.getCurrent().invoke(req, rsp, Jenkins.get(), path); } catch (Exception ex) { // cf. UnwrapSecurityExceptionFilter Throwable cause = ex.getCause(); @@ -263,7 +266,9 @@ public static class Token { private String path; private String username; private Instant timestamp; - private Token (String path, @Nullable String username, Instant timestamp) { + + @VisibleForTesting + Token (String path, @Nullable String username, Instant timestamp) { this.path = path; this.username = Util.fixNull(username); this.timestamp = timestamp; @@ -277,8 +282,8 @@ private String encode() { } private static Token decode(String value) { - byte[] byteValue = Base64.getUrlDecoder().decode(value); try { + byte[] byteValue = Base64.getUrlDecoder().decode(value); byte[] mac = Arrays.copyOf(byteValue, 32); byte[] restBytes = Arrays.copyOfRange(byteValue, 32, byteValue.length); String rest = new String(restBytes, StandardCharsets.UTF_8); diff --git a/test/src/test/java/jenkins/security/ResourceDomainTest.java b/test/src/test/java/jenkins/security/ResourceDomainTest.java index 86333d23d75f..430688c57901 100644 --- a/test/src/test/java/jenkins/security/ResourceDomainTest.java +++ b/test/src/test/java/jenkins/security/ResourceDomainTest.java @@ -3,9 +3,11 @@ import com.gargoylesoftware.htmlunit.Page; import com.gargoylesoftware.htmlunit.html.HtmlPage; import hudson.ExtensionList; +import hudson.FilePath; import hudson.model.DirectoryBrowserSupport; import hudson.model.FreeStyleProject; import hudson.model.Item; +import hudson.model.UnprotectedRootAction; import jenkins.model.Jenkins; import jenkins.model.JenkinsLocationConfiguration; import org.junit.Assert; @@ -17,8 +19,12 @@ import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.MockAuthorizationStrategy; +import org.jvnet.hudson.test.TestExtension; +import org.kohsuke.stapler.HttpResponse; +import javax.annotation.CheckForNull; import java.net.URL; +import java.time.Instant; import java.util.UUID; @Issue("JENKINS-41891") @@ -279,4 +285,76 @@ public void adminMonitorShowsUpWithOverriddenCSP() throws Exception { } Assert.assertFalse(monitor.isActivated()); } + + @Test + public void testRedirectUrls() throws Exception { + ResourceDomainRootAction rootAction = ResourceDomainRootAction.get(); + String url = rootAction.getRedirectUrl(new ResourceDomainRootAction.Token("foo", "bar", Instant.now()), "foo bar baz"); + Assert.assertFalse("urlencoded", url.contains(" ")); + } + + @Test + @Issue("JENKINS-59849") + public void testUrlEncoding() throws Exception { + FreeStyleProject project = j.createFreeStyleProject(); + project.getBuildersList().add(new CreateFileBuilder("This has spaces and is 100% evil.html", "the content")); + project.save(); + + j.buildAndAssertSuccess(project); + + JenkinsRule.WebClient webClient = j.createWebClient(); + webClient.setThrowExceptionOnFailingStatusCode(false); + webClient.setRedirectEnabled(true); + + HtmlPage page = webClient.getPage(project, "ws/This%20has%20spaces%20and%20is%20100%25%20evil.html"); + Assert.assertEquals("page is found", 200, page.getWebResponse().getStatusCode()); + Assert.assertTrue("page content is as expected", page.getWebResponse().getContentAsString().contains("the content")); + + URL url = page.getUrl(); + Assert.assertTrue("page is served by resource domain", url.toString().contains("/static-files/")); + } + + @Test + @Issue("JENKINS-59849") + public void testMoreUrlEncoding() throws Exception { + JenkinsRule.WebClient webClient = j.createWebClient(); + webClient.setThrowExceptionOnFailingStatusCode(false); + webClient.setRedirectEnabled(true); + + Page page = webClient.goTo("100%25%20evil/%20100%25%20evil%20content%20.html"); + Assert.assertEquals("page is found", 200, page.getWebResponse().getStatusCode()); + Assert.assertTrue("page content is as expected", page.getWebResponse().getContentAsString().contains("this is the content")); + + URL url = page.getUrl(); + Assert.assertTrue("page is served by resource domain", url.toString().contains("/static-files/")); + } + + @TestExtension + public static class RootActionImpl implements UnprotectedRootAction { + + @CheckForNull + @Override + public String getIconFileName() { + return null; + } + + @CheckForNull + @Override + public String getDisplayName() { + return null; + } + + @CheckForNull + @Override + public String getUrlName() { + return "100% evil"; + } + + public HttpResponse doDynamic() throws Exception { + Jenkins jenkins = Jenkins.get(); + FilePath tempDir = jenkins.getRootPath().createTempDir("root", "tmp"); + tempDir.child(" 100% evil content .html").write("this is the content", "UTF-8"); + return new DirectoryBrowserSupport(jenkins, tempDir, "title", "", false); + } + } } From a2477f64481c526279298eeacc2667da87cf5262 Mon Sep 17 00:00:00 2001 From: Daniel Beck Date: Fri, 18 Oct 2019 22:11:26 +0200 Subject: [PATCH 2/2] Extend test to cover more complex dir structure --- .../jenkins/security/ResourceDomainTest.java | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/test/src/test/java/jenkins/security/ResourceDomainTest.java b/test/src/test/java/jenkins/security/ResourceDomainTest.java index 430688c57901..c24cf9d2140f 100644 --- a/test/src/test/java/jenkins/security/ResourceDomainTest.java +++ b/test/src/test/java/jenkins/security/ResourceDomainTest.java @@ -321,12 +321,24 @@ public void testMoreUrlEncoding() throws Exception { webClient.setThrowExceptionOnFailingStatusCode(false); webClient.setRedirectEnabled(true); - Page page = webClient.goTo("100%25%20evil/%20100%25%20evil%20content%20.html"); + Page page = webClient.goTo("100%25%20evil/%20100%25%20evil%20dir%20name%20%20%20/%20100%25%20evil%20content%20.html"); Assert.assertEquals("page is found", 200, page.getWebResponse().getStatusCode()); Assert.assertTrue("page content is as expected", page.getWebResponse().getContentAsString().contains("this is the content")); URL url = page.getUrl(); Assert.assertTrue("page is served by resource domain", url.toString().contains("/static-files/")); + + URL dirUrl = new URL(url.toString().replace("%20100%25%20evil%20content%20.html", "")); + Page dirPage = webClient.getPage(dirUrl); + Assert.assertEquals("page is found", 200, dirPage.getWebResponse().getStatusCode()); + Assert.assertTrue("page content is HTML", dirPage.getWebResponse().getContentAsString().contains("href")); + Assert.assertTrue("page content references file", dirPage.getWebResponse().getContentAsString().contains("evil content")); + + URL topDirUrl = new URL(url.toString().replace("%20100%25%20evil%20dir%20name%20%20%20/%20100%25%20evil%20content%20.html", "")); + Page topDirPage = webClient.getPage(topDirUrl); + Assert.assertEquals("page is found", 200, topDirPage.getWebResponse().getStatusCode()); + Assert.assertTrue("page content is HTML", topDirPage.getWebResponse().getContentAsString().contains("href")); + Assert.assertTrue("page content references directory", topDirPage.getWebResponse().getContentAsString().contains("evil dir name")); } @TestExtension @@ -353,8 +365,8 @@ public String getUrlName() { public HttpResponse doDynamic() throws Exception { Jenkins jenkins = Jenkins.get(); FilePath tempDir = jenkins.getRootPath().createTempDir("root", "tmp"); - tempDir.child(" 100% evil content .html").write("this is the content", "UTF-8"); - return new DirectoryBrowserSupport(jenkins, tempDir, "title", "", false); + tempDir.child(" 100% evil dir name ").child(" 100% evil content .html").write("this is the content", "UTF-8"); + return new DirectoryBrowserSupport(jenkins, tempDir, "title", "", true); } } }