Merge pull request #95 from jenkinsci-cert/SECURITY-353

[SECURITY-353] Persisted XSS in parameter definition names and value descriptions
jenkinsci · Jan 3, 2017 · fd2e081 · fd2e081
2 parents 2c1c1ec + 169056e
commit fd2e081
Show file tree

Hide file tree

Showing 18 changed files with 119 additions and 26 deletions.
diff --git a/core/src/main/java/hudson/model/ParameterValue.java b/core/src/main/java/hudson/model/ParameterValue.java
@@ -31,11 +31,16 @@
 import hudson.tasks.BuildWrapper;
 import hudson.tasks.Builder;
 import hudson.util.VariableResolver;
+import java.io.IOException;
 
 import java.io.Serializable;
 import java.util.Map;
+import java.util.logging.Logger;
+import jenkins.model.Jenkins;
 
 import net.sf.json.JSONObject;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.DoNotUse;
 
 import org.kohsuke.stapler.StaplerRequest;
 import org.kohsuke.stapler.export.Exported;
@@ -70,6 +75,9 @@
  */
 @ExportedBean(defaultVisibility=3)
 public abstract class ParameterValue implements Serializable {
+
+    private static final Logger LOGGER = Logger.getLogger(ParameterValue.class.getName());
+
     protected final String name;
 
     private String description;
@@ -91,6 +99,16 @@ public void setDescription(String description) {
         this.description = description;
     }
 
+    @Restricted(DoNotUse.class) // for value.jelly
+    public String getFormattedDescription() {
+        try {
+            return Jenkins.getInstance().getMarkupFormatter().translate(description);
+        } catch (IOException e) {
+            LOGGER.warning("failed to translate description using configured markup formatter");
+            return "";
+        }
+    }
+
     /**
      * Name of the parameter.
      *

diff --git a/core/src/main/resources/hudson/model/BooleanParameterDefinition/index.jelly b/core/src/main/resources/hudson/model/BooleanParameterDefinition/index.jelly
@@ -26,7 +26,8 @@ THE SOFTWARE.
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define"
 	xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form"
 	xmlns:i="jelly:fmt" xmlns:p="/lib/hudson/project">
-	<f:entry title="${it.name}" description="${it.formattedDescription}">
+        <j:set var="escapeEntryTitleAndDescription" value="false"/>
+        <f:entry title="${h.escape(it.name)}" description="${it.formattedDescription}">
 		<div name="parameter">
 			<input type="hidden" name="name" value="${it.name}" />
 			<f:checkbox name="value" checked="${it.defaultValue}" />

diff --git a/core/src/main/resources/hudson/model/BooleanParameterValue/value.jelly b/core/src/main/resources/hudson/model/BooleanParameterValue/value.jelly
@@ -26,7 +26,8 @@ THE SOFTWARE.
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define"
 	xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form"
 	xmlns:i="jelly:fmt" xmlns:p="/lib/hudson/project">
-	<f:entry title="${it.name}" description="${it.description}">
+        <j:set var="escapeEntryTitleAndDescription" value="false"/>
+        <f:entry title="${h.escape(it.name)}" description="${it.formattedDescription}">
 		<f:checkbox name="value" checked="${it.value}" readonly="true" />
 	</f:entry>
 </j:jelly>
diff --git a/core/src/main/resources/hudson/model/ChoiceParameterDefinition/index.jelly b/core/src/main/resources/hudson/model/ChoiceParameterDefinition/index.jelly
@@ -26,7 +26,8 @@ THE SOFTWARE.
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define"
 	xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form"
 	xmlns:i="jelly:fmt" xmlns:p="/lib/hudson/project">
-	<f:entry title="${it.name}" description="${it.formattedDescription}">
+        <j:set var="escapeEntryTitleAndDescription" value="false"/>
+        <f:entry title="${h.escape(it.name)}" description="${it.formattedDescription}">
 		<div name="parameter">
 			<input type="hidden" name="name" value="${it.name}" />
             <select name="value">

diff --git a/core/src/main/resources/hudson/model/FileParameterDefinition/index.jelly b/core/src/main/resources/hudson/model/FileParameterDefinition/index.jelly
@@ -26,7 +26,8 @@ THE SOFTWARE.
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define"
 	xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form"
 	xmlns:i="jelly:fmt" xmlns:p="/lib/hudson/project">
-	<f:entry title="${it.name}" description="${it.formattedDescription}">
+        <j:set var="escapeEntryTitleAndDescription" value="false"/>
+        <f:entry title="${h.escape(it.name)}" description="${it.formattedDescription}">
 		<div name="parameter">
 			<input type="hidden" name="name" value="${it.name}" />
 			<input name="file" type="file" jsonAware="true" />

diff --git a/core/src/main/resources/hudson/model/FileParameterValue/value.jelly b/core/src/main/resources/hudson/model/FileParameterValue/value.jelly
@@ -26,7 +26,8 @@ THE SOFTWARE.
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define"
 	xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form"
 	xmlns:i="jelly:fmt" xmlns:p="/lib/hudson/project">
-    <f:entry title="${it.name}">
+        <j:set var="escapeEntryTitleAndDescription" value="false"/>
+        <f:entry title="${h.escape(it.name)}" description="${it.formattedDescription}">
         <j:if test="${it.originalFileName != null}">
             <j:invokeStatic var="encodedName" className="hudson.Util" method="rawEncode">
                 <j:arg value="${it.name}" />

diff --git a/core/src/main/resources/hudson/model/ParametersAction/index.jelly b/core/src/main/resources/hudson/model/ParametersAction/index.jelly
@@ -36,6 +36,7 @@ THE SOFTWARE.
 		<l:main-panel>
 			<h1>${%Build} ${build.displayName}</h1>
 			<l:pane title="${%Parameters}" width="3">
+                        <j:set var="escapeEntryTitleAndDescription" value="true"/> <!-- SECURITY-353 defense unless overridden -->
 			<j:forEach var="parameterValue" items="${it.parameters}">
 				<st:include it="${parameterValue}"
 					page="value.jelly" />

diff --git a/core/src/main/resources/hudson/model/ParametersDefinitionProperty/index.jelly b/core/src/main/resources/hudson/model/ParametersDefinitionProperty/index.jelly
@@ -45,6 +45,7 @@ THE SOFTWARE.
       <f:form method="post" action="build${empty(delay)?'':'?delay='+delay}" name="parameters"
               tableClass="parameters">
         <j:forEach var="parameterDefinition" items="${it.parameterDefinitions}">
+          <j:set var="escapeEntryTitleAndDescription" value="true"/> <!-- SECURITY-353 defense unless overridden -->
           <tbody>
             <st:include it="${parameterDefinition}"
                         page="${parameterDefinition.descriptor.valuePage}" />

diff --git a/core/src/main/resources/hudson/model/PasswordParameterDefinition/index.jelly b/core/src/main/resources/hudson/model/PasswordParameterDefinition/index.jelly
@@ -26,7 +26,8 @@ THE SOFTWARE.
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define"
 	xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form"
 	xmlns:i="jelly:fmt" xmlns:p="/lib/hudson/project">
-	<f:entry title="${it.name}" description="${it.formattedDescription}">
+        <j:set var="escapeEntryTitleAndDescription" value="false"/>
+        <f:entry title="${h.escape(it.name)}" description="${it.formattedDescription}">
 		<div name="parameter">
 			<input type="hidden" name="name" value="${it.name}" />
 			<f:password name="value" value="${it.defaultValueAsSecret}" />

diff --git a/core/src/main/resources/hudson/model/PasswordParameterValue/value.jelly b/core/src/main/resources/hudson/model/PasswordParameterValue/value.jelly
@@ -26,7 +26,8 @@ THE SOFTWARE.
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define"
 	xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form"
 	xmlns:i="jelly:fmt" xmlns:p="/lib/hudson/project">
-	<f:entry title="${it.name}" description="${it.description}">
+        <j:set var="escapeEntryTitleAndDescription" value="false"/>
+        <f:entry title="${h.escape(it.name)}" description="${it.formattedDescription}">
 		<f:password name="value" value="********" readonly="true" />
 	</f:entry>
 </j:jelly>
diff --git a/core/src/main/resources/hudson/model/RunParameterDefinition/index.jelly b/core/src/main/resources/hudson/model/RunParameterDefinition/index.jelly
@@ -26,7 +26,8 @@ THE SOFTWARE.
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define"
 	xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form"
 	xmlns:i="jelly:fmt" xmlns:p="/lib/hudson/project">
-	<f:entry title="${it.name}" description="${it.formattedDescription}">
+        <j:set var="escapeEntryTitleAndDescription" value="false"/>
+        <f:entry title="${h.escape(it.name)}" description="${it.formattedDescription}">
 		<div name="parameter">
 			<input type="hidden" name="name" value="${it.name}" />
             <select name="runId">

diff --git a/core/src/main/resources/hudson/model/RunParameterValue/value.jelly b/core/src/main/resources/hudson/model/RunParameterValue/value.jelly
@@ -26,7 +26,8 @@ THE SOFTWARE.
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define"
 	xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form"
 	xmlns:i="jelly:fmt" xmlns:p="/lib/hudson/project">
-	<f:entry title="${it.name}" description="${it.description}">
+        <j:set var="escapeEntryTitleAndDescription" value="false"/>
+        <f:entry title="${h.escape(it.name)}" description="${it.formattedDescription}">
 		<a href="${rootURL}/${it.run.url}" class="model-link inside">${it.run.fullDisplayName}</a>
 	</f:entry>
 </j:jelly>
diff --git a/core/src/main/resources/hudson/model/StringParameterDefinition/index.jelly b/core/src/main/resources/hudson/model/StringParameterDefinition/index.jelly
@@ -26,7 +26,8 @@ THE SOFTWARE.
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define"
 	xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form"
 	xmlns:i="jelly:fmt" xmlns:p="/lib/hudson/project">
-	<f:entry title="${it.name}" description="${it.formattedDescription}">
+        <j:set var="escapeEntryTitleAndDescription" value="false"/>
+        <f:entry title="${h.escape(it.name)}" description="${it.formattedDescription}">
 		<div name="parameter">
 			<input type="hidden" name="name" value="${it.name}" />
 			<f:textbox name="value" value="${it.defaultValue}" />

diff --git a/core/src/main/resources/hudson/model/StringParameterValue/value.jelly b/core/src/main/resources/hudson/model/StringParameterValue/value.jelly
@@ -26,7 +26,8 @@ THE SOFTWARE.
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define"
 	xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form"
 	xmlns:i="jelly:fmt" xmlns:p="/lib/hudson/project">
-	<f:entry title="${it.name}" description="${it.description}">
+        <j:set var="escapeEntryTitleAndDescription" value="false"/>
+        <f:entry title="${h.escape(it.name)}" description="${it.formattedDescription}">
 		<f:textbox name="value" value="${it.value}" readonly="true" />
 	</f:entry>
 </j:jelly>
diff --git a/core/src/main/resources/hudson/model/TextParameterDefinition/index.jelly b/core/src/main/resources/hudson/model/TextParameterDefinition/index.jelly
@@ -24,7 +24,8 @@ THE SOFTWARE.
 -->
 <?jelly escape-by-default='true'?>
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form" xmlns:i="jelly:fmt" xmlns:p="/lib/hudson/project">
-  <f:entry title="${it.name}" description="${it.formattedDescription}">
+    <j:set var="escapeEntryTitleAndDescription" value="false"/>
+    <f:entry title="${h.escape(it.name)}" description="${it.formattedDescription}">
     <div name="parameter">
       <input type="hidden" name="name" value="${it.name}"/>
       <f:textarea name="value" value="${it.defaultValue}"/>

diff --git a/core/src/main/resources/hudson/model/TextParameterValue/value.jelly b/core/src/main/resources/hudson/model/TextParameterValue/value.jelly
@@ -26,7 +26,8 @@ THE SOFTWARE.
 <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define"
 	xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form"
 	xmlns:i="jelly:fmt" xmlns:p="/lib/hudson/project">
-	<f:entry title="${it.name}" description="${it.description}">
+        <j:set var="escapeEntryTitleAndDescription" value="false"/>
+        <f:entry title="${h.escape(it.name)}" description="${it.formattedDescription}">
 		<f:textarea name="value" value="${it.value}" readonly="readonly" />
 	</f:entry>
 </j:jelly>
diff --git a/core/src/main/resources/lib/form/entry.jelly b/core/src/main/resources/lib/form/entry.jelly
@@ -32,6 +32,8 @@ THE SOFTWARE.
 
     <st:attribute name="title">
       Name of the entry. Think of this like a label for the control.
+
+      This content is HTML (unless the boolean variable escapeEntryTitleAndDescription is set). Use h.escape if necessary.
     </st:attribute>
     <st:attribute name="field">
       Used for the databinding. TBD. When this attribute
@@ -46,6 +48,8 @@ THE SOFTWARE.
       This text shouldn't get too long, and in recent Hudson, this feature
       is somewhat de-emphasized, in favor of the inline foldable help page
       specified via @help.
+
+      This content is HTML (unless the boolean variable escapeEntryTitleAndDescription is set). Use h.escape if necessary.
     </st:attribute>
     <st:attribute name="help">
       URL to the HTML page. When this attribute is specified, the entry gets
@@ -67,7 +71,7 @@ THE SOFTWARE.
   <tr>
     <td class="setting-leftspace"><st:nbsp/></td>
     <td class="setting-name">
-      <j:out value="${attrs.title}" />
+      <j:out value="${escapeEntryTitleAndDescription ? h.escape(attrs.title) : attrs.title}" />
     </td>
     <td class="setting-main">
       <d:invokeBody />
@@ -78,7 +82,7 @@ THE SOFTWARE.
   <tr class="validation-error-area"><td colspan="2" /><td /><td /></tr>
   <j:if test="${!empty(attrs.description)}">
     <f:description>
-      <j:out value="${description}"/>
+      <j:out value="${escapeEntryTitleAndDescription ? h.escape(attrs.description) : attrs.description}"/>
     </f:description>
   </j:if>
   <j:if test="${attrs.help!=null}">

diff --git a/test/src/test/java/hudson/model/ParametersTest.java b/test/src/test/java/hudson/model/ParametersTest.java
@@ -1,25 +1,30 @@
 package hudson.model;
 
-import static org.junit.Assert.*;
-
 import com.gargoylesoftware.htmlunit.html.DomNodeUtil;
+import com.gargoylesoftware.htmlunit.html.HtmlCheckBoxInput;
+import com.gargoylesoftware.htmlunit.html.HtmlElement;
+import com.gargoylesoftware.htmlunit.html.HtmlForm;
 import com.gargoylesoftware.htmlunit.html.HtmlFormUtil;
-import org.junit.Rule;
-import org.junit.Test;
-
+import com.gargoylesoftware.htmlunit.html.HtmlOption;
 import com.gargoylesoftware.htmlunit.html.HtmlPage;
-import com.gargoylesoftware.htmlunit.html.HtmlForm;
-import com.gargoylesoftware.htmlunit.html.HtmlElement;
 import com.gargoylesoftware.htmlunit.html.HtmlTextInput;
-import com.gargoylesoftware.htmlunit.html.HtmlCheckBoxInput;
-import com.gargoylesoftware.htmlunit.html.HtmlOption;
+import hudson.markup.MarkupFormatter;
+import java.io.IOException;
+import java.io.Writer;
+import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import org.apache.http.HttpStatus;
+import static org.hamcrest.Matchers.*;
+import static org.junit.Assert.*;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ErrorCollector;
 import org.jvnet.hudson.test.CaptureEnvironmentBuilder;
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.JenkinsRule.WebClient;
 
-import java.util.Set;
-
 /**
  * @author huybrechts
  */
@@ -28,6 +33,9 @@ public class ParametersTest {
     @Rule
     public JenkinsRule j = new JenkinsRule();
 
+    @Rule
+    public ErrorCollector collector = new ErrorCollector();
+
     @Test
     public void parameterTypes() throws Exception {
         FreeStyleProject otherProject = j.createFreeStyleProject();
@@ -216,4 +224,52 @@ public void unicodeParametersArePresetCorrectly() throws Exception {
         final HtmlForm form = page.getFormByName("parameters");
         HtmlFormUtil.submit(form, HtmlFormUtil.getButtonByCaption(form, "Build"));
     }
+
+    @Issue("SECURITY-353")
+    @Test
+    public void xss() throws Exception {
+        j.jenkins.setMarkupFormatter(new MyMarkupFormatter());
+        FreeStyleProject p = j.createFreeStyleProject("p");
+        StringParameterDefinition param = new StringParameterDefinition("<param name>", "<param default>", "<param description>");
+        assertEquals("<b>[</b>param description<b>]</b>", param.getFormattedDescription());
+        p.addProperty(new ParametersDefinitionProperty(param));
+        WebClient wc = j.createWebClient();
+        wc.getOptions().setThrowExceptionOnFailingStatusCode(false);
+        HtmlPage page = wc.getPage(p, "build?delay=0sec");
+        collector.checkThat(page.getWebResponse().getStatusCode(), is(HttpStatus.SC_METHOD_NOT_ALLOWED)); // 405 to dissuade scripts from thinking this triggered the build
+        String text = page.getWebResponse().getContentAsString();
+        collector.checkThat("build page should escape param name", text, containsString("&lt;param name&gt;"));
+        collector.checkThat("build page should not leave param name unescaped", text, not(containsString("<param name>")));
+        collector.checkThat("build page should escape param default", text, containsString("&lt;param default&gt;"));
+        collector.checkThat("build page should not leave param default unescaped", text, not(containsString("<param default>")));
+        collector.checkThat("build page should mark up param description", text, containsString("<b>[</b>param description<b>]</b>"));
+        collector.checkThat("build page should not leave param description unescaped", text, not(containsString("<param description>")));
+        HtmlForm form = page.getFormByName("parameters");
+        HtmlTextInput value = form.getInputByValue("<param default>");
+        value.setText("<param value>");
+        j.submit(form);
+        j.waitUntilNoActivity();
+        FreeStyleBuild b = p.getBuildByNumber(1);
+        page = j.createWebClient().getPage(b, "parameters/");
+        text = page.getWebResponse().getContentAsString();
+        collector.checkThat("parameters page should escape param name", text, containsString("&lt;param name&gt;"));
+        collector.checkThat("parameters page should not leave param name unescaped", text, not(containsString("<param name>")));
+        collector.checkThat("parameters page should escape param value", text, containsString("&lt;param value&gt;"));
+        collector.checkThat("parameters page should not leave param value unescaped", text, not(containsString("<param value>")));
+        collector.checkThat("parameters page should mark up param description", text, containsString("<b>[</b>param description<b>]</b>"));
+        collector.checkThat("parameters page should not leave param description unescaped", text, not(containsString("<param description>")));
+    }
+    static class MyMarkupFormatter extends MarkupFormatter {
+        @Override
+        public void translate(String markup, Writer output) throws IOException {
+            Matcher m = Pattern.compile("[<>]").matcher(markup);
+            StringBuffer buf = new StringBuffer();
+            while (m.find()) {
+                m.appendReplacement(buf, m.group().equals("<") ? "<b>[</b>" : "<b>]</b>");
+            }
+            m.appendTail(buf);
+            output.write(buf.toString());
+        }
+    }
+
 }