General bug fixes for jetty-start

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));
         }
     }
 

jetty-start/src/main/java/org/eclipse/jetty/start/FS.java

@@ -16,15 +16,19 @@
 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.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 +188,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 = new HashMap<>();
+        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;
+
+        try (Stream<Path> entriesStream = Files.walk(root, 30))
+        {
+            // ensure proper unpack order (eg: directories before files)
+            List<Path> sorted = entriesStream
+                .filter((path) -> path.getNameCount() > 0)
+                .filter((path) -> !path.getName(0).toString().equalsIgnoreCase("META-INF"))
+                .sorted()
+                .collect(Collectors.toList());
+
+            for (Path path : sorted)
+            {
 
-                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))
+                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 extract (file exists) %s", outputPath);
                 }
                 else
                 {
-                    StartLog.debug("skipping extract (file exists) %s", destFile);
+                    StartLog.info("copy %s to %s", path, outputPath);
+                    Files.copy(path, outputPath);
                 }
             }
         }

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);

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))
         {

jetty-start/src/main/java/org/eclipse/jetty/start/fileinits/DownloadFileInitializer.java

@@ -0,0 +1,71 @@
+//
+// ========================================================================
+// 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.HttpURLConnection;
+import java.net.URI;
+import java.net.URLConnection;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.StandardCopyOption;
+
+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
+{
+    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 (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);
+        }
+    }
+}

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"));

jetty-start/src/main/java/org/eclipse/jetty/start/fileinits/MavenMetadata.java

@@ -26,6 +26,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import javax.xml.XMLConstants;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
@@ -56,6 +57,7 @@ public MavenMetadata(Path metadataXml)
         throws IOException, ParserConfigurationException, SAXException
     {
         DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
+        factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
         DocumentBuilder builder = factory.newDocumentBuilder();
         try (InputStream input = Files.newInputStream(metadataXml))
         {

jetty-start/src/main/java/org/eclipse/jetty/start/fileinits/UriFileInitializer.java

@@ -19,13 +19,22 @@
 import java.nio.file.Path;
 
 import org.eclipse.jetty.start.BaseHome;
-import org.eclipse.jetty.start.FileInitializer;
+import org.eclipse.jetty.start.StartArgs;
 
-public class UriFileInitializer extends FileInitializer
+public class UriFileInitializer extends DownloadFileInitializer
 {
-    public UriFileInitializer(BaseHome baseHome)
+    protected final boolean _allowInsecureHttpDownloads;
+
+    public UriFileInitializer(StartArgs startArgs, BaseHome baseHome)
     {
         super(baseHome, "http", "https");
+        _allowInsecureHttpDownloads = startArgs.isAllowInsecureHttpDownloads();
+    }
+
+    @Override
+    protected boolean allowInsecureHttpDownloads()
+    {
+        return _allowInsecureHttpDownloads;
     }
 
     @Override

jetty-start/src/main/resources/org/eclipse/jetty/start/usage.txt

@@ -191,6 +191,10 @@ Options:
                    issues when the Jetty logging module has not yet started
                    due to configuration errors.
 
+  --allow-insecure-http-downloads
+                   Allow the use of insecure `http://` plaintext URI for
+                   downloading of content.
+
   --approve-all-licenses
                    Approves all license questions from modules that have
                    particular license requirements.