jetty-start cleanup (jetty#9555)

joakime · web-flow · commit 81efae2f9872 · 2023-04-10T08:53:58.000-05:00
* 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 &lt;joakim.erdfelt@gmail.com&gt;
diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/BaseBuilder.java b/jetty-start/src/main/java/org/eclipse/jetty/start/BaseBuilder.java
@@ -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)
@@ -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));
         }
     }
 
diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/FS.java b/jetty-start/src/main/java/org/eclipse/jetty/start/FS.java
@@ -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
 {
@@ -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);
                 }
             }
         }
diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/FileInitializer.java b/jetty-start/src/main/java/org/eclipse/jetty/start/FileInitializer.java
@@ -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);
 
@@ -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)
      *
@@ -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);
diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java b/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java
@@ -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";
@@ -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)
@@ -960,6 +962,11 @@ public boolean hasSystemProperties()
         return false;
     }
 
+    public boolean isAllowInsecureHttpDownloads()
+    {
+        return allowInsecureHttpDownloads;
+    }
+
     public boolean isApproveAllLicenses()
     {
         return approveAllLicenses;
@@ -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))
         {
diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/fileinits/DownloadFileInitializer.java b/jetty-start/src/main/java/org/eclipse/jetty/start/fileinits/DownloadFileInitializer.java
@@ -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;
+    }
+}
diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/fileinits/MavenLocalRepoFileInitializer.java b/jetty-start/src/main/java/org/eclipse/jetty/start/fileinits/MavenLocalRepoFileInitializer.java
@@ -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;
@@ -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
     {
@@ -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"));
diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/fileinits/MavenMetadata.java b/jetty-start/src/main/java/org/eclipse/jetty/start/fileinits/MavenMetadata.java
diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/fileinits/UriFileInitializer.java b/jetty-start/src/main/java/org/eclipse/jetty/start/fileinits/UriFileInitializer.java
diff --git a/jetty-start/src/main/resources/org/eclipse/jetty/start/usage.txt b/jetty-start/src/main/resources/org/eclipse/jetty/start/usage.txt

Original file line number	Diff line number	Diff line change
`@@ -86,7 +86,6 @@ else if (args.isCreateFiles())`
`86`	`86`	`// Handle local directories`
`87`	`87`	`fileInitializers.add(new LocalFileInitializer(baseHome));`
`88`	`88`
`89`		`- // Downloads are allowed to be performed`
`90`	`89`	`// Setup Maven Local Repo`
`91`	`90`	`Path localRepoDir = args.findMavenLocalRepoDir();`
`92`	`91`	`if (localRepoDir != null)`
`@@ -106,7 +105,7 @@ else if (args.isCreateFiles())`
`106`	`105`	`fileInitializers.add(new BaseHomeFileInitializer(baseHome));`
`107`	`106`
`108`	`107`	`// Normal URL downloads`
`109`		`- fileInitializers.add(new UriFileInitializer(baseHome));`
	`108`	`+ fileInitializers.add(new UriFileInitializer(startArgs, baseHome));`
`110`	`109`	`}`
`111`	`110`	`}`
`112`	`111`