Skip to content

Commit

Permalink
[SECURITY-904]
Browse files Browse the repository at this point in the history
  • Loading branch information
@Wadeck @daniel-beck
Wadeck authored and daniel-beck committed Nov 21, 2018
1 parent 82f5555 commit c19cc70
Show file tree
Hide file tree
Showing 14 changed files with 1,509 additions and 39 deletions.
64 changes: 47 additions & 17 deletions core/src/main/java/hudson/FilePath.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,10 @@
import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.InvalidPathException;
import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
import java.nio.file.LinkOption;
import java.nio.file.Paths;
import java.nio.file.StandardCopyOption;
import java.nio.file.attribute.FileAttribute;
import java.nio.file.attribute.PosixFilePermission;
Expand Down Expand Up @@ -3044,33 +3046,61 @@ public Boolean invoke(@Nonnull File parentFile, @Nonnull VirtualChannel channel)
if (new File(potentialChildRelativePath).isAbsolute()) {
throw new IllegalArgumentException("Only a relative path is supported, the given path is absolute: " + potentialChildRelativePath);
}

Path parentAbsolutePath = Util.fileToPath(parentFile.getAbsoluteFile());
Path parentRealPath;
try {
parentRealPath = parentAbsolutePath.toRealPath();
}
catch(NoSuchFileException e) {
throw new IllegalArgumentException("The parent does not exist");
}

Path parent = parentFile.getAbsoluteFile().toPath().normalize();

// example: "a/b/c" that will become "b/c" then just "c", and finally an empty string
String remainingPath = potentialChildRelativePath;
File currentFile = parentFile;

Path currentFilePath = parentFile.toPath();
while (!remainingPath.isEmpty()) {
File directChild = this.getDirectChild(currentFile, remainingPath);
File childUsingFullPath = new File(currentFile, remainingPath);
remainingPath = childUsingFullPath.getAbsolutePath().substring(directChild.getAbsolutePath().length());

File childFileSymbolic = Util.resolveSymlinkToFile(directChild);
Path directChild = this.getDirectChild(currentFilePath, remainingPath);
Path childUsingFullPath = currentFilePath.resolve(remainingPath);
Path rel = directChild.toAbsolutePath().relativize(childUsingFullPath.toAbsolutePath());
remainingPath = rel.toString();

File childFileSymbolic = Util.resolveSymlinkToFile(directChild.toFile());
if (childFileSymbolic == null) {
currentFile = directChild;
currentFilePath = directChild;
} else {
currentFile = childFileSymbolic;
currentFilePath = childFileSymbolic.toPath();
}

Path currentFileAbsolutePath = currentFilePath.toAbsolutePath();
try{
Path child = currentFileAbsolutePath.toRealPath();
if (!child.startsWith(parentRealPath)) {
return false;
}
} catch (NoSuchFileException e) {
// nonexistent file
// in case this folder / file will be copied somewhere else,
// it becomes the responsibility of that system to check the isDescendant with the existing links
// we are not taking the parentRealPath to avoid possible problem
try {
Path child = currentFileAbsolutePath.normalize();
Path parent = parentAbsolutePath.normalize();
return child.startsWith(parent);
} catch (InvalidPathException e2) {
throw new IOException(e2);
}
}
}

//TODO could be refactored using Util#isDescendant(File, File) from 2.80+
Path child = currentFile.getAbsoluteFile().toPath().normalize();
return child.startsWith(parent);
return true;
}

private @CheckForNull File getDirectChild(File parentFile, String childPath){
File current = new File(parentFile, childPath);
while (current != null && !parentFile.equals(current.getParentFile())) {
current = current.getParentFile();
private @CheckForNull Path getDirectChild(Path parentPath, String childPath){
Path current = parentPath.resolve(childPath);
while (current != null && !parentPath.equals(current.getParent())) {
current = current.getParent();
}
return current;
}
Expand Down
109 changes: 93 additions & 16 deletions core/src/main/java/hudson/model/DirectoryBrowserSupport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
*/
package hudson.model;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.FilePath;
import hudson.Util;
import java.io.IOException;
Expand All @@ -36,9 +37,12 @@
import java.util.Comparator;
import java.util.List;
import java.util.Locale;
import java.util.Objects;
import java.util.StringTokenizer;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServletResponse;
import jenkins.model.Jenkins;
Expand All @@ -64,6 +68,9 @@
* @author Kohsuke Kawaguchi
*/
public final class DirectoryBrowserSupport implements HttpResponse {
// escape hatch for SECURITY-904 to keep legacy behavior
@SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Accessible via System Groovy Scripts")
public static boolean ALLOW_SYMLINK_ESCAPE = Boolean.getBoolean(DirectoryBrowserSupport.class.getName() + ".allowSymlinkEscape");

public final ModelObject owner;

Expand Down Expand Up @@ -212,13 +219,19 @@ private void serveFile(StaplerRequest req, StaplerResponse rsp, VirtualFile root
String base = _base.toString();
String rest = _rest.toString();

if(!ALLOW_SYMLINK_ESCAPE && (root.supportIsDescendant() && !root.isDescendant(base))){
LOGGER.log(Level.WARNING, "Trying to access a file outside of the directory, target: "+ base);
rsp.sendError(HttpServletResponse.SC_FORBIDDEN, "Trying to access a file outside of the directory, target: " + base);
return;
}

// this is the base file/directory
VirtualFile baseFile = root.child(base);

if(baseFile.isDirectory()) {
if(zip) {
rsp.setContentType("application/zip");
zip(rsp.getOutputStream(), baseFile, rest);
zip(rsp, root, baseFile, rest);
return;
}
if (plain) {
Expand Down Expand Up @@ -246,8 +259,8 @@ private void serveFile(StaplerRequest req, StaplerResponse rsp, VirtualFile root
}

List<List<Path>> glob = null;

if(rest.length()>0) {
boolean patternUsed = rest.length() > 0;
if(patternUsed) {
// the rest is Ant glob pattern
glob = patternScan(baseFile, rest, createBackRef(restSize));
} else
Expand All @@ -257,13 +270,15 @@ private void serveFile(StaplerRequest req, StaplerResponse rsp, VirtualFile root
}

if(glob!=null) {
List<List<Path>> filteredGlob = keepReadabilityOnlyOnDescendants(baseFile, patternUsed, glob);

// serve glob
req.setAttribute("it", this);
List<Path> parentPaths = buildParentPath(base,restSize);
req.setAttribute("parentPath",parentPaths);
req.setAttribute("backPath", createBackRef(restSize));
req.setAttribute("topPath", createBackRef(parentPaths.size()+restSize));
req.setAttribute("files", glob);
req.setAttribute("files", filteredGlob);
req.setAttribute("icon", icon);
req.setAttribute("path", path);
req.setAttribute("pattern",rest);
Expand Down Expand Up @@ -319,6 +334,57 @@ private void serveFile(StaplerRequest req, StaplerResponse rsp, VirtualFile root
rsp.serveFile(req, in, lastModified, -1, length, baseFile.getName() );
}
}

private List<List<Path>> keepReadabilityOnlyOnDescendants(VirtualFile root, boolean patternUsed, List<List<Path>> pathFragmentsList){
Stream<List<Path>> pathFragmentsStream = pathFragmentsList.stream().map((List<Path> pathFragments) -> {
List<Path> mappedFragments = new ArrayList<>(pathFragments.size());
String relativePath = "";
for (int i = 0; i < pathFragments.size(); i++) {
Path current = pathFragments.get(i);
if (i == 0) {
relativePath = current.title;
} else {
relativePath += "/" + current.title;
}

if (!current.isReadable) {
if (patternUsed) {
// we do not want to leak information about existence of folders / files satisfying the pattern inside that folder
return null;
}
mappedFragments.add(current);
return mappedFragments;
} else {
if (isDescendant(root, relativePath)) {
mappedFragments.add(current);
} else {
if (patternUsed) {
// we do not want to leak information about existence of folders / files satisfying the pattern inside that folder
return null;
}
mappedFragments.add(Path.createNotReadableVersionOf(current));
return mappedFragments;
}
}
}
return mappedFragments;
});

if (patternUsed) {
pathFragmentsStream = pathFragmentsStream.filter(Objects::nonNull);
}

return pathFragmentsStream.collect(Collectors.toList());
}

private boolean isDescendant(VirtualFile root, String relativePath){
try {
return ALLOW_SYMLINK_ESCAPE || !root.supportIsDescendant() || root.isDescendant(relativePath);
}
catch (IOException e) {
return false;
}
}

private String getPath(StaplerRequest req) {
String path = req.getRestOfPath();
Expand Down Expand Up @@ -352,7 +418,8 @@ private static String createBackRef(int times) {
return buf.toString();
}

private static void zip(OutputStream outputStream, VirtualFile dir, String glob) throws IOException {
private static void zip(StaplerResponse rsp, VirtualFile root, VirtualFile dir, String glob) throws IOException, InterruptedException {
OutputStream outputStream = rsp.getOutputStream();
try (ZipOutputStream zos = new ZipOutputStream(outputStream)) {
zos.setEncoding(System.getProperty("file.encoding")); // TODO JENKINS-20663 make this overridable via query parameter
for (String n : dir.list(glob.length() == 0 ? "**" : glob)) {
Expand All @@ -363,18 +430,24 @@ private static void zip(OutputStream outputStream, VirtualFile dir, String glob)
} else {
relativePath = n;
}
// In ZIP archives "All slashes MUST be forward slashes" (http://pkware.com/documents/casestudies/APPNOTE.TXT)
// TODO On Linux file names can contain backslashes which should not treated as file separators.
// Unfortunately, only the file separator char of the master is known (File.separatorChar)
// but not the file separator char of the (maybe remote) "dir".
ZipEntry e = new ZipEntry(relativePath.replace('\\', '/'));
VirtualFile f = dir.child(n);
e.setTime(f.lastModified());
zos.putNextEntry(e);
try (InputStream in = f.open()) {
IOUtils.copy(in, zos);

String targetFile = dir.toString().substring(root.toString().length()) + n;
if (!ALLOW_SYMLINK_ESCAPE && root.supportIsDescendant() && !root.isDescendant(targetFile)) {
LOGGER.log(Level.INFO, "Trying to access a file outside of the directory: " + root + ", illicit target: " + targetFile);
} else {
// In ZIP archives "All slashes MUST be forward slashes" (http://pkware.com/documents/casestudies/APPNOTE.TXT)
// TODO On Linux file names can contain backslashes which should not treated as file separators.
// Unfortunately, only the file separator char of the master is known (File.separatorChar)
// but not the file separator char of the (maybe remote) "dir".
ZipEntry e = new ZipEntry(relativePath.replace('\\', '/'));
VirtualFile f = dir.child(n);
e.setTime(f.lastModified());
zos.putNextEntry(e);
try (InputStream in = f.open()) {
IOUtils.copy(in, zos);
}
zos.closeEntry();
}
zos.closeEntry();
}
}
}
Expand Down Expand Up @@ -446,6 +519,10 @@ public long getSize() {
return size;
}

public static Path createNotReadableVersionOf(Path that){
return new Path(that.href, that.title, that.isFolder, that.size, false);
}

private static final long serialVersionUID = 1L;
}

Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/model/Run.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1151,7 +1151,7 @@ private int addArtifacts(@Nonnull VirtualFile dir,
}
return n;
}

/**
* Maximum number of artifacts to list before using switching to the tree view.
*/
Expand Down
Loading

0 comments on commit c19cc70

Please sign in to comment.