From fa9fac9f73c100d40c91989d3ab2d18d04289641 Mon Sep 17 00:00:00 2001 From: Wadeck Follonier Date: Thu, 27 Sep 2018 11:00:36 +0200 Subject: [PATCH 01/10] Correct naming issues on test --- .../hudson/{PluginTest_SEC925.java => PluginSEC925Test.java} | 2 +- .../model/{QueueTest_SEC891.java => QueueSEC891Test.java} | 2 +- .../{UserRestartTest_SEC897.java => UserRestartSEC897Test.java} | 2 +- ...SEC996.java => TokenBasedRememberMeServices2SEC996Test.java} | 2 +- ...986.java => BasicHeaderApiTokenAuthenticatorSEC986Test.java} | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) rename test/src/test/java/hudson/{PluginTest_SEC925.java => PluginSEC925Test.java} (97%) rename test/src/test/java/hudson/model/{QueueTest_SEC891.java => QueueSEC891Test.java} (99%) rename test/src/test/java/hudson/model/{UserRestartTest_SEC897.java => UserRestartSEC897Test.java} (98%) rename test/src/test/java/hudson/security/{TokenBasedRememberMeServices2Test_SEC996.java => TokenBasedRememberMeServices2SEC996Test.java} (98%) rename test/src/test/java/jenkins/security/{BasicHeaderApiTokenAuthenticatorTest_SEC986.java => BasicHeaderApiTokenAuthenticatorSEC986Test.java} (99%) diff --git a/test/src/test/java/hudson/PluginTest_SEC925.java b/test/src/test/java/hudson/PluginSEC925Test.java similarity index 97% rename from test/src/test/java/hudson/PluginTest_SEC925.java rename to test/src/test/java/hudson/PluginSEC925Test.java index f52abce238..6ecf68ae1b 100644 --- a/test/src/test/java/hudson/PluginTest_SEC925.java +++ b/test/src/test/java/hudson/PluginSEC925Test.java @@ -13,7 +13,7 @@ import java.util.List; import java.util.concurrent.Future; //TODO merge it within PluginTest after the security release -public class PluginTest_SEC925 { +public class PluginSEC925Test { @Rule public JenkinsRule r = new JenkinsRule(); diff --git a/test/src/test/java/hudson/model/QueueTest_SEC891.java b/test/src/test/java/hudson/model/QueueSEC891Test.java similarity index 99% rename from test/src/test/java/hudson/model/QueueTest_SEC891.java rename to test/src/test/java/hudson/model/QueueSEC891Test.java index 30afef8739..ea3144eff3 100644 --- a/test/src/test/java/hudson/model/QueueTest_SEC891.java +++ b/test/src/test/java/hudson/model/QueueSEC891Test.java @@ -21,7 +21,7 @@ import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; //TODO merge into QueueTest after security patch -public class QueueTest_SEC891 { +public class QueueSEC891Test { @Rule public JenkinsRule r = new NodeProvisionerRule(-1, 0, 10); diff --git a/test/src/test/java/hudson/model/UserRestartTest_SEC897.java b/test/src/test/java/hudson/model/UserRestartSEC897Test.java similarity index 98% rename from test/src/test/java/hudson/model/UserRestartTest_SEC897.java rename to test/src/test/java/hudson/model/UserRestartSEC897Test.java index 854bbe4aec..c0c0885863 100644 --- a/test/src/test/java/hudson/model/UserRestartTest_SEC897.java +++ b/test/src/test/java/hudson/model/UserRestartSEC897Test.java @@ -16,7 +16,7 @@ import static org.hamcrest.core.IsEqual.equalTo; import static org.junit.Assert.assertThat; //TODO after the security fix, it could be merged inside UserRestartTest -public class UserRestartTest_SEC897 { +public class UserRestartSEC897Test { @Rule public RestartableJenkinsRule rr = new RestartableJenkinsRule(); diff --git a/test/src/test/java/hudson/security/TokenBasedRememberMeServices2Test_SEC996.java b/test/src/test/java/hudson/security/TokenBasedRememberMeServices2SEC996Test.java similarity index 98% rename from test/src/test/java/hudson/security/TokenBasedRememberMeServices2Test_SEC996.java rename to test/src/test/java/hudson/security/TokenBasedRememberMeServices2SEC996Test.java index b2fa21ce6c..1abecfbfc8 100644 --- a/test/src/test/java/hudson/security/TokenBasedRememberMeServices2Test_SEC996.java +++ b/test/src/test/java/hudson/security/TokenBasedRememberMeServices2SEC996Test.java @@ -37,7 +37,7 @@ import static org.hamcrest.xml.HasXPath.hasXPath; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThat; -public class TokenBasedRememberMeServices2Test_SEC996 { +public class TokenBasedRememberMeServices2SEC996Test { @Rule public JenkinsRule j = new JenkinsRule(); diff --git a/test/src/test/java/jenkins/security/BasicHeaderApiTokenAuthenticatorTest_SEC986.java b/test/src/test/java/jenkins/security/BasicHeaderApiTokenAuthenticatorSEC986Test.java similarity index 99% rename from test/src/test/java/jenkins/security/BasicHeaderApiTokenAuthenticatorTest_SEC986.java rename to test/src/test/java/jenkins/security/BasicHeaderApiTokenAuthenticatorSEC986Test.java index f72c407b70..dd63951f7c 100644 --- a/test/src/test/java/jenkins/security/BasicHeaderApiTokenAuthenticatorTest_SEC986.java +++ b/test/src/test/java/jenkins/security/BasicHeaderApiTokenAuthenticatorSEC986Test.java @@ -52,7 +52,7 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThat; -public class BasicHeaderApiTokenAuthenticatorTest_SEC986 { +public class BasicHeaderApiTokenAuthenticatorSEC986Test { @Rule public RestartableJenkinsRule rr = new RestartableJenkinsRule(); From 58fecdcdefa2ef2e9abb2e08d9d7c7a6b7d2027f Mon Sep 17 00:00:00 2001 From: Daniel Beck Date: Thu, 27 Sep 2018 00:16:04 +0200 Subject: [PATCH 02/10] [SECURITY-1162] --- .../BasicHeaderApiTokenAuthenticator.java | 39 ++++++++++--------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/core/src/main/java/jenkins/security/BasicHeaderApiTokenAuthenticator.java b/core/src/main/java/jenkins/security/BasicHeaderApiTokenAuthenticator.java index 345281fdce..f8a804c9e3 100644 --- a/core/src/main/java/jenkins/security/BasicHeaderApiTokenAuthenticator.java +++ b/core/src/main/java/jenkins/security/BasicHeaderApiTokenAuthenticator.java @@ -28,27 +28,28 @@ public class BasicHeaderApiTokenAuthenticator extends BasicHeaderAuthenticator { */ @Override public Authentication authenticate(HttpServletRequest req, HttpServletResponse rsp, String username, String password) throws ServletException { - // attempt to authenticate as API token - User u = User.getById(username, true); - ApiTokenProperty t = u.getProperty(ApiTokenProperty.class); - if (t!=null && t.matchesPassword(password)) { - Authentication auth; - try { - UserDetails userDetails = u.getUserDetailsForImpersonation(); - auth = u.impersonate(userDetails); + User u = BasicApiTokenHelper.isConnectingUsingApiToken(username, password); + if(u != null) { + ApiTokenProperty t = u.getProperty(ApiTokenProperty.class); + if (t != null && t.matchesPassword(password)) { + Authentication auth; + try { + UserDetails userDetails = u.getUserDetailsForImpersonation(); + auth = u.impersonate(userDetails); - SecurityListener.fireAuthenticated(userDetails); - } catch (UsernameNotFoundException x) { - // The token was valid, but the impersonation failed. This token is clearly not his real password, - // so there's no point in continuing the request processing. Report this error and abort. - LOGGER.log(WARNING, "API token matched for user "+username+" but the impersonation failed",x); - throw new ServletException(x); - } catch (DataAccessException x) { - throw new ServletException(x); + SecurityListener.fireAuthenticated(userDetails); + } catch (UsernameNotFoundException x) { + // The token was valid, but the impersonation failed. This token is clearly not his real password, + // so there's no point in continuing the request processing. Report this error and abort. + LOGGER.log(WARNING, "API token matched for user " + username + " but the impersonation failed", x); + throw new ServletException(x); + } catch (DataAccessException x) { + throw new ServletException(x); + } + + req.setAttribute(BasicHeaderApiTokenAuthenticator.class.getName(), true); + return auth; } - - req.setAttribute(BasicHeaderApiTokenAuthenticator.class.getName(), true); - return auth; } return null; } From 517da6ed389f0a606dd9bb8595bc79fc93f4331c Mon Sep 17 00:00:00 2001 From: Wadeck Follonier Date: Thu, 27 Sep 2018 11:12:53 +0200 Subject: [PATCH 03/10] [SECURITY-1158] --- .../security/HudsonPrivateSecurityRealm.java | 9 +++ ...udsonPrivateSecurityRealm2SEC1158Test.java | 73 +++++++++++++++++++ 2 files changed, 82 insertions(+) create mode 100644 test/src/test/java/hudson/security/HudsonPrivateSecurityRealm2SEC1158Test.java diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index c787e352f2..01b3233108 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -73,6 +73,8 @@ import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import javax.servlet.http.HttpSession; + import static javax.servlet.http.HttpServletResponse.SC_UNAUTHORIZED; import java.io.IOException; import java.lang.reflect.Constructor; @@ -263,6 +265,13 @@ public class HudsonPrivateSecurityRealm extends AbstractPasswordBasedSecurityRea */ @SuppressWarnings("ACL.impersonate") private void loginAndTakeBack(StaplerRequest req, StaplerResponse rsp, User u) throws ServletException, IOException { + HttpSession session = req.getSession(false); + if (session != null) { + // avoid session fixation + session.invalidate(); + } + req.getSession(true); + // ... and let him login Authentication a = new UsernamePasswordAuthenticationToken(u.getId(),req.getParameter("password1")); a = this.getSecurityComponents().manager.authenticate(a); diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealm2SEC1158Test.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealm2SEC1158Test.java new file mode 100644 index 0000000000..fca41a931d --- /dev/null +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealm2SEC1158Test.java @@ -0,0 +1,73 @@ +/* + * The MIT License + * + * Copyright (c) 2015, CloudBees, Inc + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +package hudson.security; + +import com.gargoylesoftware.htmlunit.html.HtmlPage; +import com.gargoylesoftware.htmlunit.util.Cookie; +import hudson.security.pages.SignupPage; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.Issue; +import org.jvnet.hudson.test.JenkinsRule; + +import static org.hamcrest.Matchers.containsString; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertThat; + +//TODO to be merged into HudsonPrivateSecurityRealm2Test after security release +public class HudsonPrivateSecurityRealm2SEC1158Test { + + @Rule + public JenkinsRule rule = new JenkinsRule(); + + @Test + @Issue("SECURITY-1158") + public void singupNoLongerVulnerableToSessionFixation() throws Exception { + HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(true, false, null); + rule.jenkins.setSecurityRealm(securityRealm); + JenkinsRule.WebClient wc = rule.createWebClient(); + + // to trigger the creation of a session + wc.goTo(""); + Cookie sessionBefore = wc.getCookieManager().getCookie("JSESSIONID"); + String sessionIdBefore = sessionBefore.getValue(); + + SignupPage signup = new SignupPage(wc.goTo("signup")); + signup.enterUsername("alice"); + signup.enterPassword("alice"); + signup.enterFullName("Alice User"); + signup.enterEmail("alice@nowhere.com"); + HtmlPage success = signup.submit(rule); + assertThat(success.getElementById("main-panel").getTextContent(), containsString("Success")); + assertThat(success.getAnchorByHref("/jenkins/user/alice").getTextContent(), containsString("Alice User")); + + assertEquals("Alice User", securityRealm.getUser("alice").getDisplayName()); + + Cookie sessionAfter = wc.getCookieManager().getCookie("JSESSIONID"); + String sessionIdAfter = sessionAfter.getValue(); + + assertNotEquals(sessionIdAfter, sessionIdBefore); + } +} From df87e12ddcfeafdba6e0de0e07b3e21f8473ece6 Mon Sep 17 00:00:00 2001 From: Ramon Leon Date: Thu, 27 Sep 2018 11:15:28 +0200 Subject: [PATCH 04/10] [SECURITY-1129] --- core/src/main/java/hudson/model/Api.java | 12 +++ .../hudson/model/Messages.properties | 1 + .../hudson/model/Messages_es.properties | 1 + .../hudson/model/Messages_fr.properties | 2 + .../hudson/model/ApiSecurity1129Test.java | 86 +++++++++++++++++++ 5 files changed, 102 insertions(+) create mode 100644 test/src/test/java/hudson/model/ApiSecurity1129Test.java diff --git a/core/src/main/java/hudson/model/Api.java b/core/src/main/java/hudson/model/Api.java index 6878a4284a..d3b904aa73 100644 --- a/core/src/main/java/hudson/model/Api.java +++ b/core/src/main/java/hudson/model/Api.java @@ -133,7 +133,19 @@ public class Api extends AbstractModelObject { XPath comp = dom.createXPath(xpath); comp.setFunctionContext(functionContext); List list = comp.selectNodes(dom); + if (wrapper!=null) { + // check if the wrapper is a valid entity name + // First position: letter or underscore + // Other positions: \w (letter, number, underscore), dash or dot + String validNameRE = "^[a-zA-Z_][\\w-\\.]*$"; + + if(!wrapper.matches(validNameRE)) { + rsp.setStatus(HttpServletResponse.SC_BAD_REQUEST); + rsp.getWriter().print(Messages.Api_WrapperParamInvalid()); + return; + } + Element root = DocumentFactory.getInstance().createElement(wrapper); for (Object o : list) { if (o instanceof String) { diff --git a/core/src/main/resources/hudson/model/Messages.properties b/core/src/main/resources/hudson/model/Messages.properties index 92812110fd..cce1073695 100644 --- a/core/src/main/resources/hudson/model/Messages.properties +++ b/core/src/main/resources/hudson/model/Messages.properties @@ -84,6 +84,7 @@ AbstractProject.LabelLink=Label {1} is serviced by {3,choic Api.MultipleMatch=XPath "{0}" matched {1} nodes. \ Create XPath that only matches one, or use the "wrapper" query parameter to wrap them all under a root element. Api.NoXPathMatch=XPath {0} didn\u2019t match +Api.WrapperParamInvalid=The wrapper parameter can only contain alphanumeric characters or dash/dot/underscore. The first character has to be a letter or underscore. BallColor.Aborted=Aborted BallColor.Disabled=Disabled diff --git a/core/src/main/resources/hudson/model/Messages_es.properties b/core/src/main/resources/hudson/model/Messages_es.properties index 8d581e01e4..4872e1025c 100644 --- a/core/src/main/resources/hudson/model/Messages_es.properties +++ b/core/src/main/resources/hudson/model/Messages_es.properties @@ -47,6 +47,7 @@ AbstractProject.ExtendedReadPermission.Description=\ Api.MultipleMatch=XPath "{0}" encontr\u00f3 coincidencias en {1} nodos. \ Crea una expresi\u00f3n XPath que s\u00f3lo encuentre uno, o utiliza el par\u00e1metro "wraper" para que todos los nodos se agrupen bajo un elemento. Api.NoXPathMatch=XPath {0} no encontr\u00f3 nada +Api.WrapperParamInvalid=El par\u00E1metro wrapper s\u00F3lo puede contener caracteres alfanum\u00E9ricos o guiones/puntos/subrayado. El primer car\u00E1cter debe ser una letra o subrayado. BallColor.Aborted=Abortado BallColor.Disabled=Desactivado diff --git a/core/src/main/resources/hudson/model/Messages_fr.properties b/core/src/main/resources/hudson/model/Messages_fr.properties index d1ded2fc0f..7b65394ff0 100644 --- a/core/src/main/resources/hudson/model/Messages_fr.properties +++ b/core/src/main/resources/hudson/model/Messages_fr.properties @@ -41,6 +41,7 @@ AbstractProject.WorkspacePermission.Description=\ Api.MultipleMatch=Le XPath "{0}" correspond \u00e0 {1} noeud(s). \ Merci de fournir un XPath qui ne correspond qu''\u00e0 un seul noeud, ou utilisez le param\u00e8tre de requ\u00e8te "wrapper" pour les encapsuler tous dans un \u00e9l\u00e9ment racine. Api.NoXPathMatch=Pas de correspondance avec le XPath {0} +Api.WrapperParamInvalid=Le param\u00E8tre wrapper ne doit contenir que des caract\u00E8res alphanum\u00E9riques ainsi que des points / tirets bas / traits d''union, et finalement le premier caract\u00E8re doit \u00EAtre une lettre ou un trait d''union. BallColor.Aborted=Annul\u00e9 BallColor.Disabled=D\u00e9sactiv\u00e9 @@ -178,3 +179,4 @@ MyViewsProperty.GlobalAction.DisplayName=Mes vues ManageJenkinsAction.DisplayName=Administrer Jenkins ParametersDefinitionProperty.DisplayName=Ce build a des paramètres + diff --git a/test/src/test/java/hudson/model/ApiSecurity1129Test.java b/test/src/test/java/hudson/model/ApiSecurity1129Test.java new file mode 100644 index 0000000000..999eacb43e --- /dev/null +++ b/test/src/test/java/hudson/model/ApiSecurity1129Test.java @@ -0,0 +1,86 @@ +package hudson.model; + +import com.gargoylesoftware.htmlunit.WebResponse; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.Issue; +import org.jvnet.hudson.test.JenkinsRule; +import org.xml.sax.SAXException; + +import javax.servlet.http.HttpServletResponse; +import java.io.IOException; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + +//TODO after the security fix, it could be merged inside ApiTest +public class ApiSecurity1129Test { + @Rule + public JenkinsRule j = new JenkinsRule(); + + /** + * Test the wrapper parameter for the api/xml urls to avoid XSS. + * @throws Exception See {@link #checkWrapperParam(String, Integer, String)} + */ + @Issue("SECURITY-1129") + @Test + public void wrapperXss() throws Exception { + String wrapper = "html%20xmlns=\"http://www.w3.org/1999/xhtml\"> diff --git a/test/src/test/java/jenkins/security/stapler/Security867Test.java b/test/src/test/java/jenkins/security/stapler/Security867Test.java new file mode 100644 index 0000000000..8527cac809 --- /dev/null +++ b/test/src/test/java/jenkins/security/stapler/Security867Test.java @@ -0,0 +1,167 @@ +/* + * The MIT License + * + * Copyright (c) 2018, CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +package jenkins.security.stapler; + +import com.gargoylesoftware.htmlunit.Page; +import hudson.model.RootAction; +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.TestExtension; + +import javax.annotation.CheckForNull; + +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; +import static org.junit.Assert.assertThat; + +public class Security867Test { + @Rule + public JenkinsRule j = new JenkinsRule(); + + @Test + @Issue("SECURITY-867") + public void folderTraversalPrevented_avoidStealingSecretInView() throws Exception { + JenkinsRule.WebClient wc = j.createWebClient(); + wc.getOptions().setThrowExceptionOnFailingStatusCode(false); + + String publicContent = "Test OK"; + String secretContent = "s3cr3t"; + + // to validate the attack reproduction you can disable the protection + // Facet.ALLOW_VIEW_NAME_PATH_TRAVERSAL = true; + + // regular behavior + assertThat(getContentAndCheck200(wc, "rootAction1/public"), containsString(publicContent)); + + // malicious usage prevention + + // looking for /jenkins/security/stapler/Security867Test/NotRootAction2/secret + assertThat(getContent(wc, "rootAction1/%2fjenkins%2fsecurity%2fstapler%2fSecurity867Test%2fNotRootAction2%2fsecret"), + not(containsString(secretContent))); + + // looking for /jenkins\security\stapler\Security867Test\NotRootAction2\secret => + // absolute path with backslash (initial forward one is required for absolute) + assertThat(getContent(wc,"rootAction1/%2fjenkins%5csecurity%5cstapler%5cSecurity867Test%5cNotRootAction2%5csecret"), + not(containsString(secretContent))); + + // looking for ../NotRootAction2/secret => relative path + assertThat(getContent(wc,"rootAction1/%2e%2e%2fNotRootAction2%2fsecret"), + not(containsString(secretContent))); + + // looking for ..\NotRootAction2\secret => relative path without forward slash + assertThat(getContent(wc, "rootAction1/%2e%2e%5cNotRootAction2%5csecret"), + not(containsString(secretContent))); + } + + private String getContent(JenkinsRule.WebClient wc, String url) throws Exception { + Page page = wc.goTo(url, null); + return page.getWebResponse().getContentAsString(); + } + + private String getContentAndCheck200(JenkinsRule.WebClient wc, String url) throws Exception { + Page page = wc.goTo(url, null); + assertThat(page.getWebResponse().getStatusCode(), equalTo(200)); + return page.getWebResponse().getContentAsString(); + } + + @Test + @Issue("SECURITY-867") + public void folderTraversalPrevented_avoidStealingSecretFromDifferentObject() throws Exception { + JenkinsRule.WebClient wc = j.createWebClient(); + wc.getOptions().setThrowExceptionOnFailingStatusCode(false); + + String action1Config = j.jenkins.getExtensionList(RootAction.class).get(RootAction1.class).getMyConfig(); + String action3Config = j.jenkins.getExtensionList(RootAction.class).get(RootAction3.class).getMyConfig(); + + // to validate the attack reproduction you can disable the protection + // Facet.ALLOW_VIEW_NAME_PATH_TRAVERSAL = true; + + // regular behavior, the config is only displayed in ActionRoot3 + assertThat(getContentAndCheck200(wc, "rootAction1/public"), not(containsString(action1Config))); + assertThat(getContentAndCheck200(wc, "rootAction3/showConfig"), allOf( + containsString(action3Config), + not(containsString(action1Config)) + )); + + // the main point here is the last node visited will be "it" for the view scope + // if we navigate by RootAction1, we pass it to the RootAction3's view + + // malicious usage prevention, looking for ../RootAction3/showConfig => relative path + // without the prevention, the config value of RootAction1 will be used here + assertThat(getContent(wc, "rootAction1/%2e%2e%2fRootAction3%2fshowConfig"), allOf( + not(containsString(action1Config)), + not(containsString(action3Config)) + )); + } + + @TestExtension + public static class RootAction1 implements RootAction { + // not displayed in its own public.jelly + public String getMyConfig() { + return "config-1"; + } + + @Override + public @CheckForNull String getIconFileName() { + return null; + } + + @Override + public @CheckForNull String getDisplayName() { + return null; + } + + @Override + public @CheckForNull String getUrlName() { + return "rootAction1"; + } + } + + @TestExtension + public static class RootAction3 implements RootAction { + // displayed in its showConfig.jelly + public String getMyConfig() { + return "config-3"; + } + + @Override + public @CheckForNull String getIconFileName() { + return null; + } + + @Override + public @CheckForNull String getDisplayName() { + return null; + } + + @Override + public @CheckForNull String getUrlName() { + return "rootAction3"; + } + } +} diff --git a/test/src/test/resources/jenkins/security/stapler/Security867Test/NotRootAction2/secret.jelly b/test/src/test/resources/jenkins/security/stapler/Security867Test/NotRootAction2/secret.jelly new file mode 100644 index 0000000000..05fb260ccb --- /dev/null +++ b/test/src/test/resources/jenkins/security/stapler/Security867Test/NotRootAction2/secret.jelly @@ -0,0 +1,5 @@ + + + +

s3cr3t

+ diff --git a/test/src/test/resources/jenkins/security/stapler/Security867Test/RootAction1/public.jelly b/test/src/test/resources/jenkins/security/stapler/Security867Test/RootAction1/public.jelly new file mode 100644 index 0000000000..aae5f62d2c --- /dev/null +++ b/test/src/test/resources/jenkins/security/stapler/Security867Test/RootAction1/public.jelly @@ -0,0 +1,5 @@ + + + +

Test OK

+ diff --git a/test/src/test/resources/jenkins/security/stapler/Security867Test/RootAction3/showConfig.jelly b/test/src/test/resources/jenkins/security/stapler/Security867Test/RootAction3/showConfig.jelly new file mode 100644 index 0000000000..32943accd9 --- /dev/null +++ b/test/src/test/resources/jenkins/security/stapler/Security867Test/RootAction3/showConfig.jelly @@ -0,0 +1,5 @@ + + + +

Config: ${it.myConfig}

+ From 7366cc50106442a021c5178cd101057ecc08f2c2 Mon Sep 17 00:00:00 2001 From: Daniel Beck Date: Thu, 27 Sep 2018 11:34:02 +0200 Subject: [PATCH 08/10] [SECURITY-765] Co-Authored-By: Wadeck Follonier --- .../main/java/hudson/model/Descriptor.java | 3 +- core/src/main/java/jenkins/model/Jenkins.java | 7 +- ...dactSecretJsonInErrorMessageSanitizer.java | 118 +++++++ ...SecretJsonInErrorMessageSanitizerTest.java | 139 ++++++++ ...etJsonInErrorMessageSanitizerHtmlTest.java | 298 ++++++++++++++++++ .../TestDescribablePage/index.jelly | 39 +++ .../TestPassword/index.jelly | 53 ++++ .../TestStaplerPage/index.jelly | 39 +++ .../main/webapp/scripts/hudson-behavior.js | 7 +- 9 files changed, 699 insertions(+), 4 deletions(-) create mode 100644 core/src/main/java/jenkins/security/RedactSecretJsonInErrorMessageSanitizer.java create mode 100644 core/src/test/java/jenkins/security/RedactSecretJsonInErrorMessageSanitizerTest.java create mode 100644 test/src/test/java/jenkins/security/RedactSecretJsonInErrorMessageSanitizerHtmlTest.java create mode 100644 test/src/test/resources/jenkins/security/RedactSecretJsonInErrorMessageSanitizerHtmlTest/TestDescribablePage/index.jelly create mode 100644 test/src/test/resources/jenkins/security/RedactSecretJsonInErrorMessageSanitizerHtmlTest/TestPassword/index.jelly create mode 100644 test/src/test/resources/jenkins/security/RedactSecretJsonInErrorMessageSanitizerHtmlTest/TestStaplerPage/index.jelly diff --git a/core/src/main/java/hudson/model/Descriptor.java b/core/src/main/java/hudson/model/Descriptor.java index 1de0c37080..9994458814 100644 --- a/core/src/main/java/hudson/model/Descriptor.java +++ b/core/src/main/java/hudson/model/Descriptor.java @@ -39,6 +39,7 @@ import hudson.views.ListViewColumn; import jenkins.model.GlobalConfiguration; import jenkins.model.GlobalConfigurationCategory; import jenkins.model.Jenkins; +import jenkins.security.RedactSecretJsonInErrorMessageSanitizer; import jenkins.util.io.OnMaster; import net.sf.json.JSONArray; import net.sf.json.JSONObject; @@ -597,7 +598,7 @@ public abstract class Descriptor> implements Saveable, } catch (NoSuchMethodException e) { throw new AssertionError(e); // impossible } catch (InstantiationException | IllegalAccessException | RuntimeException e) { - throw new Error("Failed to instantiate "+clazz+" from "+formData,e); + throw new Error("Failed to instantiate "+clazz+" from "+RedactSecretJsonInErrorMessageSanitizer.INSTANCE.sanitize(formData),e); } } diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index f7ab825696..64fe4c9947 100644 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -37,6 +37,7 @@ import hudson.*; import hudson.Launcher.LocalLauncher; import jenkins.AgentProtocol; import jenkins.diagnostics.URICheckEncodingMonitor; +import jenkins.security.RedactSecretJsonInErrorMessageSanitizer; import jenkins.util.SystemProperties; import hudson.cli.declarative.CLIMethod; import hudson.cli.declarative.CLIResolver; @@ -898,9 +899,11 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve if (pluginManager==null) pluginManager = PluginManager.createDefault(this); this.pluginManager = pluginManager; + WebApp webApp = WebApp.get(servletContext); // JSON binding needs to be able to see all the classes from all the plugins - WebApp.get(servletContext).setClassLoader(pluginManager.uberClassLoader); - + webApp.setClassLoader(pluginManager.uberClassLoader); + webApp.setJsonInErrorMessageSanitizer(RedactSecretJsonInErrorMessageSanitizer.INSTANCE); + adjuncts = new AdjunctManager(servletContext, pluginManager.uberClassLoader,"adjuncts/"+SESSION_HASH, TimeUnit2.DAYS.toMillis(365)); // TODO pending move to standard blacklist, or API to append filter diff --git a/core/src/main/java/jenkins/security/RedactSecretJsonInErrorMessageSanitizer.java b/core/src/main/java/jenkins/security/RedactSecretJsonInErrorMessageSanitizer.java new file mode 100644 index 0000000000..e7b0e7f7df --- /dev/null +++ b/core/src/main/java/jenkins/security/RedactSecretJsonInErrorMessageSanitizer.java @@ -0,0 +1,118 @@ +/* + * The MIT License + * + * Copyright (c) 2018, CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +package jenkins.security; + +import net.sf.json.JSONArray; +import net.sf.json.JSONObject; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; +import org.kohsuke.stapler.JsonInErrorMessageSanitizer; + +import java.util.HashSet; +import java.util.Set; +import java.util.logging.Level; +import java.util.logging.Logger; + +@Restricted(NoExternalUse.class) +public class RedactSecretJsonInErrorMessageSanitizer implements JsonInErrorMessageSanitizer { + private static final Logger LOGGER = Logger.getLogger(RedactSecretJsonInErrorMessageSanitizer.class.getName()); + + // must be kept in sync with hudson-behavior.js in function buildFormTree, password case + public static final String REDACT_KEY = "$redact"; + public static final String REDACT_VALUE = "[value redacted]"; + + public static final RedactSecretJsonInErrorMessageSanitizer INSTANCE = new RedactSecretJsonInErrorMessageSanitizer(); + + private RedactSecretJsonInErrorMessageSanitizer() {} + + @Override + public JSONObject sanitize(JSONObject jsonObject) { + return copyAndSanitizeObject(jsonObject); + } + + /** + * Accept anything as value for the {@link #REDACT_KEY} but only process the first level of an array and the string value. + */ + private Set retrieveRedactedKeys(JSONObject jsonObject) { + Set redactedKeySet = new HashSet<>(); + if (jsonObject.has(REDACT_KEY)) { + Object value = jsonObject.get(REDACT_KEY); + if (value instanceof JSONArray) { + for (Object o : jsonObject.getJSONArray(REDACT_KEY)) { + if (o instanceof String) { + redactedKeySet.add((String) o); + } else { + // array, object, null, number, boolean + LOGGER.log(Level.WARNING, "Unsupported type " + o.getClass().getName() + " for " + REDACT_KEY + ", please use either a single String value or an Array"); + } + } + } else if (value instanceof String) { + redactedKeySet.add((String) value); + } else { + // object, null, number, boolean + LOGGER.log(Level.WARNING, "Unsupported type " + value.getClass().getName() + " for " + REDACT_KEY + ", please use either a single String value or an Array"); + } + } + return redactedKeySet; + } + + private Object copyAndSanitize(Object value) { + if (value instanceof JSONObject) { + return copyAndSanitizeObject((JSONObject) value); + } else if (value instanceof JSONArray) { + return copyAndSanitizeArray((JSONArray) value); + } else { + // string, null, number, boolean + return value; + } + } + + @SuppressWarnings("unchecked") + private JSONObject copyAndSanitizeObject(JSONObject jsonObject) { + Set redactedKeySet = retrieveRedactedKeys(jsonObject); + JSONObject result = new JSONObject(); + + jsonObject.keySet().forEach(keyObject -> { + String key = keyObject.toString(); + if (redactedKeySet.contains(key)) { + result.accumulate(key, REDACT_VALUE); + } else { + Object value = jsonObject.get(keyObject); + result.accumulate(key, copyAndSanitize(value)); + } + }); + + return result; + } + + private JSONArray copyAndSanitizeArray(JSONArray jsonArray) { + JSONArray result = new JSONArray(); + + jsonArray.forEach(value -> + result.add(copyAndSanitize(value)) + ); + + return result; + } +} diff --git a/core/src/test/java/jenkins/security/RedactSecretJsonInErrorMessageSanitizerTest.java b/core/src/test/java/jenkins/security/RedactSecretJsonInErrorMessageSanitizerTest.java new file mode 100644 index 0000000000..9a975481c2 --- /dev/null +++ b/core/src/test/java/jenkins/security/RedactSecretJsonInErrorMessageSanitizerTest.java @@ -0,0 +1,139 @@ +/* + * The MIT License + * + * Copyright (c) 2018, CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +package jenkins.security; + +import net.sf.json.JSONObject; +import org.junit.Test; +import org.jvnet.hudson.test.Issue; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotSame; + +@Issue("SECURITY-765") +public class RedactSecretJsonInErrorMessageSanitizerTest { + @Test + public void noSecrets() throws Exception { + assertRedaction( + "{'a': 1, 'b': '2', 'c': {'c1': 1, 'c2': '2', 'c3': ['3a', '3b']}, 'd': ['4a', {'d1': 1, 'd2': '2'}]}", + "{'a': 1, 'b': '2', 'c': {'c1': 1, 'c2': '2', 'c3': ['3a', '3b']}, 'd': ['4a', {'d1': 1, 'd2': '2'}]}" + ); + } + + @Test + public void simpleWithSecret() throws Exception { + assertRedaction( + "{'a': 'secret', 'b': 'other', '$redact': 'a'}", + "{'a': '[value redacted]', 'b': 'other', '$redact': 'a'}" + ); + } + + @Test + public void singleWithRedactedInArray() throws Exception { + assertRedaction( + "{'a': 'secret', 'b': 'other', '$redact': ['a']}", + "{'a': '[value redacted]', 'b': 'other', '$redact': ['a']}" + ); + } + + @Test + public void objectRedactedAcceptedButNotProcessed() throws Exception { + assertRedaction( + "{'a': 'secret', 'b': 'other', '$redact': {'a': 'a'}}", + "{'a': 'secret', 'b': 'other', '$redact': {'a': 'a'}}" + ); + } + + @Test + public void weirdValuesInRedactedAcceptedButNotProcessed() throws Exception { + assertRedaction( + "{'a': 'secret', 'b': 'other', '$redact': [null, true, false, 1, 2, 'a']}", + "{'a': '[value redacted]', 'b': 'other', '$redact': [null, true, false, 1, 2, 'a']}" + ); + } + + @Test + public void ensureTrueAndOneAsStringAreSupportedAsRedactedKey() throws Exception { + //only null is not supported, as passing 'null' is considered as null + assertRedaction( + "{'true': 'secret1', '1': 'secret3', 'b': 'other', '$redact': ['true', '1']}", + "{'true': '[value redacted]', '1': '[value redacted]', 'b': 'other', '$redact': ['true', '1']}" + ); + } + + @Test + public void redactFullBranch() throws Exception { + assertRedaction( + "{'a': {'s1': 'secret1', 's2': 'secret2', 's3': [1,2,3]}, 'b': [4,5,6], 'c': 'other', '$redact': ['a', 'b']}", + "{'a': '[value redacted]', 'b': '[value redacted]', 'c': 'other', '$redact': ['a', 'b']}" + ); + } + + @Test + public void multipleSecretAtSameLevel() throws Exception { + assertRedaction( + "{'a1': 'secret1', 'a2': 'secret2', 'b': 'other', '$redact': ['a1', 'a2']}", + "{'a1': '[value redacted]', 'a2': '[value redacted]', 'b': 'other', '$redact': ['a1', 'a2']}" + ); + } + + @Test + public void redactedKeyWithoutCorrespondences() throws Exception { + assertRedaction( + "{'a1': 'secret1', 'a2': 'secret2', 'b': 'other', '$redact': ['a0', 'a1', 'a2', 'a3']}", + "{'a1': '[value redacted]', 'a2': '[value redacted]', 'b': 'other', '$redact': ['a0', 'a1', 'a2', 'a3']}" + ); + } + + @Test + public void secretsAtMultipleLevels() throws Exception { + assertRedaction( + "{'a1': 'secret1', 'a2': 'secret2', 'b': 'other', '$redact': ['a1', 'a2'], 'sub': {'c1': 'secret1', 'c2': 'secret2', 'c3': 'other', '$redact': ['c1', 'c2']}}", + "{'a1': '[value redacted]', 'a2': '[value redacted]', 'b': 'other', '$redact': ['a1', 'a2'], 'sub': {'c1': '[value redacted]', 'c2': '[value redacted]', 'c3': 'other', '$redact': ['c1', 'c2']}}" + ); + } + + @Test + public void noInteractionBetweenLevels() throws Exception { + assertRedaction( + "{'a': 'secret', 'b': 'other', 'c': 'other', '$redact': 'a', 'sub': {'a': 'other', 'b': 'secret', 'c': 'other', '$redact': 'b'}}", + "{'a': '[value redacted]', 'b': 'other', 'c': 'other', '$redact': 'a', 'sub': {'a': 'other', 'b': '[value redacted]', 'c': 'other', '$redact': 'b'}}" + ); + } + + @Test + public void deeplyNestedObject() throws Exception { + assertRedaction( + "{'sub': {'arr': ['d1', 2, {'a1': 'other', 'b1':'other', 'c1': 'secret', '$redact': 'c1'}, 4, {'a2': 'other', 'b2': 'other', 'c2': 'secret', '$redact': 'c2'}]}, '$redact': 'b'}", + "{'sub': {'arr': ['d1', 2, {'a1': 'other', 'b1':'other', 'c1': '[value redacted]', '$redact': 'c1'}, 4, {'a2': 'other', 'b2': 'other', 'c2': '[value redacted]', '$redact': 'c2'}]}, '$redact': 'b'}" + ); + } + + private void assertRedaction(String from, String to) { + JSONObject input = JSONObject.fromObject(from.replace('\'', '"')); + JSONObject output = RedactSecretJsonInErrorMessageSanitizer.INSTANCE.sanitize(input); + assertNotSame(output, input); + assertEquals("redaction of " + from, to.replace('\'', '"').replace(" ", ""), + output.toString().replace(" ", "")); + } +} diff --git a/test/src/test/java/jenkins/security/RedactSecretJsonInErrorMessageSanitizerHtmlTest.java b/test/src/test/java/jenkins/security/RedactSecretJsonInErrorMessageSanitizerHtmlTest.java new file mode 100644 index 0000000000..3a09f1aa80 --- /dev/null +++ b/test/src/test/java/jenkins/security/RedactSecretJsonInErrorMessageSanitizerHtmlTest.java @@ -0,0 +1,298 @@ +/* + * The MIT License + * + * Copyright (c) 2018 CloudBees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +package jenkins.security; + +import com.gargoylesoftware.htmlunit.Page; +import com.gargoylesoftware.htmlunit.html.HtmlForm; +import com.gargoylesoftware.htmlunit.html.HtmlInput; +import com.gargoylesoftware.htmlunit.html.HtmlPage; +import hudson.model.Describable; +import hudson.model.Descriptor; +import hudson.model.RootAction; +import hudson.util.Secret; +import jenkins.model.Jenkins; +import net.sf.json.JSONObject; +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.LoggerRule; +import org.jvnet.hudson.test.TestExtension; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; +import org.kohsuke.stapler.DataBoundConstructor; +import org.kohsuke.stapler.StaplerRequest; +import org.kohsuke.stapler.StaplerResponse; +import org.kohsuke.stapler.interceptor.RequirePOST; + +import java.io.PrintWriter; +import java.io.StringWriter; +import java.util.logging.Level; + +import static org.hamcrest.CoreMatchers.allOf; +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.CoreMatchers.hasItem; +import static org.hamcrest.CoreMatchers.not; +import static org.junit.Assert.assertThat; + +@Restricted(NoExternalUse.class) +public class RedactSecretJsonInErrorMessageSanitizerHtmlTest { + + @Rule + public JenkinsRule j = new JenkinsRule(); + + @Rule + public LoggerRule logging = new LoggerRule(); + + @Test + @Issue("SECURITY-765") + public void passwordsAreRedacted_andOtherStayTheSame() throws Exception { + j.jenkins.setCrumbIssuer(null); + TestPassword testPassword = j.jenkins.getExtensionList(RootAction.class).get(TestPassword.class); + + JenkinsRule.WebClient wc = j.createWebClient(); + HtmlPage page = wc.goTo("test"); + + String textSimple = "plain-1"; + String pwdSimple = "secret-1"; + ((HtmlInput) page.getElementById("text-simple")).setValueAttribute(textSimple); + ((HtmlInput) page.getElementById("pwd-simple")).setValueAttribute(pwdSimple); + + String textLevelOne = "plain-2"; + String pwdLevelOneA = "secret-2"; + String pwdLevelOneB = "secret-3"; + ((HtmlInput) page.getElementById("text-level-one")).setValueAttribute(textLevelOne); + ((HtmlInput) page.getElementById("pwd-level-one-a")).setValueAttribute(pwdLevelOneA); + ((HtmlInput) page.getElementById("pwd-level-one-b")).setValueAttribute(pwdLevelOneB); + + HtmlForm form = page.getFormByName("config"); + Page formSubmitPage = j.submit(form); + assertThat(formSubmitPage.getWebResponse().getStatusCode(), equalTo(200)); + + JSONObject rawJson = testPassword.lastJsonReceived; + String rawJsonToString = rawJson.toString(); + assertThat(rawJsonToString, containsString(textSimple)); + assertThat(rawJsonToString, containsString(pwdSimple)); + assertThat(rawJsonToString, containsString(textLevelOne)); + assertThat(rawJsonToString, containsString(pwdLevelOneA)); + + assertThat(rawJson.getString(RedactSecretJsonInErrorMessageSanitizer.REDACT_KEY), equalTo("pwd-simple")); + assertThat( + rawJson.getJSONObject("sub-one").getJSONArray(RedactSecretJsonInErrorMessageSanitizer.REDACT_KEY), + allOf( + hasItem("pwd-level-one-a"), + hasItem("pwd-level-one-b") + ) + ); + + JSONObject redactedJson = RedactSecretJsonInErrorMessageSanitizer.INSTANCE.sanitize(rawJson); + String redactedJsonToString = redactedJson.toString(); + assertThat(redactedJsonToString, containsString(textSimple)); + assertThat(redactedJsonToString, not(containsString(pwdSimple))); + assertThat(redactedJsonToString, containsString(textLevelOne)); + assertThat(redactedJsonToString, not(containsString(pwdLevelOneA))); + assertThat(redactedJsonToString, not(containsString(pwdLevelOneB))); + assertThat(redactedJsonToString, containsString(RedactSecretJsonInErrorMessageSanitizer.REDACT_VALUE)); + } + + @TestExtension("passwordsAreRedacted_andOtherStayTheSame") + public static class TestPassword implements RootAction { + + public JSONObject lastJsonReceived; + + public void doSubmitTest(StaplerRequest req, StaplerResponse res) throws Exception { + lastJsonReceived = req.getSubmittedForm(); + + res.setStatus(200); + } + + @Override + public String getIconFileName() { + return null; + } + + @Override + public String getDisplayName() { + return null; + } + + @Override + public String getUrlName() { + return "test"; + } + } + + @Test + @Issue("SECURITY-765") + public void checkSanitizationIsApplied_inDescriptor() throws Exception { + logging.record("/jenkins", Level.ALL).capture(1); + + j.jenkins.setCrumbIssuer(null); + + JenkinsRule.WebClient wc = j.createWebClient(); + wc.getOptions().setThrowExceptionOnFailingStatusCode(false); + HtmlPage page = wc.goTo("testDescribable"); + String secret = "s3cr3t"; + ((HtmlInput) page.getElementById("password")).setValueAttribute(secret); + + HtmlForm form = page.getFormByName("config"); + Page formSubmitPage = j.submit(form); + assertThat(formSubmitPage.getWebResponse().getContentAsString(), allOf( + containsString(RedactSecretJsonInErrorMessageSanitizer.REDACT_VALUE), + not(containsString(secret)) + )); + + // check the system log also + Throwable thrown = logging.getRecords().get(0).getThrown(); + // the exception from Descriptor + assertThat(thrown.getCause().getMessage(), allOf( + containsString(RedactSecretJsonInErrorMessageSanitizer.REDACT_VALUE), + not(containsString(secret)) + )); + + // the exception from RequestImpl + assertThat(thrown.getCause().getCause().getMessage(), allOf( + containsString(RedactSecretJsonInErrorMessageSanitizer.REDACT_VALUE), + not(containsString(secret)) + )); + + StringWriter buffer = new StringWriter(); + thrown.printStackTrace(new PrintWriter(buffer)); + String fullStack = buffer.getBuffer().toString(); + assertThat(fullStack, allOf( + containsString(RedactSecretJsonInErrorMessageSanitizer.REDACT_VALUE), + not(containsString(secret)) + )); + } + + @Test + @Issue("SECURITY-765") + public void checkSanitizationIsApplied_inStapler() throws Exception { + logging.record("/jenkins", Level.ALL).capture(1); + + j.jenkins.setCrumbIssuer(null); + + JenkinsRule.WebClient wc = j.createWebClient(); + wc.getOptions().setThrowExceptionOnFailingStatusCode(false); + HtmlPage page = wc.goTo("testStapler"); + String secret = "s3cr3t"; + ((HtmlInput) page.getElementById("password")).setValueAttribute(secret); + + HtmlForm form = page.getFormByName("config"); + Page formSubmitPage = j.submit(form); + assertThat(formSubmitPage.getWebResponse().getContentAsString(), allOf( + containsString(RedactSecretJsonInErrorMessageSanitizer.REDACT_VALUE), + not(containsString(secret)) + )); + + // check the system log also + Throwable thrown = logging.getRecords().get(0).getThrown(); + // the exception from RequestImpl + assertThat(thrown.getCause().getMessage(), allOf( + containsString(RedactSecretJsonInErrorMessageSanitizer.REDACT_VALUE), + not(containsString(secret)) + )); + + StringWriter buffer = new StringWriter(); + thrown.printStackTrace(new PrintWriter(buffer)); + String fullStack = buffer.getBuffer().toString(); + assertThat(fullStack, allOf( + containsString(RedactSecretJsonInErrorMessageSanitizer.REDACT_VALUE), + not(containsString(secret)) + )); + } + + public static class TestDescribable implements Describable { + + @DataBoundConstructor + public TestDescribable(Secret password) { + throw new IllegalArgumentException("Try to steal my password"); + } + + public DescriptorImpl getDescriptor() { + return Jenkins.getInstance().getDescriptorByType(TestDescribable.DescriptorImpl.class); + } + + @TestExtension({ + "checkSanitizationIsApplied_inStapler", + "checkSanitizationIsApplied_inDescriptor" + }) + public static final class DescriptorImpl extends Descriptor { + + } + } + + @TestExtension("checkSanitizationIsApplied_inDescriptor") + public static class TestDescribablePage implements RootAction { + + public TestDescribable testDescribable; + + @RequirePOST + public void doConfigSubmit(StaplerRequest req, StaplerResponse rsp) throws Exception { + Jenkins.getInstance().getDescriptorOrDie(TestDescribable.class).newInstance(req, req.getSubmittedForm()); + } + + @Override + public String getIconFileName() { + return null; + } + + @Override + public String getDisplayName() { + return null; + } + + @Override + public String getUrlName() { + return "testDescribable"; + } + } + + @TestExtension("checkSanitizationIsApplied_inStapler") + public static class TestStaplerPage implements RootAction { + + public TestDescribable testDescribable; + + @RequirePOST + public void doConfigSubmit(StaplerRequest req, StaplerResponse rsp) throws Exception { + req.bindJSON(TestDescribable.class, req.getSubmittedForm()); + } + + @Override + public String getIconFileName() { + return null; + } + + @Override + public String getDisplayName() { + return null; + } + + @Override + public String getUrlName() { + return "testStapler"; + } + } +} diff --git a/test/src/test/resources/jenkins/security/RedactSecretJsonInErrorMessageSanitizerHtmlTest/TestDescribablePage/index.jelly b/test/src/test/resources/jenkins/security/RedactSecretJsonInErrorMessageSanitizerHtmlTest/TestDescribablePage/index.jelly new file mode 100644 index 0000000000..c721c3ea0f --- /dev/null +++ b/test/src/test/resources/jenkins/security/RedactSecretJsonInErrorMessageSanitizerHtmlTest/TestDescribablePage/index.jelly @@ -0,0 +1,39 @@ + + + + + + + + + + + + + + + + + diff --git a/test/src/test/resources/jenkins/security/RedactSecretJsonInErrorMessageSanitizerHtmlTest/TestPassword/index.jelly b/test/src/test/resources/jenkins/security/RedactSecretJsonInErrorMessageSanitizerHtmlTest/TestPassword/index.jelly new file mode 100644 index 0000000000..151e1d9000 --- /dev/null +++ b/test/src/test/resources/jenkins/security/RedactSecretJsonInErrorMessageSanitizerHtmlTest/TestPassword/index.jelly @@ -0,0 +1,53 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/test/src/test/resources/jenkins/security/RedactSecretJsonInErrorMessageSanitizerHtmlTest/TestStaplerPage/index.jelly b/test/src/test/resources/jenkins/security/RedactSecretJsonInErrorMessageSanitizerHtmlTest/TestStaplerPage/index.jelly new file mode 100644 index 0000000000..c721c3ea0f --- /dev/null +++ b/test/src/test/resources/jenkins/security/RedactSecretJsonInErrorMessageSanitizerHtmlTest/TestStaplerPage/index.jelly @@ -0,0 +1,39 @@ + + + + + + + + + + + + + + + + + diff --git a/war/src/main/webapp/scripts/hudson-behavior.js b/war/src/main/webapp/scripts/hudson-behavior.js index d8989d5dea..0cf469a510 100644 --- a/war/src/main/webapp/scripts/hudson-behavior.js +++ b/war/src/main/webapp/scripts/hudson-behavior.js @@ -2579,7 +2579,12 @@ function buildFormTree(form) { addProperty(p, e.name.substring(r), e.value); } break; - + case "password": + p = findParent(e); + addProperty(p, e.name, e.value); + // must be kept in sync with RedactSecretJsonForTraceSanitizer.REDACT_KEY + addProperty(p, "$redact", shortenName(e.name)); + break; default: p = findParent(e); addProperty(p, e.name, e.value); From 41f74e2a82e95d66f6723da81dfacd376a564764 Mon Sep 17 00:00:00 2001 From: Ramon Leon Date: Thu, 27 Sep 2018 11:37:29 +0200 Subject: [PATCH 09/10] [SECURITY-857] --- .../jenkins/security/Security857Test.java | 118 ++++++++++++++++++ .../resources/jenkins/security/escape.jelly | 8 ++ 2 files changed, 126 insertions(+) create mode 100644 test/src/test/java/jenkins/security/Security857Test.java create mode 100644 test/src/test/resources/jenkins/security/escape.jelly diff --git a/test/src/test/java/jenkins/security/Security857Test.java b/test/src/test/java/jenkins/security/Security857Test.java new file mode 100644 index 0000000000..f5832c42cc --- /dev/null +++ b/test/src/test/java/jenkins/security/Security857Test.java @@ -0,0 +1,118 @@ +package jenkins.security; + +import com.gargoylesoftware.htmlunit.HttpMethod; +import com.gargoylesoftware.htmlunit.WebRequest; +import com.gargoylesoftware.htmlunit.WebResponse; +import hudson.util.IOUtils; +import org.junit.Assert; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.Issue; +import org.jvnet.hudson.test.JenkinsRule; + +import java.io.IOException; +import java.lang.reflect.Field; + +/** + * Tests about the behavior expected setting different values in the escape-by-default directive and the + * CustomJellyContext.ESCAPE_BY_DEFAULT field. + */ +public class Security857Test { + + private static String EVIDENCE = "