Skip to content

[JENKINS-59849] Don't fail to serve resource files with nontrivial names #4302

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
daniel-beck merged 2 commits into from
Oct 19, 2019
Merged

[JENKINS-59849] Don't fail to serve resource files with nontrivial names #4302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fa4ec0a
[JENKINS-59849]
daniel-beck Oct 18, 2019
a2477f6
Extend test to cover more complex dir structure
daniel-beck Oct 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions core/src/main/java/jenkins/security/ResourceDomainRootAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
*/
package jenkins.security;

import com.google.common.annotations.VisibleForTesting;
import hudson.Extension;
import hudson.ExtensionList;
import hudson.Util;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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("/"));
Comment thread
daniel-beck marked this conversation as resolved.

@jglick jglick Oct 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to drop a trailing / so that is no good. It will also convert "/" to "".

@daniel-beck daniel-beck Oct 18, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test that hopefully demonstrates that this does not matter.

Redirect URLs are only ever obtained for files, and directory browsing on the resource domain continues to work.

}

private static String getResourceRootUrl() {
Expand All @@ -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();

@daniel-beck daniel-beck Oct 18, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the most important part of the fix, otherwise the math below will be off: We subtracted the length of foo bar from a string ending in whatever/foo%20bar…


// 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.
Expand Down Expand Up @@ -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("/"));
Comment thread
daniel-beck marked this conversation as resolved.
Stapler.getCurrent().invoke(req, rsp, Jenkins.get(), path);
} catch (Exception ex) {
// cf. UnwrapSecurityExceptionFilter
Throwable cause = ex.getCause();
Expand Down Expand Up @@ -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;
Expand All @@ -277,8 +282,8 @@ private String encode() {
}

private static Token decode(String value) {
byte[] byteValue = Base64.getUrlDecoder().decode(value);

@daniel-beck daniel-beck Oct 18, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly related, but @jsoref could make this throw an exception, so just move it down.

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);
Expand Down
90 changes: 90 additions & 0 deletions test/src/test/java/jenkins/security/ResourceDomainTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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")
Expand Down Expand Up @@ -279,4 +285,88 @@ 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", "<html><body>the content</body></html>"));
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%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
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 dir name ").child(" 100% evil content .html").write("this is the content", "UTF-8");
return new DirectoryBrowserSupport(jenkins, tempDir, "title", "", true);
}
}
}