Resolve #469 : Add -C,--canonicalRoot to whitelist symlink target roots

Author: idodeclare

opengrok-indexer/src/main/java/org/opengrok/indexer/configuration/Configuration.java

@@ -19,7 +19,7 @@
 
 /*
  * Copyright (c) 2007, 2018, Oracle and/or its affiliates. All rights reserved.
- * Portions Copyright (c) 2017-2018, Chris Fraire <cfraire@me.com>.
+ * Portions Copyright (c) 2017-2019, Chris Fraire <cfraire@me.com>.
  */
 package org.opengrok.indexer.configuration;
 
@@ -201,6 +201,7 @@ public final class Configuration {
     private String CTagsExtraOptionsFile;
     private int scanningDepth;
     private Set<String> allowedSymlinks;
+    private Set<String> canonicalRoots;
     private boolean obfuscatingEMailAddresses;
     private boolean chattyStatusPage;
     private final Map<String, String> cmds;  // repository type -> command
@@ -441,6 +442,7 @@ public Configuration() {
         //setBugPage("http://bugs.myserver.org/bugdatabase/view_bug.do?bug_id=");
         setBugPattern("\\b([12456789][0-9]{6})\\b");
         setCachePages(5);
+        setCanonicalRoots(new HashSet<>());
         setCommandTimeout(600); // 10 minutes
         setInteractiveCommandTimeout(30);
         setCompressXref(true);
@@ -1173,6 +1175,14 @@ public void setAllowedSymlinks(Set<String> allowedSymlinks) {
         this.allowedSymlinks = allowedSymlinks;
     }
 
+    public Set<String> getCanonicalRoots() {
+        return canonicalRoots;
+    }
+
+    public void setCanonicalRoots(Set<String> canonicalRoots) {
+        this.canonicalRoots = canonicalRoots;
+    }
+
     public boolean isObfuscatingEMailAddresses() {
         return obfuscatingEMailAddresses;
     }

opengrok-indexer/src/main/java/org/opengrok/indexer/configuration/RuntimeEnvironment.java

@@ -431,7 +431,7 @@ public String getPathRelativeToSourceRoot(File file)
         }
 
         String maybeRelPath = PathUtils.getRelativeToCanonical(file.getPath(),
-                sourceRoot, getAllowedSymlinks());
+                sourceRoot, getAllowedSymlinks(), getCanonicalRoots());
         File maybeRelFile = new File(maybeRelPath);
         if (!maybeRelFile.isAbsolute()) {
             /*
@@ -1209,6 +1209,15 @@ public void setAllowedSymlinks(Set<String> allowedSymlinks) {
         setConfigurationValue("allowedSymlinks", allowedSymlinks);
     }
 
+    @SuppressWarnings("unchecked")
+    public Set<String> getCanonicalRoots() {
+        return (Set<String>) getConfigurationValue("canonicalRoots");
+    }
+
+    public void setCanonicalRoots(Set<String> canonicalRoots) {
+        setConfigurationValue("canonicalRoots", canonicalRoots);
+    }
+
     /**
      * Return whether e-mail addresses should be obfuscated in the xref.
      * @return if we obfuscate emails

opengrok-indexer/src/main/java/org/opengrok/indexer/index/IndexDatabase.java

@@ -1000,7 +1000,21 @@ private boolean acceptSymlink(Path absolute, File canonical, AcceptSymlinkRet re
             }
         }
 
-        Set<String> allowedSymlinks = RuntimeEnvironment.getInstance().getAllowedSymlinks();
+        RuntimeEnvironment env = RuntimeEnvironment.getInstance();
+
+        Set<String> canonicalRoots = env.getCanonicalRoots();
+        for (String canonicalRoot : canonicalRoots) {
+            if (canonical1.startsWith(canonicalRoot)) {
+                if (LOGGER.isLoggable(Level.FINE)) {
+                    LOGGER.log(Level.FINE, "Allowed symlink {0} per canonical root {1}",
+                            new Object[] {absolute1, canonical1});
+                }
+                acceptedNonlocalSymlinks.put(canonical1, absolute1);
+                return true;
+            }
+        }
+
+        Set<String> allowedSymlinks = env.getAllowedSymlinks();
         for (String allowedSymlink : allowedSymlinks) {
             String allowedTarget;
             try {

opengrok-indexer/src/main/java/org/opengrok/indexer/index/Indexer.java

@@ -116,6 +116,7 @@ public final class Indexer {
 
     private static final Set<String> repositories = new HashSet<>();
     private static final HashSet<String> allowedSymlinks = new HashSet<>();
+    private static final HashSet<String> canonicalRoots = new HashSet<>();
     private static final Set<String> defaultProjects = new TreeSet<>();
     private static RuntimeEnvironment env = null;
     private static String webappURI = null;
@@ -205,6 +206,9 @@ public static void main(String[] argv) {
             allowedSymlinks.addAll(cfg.getAllowedSymlinks());
             cfg.setAllowedSymlinks(allowedSymlinks);
 
+            canonicalRoots.addAll(cfg.getCanonicalRoots());
+            cfg.setCanonicalRoots(canonicalRoots);
+
             // Assemble the unprocessed command line arguments (possibly
             // a list of paths). This will be used to perform more fine
             // grained checking in invalidateRepositories().
@@ -473,6 +477,22 @@ public static String[] parseOptions(String[] argv) throws ParseException {
                 }
             );
 
+            parser.on("-C", "--canonicalRoot", "=/path/",
+                    "Allow symlinks to canonical targets starting with the specified",
+                    "root without otherwise needing to specify -N,--symlink for such",
+                    "symlinks. A canonical root must end with a file separator. For",
+                    "security a canonical root cannot be the root directory.",
+                    "Option may be repeated.").Do(v -> {
+                            String root = (String) v;
+                            if (!root.endsWith("/") && !root.endsWith("\\")) {
+                                die("--canonicalRoot must end with a separator");
+                            }
+                            if (root.equals("/") || root.equals("\\")) {
+                                die("--canonicalRoot cannot be the root directory");
+                            }
+                            canonicalRoots.add(root);
+                    });
+
             parser.on("-c", "--ctags", "=/path/to/ctags",
                 "Path to Universal Ctags",
                 "By default takes the Universal Ctags in PATH.").
@@ -580,11 +600,13 @@ public static String[] parseOptions(String[] argv) throws ParseException {
             parser.on("-N", "--symlink", "=/path/to/symlink",
                     "Allow this symlink to be followed. Option may be repeated.",
                     "By default only symlinks directly under source root directory",
-                    "are allowed.").Do(symlink -> allowedSymlinks.add((String) symlink));
+                    "are allowed. See also -C,--canonicalRoot").Do(v ->
+                    allowedSymlinks.add((String) v));
 
             parser.on("-n", "--noIndex",
-                "Do not generate indexes and other data (such as history cache and xref files), " +
-                "but process all other command line options.").Do(v -> runIndex = false);
+                "Do not generate indexes and other data (such as history cache",
+                "and xref files), but process all other command line options.").Do(v ->
+                runIndex = false);
 
             parser.on("-O", "--optimize", "=on|off", ON_OFF, Boolean.class,
                 "Turn on/off the optimization of the index database",
@@ -621,8 +643,8 @@ public static String[] parseOptions(String[] argv) throws ParseException {
             parser.on("-p", "--defaultProject", "=/path/to/default/project",
                 "This is the path to the project that should be selected",
                 "by default in the web application (when no other project",
-                "set either in cookie or in parameter). May be used multiple",
-                "times for several projects. Use \"__all__\" for all projects.",
+                "set either in cookie or in parameter). Option may be repeated",
+                "to specify several projects. Use \"__all__\" for all projects.",
                 "You should strip off the source root.").Do(v -> {
                 defaultProjects.add((String) v);
             });
@@ -651,10 +673,9 @@ public static String[] parseOptions(String[] argv) throws ParseException {
             });
 
             parser.on("--repository", "=repository",
-                    "Generate history for specific repository specified as relative path to source root. ",
-                    "Can be used multiple times. Assumes history is on.").Do(repo -> {
-                repositories.add((String) repo);
-            });
+                    "Generate history for specific repository specified as relative",
+                    "path to source root. Assumes -H,--history is on. Option may be",
+                    "repeated.").Do(v -> repositories.add((String) v));
 
             parser.on("-R /path/to/configuration",
                 "Read configuration from the specified file.").Do(v -> {

opengrok-indexer/src/main/java/org/opengrok/indexer/util/PathUtils.java

@@ -44,23 +44,20 @@ public class PathUtils {
         LoggerFactory.getLogger(PathUtils.class);
 
     /**
-     * Calls
-     * {@link #getRelativeToCanonical(java.lang.String, java.lang.String, java.util.Set)}
-     * with {@code path}, {@code canonical}, and {@code allowedSymlinks=null}
-     * (to disable validation of links).
+     * Calls {@link #getRelativeToCanonical(String, String, Set, Set)}
+     * with {@code path}, {@code canonical}, {@code allowedSymlinks=null}, and
+     * {@code canonicalRoots=null} (to disable validation of links).
      * @param path a non-canonical (or canonical) path to compare
      * @param canonical a canonical path to compare against
-     * @return a relative path determined as described for
-     * {@link #getRelativeToCanonical(java.lang.String, java.lang.String, java.util.Set)}
-     * when {@code allowedSymlinks==null} -- or {@code path} if no canonical
-     * relativity is found.
+     * @return a relative path determined as described -- or {@code path} if no
+     * canonical relativity is found.
      * @throws IOException if an error occurs determining canonical paths
      * for portions of {@code path}
      */
     public static String getRelativeToCanonical(String path, String canonical)
         throws IOException {
         try {
-            return getRelativeToCanonical(path, canonical, null);
+            return getRelativeToCanonical(path, canonical, null, null);
         } catch (ForbiddenSymlinkException e) {
             // should not get here with allowedSymlinks==null
             return path;
@@ -88,8 +85,12 @@ public static String getRelativeToCanonical(String path, String canonical)
      * @param path a non-canonical (or canonical) path to compare
      * @param canonical a canonical path to compare against
      * @param allowedSymlinks optional set of allowed symbolic links, so that
-     * any links encountered within {@code path} and not covered by the set
-     * will abort the algorithm
+     * any links encountered within {@code path} and not covered by the set (or
+     * whitelisted in a defined {@code canonicalRoots}) will abort the algorithm
+     * @param canonicalRoots optional set of allowed canonicalRoots, so that
+     * any checks done because of a defined {@code allowedSymlinks} will first
+     * check against the whitelist of canonical roots and possibly short-circuit
+     * the explicit validation against {@code allowedSymlinks}.
      * @return a relative path determined as described above -- or {@code path}
      * if no canonical relativity is found
      * @throws IOException if an error occurs determining canonical paths
@@ -99,7 +100,7 @@ public static String getRelativeToCanonical(String path, String canonical)
      * @throws InvalidPathException if path cannot be decoded
      */
     public static String getRelativeToCanonical(String path, String canonical,
-        Set<String> allowedSymlinks)
+            Set<String> allowedSymlinks, Set<String> canonicalRoots)
             throws IOException, ForbiddenSymlinkException, InvalidPathException {
 
         if (path.equals(canonical)) {
@@ -127,7 +128,8 @@ public static String getRelativeToCanonical(String path, String canonical,
             if (allowedSymlinks != null) {
                 String iterOriginal = iterPath.getPath();
                 if (Files.isSymbolicLink(Paths.get(iterOriginal)) &&
-                    !isAllowedSymlink(iterCanon, allowedSymlinks)) {
+                        !isWhitelisted(iterCanon, canonicalRoots) &&
+                        !isAllowedSymlink(iterCanon, allowedSymlinks)) {
                     String format = String.format("%1$s is prohibited symlink",
                         iterOriginal);
                     LOGGER.finest(format);
@@ -181,6 +183,17 @@ private static boolean isAllowedSymlink(String canonicalFile,
         return false;
     }
 
+    private static boolean isWhitelisted(String canonical, Set<String> canonicalRoots) {
+        if (canonicalRoots != null) {
+            for (String canonicalRoot : canonicalRoots) {
+                if (canonical.startsWith(canonicalRoot)) {
+                    return true;
+                }
+            }
+        }
+        return false;
+    }
+
     /** Private to enforce static. */
     private PathUtils() {
     }

opengrok-indexer/src/test/java/org/opengrok/indexer/util/PathUtilsTest.java

@@ -18,11 +18,14 @@
  */
 
 /*
- * Copyright (c) 2017, Chris Fraire <cfraire@me.com>.
+ * Copyright (c) 2017, 2019, Chris Fraire <cfraire@me.com>.
  * Copyright (c) 2008, 2018, Oracle and/or its affiliates. All rights reserved.
  */
 package org.opengrok.indexer.util;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
 import java.io.File;
 import java.io.IOException;
 import java.net.URI;
@@ -35,8 +38,6 @@
 import java.util.Set;
 import org.junit.After;
 import org.junit.Assert;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
 import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
@@ -131,7 +132,7 @@ public void shouldHandleLinksOfArbitraryDepthWithValidation()
         ForbiddenSymlinkException expex = null;
         try {
             PathUtils.getRelativeToCanonical(sympath.toString(), realDir1Canon,
-                allowedSymLinks);
+                    allowedSymLinks, null);
         } catch (ForbiddenSymlinkException e) {
             expex = e;
         }
@@ -141,7 +142,7 @@ public void shouldHandleLinksOfArbitraryDepthWithValidation()
         // Test v. realDir1 canonical with validation and an allowed link
         allowedSymLinks.add(symlink2.getPath());
         rel = PathUtils.getRelativeToCanonical(sympath.toString(),
-            realDir1Canon, allowedSymLinks);
+                realDir1Canon, allowedSymLinks, null);
         assertEquals("because link is OKed", "b/" + SYMLINK2, rel);
     }