Skip to content

Commit

Permalink
Merge branch 'security-master'
Browse files Browse the repository at this point in the history
  • Loading branch information
yaroslavafenkin committed Sep 12, 2023
2 parents 0216ffd + f06bb88 commit f21cf87
Show file tree
Hide file tree
Showing 12 changed files with 501 additions and 32 deletions.
2 changes: 1 addition & 1 deletion bom/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ THE SOFTWARE.
<properties>
<asm.version>9.5</asm.version>
<slf4jVersion>2.0.9</slf4jVersion>
<stapler.version>1802.v9e2750160d01</stapler.version>
<stapler.version>1802.1804.va_8d30483a_7f7</stapler.version>
<groovy.version>2.4.21</groovy.version>
</properties>

Expand Down
21 changes: 2 additions & 19 deletions core/src/main/java/hudson/PluginManager.java
Original file line number Diff line number Diff line change
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
3 changes: 2 additions & 1 deletion core/src/main/java/hudson/console/ExpandableDetailsNote.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

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;
Expand All @@ -52,7 +53,7 @@ public ExpandableDetailsNote(String caption, String html) {
@Override
public ConsoleAnnotator annotate(Object context, MarkupText text, int charPos) {
text.addMarkup(charPos,
"<input type=button value='" + caption + "' class='reveal-expandable-detail'><div class='expandable-detail'>" + html + "</div>");
"<input type=button value='" + Functions.htmlAttributeEscape(caption) + "' class='reveal-expandable-detail'><div class='expandable-detail'>" + html + "</div>");
return null;
}

Expand Down
12 changes: 11 additions & 1 deletion core/src/main/java/hudson/util/MultipartFormDataParser.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -46,7 +49,6 @@
* @author Kohsuke Kawaguchi
*/
public class MultipartFormDataParser implements AutoCloseable {
private final ServletFileUpload upload = new ServletFileUpload(new DiskFileItemFactory());
private final Map<String, FileItem> byName = new HashMap<>();

/**
Expand Down Expand Up @@ -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);
Expand Down
6 changes: 4 additions & 2 deletions core/src/main/java/jenkins/widgets/HistoryPageFilter.java
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Set;
import jenkins.model.queue.QueueItem;

/**
Expand Down Expand Up @@ -371,8 +372,9 @@ private boolean fitsSearchString(Object data) {

private boolean fitsSearchBuildVariables(AbstractBuild<?, ?> runAsBuild) {
Map<String, String> buildVariables = runAsBuild.getBuildVariables();
for (String paramsValues : buildVariables.values()) {
if (fitsSearchString(paramsValues)) {
Set<String> sensitiveBuildVariables = runAsBuild.getSensitiveBuildVariables();
for (Map.Entry<String, String> param : buildVariables.entrySet()) {
if (!sensitiveBuildVariables.contains(param.getKey()) && fitsSearchString(param.getValue())) {
return true;
}
}
Expand Down
7 changes: 7 additions & 0 deletions core/src/test/java/jenkins/widgets/HistoryPageFilterTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
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;
Expand Down Expand Up @@ -485,6 +486,7 @@ private static class MockBuild extends Build<FreeStyleProject, FreeStyleBuild> {
private final int buildNumber;

private Map<String, String> buildVariables = Collections.emptyMap();
private Set<String> sensitiveBuildVariables = Collections.emptySet();

private MockBuild(int buildNumber) {
super(Mockito.mock(FreeStyleProject.class), Mockito.mock(Calendar.class));
Expand All @@ -501,6 +503,11 @@ public Map<String, String> getBuildVariables() {
return buildVariables;
}

@Override
public Set<String> getSensitiveBuildVariables() {
return sensitiveBuildVariables; // TODO This is never actually set (bad Mock), actual test in test harness
}

MockBuild withBuildVariables(Map<String, String> buildVariables) {
this.buildVariables = buildVariables;
return this;
Expand Down
107 changes: 107 additions & 0 deletions test/src/test/java/hudson/PluginManagerSecurity3072Test.java
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
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
Loading

0 comments on commit f21cf87

Please sign in to comment.