From 9791dd22d4d9084989cafa3498ae313dd4cd13f0 Mon Sep 17 00:00:00 2001 From: Nicola Soranzo Date: Tue, 24 Oct 2017 19:34:40 +0100 Subject: [PATCH 1/6] Fix regressions in library API Introduced in commit 93a8bfc7cb5e9c3395c5057910ec39d68ad787b4 --- lib/galaxy/util/path/ntpath.py | 2 +- lib/galaxy/webapps/galaxy/controllers/library_common.py | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/galaxy/util/path/ntpath.py b/lib/galaxy/util/path/ntpath.py index 5363409b3af6..3c86944963a3 100644 --- a/lib/galaxy/util/path/ntpath.py +++ b/lib/galaxy/util/path/ntpath.py @@ -2,7 +2,7 @@ """ from __future__ import absolute_import -import ntpath +import ntpath # noqa: I100 See https://github.com/PyCQA/flake8-import-order/pull/115 import sys from . import _build_self diff --git a/lib/galaxy/webapps/galaxy/controllers/library_common.py b/lib/galaxy/webapps/galaxy/controllers/library_common.py index da9e67b693c6..a622f059e365 100644 --- a/lib/galaxy/webapps/galaxy/controllers/library_common.py +++ b/lib/galaxy/webapps/galaxy/controllers/library_common.py @@ -277,7 +277,6 @@ def library_permissions( self, trans, cntrller, **kwd ): status=escape( status ) ) @web.expose - @web.require_admin def create_folder( self, trans, cntrller, parent_id, library_id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -802,7 +801,6 @@ def ldda_permissions( self, trans, cntrller, library_id, folder_id, id, **kwd ): status=escape( status ) ) @web.expose - @web.require_admin def upload_library_dataset( self, trans, cntrller, library_id, folder_id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) From 899b0d806808d29291bf132a8c9e45df31fc28e1 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Tue, 24 Oct 2017 22:48:02 -0400 Subject: [PATCH 2/6] Fix galaxy.util.in_directory() - broken because it takes realpath of just one argument. Easy to see on Mac OS X since /tmp isn't really /tmp for instance: ``` % touch /tmp/moo % python >>> import galaxy.util >>> galaxy.util.in_directory("/tmp/moo", "/tmp/moo") False ``` ... and this is why the Travis tests are failing under Mac OS X. Broken with 93a8bfc7cb5e9c3395c5057910ec39d68ad787b4. --- lib/galaxy/util/path/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/util/path/__init__.py b/lib/galaxy/util/path/__init__.py index 48a8355ad79e..57dcfd2baf04 100644 --- a/lib/galaxy/util/path/__init__.py +++ b/lib/galaxy/util/path/__init__.py @@ -119,7 +119,7 @@ def __walk(path): def __contains(prefix, path, whitelist=None): real = realpath(join(prefix, path)) - yield not relpath(real, prefix).startswith(pardir) + yield not relpath(real, realpath(prefix)).startswith(pardir) for wldir in whitelist or []: yield not relpath(real, wldir).startswith(pardir) From 4b5dc399b934542092bfe66c3c4e03acc3980d49 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Wed, 25 Oct 2017 10:31:07 -0400 Subject: [PATCH 3/6] Add basic test for in_directory. --- lib/galaxy/util/__init__.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/lib/galaxy/util/__init__.py b/lib/galaxy/util/__init__.py index e7914a6af35a..423f20b26f23 100644 --- a/lib/galaxy/util/__init__.py +++ b/lib/galaxy/util/__init__.py @@ -609,6 +609,20 @@ def in_directory( file, directory, local_path_module=os.path ): local_path_module is used by Pulsar to check Windows paths while running on a POSIX-like system. + + >>> base_dir = tempfile.mkdtemp() + >>> safe_dir = os.path.join(base_dir, "user") + >>> os.mkdir(safe_dir) + >>> good_file = os.path.join(safe_dir, "1") + >>> with open(good_file, "w") as f: f.write("hello") + >>> in_directory(good_file, safe_dir) + True + >>> in_directory("/other/file/is/here.txt", safe_dir) + False + >>> unsafe_link = os.path.join(safe_dir, "2") + >>> os.symlink("/other/file/bad.fasta", unsafe_link) + >>> in_directory(unsafe_link, safe_dir) + False """ if local_path_module != os.path: _safe_contains = importlib.import_module('galaxy.util.path.%s' % local_path_module.__name__).safe_contains From fcb23eb8f878700bfd77384ef679a569c59f7a62 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Wed, 25 Oct 2017 10:34:35 -0400 Subject: [PATCH 4/6] Some documentation to clarify discussion on #4856. --- lib/galaxy/util/__init__.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/util/__init__.py b/lib/galaxy/util/__init__.py index 423f20b26f23..b4effe45ec6f 100644 --- a/lib/galaxy/util/__init__.py +++ b/lib/galaxy/util/__init__.py @@ -605,7 +605,11 @@ def which(file): def in_directory( file, directory, local_path_module=os.path ): """ Return true, if the common prefix of both is equal to directory - e.g. /a/b/c/d.rst and directory is /a/b, the common prefix is /a/b + e.g. /a/b/c/d.rst and directory is /a/b, the common prefix is /a/b. + This function isn't used exclusively for security checks, but if it is + used for such checks it is assumed that ``directory`` is a "trusted" path - + supplied by Galaxy or by the admin and ``file`` is something generated by + a tool, configuration, external web server, or user supplied input. local_path_module is used by Pulsar to check Windows paths while running on a POSIX-like system. From 95f8f48416c5a4d2590120e965cf9714c752ed61 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Wed, 25 Oct 2017 10:43:54 -0400 Subject: [PATCH 5/6] Doc fix thanks to @nsoranzo. --- lib/galaxy/util/path/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/util/path/__init__.py b/lib/galaxy/util/path/__init__.py index 57dcfd2baf04..93bddb97ff1c 100644 --- a/lib/galaxy/util/path/__init__.py +++ b/lib/galaxy/util/path/__init__.py @@ -31,7 +31,7 @@ def safe_contains(prefix, path, whitelist=None): Given any two filesystem paths, ensure that ``path`` is contained in ``prefix``. If ``path`` exists (either as an absolute path or relative to ``prefix``), it is canonicalized with :func:`os.path.realpath` to ensure it is not a - symbolic link that points outside of ``path``. If it is a symbolic link and ``whitelist`` is set, the symbolic link + symbolic link that points outside of ``prefix``. If it is a symbolic link and ``whitelist`` is set, the symbolic link may also point inside a ``whitelist`` path. The ``path`` is checked against ``whitelist`` using either its absolute pathname (if passed in as absolute) or From 7e08ba6f19346668350d358ee4eb01ef722e74e7 Mon Sep 17 00:00:00 2001 From: John Chilton Date: Wed, 25 Oct 2017 12:08:03 -0400 Subject: [PATCH 6/6] Rework in_directory fix to move realpath expansion into galaxy.util. --- lib/galaxy/util/__init__.py | 1 + lib/galaxy/util/path/__init__.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/galaxy/util/__init__.py b/lib/galaxy/util/__init__.py index b4effe45ec6f..86fa55d78bd7 100644 --- a/lib/galaxy/util/__init__.py +++ b/lib/galaxy/util/__init__.py @@ -631,6 +631,7 @@ def in_directory( file, directory, local_path_module=os.path ): if local_path_module != os.path: _safe_contains = importlib.import_module('galaxy.util.path.%s' % local_path_module.__name__).safe_contains else: + directory = os.path.realpath(directory) _safe_contains = safe_contains return _safe_contains(directory, file) diff --git a/lib/galaxy/util/path/__init__.py b/lib/galaxy/util/path/__init__.py index 93bddb97ff1c..3cde6fb09340 100644 --- a/lib/galaxy/util/path/__init__.py +++ b/lib/galaxy/util/path/__init__.py @@ -119,7 +119,7 @@ def __walk(path): def __contains(prefix, path, whitelist=None): real = realpath(join(prefix, path)) - yield not relpath(real, realpath(prefix)).startswith(pardir) + yield not relpath(real, prefix).startswith(pardir) for wldir in whitelist or []: yield not relpath(real, wldir).startswith(pardir)