From 52b6bf2486b08aa8bbe621961baed42bd27c1ddd Mon Sep 17 00:00:00 2001
From: John Chilton <jmchilton@gmail.com>
Date: Thu, 1 Mar 2018 11:00:14 -0500
Subject: [PATCH] Stronger security around test data path resolution, now with
 tests.

---
 lib/galaxy/tools/__init__.py           | 11 +++++++++-
 lib/galaxy/tools/verify/test_data.py   | 28 +++++++++-----------------
 lib/galaxy/webapps/galaxy/api/tools.py |  6 +++++-
 test/api/test_tools.py                 | 10 +++++++++
 4 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/lib/galaxy/tools/__init__.py b/lib/galaxy/tools/__init__.py
index a45745b6f49..08a1eb780db 100755
--- a/lib/galaxy/tools/__init__.py
+++ b/lib/galaxy/tools/__init__.py
@@ -67,6 +67,7 @@ from galaxy.tools.test import parse_tests
 from galaxy.tools.toolbox import BaseGalaxyToolBox
 from galaxy.util import (
     ExecutionTimer,
+    in_directory,
     listify,
     Params,
     rst_to_html,
@@ -856,7 +857,15 @@ class Tool(Dictifiable):
                 if '.' in dirs:
                     dirs.remove('.hg')
                 if 'test-data' in dirs:
-                    return os.path.abspath(os.path.join(root, 'test-data', filename))
+                    test_data_dir = os.path.join(root, 'test-data')
+                    result = os.path.abspath(os.path.join(test_data_dir, filename))
+                    if not in_directory(result, test_data_dir):
+                        # Don't raise an explicit exception and reveal details about what
+                        # files are or are not on the path, simply return None and let the
+                        # API raise a 404.
+                        return None
+                    else:
+                        return result
         else:
             return self.app.test_data_resolver.get_filename(filename)
 
diff --git a/lib/galaxy/tools/verify/test_data.py b/lib/galaxy/tools/verify/test_data.py
index 13d0ba4b724..ea63d864aa8 100644
--- a/lib/galaxy/tools/verify/test_data.py
+++ b/lib/galaxy/tools/verify/test_data.py
@@ -8,6 +8,7 @@ from string import Template
 
 from galaxy.util import (
     asbool,
+    in_directory,
     smart_str
 )
 
@@ -38,24 +39,14 @@ class TestDataResolver(object):
             self.resolvers = []
 
     def get_filename(self, name):
-        if not self.resolvers:
-            filename = None
-        else:
-            resolver = self.resolvers[0]
+        filename = None
+        for resolver in self.resolvers or []:
+            if not resolver.exists(name):
+                continue
             filename = resolver.path(name)
-            if not resolver.exists(filename):
-                for resolver in self.resolvers[1:]:
-                    if resolver.exists(name):
-                        filename = resolver.path(name)
-            else:
-                # For backward compat. returning first path if none
-                # exist - though I don't know if this function is ever
-                # actually used in a context where one should return
-                # a file even if it doesn't exist (e.g. a prefix or
-                # or something) - I am pretty sure it is not used in
-                # such a fashion in the context of tool tests.
-                filename = resolver.path(name)
-        return os.path.abspath(filename)
+
+        if filename:
+            return os.path.abspath(filename)
 
 
 def build_resolver(uri, environ):
@@ -71,7 +62,8 @@ class FileDataResolver(object):
         self.file_dir = file_dir
 
     def exists(self, filename):
-        return os.path.exists(self.path(filename))
+        path = os.path.abspath(self.path(filename))
+        return os.path.exists(path) and in_directory(path, self.file_dir)
 
     def path(self, filename):
         return os.path.join(self.file_dir, filename)
diff --git a/lib/galaxy/webapps/galaxy/api/tools.py b/lib/galaxy/webapps/galaxy/api/tools.py
index 6a52126cd58..fa0a0804b7f 100644
--- a/lib/galaxy/webapps/galaxy/api/tools.py
+++ b/lib/galaxy/webapps/galaxy/api/tools.py
@@ -112,7 +112,11 @@ class ToolsController(BaseAPIController, UsesVisualizationMixin):
             kwd = kwd.get('payload')
         tool_version = kwd.get('tool_version', None)
         tool = self._get_tool(id, tool_version=tool_version, user=trans.user)
-        return tool.test_data_path(kwd.get("filename"))
+        path = tool.test_data_path(kwd.get("filename"))
+        if path:
+            return path
+        else:
+            raise exceptions.ObjectNotFound("Specified test data path not found.")
 
     @expose_api_anonymous_and_sessionless
     def tests_summary(self, trans, **kwd):
diff --git a/test/api/test_tools.py b/test/api/test_tools.py
index fea4475b628..6f8ff1a3ed8 100644
--- a/test/api/test_tools.py
+++ b/test/api/test_tools.py
@@ -135,6 +135,16 @@ class ToolsTestCase(api.ApiTestCase):
         self._assert_has_keys(tool_info, "inputs", "outputs", "panel_section_id")
         return tool_info
 
+    @skip_without_tool("composite_output")
+    def test_test_data_filepath_security(self):
+        test_data_response = self._get("tools/%s/test_data_path?filename=../CONTRIBUTORS.md" % "composite_output", admin=True)
+        assert test_data_response.status_code == 404, test_data_response.json()
+
+    @skip_without_tool("composite_output")
+    def test_test_data_admin_security(self):
+        test_data_response = self._get("tools/%s/test_data_path?filename=../CONTRIBUTORS.md" % "composite_output")
+        assert test_data_response.status_code == 403, test_data_response.json()
+
     @skip_without_tool("composite_output")
     def test_test_data_composite_output(self):
         test_data_response = self._get("tools/%s/test_data" % "composite_output")