From a340487a7d406e73e51479a765a3d08bdb92b8d0 Mon Sep 17 00:00:00 2001
From: Peter Amstutz <peter.amstutz@curoverse.com>
Date: Tue, 16 May 2017 09:12:58 -0400
Subject: [PATCH] 11693: Detect and error on symlinks pointing to locations
 outside the output directory.

---
 services/crunch-run/crunchrun.go      | 69 +++++++++++++++++----------
 services/crunch-run/crunchrun_test.go |  2 +-
 2 files changed, 44 insertions(+), 27 deletions(-)

diff --git a/services/crunch-run/crunchrun.go b/services/crunch-run/crunchrun.go
index 5a89f6ad85..4676a4f495 100644
--- a/services/crunch-run/crunchrun.go
+++ b/services/crunch-run/crunchrun.go
@@ -942,40 +942,57 @@ func (runner *ContainerRunner) CaptureOutput() error {
 				return nil
 			}
 			// read link to get container internal path
+			// only support 1 level of symlinking here.
 			var tgt string
 			tgt, err = os.Readlink(path)
 			if err != nil {
 				return err
 			}
-			if !strings.HasPrefix(tgt, "/") {
-				// Link is relative, don't handle it
-				return nil
-			}
-			// go through mounts and reverse map to collection reference
-			for _, bind := range binds {
-				mnt := runner.Container.Mounts[bind]
-				if tgt == bind || strings.HasPrefix(tgt, bind+"/") && mnt.Kind == "collection" {
-					// get path relative to bind
-					sourceSuffix := tgt[len(bind):]
-					// get path relative to output dir
-					bindSuffix := path[len(runner.HostOutputDir):]
-
-					// Copy mount and adjust the path to add path relative to the bind
-					adjustedMount := mnt
-					adjustedMount.Path = filepath.Join(adjustedMount.Path, sourceSuffix)
-
-					// get manifest text
-					var m string
-					m, err = runner.getCollectionManifestForPath(adjustedMount, bindSuffix)
-					if err != nil {
-						return err
+
+			outputSuffix := path[len(runner.HostOutputDir):]
+
+			if strings.HasPrefix(tgt, "/") {
+				// go through mounts and try reverse map to collection reference
+				for _, bind := range binds {
+					mnt := runner.Container.Mounts[bind]
+					if tgt == bind || strings.HasPrefix(tgt, bind+"/") && mnt.Kind == "collection" {
+						// get path relative to bind
+						targetSuffix := tgt[len(bind):]
+						// get path relative to output dir
+						outputSuffix := path[len(runner.HostOutputDir):]
+
+						// Copy mount and adjust the path to add path relative to the bind
+						adjustedMount := mnt
+						adjustedMount.Path = filepath.Join(adjustedMount.Path, targetSuffix)
+
+						// get manifest text
+						var m string
+						m, err = runner.getCollectionManifestForPath(adjustedMount, outputSuffix)
+						if err != nil {
+							return err
+						}
+						manifestText = manifestText + m
+						// delete symlink so WriteTree won't try to to dereference it.
+						os.Remove(path)
+						return nil
 					}
-					manifestText = manifestText + m
-					// delete symlink so WriteTree won't try to to dereference it.
-					os.Remove(path)
-					return nil
 				}
 			}
+
+			// Not a link to a mount.  Must be dereferencible and
+			// point into the output directory.
+			tgt, err = filepath.EvalSymlinks(path)
+			if err != nil {
+				os.Remove(path)
+				return err
+			}
+
+			// Symlink target must be within the output directory otherwise it's an error.
+			if !strings.HasPrefix(tgt, runner.HostOutputDir+"/") {
+				os.Remove(path)
+				return fmt.Errorf("Output directory symlink %q points to invalid location %q, must point to mount or output directory.",
+					outputSuffix, tgt)
+			}
 			return nil
 		})
 		if err != nil {
diff --git a/services/crunch-run/crunchrun_test.go b/services/crunch-run/crunchrun_test.go
index 64eed2c1d3..37fe32a4dd 100644
--- a/services/crunch-run/crunchrun_test.go
+++ b/services/crunch-run/crunchrun_test.go
@@ -1491,7 +1491,7 @@ func (s *TestSuite) TestOutputError(c *C) {
 	extraMounts := []string{}
 
 	api, _, _ := FullRunHelper(c, helperRecord, extraMounts, 0, func(t *TestDockerClient) {
-		os.Symlink("/does/not/exist", t.realTemp+"/2/baz")
+		os.Symlink("/etc/hosts", t.realTemp+"/2/baz")
 		t.logWriter.Close()
 	})
 
-- 
2.30.2