Merge branch 'security-stable-2.176' into security-stable-2.190

core/src/main/java/jenkins/model/JenkinsLocationConfiguration.java

@@ -1,16 +1,21 @@
 package jenkins.model;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import hudson.Extension;
 import hudson.Util;
 import hudson.XmlFile;
 import hudson.model.PersistentDescriptor;
 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;
@@ -33,6 +38,18 @@ import javax.annotation.Nonnull;
  */
 @Extension @Symbol("location")
 public class JenkinsLocationConfiguration extends GlobalConfiguration implements PersistentDescriptor {
+
+    /**
+     * 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}
      */
@@ -82,6 +99,10 @@ public class JenkinsLocationConfiguration extends GlobalConfiguration implements
             super.load();
         }
 
+        if (!DISABLE_URL_VALIDATION) {
+            preventRootUrlBeingInvalid();
+        }
+
         updateSecureSessionFlag();
     }
 
@@ -119,10 +140,26 @@ public class JenkinsLocationConfiguration extends GlobalConfiguration implements
         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
      *
@@ -162,6 +199,11 @@ public class JenkinsLocationConfiguration extends GlobalConfiguration implements
     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();
     }
 

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 <nobody@nowhere>
 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
 

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)"));
+    }
 }

test/src/test/java/jenkins/model/JenkinsLocationConfigurationSEC1471Test.java

@@ -0,0 +1,187 @@
+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.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<Boolean> alertAppeared = new AtomicReference<>(false);
+        wc.setAlertHandler((page, s) -> {
+            alertAppeared.set(true);
+        });
+        HtmlPage page = wc.goTo("");
+
+        HtmlAnchor newViewLink = page.getDocumentElement().getElementsByTagName("a").stream()
+                .filter(HtmlAnchor.class::isInstance).map(HtmlAnchor.class::cast)
+                .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<Boolean> 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");
+
+        HtmlAnchor labelAnchor = projectConfigurePage.getDocumentElement().getElementsByTagName("a").stream()
+                .filter(HtmlAnchor.class::isInstance).map(HtmlAnchor.class::cast)
+                .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);
+    }
+}

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<Job<?,?>> {
+        @TestExtension("testEnsureXSSnotPossible")
+        public static class DescriptorImpl extends OptionalJobProperty.OptionalJobPropertyDescriptor {
+
+            @Override
+            public String getDisplayName() {
+                return "XSS Property";
+            }
+            
+            public ComboBoxModel doFillXssItems() {
+                return new ComboBoxModel("<h1>HACK</h1>");
+            }
+        }
+    }
+
+    @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);
+    }
+}

test/src/test/resources/jenkins/model/JenkinsLocationConfigurationSEC1471Test/xssThroughConfigXml/jenkins.model.JenkinsLocationConfiguration.xml

@@ -0,0 +1,4 @@
+<?xml version='1.1' encoding='UTF-8'?>
+<jenkins.model.JenkinsLocationConfiguration>
+    <jenkinsUrl>javascript:alert(123);//</jenkinsUrl>
+</jenkins.model.JenkinsLocationConfiguration>

test/src/test/resources/lib/form/ComboBoxSEC1525Test/XssProperty/config.jelly

@@ -0,0 +1,6 @@
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
+	<f:entry field="xss">
+	  <f:combobox />
+	</f:entry>
+</j:jelly>

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;