From b8ac8cd4c51511b9f844846ba80a8aed054288c5 Mon Sep 17 00:00:00 2001 From: Daniel Beck Date: Wed, 6 Sep 2023 11:17:13 +0000 Subject: [PATCH 1/4] [SECURITY-3261] --- .../jenkins/widgets/HistoryPageFilter.java | 6 +- .../widgets/HistoryPageFilterTest.java | 7 + .../widgets/HistoryPageFilterTest.java | 122 ++++++++++++++++++ 3 files changed, 133 insertions(+), 2 deletions(-) create mode 100644 test/src/test/java/jenkins/widgets/HistoryPageFilterTest.java diff --git a/core/src/main/java/jenkins/widgets/HistoryPageFilter.java b/core/src/main/java/jenkins/widgets/HistoryPageFilter.java index adc09b12f2..cc64f7b363 100644 --- a/core/src/main/java/jenkins/widgets/HistoryPageFilter.java +++ b/core/src/main/java/jenkins/widgets/HistoryPageFilter.java @@ -42,6 +42,7 @@ import java.util.Iterator; import java.util.LinkedList; import java.util.List; import java.util.Map; +import java.util.Set; import jenkins.model.queue.QueueItem; /** @@ -371,8 +372,9 @@ public class HistoryPageFilter { private boolean fitsSearchBuildVariables(AbstractBuild runAsBuild) { Map buildVariables = runAsBuild.getBuildVariables(); - for (String paramsValues : buildVariables.values()) { - if (fitsSearchString(paramsValues)) { + Set sensitiveBuildVariables = runAsBuild.getSensitiveBuildVariables(); + for (Map.Entry param : buildVariables.entrySet()) { + if (!sensitiveBuildVariables.contains(param.getKey()) && fitsSearchString(param.getValue())) { return true; } } diff --git a/core/src/test/java/jenkins/widgets/HistoryPageFilterTest.java b/core/src/test/java/jenkins/widgets/HistoryPageFilterTest.java index 34f7800d33..8739093c60 100644 --- a/core/src/test/java/jenkins/widgets/HistoryPageFilterTest.java +++ b/core/src/test/java/jenkins/widgets/HistoryPageFilterTest.java @@ -43,6 +43,7 @@ import java.util.Calendar; import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.stream.Collectors; import jenkins.model.queue.QueueItem; import org.junit.Assert; @@ -485,6 +486,7 @@ public class HistoryPageFilterTest { private final int buildNumber; private Map buildVariables = Collections.emptyMap(); + private Set sensitiveBuildVariables = Collections.emptySet(); private MockBuild(int buildNumber) { super(Mockito.mock(FreeStyleProject.class), Mockito.mock(Calendar.class)); @@ -501,6 +503,11 @@ public class HistoryPageFilterTest { return buildVariables; } + @Override + public Set getSensitiveBuildVariables() { + return sensitiveBuildVariables; // TODO This is never actually set (bad Mock), actual test in test harness + } + MockBuild withBuildVariables(Map buildVariables) { this.buildVariables = buildVariables; return this; diff --git a/test/src/test/java/jenkins/widgets/HistoryPageFilterTest.java b/test/src/test/java/jenkins/widgets/HistoryPageFilterTest.java new file mode 100644 index 0000000000..3dc26fbcaf --- /dev/null +++ b/test/src/test/java/jenkins/widgets/HistoryPageFilterTest.java @@ -0,0 +1,122 @@ +package jenkins.widgets; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.empty; + +import hudson.model.AbstractBuild; +import hudson.model.FreeStyleBuild; +import hudson.model.FreeStyleProject; +import hudson.model.ParametersAction; +import hudson.model.ParametersDefinitionProperty; +import hudson.model.PasswordParameterDefinition; +import hudson.model.StringParameterDefinition; +import hudson.tasks.BuildWrapper; +import hudson.util.Secret; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.stream.Collectors; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.JenkinsRule; + + +public class HistoryPageFilterTest { + @Rule + public JenkinsRule j = new JenkinsRule(); + + @Test + public void doNotFindSensitiveBuildParams() throws Exception { + final FreeStyleProject freeStyleProject = j.createFreeStyleProject(); + final PasswordParameterDefinition passwordParameterDefinition = new PasswordParameterDefinition("password", Secret.fromString("t0ps3cr3t"), "description"); + final StringParameterDefinition stringParameterDefinition = new StringParameterDefinition("key", "value", "desc"); + freeStyleProject.addProperty(new ParametersDefinitionProperty(passwordParameterDefinition, stringParameterDefinition)); + final FreeStyleBuild build1 = j.buildAndAssertSuccess(freeStyleProject); + final FreeStyleBuild build2 = j.waitForCompletion(Objects.requireNonNull(freeStyleProject.scheduleBuild2( + 0, + new ParametersAction( + passwordParameterDefinition.createValue("p4ssw0rd"), + stringParameterDefinition.createValue("value123")))) + .waitForStart()); + { + final HistoryPageFilter historyPageFilter = new HistoryPageFilter<>(30); + historyPageFilter.setSearchString("value"); + historyPageFilter.add(freeStyleProject.getBuilds()); + assertThat(historyPageFilter.runs.stream().map(HistoryPageEntry::getEntry).collect(Collectors.toList()), contains(build2, build1)); + assertThat(historyPageFilter.queueItems, empty()); + } + { + final HistoryPageFilter historyPageFilter = new HistoryPageFilter<>(30); + historyPageFilter.setSearchString("t0p"); + historyPageFilter.add(freeStyleProject.getBuilds()); + assertThat(historyPageFilter.runs, empty()); + assertThat(historyPageFilter.queueItems, empty()); + } + { + final HistoryPageFilter historyPageFilter = new HistoryPageFilter<>(30); + historyPageFilter.setSearchString("value123"); + historyPageFilter.add(freeStyleProject.getBuilds()); + assertThat(historyPageFilter.runs.stream().map(HistoryPageEntry::getEntry).collect(Collectors.toList()), contains(build2)); + assertThat(historyPageFilter.queueItems, empty()); + } + { + final HistoryPageFilter historyPageFilter = new HistoryPageFilter<>(30); + historyPageFilter.setSearchString("p4ss"); + historyPageFilter.add(freeStyleProject.getBuilds()); + assertThat(historyPageFilter.runs, empty()); + assertThat(historyPageFilter.queueItems, empty()); + } + } + + @Test + public void doNotFindSensitiveBuildWrapperVars() throws Exception { + final FreeStyleProject freeStyleProject = j.createFreeStyleProject(); + freeStyleProject.getBuildWrappersList().add(new BuildWrapperWithSomeSensitiveVars(Map.of("key1", "value123", "key2", "value234", "key3", "s3cr3t"), Set.of("key3"))); + final FreeStyleBuild build = j.buildAndAssertSuccess(freeStyleProject); + { + final HistoryPageFilter historyPageFilter = new HistoryPageFilter<>(30); + historyPageFilter.setSearchString("value"); + historyPageFilter.add(freeStyleProject.getBuilds()); + assertThat(historyPageFilter.runs.stream().map(HistoryPageEntry::getEntry).collect(Collectors.toList()), contains(build)); + assertThat(historyPageFilter.queueItems, empty()); + } + { + final HistoryPageFilter historyPageFilter = new HistoryPageFilter<>(30); + historyPageFilter.setSearchString("value123"); + historyPageFilter.add(freeStyleProject.getBuilds()); + assertThat(historyPageFilter.runs.stream().map(HistoryPageEntry::getEntry).collect(Collectors.toList()), contains(build)); + assertThat(historyPageFilter.queueItems, empty()); + } + { + final HistoryPageFilter historyPageFilter = new HistoryPageFilter<>(30); + historyPageFilter.setSearchString("s3cr3t"); + historyPageFilter.add(freeStyleProject.getBuilds()); + assertThat(historyPageFilter.runs, empty()); + assertThat(historyPageFilter.queueItems, empty()); + } + } + + private static class BuildWrapperWithSomeSensitiveVars extends BuildWrapper { + private final Map variables; + private final Set sensitiveVariables; + + private BuildWrapperWithSomeSensitiveVars(Map variables, Set sensitiveVariables) { + + this.variables = new HashMap<>(variables); + this.sensitiveVariables = new HashSet<>(sensitiveVariables); + } + + @Override + public void makeBuildVariables(AbstractBuild build, Map variables) { + variables.putAll(this.variables); + } + + @Override + public void makeSensitiveBuildVariables(AbstractBuild build, Set sensitiveVariables) { + sensitiveVariables.addAll(this.sensitiveVariables); + } + } +} From 4bf648811f328961627f484349d40420d169cdd0 Mon Sep 17 00:00:00 2001 From: Yaroslav Afenkin Date: Wed, 6 Sep 2023 11:17:19 +0000 Subject: [PATCH 2/4] [SECURITY-3245] --- .../hudson/console/ExpandableDetailsNote.java | 3 +- .../jenkins/security/Security3245Test.java | 68 +++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 test/src/test/java/jenkins/security/Security3245Test.java diff --git a/core/src/main/java/hudson/console/ExpandableDetailsNote.java b/core/src/main/java/hudson/console/ExpandableDetailsNote.java index 6dcc949199..1f558e6a26 100644 --- a/core/src/main/java/hudson/console/ExpandableDetailsNote.java +++ b/core/src/main/java/hudson/console/ExpandableDetailsNote.java @@ -26,6 +26,7 @@ package hudson.console; import edu.umd.cs.findbugs.annotations.NonNull; import hudson.Extension; +import hudson.Functions; import hudson.MarkupText; import java.io.IOException; import java.util.logging.Level; @@ -52,7 +53,7 @@ public class ExpandableDetailsNote extends ConsoleNote { @Override public ConsoleAnnotator annotate(Object context, MarkupText text, int charPos) { text.addMarkup(charPos, - "
" + html + "
"); + "
" + html + "
"); return null; } diff --git a/test/src/test/java/jenkins/security/Security3245Test.java b/test/src/test/java/jenkins/security/Security3245Test.java new file mode 100644 index 0000000000..9c9a37b465 --- /dev/null +++ b/test/src/test/java/jenkins/security/Security3245Test.java @@ -0,0 +1,68 @@ +package jenkins.security; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; + +import edu.umd.cs.findbugs.annotations.NonNull; +import hudson.EnvVars; +import hudson.FilePath; +import hudson.Launcher; +import hudson.console.ExpandableDetailsNote; +import hudson.model.FreeStyleBuild; +import hudson.model.FreeStyleProject; +import hudson.model.Run; +import hudson.model.TaskListener; +import hudson.tasks.Builder; +import java.io.IOException; +import java.util.concurrent.atomic.AtomicBoolean; +import jenkins.tasks.SimpleBuildStep; +import org.htmlunit.html.HtmlPage; +import org.junit.Assert; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.Issue; +import org.jvnet.hudson.test.JenkinsRule; + +public class Security3245Test { + + @Rule + public JenkinsRule j = new JenkinsRule(); + + @Issue("SECURITY-3245") + @Test + public void captionCannotAttributeEscape() throws Exception { + FreeStyleProject p = j.createFreeStyleProject("p"); + p.getBuildersList().add(new ExpandableDetailsNoteTestAction("' onclick=alert(1) foo='bar", "

")); + FreeStyleBuild build = j.buildAndAssertSuccess(p); + + AtomicBoolean alerts = new AtomicBoolean(); + try (JenkinsRule.WebClient wc = j.createWebClient()) { + wc.setAlertHandler((pr, s) -> alerts.set(true)); + final HtmlPage page = wc.goTo(build.getUrl() + "console"); + String content = page.getWebResponse().getContentAsString(); + assertThat(content, containsString("")); + + // Execute JavaScript code to simulate click event + String jsCode = "document.querySelector('.reveal-expandable-detail').dispatchEvent(new MouseEvent('click'));"; + page.executeJavaScript(jsCode); + + Assert.assertFalse("Alert not expected", alerts.get()); + } + } + + static class ExpandableDetailsNoteTestAction extends Builder implements SimpleBuildStep { + + final String caption; + final String html; + + ExpandableDetailsNoteTestAction(String caption, String html) { + this.caption = caption; + this.html = html; + } + + @Override + public void perform(@NonNull Run run, @NonNull FilePath workspace, @NonNull EnvVars env, @NonNull Launcher launcher, @NonNull TaskListener listener) throws IOException { + listener.annotate(new ExpandableDetailsNote(caption, html)); + } + } +} From df7c4ccda8976c06bf31b8fb9938f26fc38501ca Mon Sep 17 00:00:00 2001 From: Kevin-CB Date: Wed, 6 Sep 2023 11:17:25 +0000 Subject: [PATCH 3/4] [SECURITY-3072] --- core/src/main/java/hudson/PluginManager.java | 21 +--- .../hudson/PluginManagerSecurity3072Test.java | 107 ++++++++++++++++++ .../test/java/hudson/PluginManagerTest.java | 27 +++-- 3 files changed, 128 insertions(+), 27 deletions(-) create mode 100644 test/src/test/java/hudson/PluginManagerSecurity3072Test.java diff --git a/core/src/main/java/hudson/PluginManager.java b/core/src/main/java/hudson/PluginManager.java index 5d64485cfa..bd627324bb 100644 --- a/core/src/main/java/hudson/PluginManager.java +++ b/core/src/main/java/hudson/PluginManager.java @@ -28,8 +28,6 @@ import static hudson.init.InitMilestone.COMPLETED; import static hudson.init.InitMilestone.PLUGINS_LISTED; import static hudson.init.InitMilestone.PLUGINS_PREPARED; import static hudson.init.InitMilestone.PLUGINS_STARTED; -import static java.nio.file.attribute.PosixFilePermission.OWNER_READ; -import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE; import static java.util.logging.Level.FINE; import static java.util.logging.Level.INFO; import static java.util.logging.Level.WARNING; @@ -83,7 +81,6 @@ import java.net.URLClassLoader; import java.net.URLConnection; import java.net.http.HttpClient; import java.net.http.HttpRequest; -import java.nio.file.FileSystems; import java.nio.file.Files; import java.nio.file.InvalidPathException; import java.nio.file.Paths; @@ -91,10 +88,8 @@ import java.nio.file.attribute.FileTime; import java.security.CodeSource; import java.time.Duration; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.Collections; -import java.util.EnumSet; import java.util.Enumeration; import java.util.HashMap; import java.util.HashSet; @@ -1883,16 +1878,6 @@ public abstract class PluginManager extends AbstractModelObject implements OnMas copier = new FileUploadPluginCopier(fileItem); } - if (FileSystems.getDefault().supportedFileAttributeViews().contains("posix")) { - Arrays.stream(Objects.requireNonNull(tmpDir.listFiles())).forEach((file -> { - try { - Files.setPosixFilePermissions(file.toPath(), EnumSet.of(OWNER_READ, OWNER_WRITE)); - } catch (IOException e) { - throw new RuntimeException(e); - } - })); - } - if ("".equals(fileName)) { return new HttpRedirect("advanced"); } @@ -1902,7 +1887,8 @@ public abstract class PluginManager extends AbstractModelObject implements OnMas } // first copy into a temporary file name - File t = File.createTempFile("uploaded", ".jpi"); + File t = File.createTempFile("uploaded", ".jpi", tmpDir); + tmpDir.deleteOnExit(); t.deleteOnExit(); // TODO Remove this workaround after FILEUPLOAD-293 is resolved. Files.delete(Util.fileToPath(t)); @@ -1913,9 +1899,6 @@ public abstract class PluginManager extends AbstractModelObject implements OnMas throw new ServletException(e); } copier.cleanup(); - if (!tmpDir.delete()) { - System.err.println("Failed to delete temporary directory: " + tmpDir); - } final String baseName = identifyPluginShortName(t); diff --git a/test/src/test/java/hudson/PluginManagerSecurity3072Test.java b/test/src/test/java/hudson/PluginManagerSecurity3072Test.java new file mode 100644 index 0000000000..774d6b5ce1 --- /dev/null +++ b/test/src/test/java/hudson/PluginManagerSecurity3072Test.java @@ -0,0 +1,107 @@ +package hudson; + +import static java.nio.file.attribute.PosixFilePermission.OWNER_EXECUTE; +import static java.nio.file.attribute.PosixFilePermission.OWNER_READ; +import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE; +import static org.awaitility.Awaitility.await; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assume.assumeFalse; + +import hudson.model.RootAction; +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.LinkOption; +import java.nio.file.attribute.PosixFilePermission; +import java.util.Arrays; +import java.util.Comparator; +import java.util.EnumSet; +import java.util.HashSet; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.TimeUnit; +import javax.servlet.ServletException; +import jenkins.model.Jenkins; +import org.htmlunit.html.HtmlForm; +import org.htmlunit.html.HtmlPage; +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.TestExtension; +import org.kohsuke.stapler.StaplerRequest; +import org.kohsuke.stapler.StaplerResponse; + +public class PluginManagerSecurity3072Test { + + @Rule + public JenkinsRule r = PluginManagerUtil.newJenkinsRule(); + + @Rule + public TemporaryFolder tmp = new TemporaryFolder(); + + @Test + @Issue("SECURITY-3072") + public void verifyUploadedPluginFromURLPermission() throws Exception { + assumeFalse(Functions.isWindows()); + + HtmlPage page = r.createWebClient().goTo("pluginManager/advanced"); + HtmlForm f = page.getFormByName("uploadPlugin"); + f.getInputByName("pluginUrl").setValue(Jenkins.get().getRootUrl() + "pluginManagerGetPlugin/htmlpublisher.jpi"); + r.submit(f); + + File filesRef = Files.createTempFile("tmp", ".tmp").toFile(); + File filesTmpDir = filesRef.getParentFile(); + filesRef.deleteOnExit(); + + final Set[] filesPermission = new Set[]{new HashSet<>()}; + await().pollInterval(250, TimeUnit.MILLISECONDS) + .atMost(10, TimeUnit.SECONDS) + .until(() -> { + Optional lastUploadedPluginDir = Arrays.stream(Objects.requireNonNull( + filesTmpDir.listFiles((file, fileName) -> + fileName.startsWith("uploadDir")))). + max(Comparator.comparingLong(File::lastModified)); + if (lastUploadedPluginDir.isPresent()) { + filesPermission[0] = Files.getPosixFilePermissions(lastUploadedPluginDir.get().toPath(), LinkOption.NOFOLLOW_LINKS); + Optional pluginFile = Arrays.stream(Objects.requireNonNull( + lastUploadedPluginDir.get().listFiles((file, fileName) -> + fileName.startsWith("uploaded")))). + max(Comparator.comparingLong(File::lastModified)); + assertTrue(pluginFile.isPresent()); + return true; + } else { + return false; + } + }); + assertEquals(EnumSet.of(OWNER_EXECUTE, OWNER_READ, OWNER_WRITE), filesPermission[0]); + } + + @TestExtension("verifyUploadedPluginFromURLPermission") + public static final class ReturnPluginJpiAction implements RootAction { + + @Override + public String getIconFileName() { + return "gear2.png"; + } + + @Override + public String getDisplayName() { + return "URL to retrieve a plugin jpi"; + } + + @Override + public String getUrlName() { + return "pluginManagerGetPlugin"; + } + + public void doDynamic(StaplerRequest staplerRequest, StaplerResponse staplerResponse) throws ServletException, IOException { + staplerResponse.setContentType("application/octet-stream"); + staplerResponse.setStatus(200); + staplerResponse.serveFile(staplerRequest, PluginManagerTest.class.getClassLoader().getResource("plugins/htmlpublisher.jpi")); + } + } +} diff --git a/test/src/test/java/hudson/PluginManagerTest.java b/test/src/test/java/hudson/PluginManagerTest.java index bd761d21d4..169b88993f 100644 --- a/test/src/test/java/hudson/PluginManagerTest.java +++ b/test/src/test/java/hudson/PluginManagerTest.java @@ -24,6 +24,7 @@ package hudson; +import static java.nio.file.attribute.PosixFilePermission.OWNER_EXECUTE; import static java.nio.file.attribute.PosixFilePermission.OWNER_READ; import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE; import static org.awaitility.Awaitility.await; @@ -173,7 +174,7 @@ public class PluginManagerTest { } public void doDynamic(StaplerRequest staplerRequest, StaplerResponse staplerResponse) throws ServletException, IOException { - staplerResponse.setContentType("application/octet"); + staplerResponse.setContentType("application/octet-stream"); staplerResponse.setStatus(200); staplerResponse.serveFile(staplerRequest, PluginManagerTest.class.getClassLoader().getResource("plugins/htmlpublisher.jpi")); } @@ -743,24 +744,34 @@ public class PluginManagerTest { File dir = tmp.newFolder(); File plugin = new File(dir, "htmlpublisher.jpi"); FileUtils.copyURLToFile(Objects.requireNonNull(getClass().getClassLoader().getResource("plugins/htmlpublisher.jpi")), plugin); - f.getInputByName("name").setValue(plugin.getAbsolutePath()); + f.getInputByName("name").setValueAttribute(plugin.getAbsolutePath()); r.submit(f); - File tmpDir = new File(File.createTempFile("tmp", ".tmp").getParent()); - tmpDir.deleteOnExit(); + File filesRef = Files.createTempFile("tmp", ".tmp").toFile(); + File filesTmpDir = filesRef.getParentFile(); + filesRef.deleteOnExit(); + final Set[] filesPermission = new Set[]{new HashSet<>()}; await().pollInterval(250, TimeUnit.MILLISECONDS) .atMost(10, TimeUnit.SECONDS) .until(() -> { - Optional lastUploadedPlugin = Arrays.stream(Objects.requireNonNull(tmpDir.listFiles((file, fileName) -> fileName.startsWith("uploaded")))).max(Comparator.comparingLong(File::lastModified)); - if (lastUploadedPlugin.isPresent()) { - filesPermission[0] = Files.getPosixFilePermissions(lastUploadedPlugin.get().toPath(), LinkOption.NOFOLLOW_LINKS); + Optional lastUploadedPluginDir = Arrays.stream(Objects.requireNonNull( + filesTmpDir.listFiles((file, fileName) -> + fileName.startsWith("uploadDir")))). + max(Comparator.comparingLong(File::lastModified)); + if (lastUploadedPluginDir.isPresent()) { + filesPermission[0] = Files.getPosixFilePermissions(lastUploadedPluginDir.get().toPath(), LinkOption.NOFOLLOW_LINKS); + Optional pluginFile = Arrays.stream(Objects.requireNonNull( + lastUploadedPluginDir.get().listFiles((file, fileName) -> + fileName.startsWith("uploaded")))). + max(Comparator.comparingLong(File::lastModified)); + assertTrue(pluginFile.isPresent()); return true; } else { return false; } }); - assertEquals(EnumSet.of(OWNER_READ, OWNER_WRITE), filesPermission[0]); + assertEquals(EnumSet.of(OWNER_EXECUTE, OWNER_READ, OWNER_WRITE), filesPermission[0]); } @Test From f06bb8820115996397a2e0010b003ecbf0570865 Mon Sep 17 00:00:00 2001 From: Kevin-CB Date: Wed, 6 Sep 2023 11:17:31 +0000 Subject: [PATCH 4/4] [SECURITY-3073] --- bom/pom.xml | 2 +- .../hudson/util/MultipartFormDataParser.java | 12 ++- .../FileParameterValueSecurity3073Test.java | 81 +++++++++++++++++++ .../model/JenkinsSecurity3073Test.java | 77 ++++++++++++++++++ 4 files changed, 170 insertions(+), 2 deletions(-) create mode 100644 test/src/test/java/hudson/model/FileParameterValueSecurity3073Test.java create mode 100644 test/src/test/java/jenkins/model/JenkinsSecurity3073Test.java diff --git a/bom/pom.xml b/bom/pom.xml index 7f0162f63c..ffbb673acf 100644 --- a/bom/pom.xml +++ b/bom/pom.xml @@ -40,7 +40,7 @@ THE SOFTWARE. 9.5 2.0.7 - 1802.v9e2750160d01 + 1802.1804.va_8d30483a_7f7 2.4.21 diff --git a/core/src/main/java/hudson/util/MultipartFormDataParser.java b/core/src/main/java/hudson/util/MultipartFormDataParser.java index 762c0c35b6..ecc4ad292d 100644 --- a/core/src/main/java/hudson/util/MultipartFormDataParser.java +++ b/core/src/main/java/hudson/util/MultipartFormDataParser.java @@ -25,6 +25,9 @@ package hudson.util; import edu.umd.cs.findbugs.annotations.CheckForNull; +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; import java.util.HashMap; import java.util.Map; import javax.servlet.ServletException; @@ -46,7 +49,6 @@ import org.kohsuke.accmod.restrictions.NoExternalUse; * @author Kohsuke Kawaguchi */ public class MultipartFormDataParser implements AutoCloseable { - private final ServletFileUpload upload = new ServletFileUpload(new DiskFileItemFactory()); private final Map byName = new HashMap<>(); /** @@ -74,6 +76,14 @@ public class MultipartFormDataParser implements AutoCloseable { @Restricted(NoExternalUse.class) public MultipartFormDataParser(HttpServletRequest request, int maxParts, long maxPartSize, long maxSize) throws ServletException { + File tmpDir; + try { + tmpDir = Files.createTempDirectory("jenkins-multipart-uploads").toFile(); + } catch (IOException e) { + throw new ServletException("Error creating temporary directory", e); + } + tmpDir.deleteOnExit(); + ServletFileUpload upload = new ServletFileUpload(new DiskFileItemFactory(DiskFileItemFactory.DEFAULT_SIZE_THRESHOLD, tmpDir)); upload.setFileCountMax(maxParts); upload.setFileSizeMax(maxPartSize); upload.setSizeMax(maxSize); diff --git a/test/src/test/java/hudson/model/FileParameterValueSecurity3073Test.java b/test/src/test/java/hudson/model/FileParameterValueSecurity3073Test.java new file mode 100644 index 0000000000..bcbbd40231 --- /dev/null +++ b/test/src/test/java/hudson/model/FileParameterValueSecurity3073Test.java @@ -0,0 +1,81 @@ +package hudson.model; + +import static java.nio.file.attribute.PosixFilePermission.OWNER_EXECUTE; +import static java.nio.file.attribute.PosixFilePermission.OWNER_READ; +import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE; +import static org.awaitility.Awaitility.await; +import static org.junit.Assert.assertEquals; +import static org.junit.Assume.assumeFalse; + +import hudson.Functions; +import java.io.File; +import java.nio.file.Files; +import java.nio.file.LinkOption; +import java.nio.file.attribute.PosixFilePermission; +import java.util.Arrays; +import java.util.Comparator; +import java.util.EnumSet; +import java.util.HashSet; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.TimeUnit; +import org.apache.commons.io.FileUtils; +import org.htmlunit.html.HtmlForm; +import org.htmlunit.html.HtmlPage; +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; + +public class FileParameterValueSecurity3073Test { + + @Rule + public JenkinsRule j = new JenkinsRule(); + + @Rule + public TemporaryFolder tmp = new TemporaryFolder(); + + @Test + @Issue("SECURITY-3073") + public void verifyUploadedFileParameterPermission() throws Exception { + assumeFalse(Functions.isWindows()); + + FreeStyleProject project = j.createFreeStyleProject(); + project.addProperty(new ParametersDefinitionProperty(List.of( + new FileParameterDefinition("filePermission", null) + ))); + File dir = tmp.newFolder(); + File plugin = new File(dir, "htmlpublisher.jpi"); + // We're using a plugin to have a file above DiskFileItemFactory.DEFAULT_SIZE_THRESHOLD + FileUtils.copyURLToFile(Objects.requireNonNull(getClass().getClassLoader().getResource("plugins/htmlpublisher.jpi")), plugin); + + HtmlPage page = j.createWebClient().withThrowExceptionOnFailingStatusCode(false).goTo(project.getUrl() + "/build?delay=0sec"); + HtmlForm form = page.getFormByName("parameters"); + form.getInputByName("file").setValueAttribute(plugin.getAbsolutePath()); + j.submit(form); + + File filesRef = Files.createTempFile("tmp", ".tmp").toFile(); + File filesTmpDir = filesRef.getParentFile(); + filesRef.deleteOnExit(); + + final Set[] filesPermission = new Set[]{new HashSet<>()}; + await().pollInterval(250, TimeUnit.MILLISECONDS) + .atMost(10, TimeUnit.SECONDS) + .until(() -> { + Optional lastUploadedPlugin = Arrays.stream(Objects.requireNonNull( + filesTmpDir.listFiles((file, fileName) -> + fileName.startsWith("jenkins-stapler-uploads")))). + max(Comparator.comparingLong(File::lastModified)); + if (lastUploadedPlugin.isPresent()) { + filesPermission[0] = Files.getPosixFilePermissions(lastUploadedPlugin.get().toPath(), LinkOption.NOFOLLOW_LINKS); + return true; + } else { + return false; + } + }); + assertEquals(EnumSet.of(OWNER_EXECUTE, OWNER_READ, OWNER_WRITE), filesPermission[0]); + } +} diff --git a/test/src/test/java/jenkins/model/JenkinsSecurity3073Test.java b/test/src/test/java/jenkins/model/JenkinsSecurity3073Test.java new file mode 100644 index 0000000000..96c3fb3c63 --- /dev/null +++ b/test/src/test/java/jenkins/model/JenkinsSecurity3073Test.java @@ -0,0 +1,77 @@ +package jenkins.model; + +import static java.nio.file.attribute.PosixFilePermission.OWNER_EXECUTE; +import static java.nio.file.attribute.PosixFilePermission.OWNER_READ; +import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE; +import static org.awaitility.Awaitility.await; +import static org.junit.Assert.assertEquals; +import static org.junit.Assume.assumeFalse; + +import hudson.Functions; +import java.io.File; +import java.nio.file.Files; +import java.nio.file.LinkOption; +import java.nio.file.attribute.PosixFilePermission; +import java.util.Arrays; +import java.util.Comparator; +import java.util.EnumSet; +import java.util.HashSet; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.concurrent.TimeUnit; +import org.apache.commons.io.FileUtils; +import org.htmlunit.html.HtmlForm; +import org.htmlunit.html.HtmlPage; +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; + +public class JenkinsSecurity3073Test { + + + @Rule + public JenkinsRule j = new JenkinsRule(); + + @Rule + public TemporaryFolder tmp = new TemporaryFolder(); + + @Test + @Issue("SECURITY-3073") + public void verifyUploadedFingerprintFilePermission() throws Exception { + assumeFalse(Functions.isWindows()); + + HtmlPage page = j.createWebClient().goTo("fingerprintCheck"); + // The form doesn't have a name, the page contain the search form and the one we're interested in + HtmlForm form = page.getForms().get(1); + File dir = tmp.newFolder(); + File plugin = new File(dir, "htmlpublisher.jpi"); + // We're using a plugin to have a file above DiskFileItemFactory.DEFAULT_SIZE_THRESHOLD + FileUtils.copyURLToFile(Objects.requireNonNull(getClass().getClassLoader().getResource("plugins/htmlpublisher.jpi")), plugin); + form.getInputByName("name").setValueAttribute(plugin.getAbsolutePath()); + j.submit(form); + + File filesRef = Files.createTempFile("tmp", ".tmp").toFile(); + File filesTmpDir = filesRef.getParentFile(); + filesRef.deleteOnExit(); + + final Set[] filesPermission = new Set[]{new HashSet<>()}; + await().pollInterval(250, TimeUnit.MILLISECONDS) + .atMost(10, TimeUnit.SECONDS) + .until(() -> { + Optional lastUploadedPlugin = Arrays.stream(Objects.requireNonNull( + filesTmpDir.listFiles((file, fileName) -> + fileName.startsWith("jenkins-multipart-uploads")))). + max(Comparator.comparingLong(File::lastModified)); + if (lastUploadedPlugin.isPresent()) { + filesPermission[0] = Files.getPosixFilePermissions(lastUploadedPlugin.get().toPath(), LinkOption.NOFOLLOW_LINKS); + return true; + } else { + return false; + } + }); + assertEquals(EnumSet.of(OWNER_EXECUTE, OWNER_READ, OWNER_WRITE), filesPermission[0]); + } +}