From 00064be3661e687da7215523a3ed9a37a85f51a3 Mon Sep 17 00:00:00 2001 From: Wadeck Follonier Date: Tue, 10 Sep 2019 21:27:35 +0200 Subject: [PATCH 1/2] [SECURITY-1525] --- .../java/lib/form/ComboBoxSEC1525Test.java | 50 +++++++++++++++++++ .../XssProperty/config.jelly | 6 +++ war/src/main/webapp/scripts/combobox.js | 2 +- 3 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 test/src/test/java/lib/form/ComboBoxSEC1525Test.java create mode 100644 test/src/test/resources/lib/form/ComboBoxSEC1525Test/XssProperty/config.jelly diff --git a/test/src/test/java/lib/form/ComboBoxSEC1525Test.java b/test/src/test/java/lib/form/ComboBoxSEC1525Test.java new file mode 100644 index 0000000000..f054d531a4 --- /dev/null +++ b/test/src/test/java/lib/form/ComboBoxSEC1525Test.java @@ -0,0 +1,50 @@ +package lib.form; + +import com.gargoylesoftware.htmlunit.html.HtmlElement; +import com.gargoylesoftware.htmlunit.html.HtmlElementUtil; +import com.gargoylesoftware.htmlunit.html.HtmlPage; +import hudson.model.FreeStyleProject; +import hudson.model.Job; +import hudson.util.ComboBoxModel; +import jenkins.model.OptionalJobProperty; +import org.jvnet.hudson.test.HudsonTestCase; +import org.jvnet.hudson.test.Issue; +import org.jvnet.hudson.test.TestExtension; + +//TODO meant to be merged back into ComboBoxTest after security release to avoid conflict during the upmerge process +public class ComboBoxSEC1525Test extends HudsonTestCase { + public static class XssProperty extends OptionalJobProperty> { + @TestExtension("testEnsureXSSnotPossible") + public static class DescriptorImpl extends OptionalJobProperty.OptionalJobPropertyDescriptor { + + @Override + public String getDisplayName() { + return "XSS Property"; + } + + public ComboBoxModel doFillXssItems() { + return new ComboBoxModel("

HACK

"); + } + } + } + + @Issue("SECURITY-1525") + public void testEnsureXSSnotPossible() throws Exception { + XssProperty xssProperty = new XssProperty(); + FreeStyleProject p = createFreeStyleProject(); + p.addProperty(xssProperty); + + WebClient wc = new WebClient(); + + HtmlPage configurePage = wc.getPage(p, "configure"); + int numberOfH1Before = configurePage.getElementsByTagName("h1").size(); + + HtmlElement comboBox = configurePage.getElementByName("_.xss"); + HtmlElementUtil.click(comboBox); + + // no additional h1, meaning the "payload" is not interpreted + int numberOfH1After = configurePage.getElementsByTagName("h1").size(); + + assertEquals(numberOfH1Before, numberOfH1After); + } +} diff --git a/test/src/test/resources/lib/form/ComboBoxSEC1525Test/XssProperty/config.jelly b/test/src/test/resources/lib/form/ComboBoxSEC1525Test/XssProperty/config.jelly new file mode 100644 index 0000000000..02edb7ba55 --- /dev/null +++ b/test/src/test/resources/lib/form/ComboBoxSEC1525Test/XssProperty/config.jelly @@ -0,0 +1,6 @@ + + + + + + diff --git a/war/src/main/webapp/scripts/combobox.js b/war/src/main/webapp/scripts/combobox.js index 66d541cae0..a4da68fda8 100644 --- a/war/src/main/webapp/scripts/combobox.js +++ b/war/src/main/webapp/scripts/combobox.js @@ -246,7 +246,7 @@ ComboBox.prototype.populateDropdown = function() { for (var i = 0; i < this.availableItems.length; i++) { var item = document.createElement("div"); item.className = "comboBoxItem"; - item.innerHTML = this.availableItems[i]; + item.innerText = this.availableItems[i]; item.id = "item_" + this.availableItems[i]; item.comboBox = this; item.comboBoxIndex = i; From e2c596c049e5e09e7254ff000802709c2e01af2a Mon Sep 17 00:00:00 2001 From: Wadeck Follonier Date: Tue, 10 Sep 2019 21:29:08 +0200 Subject: [PATCH 2/2] [SECURITY-1471] --- .../model/JenkinsLocationConfiguration.java | 42 ++++ .../jenkins/model/Messages.properties | 1 + .../test/java/jenkins/util/UrlHelperTest.java | 6 + ...nkinsLocationConfigurationSEC1471Test.java | 188 ++++++++++++++++++ ...ins.model.JenkinsLocationConfiguration.xml | 4 + 5 files changed, 241 insertions(+) create mode 100644 test/src/test/java/jenkins/model/JenkinsLocationConfigurationSEC1471Test.java create mode 100644 test/src/test/resources/jenkins/model/JenkinsLocationConfigurationSEC1471Test/xssThroughConfigXml/jenkins.model.JenkinsLocationConfiguration.xml diff --git a/core/src/main/java/jenkins/model/JenkinsLocationConfiguration.java b/core/src/main/java/jenkins/model/JenkinsLocationConfiguration.java index ed402938b4..7df83fc32b 100644 --- a/core/src/main/java/jenkins/model/JenkinsLocationConfiguration.java +++ b/core/src/main/java/jenkins/model/JenkinsLocationConfiguration.java @@ -1,15 +1,20 @@ package jenkins.model; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import hudson.Extension; import hudson.Util; import hudson.XmlFile; import hudson.util.FormValidation; import hudson.util.XStream2; +import jenkins.security.ApiTokenProperty; +import jenkins.util.SystemProperties; +import jenkins.util.UrlHelper; import org.jenkinsci.Symbol; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.stapler.QueryParameter; +import javax.annotation.Nullable; import javax.mail.internet.AddressException; import javax.mail.internet.InternetAddress; import javax.servlet.ServletContext; @@ -32,6 +37,18 @@ import javax.annotation.Nonnull; */ @Extension @Symbol("location") public class JenkinsLocationConfiguration extends GlobalConfiguration { + + /** + * If disabled, the application will no longer check for URL validity in the configuration page. + * This will lead to an instance vulnerable to SECURITY-1471. + * + * @since TODO + */ + @Restricted(NoExternalUse.class) + @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Accessible via System Groovy Scripts") + public static /* not final */ boolean DISABLE_URL_VALIDATION = + SystemProperties.getBoolean(JenkinsLocationConfiguration.class.getName() + ".disableUrlValidation"); + /** * @deprecated replaced by {@link #jenkinsUrl} */ @@ -85,6 +102,10 @@ public class JenkinsLocationConfiguration extends GlobalConfiguration { super.load(); } + if (!DISABLE_URL_VALIDATION) { + preventRootUrlBeingInvalid(); + } + updateSecureSessionFlag(); } @@ -122,10 +143,26 @@ public class JenkinsLocationConfiguration extends GlobalConfiguration { if(url!=null && !url.endsWith("/")) url += '/'; this.jenkinsUrl = url; + + if (!DISABLE_URL_VALIDATION) { + preventRootUrlBeingInvalid(); + } + save(); updateSecureSessionFlag(); } + private void preventRootUrlBeingInvalid() { + if (this.jenkinsUrl != null && isInvalidRootUrl(this.jenkinsUrl)) { + LOGGER.log(Level.INFO, "Invalid URL received: {0}, considered as null", this.jenkinsUrl); + this.jenkinsUrl = null; + } + } + + private boolean isInvalidRootUrl(@Nullable String value) { + return !UrlHelper.isValidRootUrl(value); + } + /** * If the Jenkins URL starts from "https", force the secure session flag * @@ -165,6 +202,11 @@ public class JenkinsLocationConfiguration extends GlobalConfiguration { public FormValidation doCheckUrl(@QueryParameter String value) { if(value.startsWith("http://localhost")) return FormValidation.warning(Messages.Mailer_Localhost_Error()); + + if (!DISABLE_URL_VALIDATION && isInvalidRootUrl(value)) { + return FormValidation.error(Messages.Mailer_NotHttp_Error()); + } + return FormValidation.ok(); } diff --git a/core/src/main/resources/jenkins/model/Messages.properties b/core/src/main/resources/jenkins/model/Messages.properties index f7d11a855f..f115f4a02e 100644 --- a/core/src/main/resources/jenkins/model/Messages.properties +++ b/core/src/main/resources/jenkins/model/Messages.properties @@ -58,6 +58,7 @@ IdStrategy.CaseSensitiveEmailAddress.DisplayName=Case sensitive (email address) Mailer.Address.Not.Configured=address not configured yet Mailer.Localhost.Error=Please set a valid host name, instead of localhost +Mailer.NotHttp.Error=The URL is invalid, please ensure you are using http:// or https:// with a valid domain. NewViewLink.NewView=New View diff --git a/core/src/test/java/jenkins/util/UrlHelperTest.java b/core/src/test/java/jenkins/util/UrlHelperTest.java index 8f6391c3a9..39ff3907ca 100644 --- a/core/src/test/java/jenkins/util/UrlHelperTest.java +++ b/core/src/test/java/jenkins/util/UrlHelperTest.java @@ -146,4 +146,10 @@ public class UrlHelperTest { assertTrue(UrlHelper.isValidRootUrl("http://jenkins.com.")); assertTrue(UrlHelper.isValidRootUrl("http://jenkins.com......")); } + + @Test + @Issue("SECURITY-1471") + public void ensureJavascriptSchemaIsNotAllowed() { + assertFalse(UrlHelper.isValidRootUrl("javascript:alert(123)")); + } } diff --git a/test/src/test/java/jenkins/model/JenkinsLocationConfigurationSEC1471Test.java b/test/src/test/java/jenkins/model/JenkinsLocationConfigurationSEC1471Test.java new file mode 100644 index 0000000000..eef9adf291 --- /dev/null +++ b/test/src/test/java/jenkins/model/JenkinsLocationConfigurationSEC1471Test.java @@ -0,0 +1,188 @@ +package jenkins.model; + +import com.gargoylesoftware.htmlunit.html.HtmlAnchor; +import com.gargoylesoftware.htmlunit.html.HtmlElementUtil; +import com.gargoylesoftware.htmlunit.html.HtmlForm; +import com.gargoylesoftware.htmlunit.html.HtmlFormUtil; +import com.gargoylesoftware.htmlunit.html.HtmlInput; +import com.gargoylesoftware.htmlunit.html.HtmlPage; +import hudson.model.FreeStyleProject; +import hudson.model.Label; +import junit.framework.AssertionFailedError; +import org.apache.commons.io.FileUtils; +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.recipes.LocalData; + +import java.io.File; +import java.io.IOException; +import java.net.URL; +import java.util.List; +import java.util.concurrent.atomic.AtomicReference; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +//TODO merge back into JenkinsLocationConfigurationTest after the security release +public class JenkinsLocationConfigurationSEC1471Test { + + private String lastRootUrlReturned; + private boolean lastRootUrlSet; + + @Rule + public JenkinsRule j = new JenkinsRule(){ + @Override + public URL getURL() throws IOException { + // first call for the "Running on xxx" log message, Jenkins not being set at that point + // and the second call is to set the rootUrl of the JLC inside the JenkinsRule#init + if (Jenkins.getInstanceOrNull() != null) { + // only useful for doNotAcceptNonHttpBasedRootURL_fromConfigXml + lastRootUrlReturned = JenkinsLocationConfiguration.getOrDie().getUrl(); + lastRootUrlSet = true; + } + return super.getURL(); + } + }; + + @Test + @Issue("SECURITY-1471") + public void doNotAcceptNonHttpBasedRootURL_fromUI() throws Exception { + // in JenkinsRule, the URL is set to the current URL + JenkinsLocationConfiguration.getOrDie().setUrl(null); + + JenkinsRule.WebClient wc = j.createWebClient(); + + assertNull(JenkinsLocationConfiguration.getOrDie().getUrl()); + + settingRootURL("javascript:alert(123);//"); + + // no impact on the url in memory + assertNull(JenkinsLocationConfiguration.getOrDie().getUrl()); + + File configFile = new File(j.jenkins.getRootDir(), "jenkins.model.JenkinsLocationConfiguration.xml"); + String configFileContent = FileUtils.readFileToString(configFile); + assertThat(configFileContent, containsString("JenkinsLocationConfiguration")); + assertThat(configFileContent, not(containsString("javascript:alert(123);//"))); + } + + @Test + @Issue("SECURITY-1471") + public void escapeHatch_acceptNonHttpBasedRootURL_fromUI() throws Exception { + boolean previousValue = JenkinsLocationConfiguration.DISABLE_URL_VALIDATION; + JenkinsLocationConfiguration.DISABLE_URL_VALIDATION = true; + + try { + // in JenkinsRule, the URL is set to the current URL + JenkinsLocationConfiguration.getOrDie().setUrl(null); + + JenkinsRule.WebClient wc = j.createWebClient(); + + assertNull(JenkinsLocationConfiguration.getOrDie().getUrl()); + + String expectedUrl = "weirdSchema:somethingAlsoWeird"; + settingRootURL(expectedUrl); + + // the method ensures there is an trailing slash + assertEquals(expectedUrl + "/", JenkinsLocationConfiguration.getOrDie().getUrl()); + + File configFile = new File(j.jenkins.getRootDir(), "jenkins.model.JenkinsLocationConfiguration.xml"); + String configFileContent = FileUtils.readFileToString(configFile); + assertThat(configFileContent, containsString("JenkinsLocationConfiguration")); + assertThat(configFileContent, containsString(expectedUrl)); + } + finally { + JenkinsLocationConfiguration.DISABLE_URL_VALIDATION = previousValue; + } + } + + @Test + @Issue("SECURITY-1471") + @LocalData("xssThroughConfigXml") + public void doNotAcceptNonHttpBasedRootURL_fromConfigXml() { + // in JenkinsRule, the URL is set to the current URL, even if coming from LocalData + // so we need to catch the last value before the getUrl from the JenkinsRule that will be used to set the rootUrl + assertNull(lastRootUrlReturned); + assertTrue(lastRootUrlSet); + + assertThat(JenkinsLocationConfiguration.getOrDie().getUrl(), not(containsString("javascript"))); + } + + @Test + @Issue("SECURITY-1471") + public void cannotInjectJavaScriptUsingRootUrl_inNewViewLinkAction() throws Exception { + JenkinsRule.WebClient wc = j.createWebClient(); + + settingRootURL("javascript:alert(123);//"); + + // setup the victim + AtomicReference alertAppeared = new AtomicReference<>(false); + wc.setAlertHandler((page, s) -> { + alertAppeared.set(true); + }); + HtmlPage page = wc.goTo(""); + + List allAnchors = page.getDocumentElement().getHtmlElementsByTagName("a"); + HtmlAnchor newViewLink = allAnchors.stream() + .filter(a -> a.getHrefAttribute().endsWith("newView")) + .findFirst().orElseThrow(AssertionFailedError::new); + + // last verification + assertFalse(alertAppeared.get()); + + HtmlElementUtil.click(newViewLink); + + assertFalse(alertAppeared.get()); + } + + @Test + @Issue("SECURITY-1471") + public void cannotInjectJavaScriptUsingRootUrl_inLabelAbsoluteLink() throws Exception { + String masterLabel = "master-node"; + j.jenkins.setLabelString(masterLabel); + + JenkinsRule.WebClient wc = j.createWebClient(); + + settingRootURL("javascript:alert(123);//"); + + // setup the victim + AtomicReference alertAppeared = new AtomicReference<>(false); + wc.setAlertHandler((page, s) -> { + alertAppeared.set(true); + }); + + FreeStyleProject p = j.createFreeStyleProject(); + p.setAssignedLabel(Label.get(masterLabel)); + + HtmlPage projectConfigurePage = wc.getPage(p, "/configure"); + + List allAnchors = projectConfigurePage.getDocumentElement().getHtmlElementsByTagName("a"); + HtmlAnchor labelAnchor = allAnchors.stream() + .filter(a -> a.getHrefAttribute().contains("/label/")) + .findFirst().orElseThrow(AssertionFailedError::new); + + assertFalse(alertAppeared.get()); + HtmlElementUtil.click(labelAnchor); + assertFalse(alertAppeared.get()); + + String labelHref = labelAnchor.getHrefAttribute(); + assertThat(labelHref, not(containsString("javascript:alert(123)"))); + + String responseContent = projectConfigurePage.getWebResponse().getContentAsString(); + assertThat(responseContent, not(containsString("javascript:alert(123)"))); + } + + private void settingRootURL(String desiredRootUrl) throws Exception { + HtmlPage configurePage = j.createWebClient().goTo("configure"); + HtmlForm configForm = configurePage.getFormByName("config"); + HtmlInput url = configForm.getInputByName("_.url"); + url.setValueAttribute(desiredRootUrl); + HtmlFormUtil.submit(configForm); + } +} diff --git a/test/src/test/resources/jenkins/model/JenkinsLocationConfigurationSEC1471Test/xssThroughConfigXml/jenkins.model.JenkinsLocationConfiguration.xml b/test/src/test/resources/jenkins/model/JenkinsLocationConfigurationSEC1471Test/xssThroughConfigXml/jenkins.model.JenkinsLocationConfiguration.xml new file mode 100644 index 0000000000..39ddd91319 --- /dev/null +++ b/test/src/test/resources/jenkins/model/JenkinsLocationConfigurationSEC1471Test/xssThroughConfigXml/jenkins.model.JenkinsLocationConfiguration.xml @@ -0,0 +1,4 @@ + + + javascript:alert(123);// +