From df7c4ccda8976c06bf31b8fb9938f26fc38501ca Mon Sep 17 00:00:00 2001 From: Kevin-CB Date: Wed, 6 Sep 2023 11:17:25 +0000 Subject: [PATCH] [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