Skip to content

Commit

Permalink
[SECURITY-3072]
Browse files Browse the repository at this point in the history
  • Loading branch information
Kevin-CB authored and jenkinsci-cert-ci committed Sep 6, 2023
1 parent 5d7f168 commit 18cb013
Show file tree
Hide file tree
Showing 3 changed files with 128 additions and 27 deletions.
21 changes: 2 additions & 19 deletions core/src/main/java/hudson/PluginManager.java
Expand Up @@ -28,8 +28,6 @@
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;
Expand Down Expand Up @@ -83,18 +81,15 @@
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;
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;
Expand Down Expand Up @@ -1883,16 +1878,6 @@ public HttpResponse doUploadPlugin(StaplerRequest req) throws IOException, Servl
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");
}
Expand All @@ -1902,7 +1887,8 @@ public HttpResponse doUploadPlugin(StaplerRequest req) throws IOException, Servl
}

// 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));
Expand All @@ -1913,9 +1899,6 @@ public HttpResponse doUploadPlugin(StaplerRequest req) throws IOException, Servl
throw new ServletException(e);
}
copier.cleanup();
if (!tmpDir.delete()) {
System.err.println("Failed to delete temporary directory: " + tmpDir);
}

final String baseName = identifyPluginShortName(t);

Expand Down
107 changes: 107 additions & 0 deletions 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<PosixFilePermission>[] filesPermission = new Set[]{new HashSet<>()};
await().pollInterval(250, TimeUnit.MILLISECONDS)
.atMost(10, TimeUnit.SECONDS)
.until(() -> {
Optional<File> 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<File> 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"));
}
}
}
27 changes: 19 additions & 8 deletions test/src/test/java/hudson/PluginManagerTest.java
Expand Up @@ -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;
Expand Down Expand Up @@ -173,7 +174,7 @@ public String getUrlName() {
}

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"));
}
Expand Down Expand Up @@ -743,24 +744,34 @@ public void verifyUploadedPluginPermission() throws Exception {
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<PosixFilePermission>[] filesPermission = new Set[]{new HashSet<>()};
await().pollInterval(250, TimeUnit.MILLISECONDS)
.atMost(10, TimeUnit.SECONDS)
.until(() -> {
Optional<File> 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<File> 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<File> 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
Expand Down

0 comments on commit 18cb013

Please sign in to comment.