Skip to content

Commit

Permalink
jetty-start cleanup (#9555)
Browse files Browse the repository at this point in the history
* Extract jars/zips using zipfs
* Restrict MavenMetadata external DTD/XSD access
* Introduce --allow-insecure-http-downloads
* Produce debug log if file exists in destination during copy.
* Simpler MavenMetadata
* If `maven.repo.uri` has been redeclared by user, automatically set allow insecure http downloads.

---------

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
  • Loading branch information
joakime committed Apr 10, 2023
1 parent 2c61011 commit 81efae2
Show file tree
Hide file tree
Showing 9 changed files with 232 additions and 67 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ else if (args.isCreateFiles())
// Handle local directories
fileInitializers.add(new LocalFileInitializer(baseHome));

// Downloads are allowed to be performed
// Setup Maven Local Repo
Path localRepoDir = args.findMavenLocalRepoDir();
if (localRepoDir != null)
Expand All @@ -106,7 +105,7 @@ else if (args.isCreateFiles())
fileInitializers.add(new BaseHomeFileInitializer(baseHome));

// Normal URL downloads
fileInitializers.add(new UriFileInitializer(baseHome));
fileInitializers.add(new UriFileInitializer(startArgs, baseHome));
}
}

Expand Down
83 changes: 54 additions & 29 deletions jetty-start/src/main/java/org/eclipse/jetty/start/FS.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,20 @@
import java.io.Closeable;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.net.URI;
import java.nio.file.FileSystem;
import java.nio.file.FileSystemAlreadyExistsException;
import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.attribute.FileTime;
import java.util.Enumeration;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Locale;
import java.util.zip.ZipEntry;
import java.util.zip.ZipFile;
import java.util.Map;
import java.util.stream.Collectors;
import java.util.stream.Stream;

public class FS
{
Expand Down Expand Up @@ -184,40 +189,60 @@ public static void extract(Path archive, Path destination) throws IOException

public static void extractZip(Path archive, Path destination) throws IOException
{
try (ZipFile zip = new ZipFile(archive.toFile()))
StartLog.info("extract %s to %s", archive, destination);
URI jaruri = URI.create("jar:" + archive.toUri());
Map<String, ?> fsEnv = Map.of();
try (FileSystem zipFs = FileSystems.newFileSystem(jaruri, fsEnv))
{
StartLog.info("extract %s to %s", archive, destination);
Enumeration<? extends ZipEntry> entries = zip.entries();
while (entries.hasMoreElements())
{
ZipEntry entry = entries.nextElement();
copyZipContents(zipFs.getPath("/"), destination);
}
catch (FileSystemAlreadyExistsException e)
{
FileSystem zipFs = FileSystems.getFileSystem(jaruri);
copyZipContents(zipFs.getPath("/"), destination);
}
}

if (entry.isDirectory() || entry.getName().startsWith("/META-INF"))
{
// skip
continue;
}
public static void copyZipContents(Path root, Path destination) throws IOException
{
if (!Files.exists(destination))
{
Files.createDirectories(destination);
}
URI outputDirURI = destination.toUri();
URI archiveURI = root.toUri();
int archiveURISubIndex = archiveURI.toASCIIString().indexOf("!/") + 2;

String entryName = entry.getName();
Path destFile = destination.resolve(entryName).normalize().toAbsolutePath();
// make sure extracted path does not escape the destination directory
if (!destFile.startsWith(destination))
try (Stream<Path> entriesStream = Files.walk(root))
{
// ensure proper unpack order (eg: directories before files)
Stream<Path> sorted = entriesStream
.filter((path) -> path.getNameCount() > 0)
.filter((path) -> !path.getName(0).toString().equalsIgnoreCase("META-INF"))
.sorted();

Iterator<Path> pathIterator = sorted.iterator();
while (pathIterator.hasNext())
{
Path path = pathIterator.next();
URI entryURI = path.toUri();
String subURI = entryURI.toASCIIString().substring(archiveURISubIndex);
URI outputPathURI = outputDirURI.resolve(subURI);
Path outputPath = Path.of(outputPathURI);
StartLog.info("zipFs: %s > %s", path, outputPath);
if (Files.isDirectory(path))
{
throw new IOException(String.format("Malicious Archive %s found with bad entry \"%s\"",
archive, entryName));
if (!Files.exists(outputPath))
Files.createDirectory(outputPath);
}
if (!Files.exists(destFile))
else if (Files.exists(outputPath))
{
FS.ensureDirectoryExists(destFile.getParent());
try (InputStream input = zip.getInputStream(entry))
{
StartLog.debug("extracting %s", destFile);
Files.copy(input, destFile);
}
StartLog.debug("skipping extraction (file exists) %s", outputPath);
}
else
{
StartLog.debug("skipping extract (file exists) %s", destFile);
StartLog.info("extracting %s to %s", path, outputPath);
Files.copy(path, outputPath);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,11 @@ protected Path getDestination(URI uri, String location) throws IOException

Path destination = _basehome.getBasePath(location);

// now on copy/download paths (be safe above all else)
// We restrict our behavior to only modifying what exists in ${jetty.base}.
// If the user decides they want to use advanced setups, such as symlinks to point
// to content outside of ${jetty.base}, that is their decision ad we will not
// attempt to save them from themselves.
// Note: All copy and extract steps, will not replace files that already exist.
if (destination != null && !destination.startsWith(_basehome.getBasePath()))
throw new IOException("For security reasons, Jetty start is unable to process file resource not in ${jetty.base} - " + location);

Expand All @@ -120,35 +124,6 @@ protected Path getDestination(URI uri, String location) throws IOException
return destination;
}

protected void download(URI uri, Path destination) throws IOException
{
if (FS.ensureDirectoryExists(destination.getParent()))
StartLog.info("mkdir " + _basehome.toShortForm(destination.getParent()));

StartLog.info("download %s to %s", uri, _basehome.toShortForm(destination));

URLConnection connection = uri.toURL().openConnection();

if (connection instanceof HttpURLConnection)
{
HttpURLConnection http = (HttpURLConnection)uri.toURL().openConnection();
http.setInstanceFollowRedirects(true);
http.setAllowUserInteraction(false);

int status = http.getResponseCode();

if (status != HttpURLConnection.HTTP_OK)
{
throw new IOException("URL GET Failure [" + status + "/" + http.getResponseMessage() + "] on " + uri);
}
}

try (InputStream in = connection.getInputStream())
{
Files.copy(in, destination, StandardCopyOption.REPLACE_EXISTING);
}
}

/**
* Test if any of the Paths exist (as files)
*
Expand Down Expand Up @@ -199,7 +174,11 @@ public boolean copyDirectory(Path source, Path destination) throws IOException
if (copyDirectory(from, to))
modified = true;
}
else if (!Files.exists(to))
else if (Files.exists(to))
{
StartLog.debug("skipping copy (file exists in destination) %s", to);
}
else
{
StartLog.info("copy %s to %s", _basehome.toShortForm(from), _basehome.toShortForm(to));
Files.copy(from, to);
Expand Down
14 changes: 14 additions & 0 deletions jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ public class StartArgs
public static final String VERSION;
public static final Set<String> ALL_PARTS = Set.of("java", "opts", "path", "main", "args");
public static final Set<String> ARG_PARTS = Set.of("args");
public static final String ARG_ALLOW_INSECURE_HTTP_DOWNLOADS = "--allow-insecure-http-downloads";

private static final String JETTY_VERSION_KEY = "jetty.version";
private static final String JETTY_TAG_NAME_KEY = "jetty.tag.version";
Expand Down Expand Up @@ -216,6 +217,7 @@ public class StartArgs

private boolean exec = false;
private String execProperties;
private boolean allowInsecureHttpDownloads = false;
private boolean approveAllLicenses = false;

public StartArgs(BaseHome baseHome)
Expand Down Expand Up @@ -960,6 +962,11 @@ public boolean hasSystemProperties()
return false;
}

public boolean isAllowInsecureHttpDownloads()
{
return allowInsecureHttpDownloads;
}

public boolean isApproveAllLicenses()
{
return approveAllLicenses;
Expand Down Expand Up @@ -1244,6 +1251,13 @@ public void parse(final String rawarg, String source)
return;
}

// Allow insecure-http downloads
if (ARG_ALLOW_INSECURE_HTTP_DOWNLOADS.equals(arg))
{
allowInsecureHttpDownloads = true;
return;
}

// Enable forked execution of Jetty server
if ("--approve-all-licenses".equals(arg))
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
//
// ========================================================================
// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others.
//
// This program and the accompanying materials are made available under the
// terms of the Eclipse Public License v. 2.0 which is available at
// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
// which is available at https://www.apache.org/licenses/LICENSE-2.0.
//
// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
// ========================================================================
//

package org.eclipse.jetty.start.fileinits;

import java.io.IOException;
import java.io.InputStream;
import java.net.ProxySelector;
import java.net.URI;
import java.net.http.HttpClient;
import java.net.http.HttpRequest;
import java.net.http.HttpResponse;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
import java.util.Optional;

import org.eclipse.jetty.start.BaseHome;
import org.eclipse.jetty.start.FS;
import org.eclipse.jetty.start.FileInitializer;
import org.eclipse.jetty.start.StartArgs;
import org.eclipse.jetty.start.StartLog;

public abstract class DownloadFileInitializer extends FileInitializer
{
private HttpClient httpClient;

protected DownloadFileInitializer(BaseHome basehome, String... scheme)
{
super(basehome, scheme);
}

protected abstract boolean allowInsecureHttpDownloads();

protected void download(URI uri, Path destination) throws IOException
{
if ("http".equalsIgnoreCase(uri.getScheme()) && !allowInsecureHttpDownloads())
throw new IOException("Insecure HTTP download not allowed (use " + StartArgs.ARG_ALLOW_INSECURE_HTTP_DOWNLOADS + " to bypass): " + uri);

if (Files.exists(destination))
{
if (Files.isRegularFile(destination))
{
StartLog.warn("skipping download of %s : file exists in destination %s", uri, destination);
}
else
{
StartLog.warn("skipping download of %s : path conflict at destination %s", uri, destination);
}
return;
}

if (FS.ensureDirectoryExists(destination.getParent()))
StartLog.info("mkdir " + _basehome.toShortForm(destination.getParent()));

StartLog.info("download %s to %s", uri, _basehome.toShortForm(destination));

HttpClient httpClient = getHttpClient();

try
{
HttpRequest request = HttpRequest.newBuilder()
.uri(uri)
.GET()
.build();

HttpResponse<InputStream> response =
httpClient.send(request, HttpResponse.BodyHandlers.ofInputStream());

int status = response.statusCode();

if (!allowInsecureHttpDownloads() && (status >= 300) && (status <= 399))
{
// redirection status, provide more details in error
Optional<String> location = response.headers().firstValue("Location");
if (location.isPresent())
{
throw new IOException("URL GET Failure [status " + status + "] on " + uri +
" wanting to redirect to insecure HTTP location (use " +
StartArgs.ARG_ALLOW_INSECURE_HTTP_DOWNLOADS + " to bypass): " +
location.get());
}
}

if (status != 200)
{
throw new IOException("URL GET Failure [status " + status + "] on " + uri);
}

try (InputStream in = response.body())
{
Files.copy(in, destination, StandardCopyOption.REPLACE_EXISTING);
}
}
catch (InterruptedException e)
{
throw new IOException("Failed to GET: " + uri, e);
}
}

private HttpClient getHttpClient()
{
if (httpClient == null)
{
httpClient = HttpClient.newBuilder()
.followRedirects(allowInsecureHttpDownloads() ? HttpClient.Redirect.ALWAYS : HttpClient.Redirect.NORMAL)
.proxy(ProxySelector.getDefault())
.build();
}
return httpClient;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@

import org.eclipse.jetty.start.BaseHome;
import org.eclipse.jetty.start.FS;
import org.eclipse.jetty.start.FileInitializer;
import org.eclipse.jetty.start.StartLog;
import org.eclipse.jetty.start.Utils;
import org.xml.sax.SAXException;
Expand All @@ -42,7 +41,7 @@
* <dd>optional type and classifier requirement</dd>
* </dl>
*/
public class MavenLocalRepoFileInitializer extends FileInitializer
public class MavenLocalRepoFileInitializer extends DownloadFileInitializer
{
public static class Coordinates
{
Expand Down Expand Up @@ -126,6 +125,19 @@ public MavenLocalRepoFileInitializer(BaseHome baseHome, Path localRepoDir, boole
this.mavenRepoUri = mavenRepoUri;
}

@Override
protected boolean allowInsecureHttpDownloads()
{
// Always allow insecure http downloads in this file initializer.

// The user is either using the DEFAULT_REMOTE_REPO, or has redeclared it to a new URI.
// If the `maven.repo.uri` property has been changed from default, this indicates a change
// to a different maven uri, overwhelmingly pointing to a maven repository manager
// like artifactory or nexus. This is viewed as an intentional decision by the
// user and as such we should not put additional hurdles in their way.
return true;
}

private static Path newTempRepo()
{
Path javaTempDir = Paths.get(System.getProperty("java.io.tmpdir"));
Expand Down

0 comments on commit 81efae2

Please sign in to comment.