From 6eea1e97840b5623829b2c1fd2e363c045bdc230 Mon Sep 17 00:00:00 2001 From: Wadeck Follonier Date: Wed, 25 Apr 2018 23:37:50 +0200 Subject: [PATCH 1/5] [SECURITY-794] --- .../hudson/tools/ZipExtractionInstaller.java | 5 + .../tools/ZipExtractionInstaller/config.jelly | 2 +- .../tools/ZipExtractionInstallerTest.java | 183 ++++++++++++++++++ 3 files changed, 189 insertions(+), 1 deletion(-) create mode 100644 test/src/test/java/hudson/tools/ZipExtractionInstallerTest.java diff --git a/core/src/main/java/hudson/tools/ZipExtractionInstaller.java b/core/src/main/java/hudson/tools/ZipExtractionInstaller.java index ba91cfe457..af9bffda11 100644 --- a/core/src/main/java/hudson/tools/ZipExtractionInstaller.java +++ b/core/src/main/java/hudson/tools/ZipExtractionInstaller.java @@ -42,9 +42,11 @@ import java.net.MalformedURLException; import java.net.URL; import java.net.URLConnection; +import jenkins.model.Jenkins; import org.jenkinsci.Symbol; import org.kohsuke.stapler.DataBoundConstructor; import org.kohsuke.stapler.QueryParameter; +import org.kohsuke.stapler.interceptor.RequirePOST; /** * Installs a tool into the Hudson working area by downloading and unpacking a ZIP file. @@ -95,7 +97,10 @@ public class ZipExtractionInstaller extends ToolInstaller { return Messages.ZipExtractionInstaller_DescriptorImpl_displayName(); } + @RequirePOST public FormValidation doCheckUrl(@QueryParameter String value) { + Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER); + try { URLConnection conn = ProxyConfiguration.open(new URL(value)); conn.connect(); diff --git a/core/src/main/resources/hudson/tools/ZipExtractionInstaller/config.jelly b/core/src/main/resources/hudson/tools/ZipExtractionInstaller/config.jelly index 952041a19f..dc5212de08 100644 --- a/core/src/main/resources/hudson/tools/ZipExtractionInstaller/config.jelly +++ b/core/src/main/resources/hudson/tools/ZipExtractionInstaller/config.jelly @@ -25,7 +25,7 @@ THE SOFTWARE. - + diff --git a/test/src/test/java/hudson/tools/ZipExtractionInstallerTest.java b/test/src/test/java/hudson/tools/ZipExtractionInstallerTest.java new file mode 100644 index 0000000000..e1e882d10f --- /dev/null +++ b/test/src/test/java/hudson/tools/ZipExtractionInstallerTest.java @@ -0,0 +1,183 @@ +/* + * The MIT License + * + * Copyright 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 hudson.tools; + +import com.gargoylesoftware.htmlunit.HttpMethod; +import com.gargoylesoftware.htmlunit.InteractivePage; +import com.gargoylesoftware.htmlunit.WebRequest; +import com.gargoylesoftware.htmlunit.html.HtmlPage; +import com.gargoylesoftware.htmlunit.html.HtmlTextInput; +import com.gargoylesoftware.htmlunit.javascript.JavaScriptEngine; +import com.gargoylesoftware.htmlunit.javascript.host.xml.XMLHttpRequest; +import hudson.model.JDK; +import hudson.model.User; +import hudson.util.FormValidation; +import jenkins.model.Jenkins; +import net.sourceforge.htmlunit.corejs.javascript.Function; +import net.sourceforge.htmlunit.corejs.javascript.Scriptable; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.jvnet.hudson.test.Issue; +import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.MockAuthorizationStrategy; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import java.lang.reflect.Field; +import java.net.URL; +import java.net.URLDecoder; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +import static org.hamcrest.Matchers.containsString; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; + +public class ZipExtractionInstallerTest { + + @Rule + public JenkinsRule j = new JenkinsRule(); + + @Rule + public TemporaryFolder tmp = new TemporaryFolder(); + + @Test + @Issue("SECURITY-794") + public void onlyAdminCanReachTheDoCheck() throws Exception { + final String ADMIN = "admin"; + final String USER = "user"; + + j.jenkins.setCrumbIssuer(null); + j.jenkins.setSecurityRealm(j.createDummySecurityRealm()); + j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy() + .grant(Jenkins.ADMINISTER).everywhere().to(ADMIN) + .grant(Jenkins.READ).everywhere().to(USER) + ); + + User.getById(ADMIN, true); + User.getById(USER, true); + + WebRequest request = new WebRequest(new URL(j.getURL() + "descriptorByName/hudson.tools.ZipExtractionInstaller/checkUrl"), HttpMethod.POST); + request.setRequestBody(URLEncoder.encode("value=https://www.google.com", StandardCharsets.UTF_8.name())); + + JenkinsRule.WebClient adminWc = j.createWebClient(); + adminWc.login(ADMIN); + assertEquals(200, adminWc.getPage(request).getWebResponse().getStatusCode()); + + JenkinsRule.WebClient userWc = j.createWebClient(); + userWc.getOptions().setThrowExceptionOnFailingStatusCode(false); + userWc.login(USER); + assertEquals(403, userWc.getPage(request).getWebResponse().getStatusCode()); + } + + @Test + @Issue("SECURITY-794") + public void roundtrip() throws Exception { + final String VALID_URL = "https://www.google.com"; + final String INVALID_URL = "only-crappy-letters"; + + ZipExtractionInstaller installer = new ZipExtractionInstaller("", VALID_URL, ""); + + j.jenkins.getJDKs().add(new JDK("test", tmp.getRoot().getAbsolutePath(), Arrays.asList( + new InstallSourceProperty(Arrays.asList(installer))))); + + JenkinsRule.WebClient wc = j.createWebClient(); + + SpyingJavaScriptEngine jsEngine = new SpyingJavaScriptEngine(wc, "ZipExtractionInstaller/checkUrl", HttpMethod.POST); + wc.setJavaScriptEngine(jsEngine); + + HtmlPage page = wc.goTo("configureTools"); + + XMLHttpRequest lastRequest = jsEngine.getLastRequest(); + String body = URLDecoder.decode(getPrivateWebRequestField(lastRequest).getRequestBody(), "UTF-8"); + assertThat(body, containsString(VALID_URL)); + assertEquals(FormValidation.ok().renderHtml(), lastRequest.getResponseText()); + + HtmlTextInput urlInput = page.getDocumentElement().getOneHtmlElementByAttribute("input", "value", VALID_URL); + urlInput.setAttribute("value", INVALID_URL); + j.submit(page.getFormByName("config")); + + JDK jdk = j.jenkins.getJDK("test"); + InstallSourceProperty isp = jdk.getProperties().get(InstallSourceProperty.class); + assertEquals(1, isp.installers.size()); + assertEquals(INVALID_URL, isp.installers.get(ZipExtractionInstaller.class).getUrl()); + + wc.goTo("configureTools"); + + lastRequest = jsEngine.getLastRequest(); + body = URLDecoder.decode(getPrivateWebRequestField(lastRequest).getRequestBody(), "UTF-8"); + assertThat(body, containsString(INVALID_URL)); + assertThat(lastRequest.getResponseText(), containsString(Messages.ZipExtractionInstaller_malformed_url())); + } + + private class SpyingJavaScriptEngine extends JavaScriptEngine { + private List storedRequests = new ArrayList<>(); + private String urlToMatch; + private HttpMethod method; + + SpyingJavaScriptEngine(JenkinsRule.WebClient wc, @Nullable String urlToMatch, @Nullable HttpMethod method) { + super(wc); + this.urlToMatch = urlToMatch; + this.method = method; + } + + @Override + public Object callFunction(InteractivePage page, Function function, Scriptable scope, Scriptable thisObject, Object[] args) { + if (thisObject instanceof XMLHttpRequest) { + try { + WebRequest request = getPrivateWebRequestField((XMLHttpRequest) thisObject); + boolean correctUrl = urlToMatch == null || request.getUrl().toString().contains(urlToMatch); + boolean correctMethod = method == null || request.getHttpMethod().equals(method); + if (correctUrl && correctMethod) { + if (((XMLHttpRequest) thisObject).getReadyState() == 4) { + storedRequests.add((XMLHttpRequest) thisObject); + } + } + } catch (NoSuchFieldException | IllegalAccessException e) { + e.printStackTrace(); + } + } + return super.callFunction(page, function, scope, thisObject, args); + } + + @Nonnull + public XMLHttpRequest getLastRequest() { + if (storedRequests.isEmpty()) { + fail("There is no available requests for the proposed url/method"); + } + return storedRequests.get(storedRequests.size() - 1); + } + } + + private static WebRequest getPrivateWebRequestField(XMLHttpRequest xmlHttpRequest) throws NoSuchFieldException, IllegalAccessException { + Field webRequest_Field = XMLHttpRequest.class.getDeclaredField("webRequest_"); + webRequest_Field.setAccessible(true); + return (WebRequest) webRequest_Field.get(xmlHttpRequest); + } +} From de7aaab441151fb1760855fec83681c6a8756a45 Mon Sep 17 00:00:00 2001 From: Wadeck Follonier Date: Wed, 25 Apr 2018 23:39:43 +0200 Subject: [PATCH 2/5] [SECURITY-786] --- .../security/HudsonPrivateSecurityRealm.java | 24 ++++ .../hudson/security/Messages.properties | 2 + .../HudsonPrivateSecurityRealmTest.java | 129 +++++++++++++++++- 3 files changed, 154 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index b82e8b70d0..c787e352f2 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -64,6 +64,7 @@ import org.kohsuke.stapler.interceptor.RequirePOST; import org.mindrot.jbcrypt.BCrypt; import org.springframework.dao.DataAccessException; +import javax.annotation.Nonnull; import javax.servlet.Filter; import javax.servlet.FilterChain; import javax.servlet.FilterConfig; @@ -95,6 +96,15 @@ import org.kohsuke.accmod.restrictions.NoExternalUse; * @author Kohsuke Kawaguchi */ public class HudsonPrivateSecurityRealm extends AbstractPasswordBasedSecurityRealm implements ModelObject, AccessControlled { + private static /* not final */ String ID_REGEX = System.getProperty(HudsonPrivateSecurityRealm.class.getName() + ".ID_REGEX"); + + /** + * Default REGEX for the user ID check in case the ID_REGEX is not set + * It allows A-Za-z0-9 + "_-" + * in Java {@code \w} is equivalent to {@code [A-Za-z0-9_]} (take care of "_") + */ + private static final String DEFAULT_ID_REGEX = "^[\\w-]+$"; + /** * If true, sign up is not allowed. *

@@ -338,6 +348,12 @@ public class HudsonPrivateSecurityRealm extends AbstractPasswordBasedSecurityRea if(si.username==null || si.username.length()==0) si.errorMessage = Messages.HudsonPrivateSecurityRealm_CreateAccount_UserNameRequired(); + else if(!containsOnlyAcceptableCharacters(si.username)) + if(ID_REGEX == null){ + si.errorMessage = Messages.HudsonPrivateSecurityRealm_CreateAccount_UserNameInvalidCharacters(); + }else{ + si.errorMessage = Messages.HudsonPrivateSecurityRealm_CreateAccount_UserNameInvalidCharactersCustom(ID_REGEX); + } else { // do not create the user - we just want to check if the user already exists but is not a "login" user. User user = User.getById(si.username, false); @@ -385,6 +401,14 @@ public class HudsonPrivateSecurityRealm extends AbstractPasswordBasedSecurityRea return user; } + private boolean containsOnlyAcceptableCharacters(@Nonnull String value){ + if(ID_REGEX == null){ + return value.matches(DEFAULT_ID_REGEX); + }else{ + return value.matches(ID_REGEX); + } + } + @Restricted(NoExternalUse.class) public boolean isMailerPluginPresent() { try { diff --git a/core/src/main/resources/hudson/security/Messages.properties b/core/src/main/resources/hudson/security/Messages.properties index 086cc59afb..951a1bfbc5 100644 --- a/core/src/main/resources/hudson/security/Messages.properties +++ b/core/src/main/resources/hudson/security/Messages.properties @@ -37,6 +37,8 @@ HudsonPrivateSecurityRealm.CreateAccount.TextNotMatchWordInImage=Text didn't mat HudsonPrivateSecurityRealm.CreateAccount.PasswordNotMatch=Password didn't match HudsonPrivateSecurityRealm.CreateAccount.PasswordRequired=Password is required HudsonPrivateSecurityRealm.CreateAccount.UserNameRequired=User name is required +HudsonPrivateSecurityRealm.CreateAccount.UserNameInvalidCharacters=User name must only contain alphanumeric characters, underscore and dash +HudsonPrivateSecurityRealm.CreateAccount.UserNameInvalidCharactersCustom=User name must match the following expression: {0} HudsonPrivateSecurityRealm.CreateAccount.InvalidEmailAddress=Invalid e-mail address HudsonPrivateSecurityRealm.CreateAccount.UserNameAlreadyTaken=User name is already taken diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index 258a47c474..db7666f630 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -2,15 +2,21 @@ package hudson.security; import static hudson.security.HudsonPrivateSecurityRealm.CLASSIC; import static hudson.security.HudsonPrivateSecurityRealm.PASSWORD_ENCODER; +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.assertNotNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.assertThat; import static org.hamcrest.Matchers.is; import static org.hamcrest.xml.HasXPath.hasXPath; import java.io.UnsupportedEncodingException; +import java.lang.reflect.Field; +import hudson.security.pages.SignupPage; +import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.Issue; @@ -34,6 +40,13 @@ public class HudsonPrivateSecurityRealmTest { @Rule public JenkinsRule j = new JenkinsRule(); + @Before + public void setup() throws Exception { + Field field = HudsonPrivateSecurityRealm.class.getDeclaredField("ID_REGEX"); + field.setAccessible(true); + field.set(null, null); + } + /** * Tests the data compatibility with Hudson before 1.283. * Starting 1.283, passwords are now stored hashed. @@ -170,5 +183,119 @@ public class HudsonPrivateSecurityRealmTest { String authHeader = "Basic " + auth; return authHeader; } - + + @Issue("SECURITY-786") + @Test + public void controlCharacterAreNoMoreValid() throws Exception { + HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(true, false, null); + j.jenkins.setSecurityRealm(securityRealm); + + String password = "testPwd"; + String email = "test@test.com"; + int i = 0; + + // regular case = only accepting a-zA-Z0-9 + "-_" + checkUserCanBeCreatedWith(securityRealm, "test" + i, password, "Test" + i, email); + assertNotNull(User.getById("test" + i, false)); + i++; + checkUserCanBeCreatedWith(securityRealm, "te-st_123" + i, password, "Test" + i, email); + assertNotNull(User.getById("te-st_123" + i, false)); + i++; + {// user id that contains invalid characters + checkUserCannotBeCreatedWith(securityRealm, "test " + i, password, "Test" + i, email); + i++; + checkUserCannotBeCreatedWith(securityRealm, "te@st" + i, password, "Test" + i, email); + i++; + checkUserCannotBeCreatedWith(securityRealm, "test.com" + i, password, "Test" + i, email); + i++; + checkUserCannotBeCreatedWith(securityRealm, "test,com" + i, password, "Test" + i, email); + i++; + checkUserCannotBeCreatedWith(securityRealm, "test,com" + i, password, "Test" + i, email); + i++; + checkUserCannotBeCreatedWith(securityRealm, "testécom" + i, password, "Test" + i, email); + i++; + checkUserCannotBeCreatedWith(securityRealm, "Stargåte" + i, password, "Test" + i, email); + i++; + checkUserCannotBeCreatedWith(securityRealm, "te\u0000st" + i, password, "Test" + i, email); + i++; + } + } + + @Issue("SECURITY-786") + @Test + public void controlCharacterAreNoMoreValid_CustomRegex() throws Exception { + HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(true, false, null); + j.jenkins.setSecurityRealm(securityRealm); + + String currentRegex = "^[A-Z]+[0-9]*$"; + + Field field = HudsonPrivateSecurityRealm.class.getDeclaredField("ID_REGEX"); + field.setAccessible(true); + field.set(null, currentRegex); + + String password = "testPwd"; + String email = "test@test.com"; + int i = 0; + + // regular case = only accepting a-zA-Z0-9 + "-_" + checkUserCanBeCreatedWith(securityRealm, "TEST" + i, password, "Test" + i, email); + assertNotNull(User.getById("TEST" + i, false)); + i++; + checkUserCanBeCreatedWith(securityRealm, "TEST123" + i, password, "Test" + i, email); + assertNotNull(User.getById("TEST123" + i, false)); + i++; + {// user id that do not follow custom regex + checkUserCannotBeCreatedWith_custom(securityRealm, "test " + i, password, "Test" + i, email, currentRegex); + i++; + checkUserCannotBeCreatedWith_custom(securityRealm, "@" + i, password, "Test" + i, email, currentRegex); + i++; + checkUserCannotBeCreatedWith_custom(securityRealm, "T2A" + i, password, "Test" + i, email, currentRegex); + i++; + } + { // we can even change regex on the fly + currentRegex = "^[0-9]*$"; + field.set(null, currentRegex); + + checkUserCanBeCreatedWith(securityRealm, "125213" + i, password, "Test" + i, email); + assertNotNull(User.getById("125213" + i, false)); + i++; + checkUserCannotBeCreatedWith_custom(securityRealm, "TEST12" + i, password, "Test" + i, email, currentRegex); + i++; + } + } + + private void checkUserCanBeCreatedWith(HudsonPrivateSecurityRealm securityRealm, String id, String password, String fullName, String email) throws Exception { + JenkinsRule.WebClient wc = j.createWebClient(); + SignupPage signup = new SignupPage(wc.goTo("signup")); + signup.enterUsername(id); + signup.enterPassword(password); + signup.enterFullName(fullName); + signup.enterEmail(email); + HtmlPage success = signup.submit(j); + assertThat(success.getElementById("main-panel").getTextContent(), containsString("Success")); + } + + private void checkUserCannotBeCreatedWith(HudsonPrivateSecurityRealm securityRealm, String id, String password, String fullName, String email) throws Exception { + JenkinsRule.WebClient wc = j.createWebClient(); + SignupPage signup = new SignupPage(wc.goTo("signup")); + signup.enterUsername(id); + signup.enterPassword(password); + signup.enterFullName(fullName); + signup.enterEmail(email); + HtmlPage success = signup.submit(j); + assertThat(success.getElementById("main-panel").getTextContent(), not(containsString("Success"))); + assertThat(success.getElementById("main-panel").getTextContent(), containsString(Messages.HudsonPrivateSecurityRealm_CreateAccount_UserNameInvalidCharacters())); + } + + private void checkUserCannotBeCreatedWith_custom(HudsonPrivateSecurityRealm securityRealm, String id, String password, String fullName, String email, String regex) throws Exception { + JenkinsRule.WebClient wc = j.createWebClient(); + SignupPage signup = new SignupPage(wc.goTo("signup")); + signup.enterUsername(id); + signup.enterPassword(password); + signup.enterFullName(fullName); + signup.enterEmail(email); + HtmlPage success = signup.submit(j); + assertThat(success.getElementById("main-panel").getTextContent(), not(containsString("Success"))); + assertThat(success.getElementById("main-panel").getTextContent(), containsString(regex)); + } } From 809916b59b7c7678dc3c1c76338452121fed1424 Mon Sep 17 00:00:00 2001 From: Wadeck Follonier Date: Wed, 25 Apr 2018 23:40:33 +0200 Subject: [PATCH 3/5] [SECURITY-771] --- .../java/hudson/cli/ListPluginsCommand.java | 4 +- .../hudson/cli/ListPluginsCommandTest.java | 79 +++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) create mode 100644 test/src/test/java/hudson/cli/ListPluginsCommandTest.java diff --git a/core/src/main/java/hudson/cli/ListPluginsCommand.java b/core/src/main/java/hudson/cli/ListPluginsCommand.java index 99c926cdaf..faa35dfa67 100644 --- a/core/src/main/java/hudson/cli/ListPluginsCommand.java +++ b/core/src/main/java/hudson/cli/ListPluginsCommand.java @@ -47,7 +47,9 @@ public class ListPluginsCommand extends CLICommand { public String name; protected int run() { - Jenkins h = Jenkins.getActiveInstance(); + Jenkins h = Jenkins.getInstance(); + h.checkPermission(Jenkins.ADMINISTER); + PluginManager pluginManager = h.getPluginManager(); if (this.name != null) { diff --git a/test/src/test/java/hudson/cli/ListPluginsCommandTest.java b/test/src/test/java/hudson/cli/ListPluginsCommandTest.java new file mode 100644 index 0000000000..530fa8875f --- /dev/null +++ b/test/src/test/java/hudson/cli/ListPluginsCommandTest.java @@ -0,0 +1,79 @@ +/* + * The MIT License + * + * Copyright 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 hudson.cli; + +import jenkins.model.Jenkins; +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.hamcrest.Matchers.not; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThat; + +public class ListPluginsCommandTest { + + @Rule + public JenkinsRule j = new JenkinsRule(); + + @Test + public void listPluginsExpectedUsage() throws Exception { + assertNull(j.jenkins.getPluginManager().getPlugin("token-macro")); + CLICommandInvoker.Result result = new CLICommandInvoker(j, new ListPluginsCommand()) + .invoke(); + assertThat(result, CLICommandInvoker.Matcher.succeeded()); + assertThat(result, CLICommandInvoker.Matcher.hasNoStandardOutput()); + assertThat(result.stdout(), not(containsString("token-macro"))); + + assertThat(new CLICommandInvoker(j, new InstallPluginCommand()). + withStdin(ListPluginsCommandTest.class.getResourceAsStream("/plugins/token-macro.hpi")). + invokeWithArgs("-name", "token-macro", "-deploy", "="), + CLICommandInvoker.Matcher.succeeded()); + assertNotNull(j.jenkins.getPluginManager().getPlugin("token-macro")); + + result = new CLICommandInvoker(j, new ListPluginsCommand()) + .invoke() + ; + assertThat(result, CLICommandInvoker.Matcher.succeeded()); + assertThat(result.stdout(), containsString("token-macro")); + } + + @Test + @Issue("SECURITY-771") + public void onlyAccessibleForAdmin() throws Exception { + CLICommandInvoker.Result result = new CLICommandInvoker(j, new ListPluginsCommand()) + .authorizedTo(Jenkins.READ) + .invoke(); + assertThat(result, CLICommandInvoker.Matcher.failedWith(6 /* not authorized */)); + + result = new CLICommandInvoker(j, new ListPluginsCommand()) + .authorizedTo(Jenkins.ADMINISTER) + .invoke() + ; + assertThat(result, CLICommandInvoker.Matcher.succeeded()); + } +} From 7c5b41bfd5d8004f82684a9168dd627e20ea5f35 Mon Sep 17 00:00:00 2001 From: Wadeck Follonier Date: Wed, 25 Apr 2018 23:40:56 +0200 Subject: [PATCH 4/5] [SECURITY-771] --- .../resources/hudson/AboutJenkins/index.jelly | 2 +- .../test/java/hudson/AboutJenkinsTest.java | 72 +++++++++++++++++++ 2 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 test/src/test/java/hudson/AboutJenkinsTest.java diff --git a/core/src/main/resources/hudson/AboutJenkins/index.jelly b/core/src/main/resources/hudson/AboutJenkins/index.jelly index 68e063af6d..cc668b1f64 100644 --- a/core/src/main/resources/hudson/AboutJenkins/index.jelly +++ b/core/src/main/resources/hudson/AboutJenkins/index.jelly @@ -26,7 +26,7 @@ THE SOFTWARE. - +

diff --git a/test/src/test/java/hudson/AboutJenkinsTest.java b/test/src/test/java/hudson/AboutJenkinsTest.java new file mode 100644 index 0000000000..7972aa1021 --- /dev/null +++ b/test/src/test/java/hudson/AboutJenkinsTest.java @@ -0,0 +1,72 @@ +/* + * The MIT License + * + * Copyright 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 hudson; + +import com.gargoylesoftware.htmlunit.html.HtmlPage; +import jenkins.model.Jenkins; +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 static org.hamcrest.Matchers.containsString; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; + +public class AboutJenkinsTest { + + @Rule + public JenkinsRule j = new JenkinsRule(); + + @Test + @Issue("SECURITY-771") + public void onlyAdminCanReadAbout() throws Exception { + final String ADMIN = "admin"; + final String USER = "user"; + + j.jenkins.setSecurityRealm(j.createDummySecurityRealm()); + j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy() + .grant(Jenkins.ADMINISTER).everywhere().to(ADMIN) + .grant(Jenkins.READ).everywhere().to(USER) + ); + + JenkinsRule.WebClient wc = j.createWebClient(); + wc.getOptions().setThrowExceptionOnFailingStatusCode(false); + + { // user cannot see it + wc.login(USER); + HtmlPage page = wc.goTo("about/"); + assertEquals(403, page.getWebResponse().getStatusCode()); + } + + { // admin can access it + wc.login(ADMIN); + HtmlPage page = wc.goTo("about/"); + assertEquals(200, page.getWebResponse().getStatusCode()); + assertThat(page.getWebResponse().getContentAsString(), containsString("Mavenized dependencies")); + } + } + +} From 5cf0a77d44310523b763698f67d645c1f2427f30 Mon Sep 17 00:00:00 2001 From: Wadeck Follonier Date: Wed, 25 Apr 2018 23:42:19 +0200 Subject: [PATCH 5/5] [SECURITY-788] --- core/src/main/java/hudson/FilePath.java | 19 ++- .../main/java/jenkins/SoloFilePathFilter.java | 20 ++- core/src/test/java/hudson/FilePathTest.java | 4 +- .../security/s2m/AdminFilePathFilterTest.java | 145 ++++++++++++++++++ 4 files changed, 175 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/hudson/FilePath.java b/core/src/main/java/hudson/FilePath.java index cd3fa60451..8f112f1886 100644 --- a/core/src/main/java/hudson/FilePath.java +++ b/core/src/main/java/hudson/FilePath.java @@ -214,9 +214,14 @@ public final class FilePath implements Serializable { * This is used to determine whether we are running on the master or the agent. */ private transient VirtualChannel channel; - - // since the platform of the agent might be different, can't use java.io.File - private final String remote; + + /** + * Represent the path to the file in the master or the agent + * Since the platform of the agent might be different, can't use java.io.File + * + * The field could not be final since it's modified in {@link #readResolve()} + */ + private /*final*/ String remote; /** * If this {@link FilePath} is deserialized to handle file access request from a remote computer, @@ -264,6 +269,11 @@ public final class FilePath implements Serializable { this.remote = normalize(resolvePathIfRelative(base, rel)); } + private Object readResolve() { + this.remote = normalize(this.remote); + return this; + } + private String resolvePathIfRelative(@Nonnull FilePath base, @Nonnull String rel) { if(isAbsolute(rel)) return rel; if(base.isUnix()) { @@ -291,7 +301,8 @@ public final class FilePath implements Serializable { * {@link File#getParent()} etc cannot handle ".." and "." in the path component very well, * so remove them. */ - private static String normalize(@Nonnull String path) { + @Restricted(NoExternalUse.class) + public static String normalize(@Nonnull String path) { StringBuilder buf = new StringBuilder(); // Check for prefix designating absolute path Matcher m = ABSOLUTE_PREFIX_PATTERN.matcher(path); diff --git a/core/src/main/java/jenkins/SoloFilePathFilter.java b/core/src/main/java/jenkins/SoloFilePathFilter.java index ce13575982..aa0ebbefc8 100644 --- a/core/src/main/java/jenkins/SoloFilePathFilter.java +++ b/core/src/main/java/jenkins/SoloFilePathFilter.java @@ -1,5 +1,7 @@ package jenkins; +import hudson.FilePath; + import javax.annotation.Nullable; import java.io.File; @@ -31,39 +33,43 @@ public final class SoloFilePathFilter extends FilePathFilter { throw new SecurityException("agent may not " + op + " " + f+"\nSee https://jenkins.io/redirect/security-144 for more details"); return true; } + + private File normalize(File file){ + return new File(FilePath.normalize(file.getAbsolutePath())); + } @Override public boolean read(File f) throws SecurityException { - return noFalse("read",f,base.read(f)); + return noFalse("read",f,base.read(normalize(f))); } @Override public boolean write(File f) throws SecurityException { - return noFalse("write",f,base.write(f)); + return noFalse("write",f,base.write(normalize(f))); } @Override public boolean symlink(File f) throws SecurityException { - return noFalse("symlink",f,base.write(f)); + return noFalse("symlink",f,base.write(normalize(f))); } @Override public boolean mkdirs(File f) throws SecurityException { - return noFalse("mkdirs",f,base.mkdirs(f)); + return noFalse("mkdirs",f,base.mkdirs(normalize(f))); } @Override public boolean create(File f) throws SecurityException { - return noFalse("create",f,base.create(f)); + return noFalse("create",f,base.create(normalize(f))); } @Override public boolean delete(File f) throws SecurityException { - return noFalse("delete",f,base.delete(f)); + return noFalse("delete",f,base.delete(normalize(f))); } @Override public boolean stat(File f) throws SecurityException { - return noFalse("stat",f,base.stat(f)); + return noFalse("stat",f,base.stat(normalize(f))); } } diff --git a/core/src/test/java/hudson/FilePathTest.java b/core/src/test/java/hudson/FilePathTest.java index 3608318286..257f10d32c 100644 --- a/core/src/test/java/hudson/FilePathTest.java +++ b/core/src/test/java/hudson/FilePathTest.java @@ -599,7 +599,7 @@ public class FilePathTest { when(con.getResponseCode()) .thenReturn(HttpURLConnection.HTTP_NOT_MODIFIED); - assertFalse(d.installIfNecessaryFrom(url, null, null)); + assertFalse(d.installIfNecessaryFrom(url, null, "message if failed")); verify(con).setIfModifiedSince(123000); } @@ -618,7 +618,7 @@ public class FilePathTest { when(con.getInputStream()) .thenReturn(someZippedContent()); - assertTrue(d.installIfNecessaryFrom(url, null, null)); + assertTrue(d.installIfNecessaryFrom(url, null, "message if failed")); } @Issue("JENKINS-26196") diff --git a/test/src/test/java/jenkins/security/s2m/AdminFilePathFilterTest.java b/test/src/test/java/jenkins/security/s2m/AdminFilePathFilterTest.java index 80d137d6a2..37af2825c0 100644 --- a/test/src/test/java/jenkins/security/s2m/AdminFilePathFilterTest.java +++ b/test/src/test/java/jenkins/security/s2m/AdminFilePathFilterTest.java @@ -25,8 +25,17 @@ package jenkins.security.s2m; import java.io.File; +import java.io.IOException; +import java.io.PrintWriter; +import java.io.StringWriter; +import java.lang.reflect.Field; import javax.inject.Inject; import static org.junit.Assert.*; + +import hudson.FilePath; +import hudson.model.Slave; +import hudson.remoting.Callable; +import org.jenkinsci.remoting.RoleChecker; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -57,5 +66,141 @@ public class AdminFilePathFilterTest { assertFalse(rule.checkFileAccess("write", new File(buildDir, "program.dat"))); assertFalse(rule.checkFileAccess("write", new File(buildDir, "workflow/23.xml"))); } + + @Test + public void slaveCannotReadFileFromSecrets_butCanFromUserContent() throws Exception { + Slave s = r.createOnlineSlave(); + FilePath root = r.jenkins.getRootPath(); + + { // agent can read userContent folder + FilePath rootUserContentFolder = root.child("userContent"); + FilePath rootTargetPublic = rootUserContentFolder.child("target_public.txt"); + rootTargetPublic.write("target_public", null); + checkSlave_can_readFile(s, rootTargetPublic); + } + + { // agent cannot read files inside secrets + FilePath rootSecretFolder = root.child("secrets"); + FilePath rootTargetPrivate = rootSecretFolder.child("target_private.txt"); + rootTargetPrivate.write("target_private", null); + + checkSlave_cannot_readFile(s, rootTargetPrivate); + } + + rule.setMasterKillSwitch(true); + + { // with the master kill switch activated, agent can read files inside secrets + FilePath rootSecretFolder = root.child("secrets"); + FilePath rootTargetPrivate = rootSecretFolder.child("target_private.txt"); + + checkSlave_can_readFile(s, rootTargetPrivate); + } + } + + private static class ReadFileS2MCallable implements Callable { + private final FilePath p; + ReadFileS2MCallable(FilePath p) { + this.p = p; + } + @Override + public String call() throws Exception { + assertTrue(p.isRemote()); + return p.readToString(); + } + @Override + public void checkRoles(RoleChecker checker) throws SecurityException { + // simulate legacy Callable impls + throw new NoSuchMethodError(); + } + } + + @Test + @Issue("SECURITY-788") + public void slaveCannotUse_dotDotSlashStuff_toBypassRestriction() throws Exception { + Slave s = r.createOnlineSlave(); + FilePath root = r.jenkins.getRootPath(); + + { // use ../ to access a non-restricted folder + FilePath rootUserContentFolder = root.child("userContent"); + FilePath rootTargetPublic = rootUserContentFolder.child("target_public.txt"); + rootTargetPublic.write("target_public", null); + FilePath dotDotSlashTargetPublic = root.child("logs/target_public.txt"); + replaceRemote(dotDotSlashTargetPublic, "logs", "logs/../userContent"); + + checkSlave_can_readFile(s, dotDotSlashTargetPublic); + } + + { // use ../ to try to bypass the rules + FilePath rootSecretFolder = root.child("secrets"); + FilePath rootTargetPrivate = rootSecretFolder.child("target_private.txt"); + rootTargetPrivate.write("target_private", null); + + FilePath dotDotSlashTargetPrivate = root.child("userContent/target_private.txt"); + replaceRemote(dotDotSlashTargetPrivate, "userContent", "userContent/../secrets"); + + checkSlave_cannot_readFile(s, dotDotSlashTargetPrivate); + } + } + + @Test + @Issue("SECURITY-788") + public void slaveCannotUse_encodedCharacters_toBypassRestriction() throws Exception { + Slave s = r.createOnlineSlave(); + FilePath root = r.jenkins.getRootPath(); + + // \u002e is the Unicode of . and is interpreted directly by Java as . + + { // use ../ to access a non-restricted folder + FilePath rootUserContentFolder = root.child("userContent"); + FilePath rootTargetPublic = rootUserContentFolder.child("target_public.txt"); + rootTargetPublic.write("target_public", null); + + FilePath dotDotSlashTargetPublic = root.child("logs/target_public.txt"); + replaceRemote(dotDotSlashTargetPublic, "logs", "logs/\u002e\u002e/userContent"); + + checkSlave_can_readFile(s, dotDotSlashTargetPublic); + } + + { // use ../ to try to bypass the rules + FilePath rootSecretFolder = root.child("secrets"); + FilePath rootTargetPrivate = rootSecretFolder.child("target_private.txt"); + rootTargetPrivate.write("target_private", null); + + FilePath dotDotSlashTargetPrivate = root.child("userContent/target_private.txt"); + replaceRemote(dotDotSlashTargetPrivate, "userContent", "userContent/\u002e\u002e/secrets"); + + checkSlave_cannot_readFile(s, dotDotSlashTargetPrivate); + } + } + + private void checkSlave_can_readFile(Slave s, FilePath target) throws Exception { + // slave can read file from userContent + String content = s.getChannel().call(new ReadFileS2MCallable(target)); + // and the master can directly reach it + assertEquals(target.readToString(), content); + } + + private void checkSlave_cannot_readFile(Slave s, FilePath target) throws Exception { + try { + s.getChannel().call(new ReadFileS2MCallable(target)); + fail("Slave should not be able to read file in " + target.getRemote()); + } catch (IOException e){ + Throwable t = e.getCause(); + assertTrue(t instanceof SecurityException); + SecurityException se = (SecurityException) t; + StringWriter sw = new StringWriter(); + se.printStackTrace(new PrintWriter(sw)); + assertTrue(sw.toString().contains("agent may not read")); + } + } + + // to bypass the normalization done in constructor + private void replaceRemote(FilePath p, String before, String after) throws Exception { + Field field = FilePath.class.getDeclaredField("remote"); + field.setAccessible(true); + String currentRemote = (String) field.get(p); + String newRemote = currentRemote.replace(before, after); + field.set(p, newRemote); + } }