diff --git a/core/src/main/java/hudson/model/UpdateCenter.java b/core/src/main/java/hudson/model/UpdateCenter.java index 63e517dd4e485bef411cc666e30be9e7e045e411..e64c3fa3285a8033dc8f37616168a79ad793e671 100644 --- a/core/src/main/java/hudson/model/UpdateCenter.java +++ b/core/src/main/java/hudson/model/UpdateCenter.java @@ -1515,7 +1515,7 @@ public class UpdateCenter extends AbstractModelObject implements Saveable, OnMas if(e.getMessage().contains("Connection timed out")) { // Google can't be down, so this is probably a proxy issue connectionStates.put(ConnectionStatus.INTERNET, ConnectionStatus.FAILED); - statuses.add(Messages.UpdateCenter_Status_ConnectionFailed(connectionCheckUrl)); + statuses.add(Messages.UpdateCenter_Status_ConnectionFailed(Functions.xmlEscape(connectionCheckUrl))); return; } } @@ -1537,12 +1537,12 @@ public class UpdateCenter extends AbstractModelObject implements Saveable, OnMas statuses.add(Messages.UpdateCenter_Status_Success()); } catch (UnknownHostException e) { connectionStates.put(ConnectionStatus.UPDATE_SITE, ConnectionStatus.FAILED); - statuses.add(Messages.UpdateCenter_Status_UnknownHostException(e.getMessage())); + statuses.add(Messages.UpdateCenter_Status_UnknownHostException(Functions.xmlEscape(e.getMessage()))); addStatus(e); error = e; } catch (Exception e) { connectionStates.put(ConnectionStatus.UPDATE_SITE, ConnectionStatus.FAILED); - statuses.add(Functions.printThrowable(e)); + addStatus(e); error = e; } @@ -1556,7 +1556,7 @@ public class UpdateCenter extends AbstractModelObject implements Saveable, OnMas } } - private void addStatus(UnknownHostException e) { + private void addStatus(Throwable e) { statuses.add("
"+ Functions.xmlEscape(Functions.printThrowable(e))+"
"); } diff --git a/core/src/main/java/hudson/security/csrf/DefaultCrumbIssuer.java b/core/src/main/java/hudson/security/csrf/DefaultCrumbIssuer.java index e355360c9360eeda6bb9936ede3a6c034afd0596..91b3e968b85fc411084b6f0f858abf8dfdd091d4 100644 --- a/core/src/main/java/hudson/security/csrf/DefaultCrumbIssuer.java +++ b/core/src/main/java/hudson/security/csrf/DefaultCrumbIssuer.java @@ -91,7 +91,7 @@ public class DefaultCrumbIssuer extends CrumbIssuer { } if (!EXCLUDE_SESSION_ID) { buffer.append(';'); - buffer.append(getSessionId(req)); + buffer.append(req.getSession().getId()); } md.update(buffer.toString().getBytes()); @@ -101,14 +101,6 @@ public class DefaultCrumbIssuer extends CrumbIssuer { return null; } - private String getSessionId(@Nonnull HttpServletRequest request) { - HttpSession session = request.getSession(false); - if (session == null) { - return "NO_SESSION"; - } - return session.getId(); - } - /** * {@inheritDoc} */ diff --git a/test/src/test/java/hudson/security/csrf/DefaultCrumbIssuerSEC1491Test.java b/test/src/test/java/hudson/security/csrf/DefaultCrumbIssuerSEC1491Test.java new file mode 100644 index 0000000000000000000000000000000000000000..f27bea8eb83b3fe82e426c2cbb520731dd83bb56 --- /dev/null +++ b/test/src/test/java/hudson/security/csrf/DefaultCrumbIssuerSEC1491Test.java @@ -0,0 +1,135 @@ +package hudson.security.csrf; + +import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException; +import com.gargoylesoftware.htmlunit.HttpMethod; +import com.gargoylesoftware.htmlunit.WebRequest; +import com.gargoylesoftware.htmlunit.html.HtmlPage; +import jenkins.model.Jenkins; +import org.junit.Assert; +import org.junit.Before; +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.MockAuthorizationStrategy; + +import java.net.HttpURLConnection; +import java.net.URL; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +//TODO merge back to DefaultCrumbIssuerTest +public class DefaultCrumbIssuerSEC1491Test { + + @Rule + public JenkinsRule r = new JenkinsRule(); + + @Before public void setIssuer() { + r.jenkins.setCrumbIssuer(new DefaultCrumbIssuer(false)); + } + + @Test + @Issue("SECURITY-1491") + public void sessionIncludedEvenForAnonymousCall() throws Exception { + boolean previousValue = DefaultCrumbIssuer.EXCLUDE_SESSION_ID; + + try { + r.jenkins.setSecurityRealm(r.createDummySecurityRealm()); + + // let anonymous user have read access + MockAuthorizationStrategy authorizationStrategy = new MockAuthorizationStrategy(); + authorizationStrategy.grant(Jenkins.ADMINISTER).everywhere().toEveryone(); + r.jenkins.setAuthorizationStrategy(authorizationStrategy); + + DefaultCrumbIssuer issuer = new DefaultCrumbIssuer(true); + r.jenkins.setCrumbIssuer(issuer); + + DefaultCrumbIssuer.EXCLUDE_SESSION_ID = true; + sameCrumbUsedOnDifferentAnonymousRequest_tokenAreEqual(true, "job_noSession"); + + DefaultCrumbIssuer.EXCLUDE_SESSION_ID = false; + sameCrumbUsedOnDifferentAnonymousRequest_tokenAreEqual(false, "job_session"); + } finally { + DefaultCrumbIssuer.EXCLUDE_SESSION_ID = previousValue; + } + } + + private void sameCrumbUsedOnDifferentAnonymousRequest_tokenAreEqual(boolean areEqual, String namePrefix) throws Exception { + String responseForCrumb = r.createWebClient().goTo("crumbIssuer/api/xml?xpath=concat(//crumbRequestField,'=',//crumb)", "text/plain") + .getWebResponse().getContentAsString(); + // responseForCrumb = Jenkins-Crumb=xxxx + String crumb1 = responseForCrumb.substring(CrumbIssuer.DEFAULT_CRUMB_NAME.length() + "=".length()); + + String jobName1 = namePrefix + "-test1"; + String jobName2 = namePrefix + "-test2"; + + WebRequest request1 = createRequestForJobCreation(jobName1); + try { + r.createWebClient().getPage(request1); + fail(); + } catch (FailingHttpStatusCodeException e) { + assertTrue(e.getMessage().contains("No valid crumb")); + } + // cannot create new job due to missing crumb + assertNull(r.jenkins.getItem(jobName1)); + + WebRequest request2 = createRequestForJobCreation(jobName2); + request2.setAdditionalHeader(CrumbIssuer.DEFAULT_CRUMB_NAME, crumb1); + if (areEqual) { + r.createWebClient().getPage(request2); + + assertNotNull(r.jenkins.getItem(jobName2)); + } else { + try { + r.createWebClient().getPage(request2); + fail("Should have failed due to invalid crumb"); + } catch (FailingHttpStatusCodeException e) { + assertEquals(HttpURLConnection.HTTP_FORBIDDEN, e.getStatusCode()); + // cannot create new job due to invalid crumb + assertNull(r.jenkins.getItem(jobName2)); + } + } + } + + @Test + @Issue("SECURITY-1491") + public void twoRequestsWithoutSessionGetDifferentCrumbs() throws Exception { + String responseForCrumb = r.createWebClient().goTo("crumbIssuer/api/xml?xpath=concat(//crumbRequestField,'=',//crumb)", "text/plain") + .getWebResponse().getContentAsString(); + // responseForCrumb = Jenkins-Crumb=xxxx + String crumb1 = responseForCrumb.substring(CrumbIssuer.DEFAULT_CRUMB_NAME.length() + "=".length()); + + responseForCrumb = r.createWebClient().goTo("crumbIssuer/api/xml?xpath=concat(//crumbRequestField,'=',//crumb)", "text/plain") + .getWebResponse().getContentAsString(); + // responseForCrumb = Jenkins-Crumb=xxxx + String crumb2 = responseForCrumb.substring(CrumbIssuer.DEFAULT_CRUMB_NAME.length() + "=".length()); + + Assert.assertNotEquals("should be different crumbs", crumb1, crumb2); + } + + private WebRequest createRequestForJobCreation(String jobName) throws Exception { + WebRequest req = new WebRequest(new URL(r.getURL() + "createItem?name=" + jobName), HttpMethod.POST); + req.setAdditionalHeader("Content-Type", "application/xml"); + req.setRequestBody(""); + return req; + } + + @Test + public void anonCanStillPostRequestUsingBrowsers() throws Exception { + r.jenkins.setSecurityRealm(r.createDummySecurityRealm()); + + MockAuthorizationStrategy authorizationStrategy = new MockAuthorizationStrategy(); + authorizationStrategy.grant(Jenkins.ADMINISTER).everywhere().toEveryone(); + r.jenkins.setAuthorizationStrategy(authorizationStrategy); + + DefaultCrumbIssuer issuer = new DefaultCrumbIssuer(true); + r.jenkins.setCrumbIssuer(issuer); + + HtmlPage p = r.createWebClient().goTo("configure"); + r.submit(p.getFormByName("config")); + } +}