From 35e2736cfd5c56799eece176328906d92b6a0dd1 Mon Sep 17 00:00:00 2001 From: James Nord Date: Wed, 12 Oct 2022 21:22:42 +0200 Subject: [PATCH] [SECURITY-2881] --- .gitignore | 4 +- .../steps/input/POSTHyperlinkNote.java | 20 ++- .../steps/input/POSTHyperlinkNoteTest.java | 155 ++++++++++++++++++ 3 files changed, 177 insertions(+), 2 deletions(-) create mode 100644 src/test/java/org/jenkinsci/plugins/workflow/test/steps/input/POSTHyperlinkNoteTest.java diff --git a/.gitignore b/.gitignore index 5f8cf71e..3be34f6a 100644 --- a/.gitignore +++ b/.gitignore @@ -2,6 +2,8 @@ target work *.iml .idea - +/.classpath +/.project +/.settings/ # macOS .DS_Store diff --git a/src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/POSTHyperlinkNote.java b/src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/POSTHyperlinkNote.java index d00f16ab..109a9410 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/POSTHyperlinkNote.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/support/steps/input/POSTHyperlinkNote.java @@ -28,6 +28,10 @@ import hudson.console.ConsoleAnnotationDescriptor; import hudson.console.HyperlinkNote; import java.io.IOException; +import java.io.UnsupportedEncodingException; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.Base64; import java.util.logging.Level; import java.util.logging.Logger; import jenkins.model.Jenkins; @@ -80,7 +84,21 @@ public POSTHyperlinkNote(String url, int length) { @Override protected String extraAttributes() { // TODO perhaps add hoverNotification - return " onclick=\"new Ajax.Request('" + url + "'); return false\""; + return " onclick=\"new Ajax.Request(decodeURIComponent(atob('" + encodeForJavascript(url) + "'))); return false\""; + } + + /** + * Encode the String (using URLEncoding and then base64 encoding) so we can safely pass it to javascript where it can be decoded safely. + * Javascript strings are UTF-16 and the endianness depends on the platform so we use URL encoding to ensure the String is all 7bit clean ascii and base64 encoding to fix passing any "unsafe" characters. + */ + private static String encodeForJavascript(String str) { + // https://developer.mozilla.org/en-US/docs/Glossary/Base64#the_unicode_problem + try { + String encode = URLEncoder.encode(str, StandardCharsets.UTF_8.name()); + return Base64.getUrlEncoder().encodeToString(encode.getBytes(StandardCharsets.UTF_8)); + } catch (UnsupportedEncodingException e) { + throw new InternalError("UTF-8 is missing but mandated by the JVM specification", e); + } } // TODO why does there need to be a descriptor at all? diff --git a/src/test/java/org/jenkinsci/plugins/workflow/test/steps/input/POSTHyperlinkNoteTest.java b/src/test/java/org/jenkinsci/plugins/workflow/test/steps/input/POSTHyperlinkNoteTest.java new file mode 100644 index 00000000..eff9c1bc --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/workflow/test/steps/input/POSTHyperlinkNoteTest.java @@ -0,0 +1,155 @@ +package org.jenkinsci.plugins.workflow.test.steps.input; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.hasProperty; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; +import java.net.URL; +import java.util.Collections; +import java.util.Map; +import java.util.Set; +import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition; +import org.jenkinsci.plugins.workflow.job.WorkflowJob; +import org.jenkinsci.plugins.workflow.job.WorkflowRun; +import org.jenkinsci.plugins.workflow.steps.Step; +import org.jenkinsci.plugins.workflow.steps.StepContext; +import org.jenkinsci.plugins.workflow.steps.StepDescriptor; +import org.jenkinsci.plugins.workflow.steps.StepExecution; +import org.jenkinsci.plugins.workflow.steps.SynchronousStepExecution; +import org.jenkinsci.plugins.workflow.support.steps.input.POSTHyperlinkNote; +import org.junit.Ignore; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.Issue; +import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.JenkinsRule.WebClient; +import org.jvnet.hudson.test.TestExtension; +import org.kohsuke.stapler.DataBoundConstructor; +import com.gargoylesoftware.htmlunit.HttpMethod; +import com.gargoylesoftware.htmlunit.MockWebConnection; +import com.gargoylesoftware.htmlunit.Page; +import com.gargoylesoftware.htmlunit.WebRequest; +import com.gargoylesoftware.htmlunit.html.HtmlAnchor; +import com.gargoylesoftware.htmlunit.html.HtmlPage; +import edu.umd.cs.findbugs.annotations.NonNull; +import hudson.model.ParametersAction; +import hudson.model.ParametersDefinitionProperty; +import hudson.model.Result; +import hudson.model.StringParameterDefinition; +import hudson.model.StringParameterValue; +import hudson.model.TaskListener; +import hudson.model.queue.QueueTaskFuture; +import jenkins.model.Jenkins; + +public class POSTHyperlinkNoteTest { + + @Rule + public JenkinsRule jr = new JenkinsRule(); + + @Test + @Issue("SECURITY-2881") + public void urlsAreSafeFromJavascriptInjection() throws Exception { + testSanitization("whatever/'+alert(1)+'"); + } + + @Test + @Ignore("webclient does not support unicode URLS and this is passed as /jenkins/whatever/%F0%9F%99%88%F0%9F%99%89%F0%9F%99%8A%F0%9F%98%80%E2%98%BA") + public void testPassingMultiByteCharacters() throws Exception { + // this is actually illegal in HTML4 but common -> https://www.w3.org/TR/html40/appendix/notes.html#non-ascii-chars + // browsers infer the URL from the charset and then encode the escaped characters... + testSanitization("whatever/🙈🙉🙊😀☺"); + } + + @Test + public void testPassingSingleByte() throws Exception { + testSanitization("whatever/something?withparameter=baa"); + } + + void testSanitization(String fragment) throws Exception { + WorkflowJob project = jr.createProject(WorkflowJob.class); + project.setDefinition(new CpsFlowDefinition("security2881(params.TEST_URL)\n", true)); + project.addProperty(new ParametersDefinitionProperty(new StringParameterDefinition("TEST_URL", "WHOOPS"))); + + QueueTaskFuture scheduleBuild = project.scheduleBuild2(0, new ParametersAction(new StringParameterValue("TEST_URL", fragment))); + WorkflowRun run = jr.assertBuildStatus(Result.SUCCESS, scheduleBuild); + WebClient wc = jr.createWebClient(); + + HtmlPage page = wc.getPage(run, "console"); + HtmlAnchor anchor = page.getAnchorByText("SECURITY-2881"); + assertThat(anchor, notNullValue()); + MockWebConnection mwc = new MockWebConnection(); + mwc.setDefaultResponse("Hello"); + wc.setWebConnection(mwc); + System.out.println(anchor); + Page p = anchor.click(); + + // the click executes an ajax request - and so we need to wait until that has completed + // ideally we would pass zero here as we have already clicked the javascript should have + // started executing - but this is not always the case + wc.waitForBackgroundJavaScriptStartingBefore(500); + + // check we have an interaction at the correct place and its not a javascript issue. + WebRequest request = mwc.getLastWebRequest(); + assertThat(request, notNullValue()); + assertThat(request.getHttpMethod(), is(HttpMethod.POST)); + URL url = request.getUrl(); + System.out.println(url.toExternalForm()); + assertThat(url, allOf(hasProperty("host", is(new URL(jr.jenkins.getConfiguredRootUrl()).getHost())), + hasProperty("file", is(jr.contextPath + '/' + fragment)))); + } + + public static class Security2881ConsoleStep extends Step { + + private final String urlFragment; + + @DataBoundConstructor + public Security2881ConsoleStep(String urlFragment) { + this.urlFragment = urlFragment; + } + + @Override + public StepExecution start(StepContext context) throws Exception { + return new Security2881ConsoleStepExecution(context, urlFragment); + } + + @TestExtension + public static final class DescriptorImpl extends StepDescriptor { + + @Override public String getFunctionName() { + return "security2881"; + } + + @NonNull + @Override public String getDisplayName() { + return "Security2881"; + } + + @Override public Set> getRequiredContext() { + return Collections.singleton(TaskListener.class); + } + + @Override public String argumentsToString(@NonNull Map namedArgs) { + return null; + } + } + + public static class Security2881ConsoleStepExecution extends SynchronousStepExecution { + + private final String urlFragment; + + protected Security2881ConsoleStepExecution(StepContext context, String urlFragment) { + super(context); + this.urlFragment = urlFragment; + } + + @Override + protected Void run() throws Exception { + TaskListener taskListener = getContext().get(TaskListener.class); + // use the same URL for CORS. + taskListener.getLogger().print(POSTHyperlinkNote.encodeTo(Jenkins.get().getConfiguredRootUrl() + urlFragment, "SECURITY-2881")); + return null; + } + } + } +}