From 93a8bfc7cb5e9c3395c5057910ec39d68ad787b4 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Tue, 12 Sep 2017 15:06:30 -0400 Subject: [PATCH 01/13] Security: Fix issues with path handling in libraries and in general. --- config/galaxy.ini.sample | 8 + lib/galaxy/config.py | 1 + lib/galaxy/managers/folders.py | 4 +- lib/galaxy/managers/libraries.py | 2 +- lib/galaxy/tools/parameters/grouping.py | 2 + lib/galaxy/util/__init__.py | 48 ++--- lib/galaxy/util/path/__init__.py | 185 ++++++++++++++++++ lib/galaxy/util/path/ntpath.py | 11 ++ lib/galaxy/util/path/posixpath.py | 11 ++ lib/galaxy/webapps/galaxy/api/lda_datasets.py | 33 +++- lib/galaxy/webapps/galaxy/api/remote_files.py | 4 +- .../webapps/galaxy/controllers/library.py | 2 + .../galaxy/controllers/library_common.py | 47 ++++- lib/galaxy/webapps/galaxy/controllers/user.py | 2 +- lib/tool_shed/managers/groups.py | 2 +- 15 files changed, 304 insertions(+), 58 deletions(-) create mode 100644 lib/galaxy/util/path/__init__.py create mode 100644 lib/galaxy/util/path/ntpath.py create mode 100644 lib/galaxy/util/path/posixpath.py diff --git a/config/galaxy.ini.sample b/config/galaxy.ini.sample index cc3f972e04e..40b06aa5e4e 100644 --- a/config/galaxy.ini.sample +++ b/config/galaxy.ini.sample @@ -801,6 +801,14 @@ use_interactive = True # sub-directories of files contained in their directory. #user_library_import_dir = None +# For security reasons, users may not import any files that actually lie +# outside of their `user_library_import_dir` (e.g. using symbolic links). A +# list of directories can be allowed by setting the following option (the list +# is comma-separated). Be aware that *any* user with library import permissions +# can import from anywhere in these directories (assuming they are able to +# create symlinks to them). +#user_library_import_symlink_whitelist = None + # Add an option to the admin library upload tool allowing admins to paste # filesystem paths to files and directories in a box, and these paths will be # added to a library. Set to True to enable. Please note the security diff --git a/lib/galaxy/config.py b/lib/galaxy/config.py index 5d67e3d78c8..a4c21887778 100644 --- a/lib/galaxy/config.py +++ b/lib/galaxy/config.py @@ -285,6 +285,7 @@ class Configuration( object ): self.screencasts_url = kwargs.get( 'screencasts_url', None ) self.library_import_dir = kwargs.get( 'library_import_dir', None ) self.user_library_import_dir = kwargs.get( 'user_library_import_dir', None ) + self.user_library_import_symlink_whitelist = listify( kwargs.get( 'user_library_import_symlink_whitelist', []), do_strip=True ) # Searching data libraries self.enable_lucene_library_search = string_as_bool( kwargs.get( 'enable_lucene_library_search', False ) ) self.enable_whoosh_library_search = string_as_bool( kwargs.get( 'enable_whoosh_library_search', False ) ) diff --git a/lib/galaxy/managers/folders.py b/lib/galaxy/managers/folders.py index cbb4ad3e9b7..b835ae76e50 100644 --- a/lib/galaxy/managers/folders.py +++ b/lib/galaxy/managers/folders.py @@ -132,7 +132,7 @@ class FolderManager( object ): """ parent_folder = self.get( trans, parent_folder_id ) current_user_roles = trans.get_current_user_roles() - if not ( trans.user_is_admin or trans.app.security_agent.can_add_library_item( current_user_roles, parent_folder ) ): + if not ( trans.user_is_admin() or trans.app.security_agent.can_add_library_item( current_user_roles, parent_folder ) ): raise InsufficientPermissionsException( 'You do not have proper permission to create folders under given folder.' ) new_folder = trans.app.model.LibraryFolder( name=new_folder_name, description=new_folder_description ) # We are associating the last used genome build with folders, so we will always @@ -230,7 +230,7 @@ class FolderManager( object ): """ Return true if the user has permissions to add item to the given folder. """ - if trans.user_is_admin: + if trans.user_is_admin(): return True current_user_roles = trans.get_current_user_roles() add_roles = set( trans.app.security_agent.get_roles_for_action( folder, trans.app.security_agent.permitted_actions.LIBRARY_ADD ) ) diff --git a/lib/galaxy/managers/libraries.py b/lib/galaxy/managers/libraries.py index 3fb32a2cb54..b4ca87538fe 100644 --- a/lib/galaxy/managers/libraries.py +++ b/lib/galaxy/managers/libraries.py @@ -50,7 +50,7 @@ class LibraryManager( object ): """ Create a new library. """ - if not trans.user_is_admin: + if not trans.user_is_admin(): raise exceptions.ItemAccessibilityException( 'Only administrators can create libraries.' ) else: library = trans.app.model.Library( name=name, description=description, synopsis=synopsis ) diff --git a/lib/galaxy/tools/parameters/grouping.py b/lib/galaxy/tools/parameters/grouping.py index 290534977f0..83e9d8ba692 100644 --- a/lib/galaxy/tools/parameters/grouping.py +++ b/lib/galaxy/tools/parameters/grouping.py @@ -356,6 +356,7 @@ class UploadDataset( Group ): warnings.append( "All FTP uploaded file selections were ignored." ) elif ftp_files is not None and trans.user is not None: # look for files uploaded via FTP user_ftp_dir = trans.user_ftp_dir + assert not os.path.islink(user_ftp_dir), "User FTP directory cannot be a symbolic link" for ( dirpath, dirnames, filenames ) in os.walk( user_ftp_dir ): for filename in filenames: for ftp_filename in ftp_files: @@ -419,6 +420,7 @@ class UploadDataset( Group ): # TODO: warning to the user (could happen if session has become invalid) else: user_ftp_dir = trans.user_ftp_dir + assert not os.path.islink(user_ftp_dir), "User FTP directory cannot be a symbolic link" for ( dirpath, dirnames, filenames ) in os.walk( user_ftp_dir ): for filename in filenames: path = relpath( os.path.join( dirpath, filename ), user_ftp_dir ) diff --git a/lib/galaxy/util/__init__.py b/lib/galaxy/util/__init__.py index 707def64e84..e7914a6af35 100644 --- a/lib/galaxy/util/__init__.py +++ b/lib/galaxy/util/__init__.py @@ -9,6 +9,7 @@ import binascii import collections import errno import grp +import importlib import json import logging import os @@ -24,7 +25,7 @@ import threading import time from datetime import datetime from hashlib import md5 -from os.path import normpath, relpath +from os.path import relpath from xml.etree import ElementInclude, ElementTree from xml.etree.ElementTree import ParseError @@ -41,6 +42,7 @@ except ImportError: docutils_html4css1 = None from .inflection import English, Inflector +from .path import safe_contains, safe_makedirs, safe_relpath # noqa: F401 inflector = Inflector(English) @@ -600,31 +602,19 @@ def which(file): return None -def safe_makedirs(path): - """ Safely make a directory, do not fail if it already exist or - is created during execution. - """ - if not os.path.exists(path): - try: - os.makedirs(path) - except OSError as e: - # review source for Python 2.7 this would only ever happen - # for the last path anyway so need to recurse - this exception - # means the last part of the path was already in existence. - if e.errno != errno.EEXIST: - raise - - 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 - """ - # Make both absolute. - directory = local_path_module.abspath(directory) - file = local_path_module.abspath(file) - return local_path_module.commonprefix([file, directory]) == directory + local_path_module is used by Pulsar to check Windows paths while running on + a POSIX-like system. + """ + if local_path_module != os.path: + _safe_contains = importlib.import_module('galaxy.util.path.%s' % local_path_module.__name__).safe_contains + else: + _safe_contains = safe_contains + return _safe_contains(directory, file) def merge_sorted_iterables( operator, *iterables ): @@ -1487,22 +1477,6 @@ def url_get( base_url, password_mgr=None, pathspec=None, params=None ): return content -def safe_relpath(path): - """ - Given what we expect to be a relative path, determine whether the path - would exist inside the current directory. - - :type path: string - :param path: a path to check - :rtype: bool - :returns: ``True`` if path is relative and does not reference a path - in a parent directory, ``False`` otherwise. - """ - if path.startswith(os.sep) or normpath(path).startswith(os.pardir): - return False - return True - - class ExecutionTimer(object): def __init__(self): diff --git a/lib/galaxy/util/path/__init__.py b/lib/galaxy/util/path/__init__.py new file mode 100644 index 00000000000..48a8355ad79 --- /dev/null +++ b/lib/galaxy/util/path/__init__.py @@ -0,0 +1,185 @@ +"""Path manipulation functions. +""" +from __future__ import absolute_import + +import errno +import imp +from functools import partial +from itertools import starmap +from operator import getitem +from os import ( + makedirs, + walk, +) +from os.path import ( + abspath, + exists, + isabs, + join, + normpath, + pardir, + realpath, + relpath, +) + +from six import string_types +from six.moves import filterfalse, map, zip + + +def safe_contains(prefix, path, whitelist=None): + """Ensure a path is contained within another path. + + 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 + may also point inside a ``whitelist`` path. + + The ``path`` is checked against ``whitelist`` using either its absolute pathname (if passed in as absolute) or + relative to ``prefix`` and canonicalized (if applicable). It is *not* ``os.path.join()``ed with each ``whitelist`` + directory. + + :type prefix: string + :param prefix: a directory under which ``path`` is to be checked + :type path: string + :param path: a filename to check + :type whitelist: list of strings + :param whitelist: list of additional paths under which ``path`` may be located + :rtype: bool + :returns: ``True`` if ``path`` is contained within ``prefix`` or ``whitelist``, ``False`` otherwise. + """ + return any(__contains(prefix, path, whitelist=whitelist)) + + +def safe_makedirs(path): + """Safely make a directory, do not fail if it already exists or is created during execution. + + :type path: string + :param path: a directory to create + """ + # prechecking for existence is faster than try/except + if not exists(path): + try: + makedirs(path) + except OSError as e: + # reviewing the source for Python 2.7, this would only ever happen for the last path element anyway so no + # need to recurse - this exception means the last part of the path was already in existence. + if e.errno != errno.EEXIST: + raise + + +def safe_relpath(path): + """Determine whether a relative path references a path outside its root. + + This is a path computation: the filesystem is not accessed to confirm the existence or nature of ``path``. + + :type path: string + :param path: a path to check + :rtype: bool + :returns: ``True`` if path is relative and does not reference a path in a parent directory, ``False`` + otherwise. + """ + return not (isabs(path) or normpath(path).startswith(pardir)) + + +def unsafe_walk(path, whitelist=None): + """Walk a path and ensure that none of its contents are symlinks outside the path. + + It is assumed that ``path`` itself has already been validated e.g. with :func:`safe_relpath` or + :func:`safe_contains`. + + :type path: string + :param path: a directory to check for unsafe contents + :type whitelist: list of strings + :param whitelist: list of additional paths under which contents may be located + :rtype: iterator + :returns: Iterator of "bad" files found under ``path`` + """ + return filterfalse(partial(safe_contains, path, whitelist=whitelist), __walk(abspath(path))) + + +def __listify(item): + """A non-splitting version of :func:`galaxy.util.listify`. + """ + if not item: + return [] + elif isinstance(item, list) or isinstance(item, tuple): + return item + else: + return [item] + + +# helpers + + +def __walk(path): + for dirpath, dirnames, filenames in walk(path): + for name in dirnames + filenames: + yield join(dirpath, name) + + +def __contains(prefix, path, whitelist=None): + real = realpath(join(prefix, path)) + yield not relpath(real, prefix).startswith(pardir) + for wldir in whitelist or []: + yield not relpath(real, wldir).startswith(pardir) + + +# cross-platform support + + +def _build_self(target, path_module): + """Populate a module with the same exported functions as this module, but using the given os.path module. + + :type target: module + :param target: module on which to set ``galaxy.util.path`` functions + :type path_module: ``ntpath`` or ``posixpath`` module + :param path_module: module implementing ``os.path`` API to use for path functions + """ + __copy_self().__set_fxns_on(target, path_module) + + +def __copy_self(names=__name__, parent=None): + """Returns a copy of this module that can be modified without modifying `galaxy.util.path`` in ``sys.modules``. + """ + if isinstance(names, string_types): + names = iter(names.split('.')) + try: + name = names.next() + except StopIteration: + return parent + path = parent and parent.__path__ + parent = imp.load_module(name, *imp.find_module(name, path)) + return __copy_self(names, parent) + + +def __set_fxns_on(target, path_module): + """Overrides imported os.path functions with the ones from path_module and populates target with the global + functions from this module. + """ + for name in __pathfxns__: + globals()[name] = getattr(path_module, name) + __get = partial(getitem, globals()) + __set = partial(setattr, target) + # this is actually izip(..., imap(...)) + __fxns = zip(__all__, map(__get, __all__)) + # list() to execute + list(starmap(__set, __fxns)) + + +__pathfxns__ = ( + 'abspath', + 'exists', + 'isabs', + 'join', + 'normpath', + 'pardir', + 'realpath', + 'relpath', +) + +__all__ = ( + 'safe_contains', + 'safe_makedirs', + 'safe_relpath', + 'unsafe_walk', +) diff --git a/lib/galaxy/util/path/ntpath.py b/lib/galaxy/util/path/ntpath.py new file mode 100644 index 00000000000..5363409b3af --- /dev/null +++ b/lib/galaxy/util/path/ntpath.py @@ -0,0 +1,11 @@ +"""Galaxy "safe" path functions forced to work with Windows-style paths regardless of current platform +""" +from __future__ import absolute_import + +import ntpath +import sys + +from . import _build_self + + +_build_self(sys.modules[__name__], ntpath) diff --git a/lib/galaxy/util/path/posixpath.py b/lib/galaxy/util/path/posixpath.py new file mode 100644 index 00000000000..28232649b57 --- /dev/null +++ b/lib/galaxy/util/path/posixpath.py @@ -0,0 +1,11 @@ +"""Galaxy "safe" path functions forced to work with POSIX-style paths regardless of current platform +""" +from __future__ import absolute_import + +import posixpath +import sys + +from . import _build_self + + +_build_self(sys.modules[__name__], posixpath) diff --git a/lib/galaxy/webapps/galaxy/api/lda_datasets.py b/lib/galaxy/webapps/galaxy/api/lda_datasets.py index 8e94edb29d2..05f9220bfed 100644 --- a/lib/galaxy/webapps/galaxy/api/lda_datasets.py +++ b/lib/galaxy/webapps/galaxy/api/lda_datasets.py @@ -19,6 +19,7 @@ from galaxy import web from galaxy.exceptions import ObjectNotFound from galaxy.managers import folders, roles from galaxy.tools.actions import upload_common +from galaxy.util.path import safe_contains, safe_relpath, unsafe_walk from galaxy.util.streamball import StreamBall from galaxy.web import _future_expose_api as expose_api from galaxy.web import _future_expose_api_anonymous as expose_api_anonymous @@ -422,29 +423,45 @@ class LibraryDatasetsController( BaseAPIController, UsesVisualizationMixin ): source = kwd.get( 'source', None ) if source not in [ 'userdir_file', 'userdir_folder', 'importdir_file', 'importdir_folder', 'admin_path' ]: raise exceptions.RequestParameterMissingException( 'You have to specify "source" parameter. Possible values are "userdir_file", "userdir_folder", "admin_path", "importdir_file" and "importdir_folder". ') - if source in [ 'importdir_file', 'importdir_folder' ]: - if not trans.user_is_admin: + elif source in [ 'importdir_file', 'importdir_folder' ]: + if not trans.user_is_admin(): raise exceptions.AdminRequiredException( 'Only admins can import from importdir.' ) if not trans.app.config.library_import_dir: raise exceptions.ConfigDoesNotAllowException( 'The configuration of this Galaxy instance does not allow admins to import into library from importdir.' ) import_base_dir = trans.app.config.library_import_dir + if not safe_relpath(path): + # admins shouldn't be able to explicitly specify a path outside server_dir, but symlinks are allowed. + # the reasoning here is that galaxy admins may not have direct filesystem access or can only access + # library_import_dir via FTP (which cannot create symlinks), and may rely on sysadmins to set up the + # import directory. if they have filesystem access, all bets are off. + raise exceptions.RequestParameterInvalidException( 'The given path is invalid.' ) path = os.path.join( import_base_dir, path ) - if source in [ 'userdir_file', 'userdir_folder' ]: + elif source in [ 'userdir_file', 'userdir_folder' ]: + unsafe = None user_login = trans.user.email user_base_dir = trans.app.config.user_library_import_dir if user_base_dir is None: raise exceptions.ConfigDoesNotAllowException( 'The configuration of this Galaxy instance does not allow upload from user directories.' ) full_dir = os.path.join( user_base_dir, user_login ) - if not path.lower().startswith( full_dir.lower() ): + if not safe_contains( full_dir, path, whitelist=trans.app.config.user_library_import_symlink_whitelist ): + # the path is a symlink outside the user dir path = os.path.join( full_dir, path ) + log.error( 'User attempted to import a path that resolves to a path outside of their import dir: %s -> %s', path, os.path.realpath(path) ) + raise exceptions.RequestParameterInvalidException( 'The given path is invalid.' ) + path = os.path.join( full_dir, path ) + for unsafe in unsafe_walk( path, whitelist=[full_dir] + trans.app.config.user_library_import_symlink_whitelist ): + # the path is a dir and contains files that symlink outside the user dir + log.error( 'User attempted to import a directory containing a path that resolves to a path outside of their import dir: %s -> %s', unsafe, os.path.realpath(unsafe) ) + if unsafe: + raise exceptions.RequestParameterInvalidException( 'The given path is invalid.' ) if not os.path.exists( path ): raise exceptions.RequestParameterInvalidException( 'Given path does not exist on the host.' ) if not self.folder_manager.can_add_item( trans, folder ): raise exceptions.InsufficientPermissionsException( 'You do not have proper permission to add items to the given folder.' ) - if source == 'admin_path': + elif source == 'admin_path': if not trans.app.config.allow_library_path_paste: raise exceptions.ConfigDoesNotAllowException( 'The configuration of this Galaxy instance does not allow admins to import into library from path.' ) - if not trans.user_is_admin: + if not trans.user_is_admin(): raise exceptions.AdminRequiredException( 'Only admins can import from path.' ) # Set up the traditional tool state/params @@ -463,12 +480,12 @@ class LibraryDatasetsController( BaseAPIController, UsesVisualizationMixin ): if source in [ 'importdir_folder' ]: kwd[ 'filesystem_paths' ] = os.path.join( import_base_dir, path ) # user wants to import one file only - if source in [ "userdir_file", "importdir_file" ]: + elif source in [ "userdir_file", "importdir_file" ]: file = os.path.abspath( path ) abspath_datasets.append( trans.webapp.controllers[ 'library_common' ].make_library_uploaded_dataset( trans, 'api', kwd, os.path.basename( file ), file, 'server_dir', library_bunch ) ) # user wants to import whole folder - if source == "userdir_folder": + elif source == "userdir_folder": uploaded_datasets_bunch = trans.webapp.controllers[ 'library_common' ].get_path_paste_uploaded_datasets( trans, 'api', kwd, library_bunch, 200, '' ) uploaded_datasets = uploaded_datasets_bunch[ 0 ] diff --git a/lib/galaxy/webapps/galaxy/api/remote_files.py b/lib/galaxy/webapps/galaxy/api/remote_files.py index 28d42cd45c0..904c186e275 100644 --- a/lib/galaxy/webapps/galaxy/api/remote_files.py +++ b/lib/galaxy/webapps/galaxy/api/remote_files.py @@ -107,7 +107,7 @@ class RemoteFilesAPIController( BaseAPIController ): subfolders and returns a flat list. """ response = [] - if os.path.exists( directory ): + if os.path.exists( directory ) and not os.path.islink( directory ): for ( dirpath, dirnames, filenames ) in os.walk( directory ): for filename in filenames: path = os.path.relpath( os.path.join( dirpath, filename ), directory ) @@ -130,7 +130,7 @@ class RemoteFilesAPIController( BaseAPIController ): """ userdir_jstree = None jstree_paths = [] - if os.path.exists( directory ): + if os.path.exists( directory ) and not os.path.islink( directory ): for ( dirpath, dirnames, filenames ) in os.walk( directory ): for dirname in dirnames: dir_path = os.path.relpath( os.path.join( dirpath, dirname ), directory ) diff --git a/lib/galaxy/webapps/galaxy/controllers/library.py b/lib/galaxy/webapps/galaxy/controllers/library.py index 3df63fa6584..245410bd6e5 100644 --- a/lib/galaxy/webapps/galaxy/controllers/library.py +++ b/lib/galaxy/webapps/galaxy/controllers/library.py @@ -91,6 +91,7 @@ class Library( BaseUIController ): 'app': app } ) @web.expose + @web.require_admin def index( self, trans, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = escape( kwd.get( 'status', 'done' ) ) @@ -101,6 +102,7 @@ class Library( BaseUIController ): status=status ) @web.expose + @web.require_admin def browse_libraries( self, trans, **kwd ): if 'operation' in kwd: operation = kwd['operation'].lower() diff --git a/lib/galaxy/webapps/galaxy/controllers/library_common.py b/lib/galaxy/webapps/galaxy/controllers/library_common.py index 6ecc6acba5d..da9e67b693c 100644 --- a/lib/galaxy/webapps/galaxy/controllers/library_common.py +++ b/lib/galaxy/webapps/galaxy/controllers/library_common.py @@ -22,6 +22,7 @@ from galaxy import util, web from galaxy.security import Action from galaxy.tools.actions import upload_common from galaxy.util import inflector, unicodify +from galaxy.util.path import safe_contains, safe_relpath, unsafe_walk from galaxy.util.streamball import StreamBall from galaxy.web.base.controller import BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMetadataMixin, UsesLibraryMixinItems from galaxy.web.form_builder import AddressField, CheckboxField, SelectField, build_select_field @@ -71,6 +72,7 @@ except: class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMetadataMixin, UsesLibraryMixinItems ): @web.json + @web.require_admin def library_item_updates( self, trans, ids=None, states=None ): # Avoid caching trans.response.headers['Pragma'] = 'no-cache' @@ -94,6 +96,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet return rval @web.expose + @web.require_admin def browse_library( self, trans, cntrller='library', **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -166,6 +169,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=status ) ) @web.expose + @web.require_admin def library_info( self, trans, cntrller, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -225,6 +229,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def library_permissions( self, trans, cntrller, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -272,6 +277,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet 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' ) @@ -347,6 +353,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def folder_info( self, trans, cntrller, id, library_id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -403,6 +410,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def folder_permissions( self, trans, cntrller, id, library_id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -453,6 +461,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def ldda_edit_info( self, trans, cntrller, library_id, folder_id, id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -604,6 +613,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def ldda_info( self, trans, cntrller, library_id, folder_id, id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -653,6 +663,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def ldda_permissions( self, trans, cntrller, library_id, folder_id, id, **kwd ): message = str( escape( kwd.get( 'message', '' ) ) ) status = kwd.get( 'status', 'done' ) @@ -791,6 +802,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet 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' ) @@ -846,6 +858,9 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet elif upload_option == 'upload_paths' and not is_admin: error = True message = 'Uploading files via filesystem paths can only be performed by administrators' + elif upload_option not in ('upload_file', 'upload_directory', 'upload_paths'): + error = True + message = 'Invalid upload_option' elif roles: # Check to see if the user selected roles to associate with the DATASET_ACCESS permission # on the dataset that would cause accessibility issues. @@ -1060,17 +1075,26 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet if upload_option == 'upload_directory': if server_dir in [ None, 'None', '' ]: response_code = 400 - if cntrller == 'library_admin' or ( cntrller == 'api' and trans.user_is_admin ): + if trans.user_is_admin() and cntrller in ( 'library_admin', 'api' ): import_dir = trans.app.config.library_import_dir import_dir_desc = 'library_import_dir' - full_dir = os.path.join( import_dir, server_dir ) else: import_dir = trans.app.config.user_library_import_dir + if server_dir != trans.user.email: + import_dir = os.path.join( import_dir, trans.user.email ) import_dir_desc = 'user_library_import_dir' - if server_dir == trans.user.email: - full_dir = os.path.join( import_dir, server_dir ) - else: - full_dir = os.path.join( import_dir, trans.user.email, server_dir ) + full_dir = os.path.join( import_dir, server_dir ) + unsafe = None + if safe_relpath( server_dir ): + if import_dir_desc == 'user_library_import_dir' and safe_contains( import_dir, full_dir, whitelist=trans.app.config.user_library_import_symlink_whitelist ): + for unsafe in unsafe_walk( full_dir, whitelist=[import_dir] + trans.app.config.user_library_import_symlink_whitelist ): + log.error( 'User attempted to import a path that resolves to a path outside of their import dir: %s -> %s', unsafe, os.path.realpath(unsafe) ) + else: + log.error( 'User attempted to import a directory path that resolves to a path outside of their import dir: %s -> %s', server_dir, os.path.realpath(full_dir) ) + unsafe = True + if unsafe: + response_code = 403 + message = 'Invalid server_dir' if import_dir: message = 'Select a directory' else: @@ -1262,6 +1286,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet return None @web.expose + @web.require_admin def add_history_datasets_to_library( self, trans, cntrller, library_id, folder_id, hda_ids='', **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -1499,6 +1524,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet return upload_option_select_list @web.expose + @web.require_admin def download_dataset_from_folder( self, trans, cntrller, id, library_id=None, **kwd ): """Catches the dataset id and displays file contents as directed""" show_deleted = util.string_as_bool( kwd.get( 'show_deleted', False ) ) @@ -1539,6 +1565,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status='error' ) ) @web.expose + @web.require_admin def library_dataset_info( self, trans, cntrller, id, library_id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -1587,6 +1614,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def library_dataset_permissions( self, trans, cntrller, id, library_id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -1635,6 +1663,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def make_library_item_public( self, trans, cntrller, library_id, item_type, id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -1677,6 +1706,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=status ) ) @web.expose + @web.require_admin def act_on_multiple_datasets( self, trans, cntrller, library_id=None, ldda_ids='', **kwd ): # This method is called from 1 of 3 places: # - this controller's download_dataset_from_folder() method @@ -1969,6 +1999,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def import_datasets_to_histories( self, trans, cntrller, library_id='', folder_id='', ldda_ids='', target_history_id='', target_history_ids='', new_history_name='', **kwd ): # This method is called from one of the following places: # - a menu option for a library dataset ( ldda_ids is a single ldda id ) @@ -2099,6 +2130,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def manage_template_inheritance( self, trans, cntrller, item_type, library_id, folder_id=None, ldda_id=None, **kwd ): show_deleted = util.string_as_bool( kwd.get( 'show_deleted', False ) ) use_panels = util.string_as_bool( kwd.get( 'use_panels', False ) ) @@ -2144,6 +2176,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status='done' ) ) @web.expose + @web.require_admin def move_library_item( self, trans, cntrller, item_type, item_id, source_library_id='', make_target_current=True, **kwd ): # This method is called from one of the following places: # - a menu option for a library dataset ( item_type is 'ldda' and item_id is a single ldda id ) @@ -2361,6 +2394,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def delete_library_item( self, trans, cntrller, library_id, item_id, item_type, **kwd ): # This action will handle deleting all types of library items. State is saved for libraries and # folders ( i.e., if undeleted, the state of contents of the library or folder will remain, so previously @@ -2429,6 +2463,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=status ) ) @web.expose + @web.require_admin def undelete_library_item( self, trans, cntrller, library_id, item_id, item_type, **kwd ): # This action will handle undeleting all types of library items status = kwd.get( 'status', 'done' ) diff --git a/lib/galaxy/webapps/galaxy/controllers/user.py b/lib/galaxy/webapps/galaxy/controllers/user.py index 8acb135afb1..46746797d93 100644 --- a/lib/galaxy/webapps/galaxy/controllers/user.py +++ b/lib/galaxy/webapps/galaxy/controllers/user.py @@ -686,7 +686,7 @@ class User( BaseUIController, UsesFormDefinitionsMixin, CreatesUsersMixin, Creat subscribe_checked = CheckboxField.is_checked( subscribe ) referer = trans.request.referer or '' redirect = kwd.get( 'redirect', referer ).strip() - is_admin = cntrller == 'admin' and trans.user_is_admin + is_admin = cntrller == 'admin' and trans.user_is_admin() if not trans.app.config.allow_user_creation and not trans.user_is_admin(): message = 'User registration is disabled. Please contact your local Galaxy administrator for an account.' if trans.app.config.error_email_to is not None: diff --git a/lib/tool_shed/managers/groups.py b/lib/tool_shed/managers/groups.py index a85fde86959..81b069e9c79 100644 --- a/lib/tool_shed/managers/groups.py +++ b/lib/tool_shed/managers/groups.py @@ -56,7 +56,7 @@ class GroupManager( object ): """ Create a new group. """ - if not trans.user_is_admin: + if not trans.user_is_admin(): raise ItemAccessibilityException( 'Only administrators can create groups.' ) else: if self.get( trans, name=name ): From 0e698813a96f1ad61d797255686f69cf5e6b1280 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Thu, 19 Oct 2017 09:29:00 -0400 Subject: [PATCH 02/13] [GX-2017-0003]: Fix for the reported issue, only allow http, https, and ftp schemes in the data_source tool. --- tools/data_source/data_source.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/data_source/data_source.py b/tools/data_source/data_source.py index 3660c5999f9..95c65b81bea 100644 --- a/tools/data_source/data_source.py +++ b/tools/data_source/data_source.py @@ -6,7 +6,7 @@ import socket import sys from json import loads, dumps -from six.moves.urllib.parse import urlencode +from six.moves.urllib.parse import urlencode, urlparse from six.moves.urllib.request import urlopen from galaxy.jobs import TOOL_PROVIDED_JOB_METADATA_FILE @@ -80,7 +80,7 @@ def __main__(): for data_dict in job_params[ 'output_data' ]: cur_filename = data_dict.get( 'file_name', filename ) cur_URL = params.get( '%s|%s|URL' % ( GALAXY_PARAM_PREFIX, data_dict[ 'out_data_name' ] ), URL ) - if not cur_URL: + if not cur_URL or urlparse(cur_URL).scheme not in ('http', 'https', 'ftp'): open( cur_filename, 'w' ).write( "" ) stop_err( 'The remote data source application has not sent back a URL parameter in the request.' ) From ed045cd570cf6b2198fb496852458194c8e28d6f Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Tue, 12 Sep 2017 15:06:30 -0400 Subject: [PATCH 03/13] Security: Fix issues with path handling in libraries and in general. --- config/galaxy.ini.sample | 8 + lib/galaxy/config.py | 1 + lib/galaxy/managers/folders.py | 4 +- lib/galaxy/managers/libraries.py | 2 +- lib/galaxy/tools/parameters/grouping.py | 2 + lib/galaxy/util/__init__.py | 48 ++--- lib/galaxy/util/path/__init__.py | 185 ++++++++++++++++++ lib/galaxy/util/path/ntpath.py | 11 ++ lib/galaxy/util/path/posixpath.py | 11 ++ lib/galaxy/webapps/galaxy/api/lda_datasets.py | 33 +++- lib/galaxy/webapps/galaxy/api/remote_files.py | 4 +- .../webapps/galaxy/controllers/library.py | 2 + .../galaxy/controllers/library_common.py | 47 ++++- lib/galaxy/webapps/galaxy/controllers/user.py | 2 +- lib/tool_shed/managers/groups.py | 2 +- 15 files changed, 304 insertions(+), 58 deletions(-) create mode 100644 lib/galaxy/util/path/__init__.py create mode 100644 lib/galaxy/util/path/ntpath.py create mode 100644 lib/galaxy/util/path/posixpath.py diff --git a/config/galaxy.ini.sample b/config/galaxy.ini.sample index 2e4a325377f..181b5ee605b 100644 --- a/config/galaxy.ini.sample +++ b/config/galaxy.ini.sample @@ -853,6 +853,14 @@ use_interactive = True # sub-directories of files contained in their directory. #user_library_import_dir = None +# For security reasons, users may not import any files that actually lie +# outside of their `user_library_import_dir` (e.g. using symbolic links). A +# list of directories can be allowed by setting the following option (the list +# is comma-separated). Be aware that *any* user with library import permissions +# can import from anywhere in these directories (assuming they are able to +# create symlinks to them). +#user_library_import_symlink_whitelist = None + # Add an option to the admin library upload tool allowing admins to paste # filesystem paths to files and directories in a box, and these paths will be # added to a library. Set to True to enable. Please note the security diff --git a/lib/galaxy/config.py b/lib/galaxy/config.py index 49f49bd3c36..85acd5ba747 100644 --- a/lib/galaxy/config.py +++ b/lib/galaxy/config.py @@ -288,6 +288,7 @@ class Configuration( object ): self.screencasts_url = kwargs.get( 'screencasts_url', None ) self.library_import_dir = kwargs.get( 'library_import_dir', None ) self.user_library_import_dir = kwargs.get( 'user_library_import_dir', None ) + self.user_library_import_symlink_whitelist = listify( kwargs.get( 'user_library_import_symlink_whitelist', []), do_strip=True ) # Searching data libraries self.enable_lucene_library_search = string_as_bool( kwargs.get( 'enable_lucene_library_search', False ) ) self.enable_whoosh_library_search = string_as_bool( kwargs.get( 'enable_whoosh_library_search', False ) ) diff --git a/lib/galaxy/managers/folders.py b/lib/galaxy/managers/folders.py index cbb4ad3e9b7..b835ae76e50 100644 --- a/lib/galaxy/managers/folders.py +++ b/lib/galaxy/managers/folders.py @@ -132,7 +132,7 @@ class FolderManager( object ): """ parent_folder = self.get( trans, parent_folder_id ) current_user_roles = trans.get_current_user_roles() - if not ( trans.user_is_admin or trans.app.security_agent.can_add_library_item( current_user_roles, parent_folder ) ): + if not ( trans.user_is_admin() or trans.app.security_agent.can_add_library_item( current_user_roles, parent_folder ) ): raise InsufficientPermissionsException( 'You do not have proper permission to create folders under given folder.' ) new_folder = trans.app.model.LibraryFolder( name=new_folder_name, description=new_folder_description ) # We are associating the last used genome build with folders, so we will always @@ -230,7 +230,7 @@ class FolderManager( object ): """ Return true if the user has permissions to add item to the given folder. """ - if trans.user_is_admin: + if trans.user_is_admin(): return True current_user_roles = trans.get_current_user_roles() add_roles = set( trans.app.security_agent.get_roles_for_action( folder, trans.app.security_agent.permitted_actions.LIBRARY_ADD ) ) diff --git a/lib/galaxy/managers/libraries.py b/lib/galaxy/managers/libraries.py index 9bcb4d82fd6..337cb3ac807 100644 --- a/lib/galaxy/managers/libraries.py +++ b/lib/galaxy/managers/libraries.py @@ -50,7 +50,7 @@ class LibraryManager( object ): """ Create a new library. """ - if not trans.user_is_admin: + if not trans.user_is_admin(): raise exceptions.ItemAccessibilityException( 'Only administrators can create libraries.' ) else: library = trans.app.model.Library( name=name, description=description, synopsis=synopsis ) diff --git a/lib/galaxy/tools/parameters/grouping.py b/lib/galaxy/tools/parameters/grouping.py index 4a5fc1b0023..0c33ee64185 100644 --- a/lib/galaxy/tools/parameters/grouping.py +++ b/lib/galaxy/tools/parameters/grouping.py @@ -355,6 +355,7 @@ class UploadDataset( Group ): warnings.append( "All FTP uploaded file selections were ignored." ) elif ftp_files is not None and trans.user is not None: # look for files uploaded via FTP user_ftp_dir = trans.user_ftp_dir + assert not os.path.islink(user_ftp_dir), "User FTP directory cannot be a symbolic link" for ( dirpath, dirnames, filenames ) in os.walk( user_ftp_dir ): for filename in filenames: for ftp_filename in ftp_files: @@ -418,6 +419,7 @@ class UploadDataset( Group ): # TODO: warning to the user (could happen if session has become invalid) else: user_ftp_dir = trans.user_ftp_dir + assert not os.path.islink(user_ftp_dir), "User FTP directory cannot be a symbolic link" for ( dirpath, dirnames, filenames ) in os.walk( user_ftp_dir ): for filename in filenames: path = relpath( os.path.join( dirpath, filename ), user_ftp_dir ) diff --git a/lib/galaxy/util/__init__.py b/lib/galaxy/util/__init__.py index 02055f4963d..df32a3d2923 100644 --- a/lib/galaxy/util/__init__.py +++ b/lib/galaxy/util/__init__.py @@ -8,6 +8,7 @@ from __future__ import absolute_import import binascii import collections import errno +import importlib import json import logging import os @@ -29,7 +30,7 @@ except ImportError: from datetime import datetime from hashlib import md5 -from os.path import normpath, relpath +from os.path import relpath from xml.etree import ElementInclude, ElementTree from xml.etree.ElementTree import ParseError @@ -49,6 +50,7 @@ except ImportError: docutils_html4css1 = None from .inflection import English, Inflector +from .path import safe_contains, safe_makedirs, safe_relpath # noqa: F401 inflector = Inflector(English) @@ -604,31 +606,19 @@ def which(file): return None -def safe_makedirs(path): - """ Safely make a directory, do not fail if it already exist or - is created during execution. - """ - if not os.path.exists(path): - try: - os.makedirs(path) - except OSError as e: - # review source for Python 2.7 this would only ever happen - # for the last path anyway so need to recurse - this exception - # means the last part of the path was already in existence. - if e.errno != errno.EEXIST: - raise - - 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 - """ - # Make both absolute. - directory = local_path_module.abspath(directory) - file = local_path_module.abspath(file) - return local_path_module.commonprefix([file, directory]) == directory + local_path_module is used by Pulsar to check Windows paths while running on + a POSIX-like system. + """ + if local_path_module != os.path: + _safe_contains = importlib.import_module('galaxy.util.path.%s' % local_path_module.__name__).safe_contains + else: + _safe_contains = safe_contains + return _safe_contains(directory, file) def merge_sorted_iterables( operator, *iterables ): @@ -1503,22 +1493,6 @@ def download_to_file(url, dest_file_path, timeout=30, chunk_size=2 ** 20): f.write(chunk) -def safe_relpath(path): - """ - Given what we expect to be a relative path, determine whether the path - would exist inside the current directory. - - :type path: string - :param path: a path to check - :rtype: bool - :returns: ``True`` if path is relative and does not reference a path - in a parent directory, ``False`` otherwise. - """ - if path.startswith(os.sep) or normpath(path).startswith(os.pardir): - return False - return True - - class ExecutionTimer(object): def __init__(self): diff --git a/lib/galaxy/util/path/__init__.py b/lib/galaxy/util/path/__init__.py new file mode 100644 index 00000000000..48a8355ad79 --- /dev/null +++ b/lib/galaxy/util/path/__init__.py @@ -0,0 +1,185 @@ +"""Path manipulation functions. +""" +from __future__ import absolute_import + +import errno +import imp +from functools import partial +from itertools import starmap +from operator import getitem +from os import ( + makedirs, + walk, +) +from os.path import ( + abspath, + exists, + isabs, + join, + normpath, + pardir, + realpath, + relpath, +) + +from six import string_types +from six.moves import filterfalse, map, zip + + +def safe_contains(prefix, path, whitelist=None): + """Ensure a path is contained within another path. + + 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 + may also point inside a ``whitelist`` path. + + The ``path`` is checked against ``whitelist`` using either its absolute pathname (if passed in as absolute) or + relative to ``prefix`` and canonicalized (if applicable). It is *not* ``os.path.join()``ed with each ``whitelist`` + directory. + + :type prefix: string + :param prefix: a directory under which ``path`` is to be checked + :type path: string + :param path: a filename to check + :type whitelist: list of strings + :param whitelist: list of additional paths under which ``path`` may be located + :rtype: bool + :returns: ``True`` if ``path`` is contained within ``prefix`` or ``whitelist``, ``False`` otherwise. + """ + return any(__contains(prefix, path, whitelist=whitelist)) + + +def safe_makedirs(path): + """Safely make a directory, do not fail if it already exists or is created during execution. + + :type path: string + :param path: a directory to create + """ + # prechecking for existence is faster than try/except + if not exists(path): + try: + makedirs(path) + except OSError as e: + # reviewing the source for Python 2.7, this would only ever happen for the last path element anyway so no + # need to recurse - this exception means the last part of the path was already in existence. + if e.errno != errno.EEXIST: + raise + + +def safe_relpath(path): + """Determine whether a relative path references a path outside its root. + + This is a path computation: the filesystem is not accessed to confirm the existence or nature of ``path``. + + :type path: string + :param path: a path to check + :rtype: bool + :returns: ``True`` if path is relative and does not reference a path in a parent directory, ``False`` + otherwise. + """ + return not (isabs(path) or normpath(path).startswith(pardir)) + + +def unsafe_walk(path, whitelist=None): + """Walk a path and ensure that none of its contents are symlinks outside the path. + + It is assumed that ``path`` itself has already been validated e.g. with :func:`safe_relpath` or + :func:`safe_contains`. + + :type path: string + :param path: a directory to check for unsafe contents + :type whitelist: list of strings + :param whitelist: list of additional paths under which contents may be located + :rtype: iterator + :returns: Iterator of "bad" files found under ``path`` + """ + return filterfalse(partial(safe_contains, path, whitelist=whitelist), __walk(abspath(path))) + + +def __listify(item): + """A non-splitting version of :func:`galaxy.util.listify`. + """ + if not item: + return [] + elif isinstance(item, list) or isinstance(item, tuple): + return item + else: + return [item] + + +# helpers + + +def __walk(path): + for dirpath, dirnames, filenames in walk(path): + for name in dirnames + filenames: + yield join(dirpath, name) + + +def __contains(prefix, path, whitelist=None): + real = realpath(join(prefix, path)) + yield not relpath(real, prefix).startswith(pardir) + for wldir in whitelist or []: + yield not relpath(real, wldir).startswith(pardir) + + +# cross-platform support + + +def _build_self(target, path_module): + """Populate a module with the same exported functions as this module, but using the given os.path module. + + :type target: module + :param target: module on which to set ``galaxy.util.path`` functions + :type path_module: ``ntpath`` or ``posixpath`` module + :param path_module: module implementing ``os.path`` API to use for path functions + """ + __copy_self().__set_fxns_on(target, path_module) + + +def __copy_self(names=__name__, parent=None): + """Returns a copy of this module that can be modified without modifying `galaxy.util.path`` in ``sys.modules``. + """ + if isinstance(names, string_types): + names = iter(names.split('.')) + try: + name = names.next() + except StopIteration: + return parent + path = parent and parent.__path__ + parent = imp.load_module(name, *imp.find_module(name, path)) + return __copy_self(names, parent) + + +def __set_fxns_on(target, path_module): + """Overrides imported os.path functions with the ones from path_module and populates target with the global + functions from this module. + """ + for name in __pathfxns__: + globals()[name] = getattr(path_module, name) + __get = partial(getitem, globals()) + __set = partial(setattr, target) + # this is actually izip(..., imap(...)) + __fxns = zip(__all__, map(__get, __all__)) + # list() to execute + list(starmap(__set, __fxns)) + + +__pathfxns__ = ( + 'abspath', + 'exists', + 'isabs', + 'join', + 'normpath', + 'pardir', + 'realpath', + 'relpath', +) + +__all__ = ( + 'safe_contains', + 'safe_makedirs', + 'safe_relpath', + 'unsafe_walk', +) diff --git a/lib/galaxy/util/path/ntpath.py b/lib/galaxy/util/path/ntpath.py new file mode 100644 index 00000000000..5363409b3af --- /dev/null +++ b/lib/galaxy/util/path/ntpath.py @@ -0,0 +1,11 @@ +"""Galaxy "safe" path functions forced to work with Windows-style paths regardless of current platform +""" +from __future__ import absolute_import + +import ntpath +import sys + +from . import _build_self + + +_build_self(sys.modules[__name__], ntpath) diff --git a/lib/galaxy/util/path/posixpath.py b/lib/galaxy/util/path/posixpath.py new file mode 100644 index 00000000000..28232649b57 --- /dev/null +++ b/lib/galaxy/util/path/posixpath.py @@ -0,0 +1,11 @@ +"""Galaxy "safe" path functions forced to work with POSIX-style paths regardless of current platform +""" +from __future__ import absolute_import + +import posixpath +import sys + +from . import _build_self + + +_build_self(sys.modules[__name__], posixpath) diff --git a/lib/galaxy/webapps/galaxy/api/lda_datasets.py b/lib/galaxy/webapps/galaxy/api/lda_datasets.py index 14dc75158b7..b7574301577 100644 --- a/lib/galaxy/webapps/galaxy/api/lda_datasets.py +++ b/lib/galaxy/webapps/galaxy/api/lda_datasets.py @@ -19,6 +19,7 @@ from galaxy import web from galaxy.exceptions import ObjectNotFound from galaxy.managers import folders, roles from galaxy.tools.actions import upload_common +from galaxy.util.path import safe_contains, safe_relpath, unsafe_walk from galaxy.util.streamball import StreamBall from galaxy.web import _future_expose_api as expose_api from galaxy.web import _future_expose_api_anonymous as expose_api_anonymous @@ -422,29 +423,45 @@ class LibraryDatasetsController( BaseAPIController, UsesVisualizationMixin ): source = kwd.get( 'source', None ) if source not in [ 'userdir_file', 'userdir_folder', 'importdir_file', 'importdir_folder', 'admin_path' ]: raise exceptions.RequestParameterMissingException( 'You have to specify "source" parameter. Possible values are "userdir_file", "userdir_folder", "admin_path", "importdir_file" and "importdir_folder". ') - if source in [ 'importdir_file', 'importdir_folder' ]: - if not trans.user_is_admin: + elif source in [ 'importdir_file', 'importdir_folder' ]: + if not trans.user_is_admin(): raise exceptions.AdminRequiredException( 'Only admins can import from importdir.' ) if not trans.app.config.library_import_dir: raise exceptions.ConfigDoesNotAllowException( 'The configuration of this Galaxy instance does not allow admins to import into library from importdir.' ) import_base_dir = trans.app.config.library_import_dir + if not safe_relpath(path): + # admins shouldn't be able to explicitly specify a path outside server_dir, but symlinks are allowed. + # the reasoning here is that galaxy admins may not have direct filesystem access or can only access + # library_import_dir via FTP (which cannot create symlinks), and may rely on sysadmins to set up the + # import directory. if they have filesystem access, all bets are off. + raise exceptions.RequestParameterInvalidException( 'The given path is invalid.' ) path = os.path.join( import_base_dir, path ) - if source in [ 'userdir_file', 'userdir_folder' ]: + elif source in [ 'userdir_file', 'userdir_folder' ]: + unsafe = None user_login = trans.user.email user_base_dir = trans.app.config.user_library_import_dir if user_base_dir is None: raise exceptions.ConfigDoesNotAllowException( 'The configuration of this Galaxy instance does not allow upload from user directories.' ) full_dir = os.path.join( user_base_dir, user_login ) - if not path.lower().startswith( full_dir.lower() ): + if not safe_contains( full_dir, path, whitelist=trans.app.config.user_library_import_symlink_whitelist ): + # the path is a symlink outside the user dir path = os.path.join( full_dir, path ) + log.error( 'User attempted to import a path that resolves to a path outside of their import dir: %s -> %s', path, os.path.realpath(path) ) + raise exceptions.RequestParameterInvalidException( 'The given path is invalid.' ) + path = os.path.join( full_dir, path ) + for unsafe in unsafe_walk( path, whitelist=[full_dir] + trans.app.config.user_library_import_symlink_whitelist ): + # the path is a dir and contains files that symlink outside the user dir + log.error( 'User attempted to import a directory containing a path that resolves to a path outside of their import dir: %s -> %s', unsafe, os.path.realpath(unsafe) ) + if unsafe: + raise exceptions.RequestParameterInvalidException( 'The given path is invalid.' ) if not os.path.exists( path ): raise exceptions.RequestParameterInvalidException( 'Given path does not exist on the host.' ) if not self.folder_manager.can_add_item( trans, folder ): raise exceptions.InsufficientPermissionsException( 'You do not have proper permission to add items to the given folder.' ) - if source == 'admin_path': + elif source == 'admin_path': if not trans.app.config.allow_library_path_paste: raise exceptions.ConfigDoesNotAllowException( 'The configuration of this Galaxy instance does not allow admins to import into library from path.' ) - if not trans.user_is_admin: + if not trans.user_is_admin(): raise exceptions.AdminRequiredException( 'Only admins can import from path.' ) # Set up the traditional tool state/params @@ -463,12 +480,12 @@ class LibraryDatasetsController( BaseAPIController, UsesVisualizationMixin ): if source in [ 'importdir_folder' ]: kwd[ 'filesystem_paths' ] = os.path.join( import_base_dir, path ) # user wants to import one file only - if source in [ "userdir_file", "importdir_file" ]: + elif source in [ "userdir_file", "importdir_file" ]: file = os.path.abspath( path ) abspath_datasets.append( trans.webapp.controllers[ 'library_common' ].make_library_uploaded_dataset( trans, 'api', kwd, os.path.basename( file ), file, 'server_dir', library_bunch ) ) # user wants to import whole folder - if source == "userdir_folder": + elif source == "userdir_folder": uploaded_datasets_bunch = trans.webapp.controllers[ 'library_common' ].get_path_paste_uploaded_datasets( trans, 'api', kwd, library_bunch, 200, '' ) uploaded_datasets = uploaded_datasets_bunch[ 0 ] diff --git a/lib/galaxy/webapps/galaxy/api/remote_files.py b/lib/galaxy/webapps/galaxy/api/remote_files.py index 28d42cd45c0..904c186e275 100644 --- a/lib/galaxy/webapps/galaxy/api/remote_files.py +++ b/lib/galaxy/webapps/galaxy/api/remote_files.py @@ -107,7 +107,7 @@ class RemoteFilesAPIController( BaseAPIController ): subfolders and returns a flat list. """ response = [] - if os.path.exists( directory ): + if os.path.exists( directory ) and not os.path.islink( directory ): for ( dirpath, dirnames, filenames ) in os.walk( directory ): for filename in filenames: path = os.path.relpath( os.path.join( dirpath, filename ), directory ) @@ -130,7 +130,7 @@ class RemoteFilesAPIController( BaseAPIController ): """ userdir_jstree = None jstree_paths = [] - if os.path.exists( directory ): + if os.path.exists( directory ) and not os.path.islink( directory ): for ( dirpath, dirnames, filenames ) in os.walk( directory ): for dirname in dirnames: dir_path = os.path.relpath( os.path.join( dirpath, dirname ), directory ) diff --git a/lib/galaxy/webapps/galaxy/controllers/library.py b/lib/galaxy/webapps/galaxy/controllers/library.py index 3df63fa6584..245410bd6e5 100644 --- a/lib/galaxy/webapps/galaxy/controllers/library.py +++ b/lib/galaxy/webapps/galaxy/controllers/library.py @@ -91,6 +91,7 @@ class Library( BaseUIController ): 'app': app } ) @web.expose + @web.require_admin def index( self, trans, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = escape( kwd.get( 'status', 'done' ) ) @@ -101,6 +102,7 @@ class Library( BaseUIController ): status=status ) @web.expose + @web.require_admin def browse_libraries( self, trans, **kwd ): if 'operation' in kwd: operation = kwd['operation'].lower() diff --git a/lib/galaxy/webapps/galaxy/controllers/library_common.py b/lib/galaxy/webapps/galaxy/controllers/library_common.py index 87252bd143b..a05b9c97647 100644 --- a/lib/galaxy/webapps/galaxy/controllers/library_common.py +++ b/lib/galaxy/webapps/galaxy/controllers/library_common.py @@ -22,6 +22,7 @@ from galaxy import util, web from galaxy.security import Action from galaxy.tools.actions import upload_common from galaxy.util import inflector, unicodify, FILENAME_VALID_CHARS +from galaxy.util.path import safe_contains, safe_relpath, unsafe_walk from galaxy.util.streamball import StreamBall from galaxy.web.base.controller import BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMetadataMixin, UsesLibraryMixinItems from galaxy.web.form_builder import AddressField, CheckboxField, SelectField, build_select_field @@ -71,6 +72,7 @@ except: class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMetadataMixin, UsesLibraryMixinItems ): @web.json + @web.require_admin def library_item_updates( self, trans, ids=None, states=None ): # Avoid caching trans.response.headers['Pragma'] = 'no-cache' @@ -94,6 +96,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet return rval @web.expose + @web.require_admin def browse_library( self, trans, cntrller='library', **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -166,6 +169,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=status ) ) @web.expose + @web.require_admin def library_info( self, trans, cntrller, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -225,6 +229,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def library_permissions( self, trans, cntrller, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -272,6 +277,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet 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' ) @@ -347,6 +353,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def folder_info( self, trans, cntrller, id, library_id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -403,6 +410,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def folder_permissions( self, trans, cntrller, id, library_id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -453,6 +461,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def ldda_edit_info( self, trans, cntrller, library_id, folder_id, id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -604,6 +613,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def ldda_info( self, trans, cntrller, library_id, folder_id, id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -653,6 +663,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def ldda_permissions( self, trans, cntrller, library_id, folder_id, id, **kwd ): message = str( escape( kwd.get( 'message', '' ) ) ) status = kwd.get( 'status', 'done' ) @@ -791,6 +802,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet 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' ) @@ -850,6 +862,9 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet elif upload_option == 'upload_paths' and not is_admin: error = True message = 'Uploading files via filesystem paths can only be performed by administrators' + elif upload_option not in ('upload_file', 'upload_directory', 'upload_paths'): + error = True + message = 'Invalid upload_option' elif roles: # Check to see if the user selected roles to associate with the DATASET_ACCESS permission # on the dataset that would cause accessibility issues. @@ -1065,17 +1080,26 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet if upload_option == 'upload_directory': if server_dir in [ None, 'None', '' ]: response_code = 400 - if cntrller == 'library_admin' or ( cntrller == 'api' and trans.user_is_admin ): + if trans.user_is_admin() and cntrller in ( 'library_admin', 'api' ): import_dir = trans.app.config.library_import_dir import_dir_desc = 'library_import_dir' - full_dir = os.path.join( import_dir, server_dir ) else: import_dir = trans.app.config.user_library_import_dir + if server_dir != trans.user.email: + import_dir = os.path.join( import_dir, trans.user.email ) import_dir_desc = 'user_library_import_dir' - if server_dir == trans.user.email: - full_dir = os.path.join( import_dir, server_dir ) - else: - full_dir = os.path.join( import_dir, trans.user.email, server_dir ) + full_dir = os.path.join( import_dir, server_dir ) + unsafe = None + if safe_relpath( server_dir ): + if import_dir_desc == 'user_library_import_dir' and safe_contains( import_dir, full_dir, whitelist=trans.app.config.user_library_import_symlink_whitelist ): + for unsafe in unsafe_walk( full_dir, whitelist=[import_dir] + trans.app.config.user_library_import_symlink_whitelist ): + log.error( 'User attempted to import a path that resolves to a path outside of their import dir: %s -> %s', unsafe, os.path.realpath(unsafe) ) + else: + log.error( 'User attempted to import a directory path that resolves to a path outside of their import dir: %s -> %s', server_dir, os.path.realpath(full_dir) ) + unsafe = True + if unsafe: + response_code = 403 + message = 'Invalid server_dir' if import_dir: message = 'Select a directory' else: @@ -1268,6 +1292,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet return None @web.expose + @web.require_admin def add_history_datasets_to_library( self, trans, cntrller, library_id, folder_id, hda_ids='', **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -1510,6 +1535,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet return upload_option_select_list @web.expose + @web.require_admin def download_dataset_from_folder( self, trans, cntrller, id, library_id=None, **kwd ): """Catches the dataset id and displays file contents as directed""" show_deleted = util.string_as_bool( kwd.get( 'show_deleted', False ) ) @@ -1549,6 +1575,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status='error' ) ) @web.expose + @web.require_admin def library_dataset_info( self, trans, cntrller, id, library_id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -1597,6 +1624,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def library_dataset_permissions( self, trans, cntrller, id, library_id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -1645,6 +1673,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def make_library_item_public( self, trans, cntrller, library_id, item_type, id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -1687,6 +1716,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=status ) ) @web.expose + @web.require_admin def act_on_multiple_datasets( self, trans, cntrller, library_id=None, ldda_ids='', **kwd ): # This method is called from 1 of 3 places: # - this controller's download_dataset_from_folder() method @@ -1979,6 +2009,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def import_datasets_to_histories( self, trans, cntrller, library_id='', folder_id='', ldda_ids='', target_history_id='', target_history_ids='', new_history_name='', **kwd ): # This method is called from one of the following places: # - a menu option for a library dataset ( ldda_ids is a single ldda id ) @@ -2109,6 +2140,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def manage_template_inheritance( self, trans, cntrller, item_type, library_id, folder_id=None, ldda_id=None, **kwd ): show_deleted = util.string_as_bool( kwd.get( 'show_deleted', False ) ) use_panels = util.string_as_bool( kwd.get( 'use_panels', False ) ) @@ -2154,6 +2186,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status='done' ) ) @web.expose + @web.require_admin def move_library_item( self, trans, cntrller, item_type, item_id, source_library_id='', make_target_current=True, **kwd ): # This method is called from one of the following places: # - a menu option for a library dataset ( item_type is 'ldda' and item_id is a single ldda id ) @@ -2371,6 +2404,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def delete_library_item( self, trans, cntrller, library_id, item_id, item_type, **kwd ): # This action will handle deleting all types of library items. State is saved for libraries and # folders ( i.e., if undeleted, the state of contents of the library or folder will remain, so previously @@ -2439,6 +2473,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=status ) ) @web.expose + @web.require_admin def undelete_library_item( self, trans, cntrller, library_id, item_id, item_type, **kwd ): # This action will handle undeleting all types of library items status = kwd.get( 'status', 'done' ) diff --git a/lib/galaxy/webapps/galaxy/controllers/user.py b/lib/galaxy/webapps/galaxy/controllers/user.py index 712bc49be0d..b3aa7341592 100644 --- a/lib/galaxy/webapps/galaxy/controllers/user.py +++ b/lib/galaxy/webapps/galaxy/controllers/user.py @@ -687,7 +687,7 @@ class User( BaseUIController, UsesFormDefinitionsMixin, CreatesUsersMixin, Creat subscribe_checked = CheckboxField.is_checked( subscribe ) referer = trans.request.referer or '' redirect = kwd.get( 'redirect', referer ).strip() - is_admin = cntrller == 'admin' and trans.user_is_admin + is_admin = cntrller == 'admin' and trans.user_is_admin() if not trans.app.config.allow_user_creation and not trans.user_is_admin(): message = 'User registration is disabled. Please contact your local Galaxy administrator for an account.' if trans.app.config.error_email_to is not None: diff --git a/lib/tool_shed/managers/groups.py b/lib/tool_shed/managers/groups.py index a85fde86959..81b069e9c79 100644 --- a/lib/tool_shed/managers/groups.py +++ b/lib/tool_shed/managers/groups.py @@ -56,7 +56,7 @@ class GroupManager( object ): """ Create a new group. """ - if not trans.user_is_admin: + if not trans.user_is_admin(): raise ItemAccessibilityException( 'Only administrators can create groups.' ) else: if self.get( trans, name=name ): From 9e672f94588b8eeecee745665162fc92f0158e27 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Thu, 19 Oct 2017 09:29:00 -0400 Subject: [PATCH 04/13] [GX-2017-0003]: Fix for the reported issue, only allow http, https, and ftp schemes in the data_source tool. --- tools/data_source/data_source.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/data_source/data_source.py b/tools/data_source/data_source.py index 53121f1376c..f513cec5495 100644 --- a/tools/data_source/data_source.py +++ b/tools/data_source/data_source.py @@ -6,7 +6,7 @@ import socket import sys from json import dumps, loads -from six.moves.urllib.parse import urlencode +from six.moves.urllib.parse import urlencode, urlparse from six.moves.urllib.request import urlopen from galaxy.datatypes import sniff @@ -80,7 +80,7 @@ def __main__(): for data_dict in job_params[ 'output_data' ]: cur_filename = data_dict.get( 'file_name', filename ) cur_URL = params.get( '%s|%s|URL' % ( GALAXY_PARAM_PREFIX, data_dict[ 'out_data_name' ] ), URL ) - if not cur_URL: + if not cur_URL or urlparse(cur_URL).scheme not in ('http', 'https', 'ftp'): open( cur_filename, 'w' ).write( "" ) stop_err( 'The remote data source application has not sent back a URL parameter in the request.' ) From 1bccbefbffc02803e93913b145f4214b8eb78d54 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Tue, 12 Sep 2017 15:06:30 -0400 Subject: [PATCH 05/13] Security: Fix issues with path handling in libraries and in general. --- config/galaxy.ini.sample | 8 + lib/galaxy/config.py | 1 + lib/galaxy/managers/folders.py | 4 +- lib/galaxy/managers/libraries.py | 2 +- lib/galaxy/tools/parameters/grouping.py | 2 + lib/galaxy/util/__init__.py | 48 ++--- lib/galaxy/util/path/__init__.py | 185 ++++++++++++++++++ lib/galaxy/util/path/ntpath.py | 11 ++ lib/galaxy/util/path/posixpath.py | 11 ++ lib/galaxy/webapps/galaxy/api/lda_datasets.py | 33 +++- lib/galaxy/webapps/galaxy/api/remote_files.py | 4 +- .../webapps/galaxy/controllers/library.py | 2 + .../galaxy/controllers/library_common.py | 47 ++++- lib/galaxy/webapps/galaxy/controllers/user.py | 2 +- lib/tool_shed/managers/groups.py | 2 +- 15 files changed, 304 insertions(+), 58 deletions(-) create mode 100644 lib/galaxy/util/path/__init__.py create mode 100644 lib/galaxy/util/path/ntpath.py create mode 100644 lib/galaxy/util/path/posixpath.py diff --git a/config/galaxy.ini.sample b/config/galaxy.ini.sample index be3366072fe..3fbc11ecc1f 100644 --- a/config/galaxy.ini.sample +++ b/config/galaxy.ini.sample @@ -864,6 +864,14 @@ use_interactive = True # sub-directories of files contained in their directory. #user_library_import_dir = None +# For security reasons, users may not import any files that actually lie +# outside of their `user_library_import_dir` (e.g. using symbolic links). A +# list of directories can be allowed by setting the following option (the list +# is comma-separated). Be aware that *any* user with library import permissions +# can import from anywhere in these directories (assuming they are able to +# create symlinks to them). +#user_library_import_symlink_whitelist = None + # Add an option to the admin library upload tool allowing admins to paste # filesystem paths to files and directories in a box, and these paths will be # added to a library. Set to True to enable. Please note the security diff --git a/lib/galaxy/config.py b/lib/galaxy/config.py index d3af12b4305..32cff60157b 100644 --- a/lib/galaxy/config.py +++ b/lib/galaxy/config.py @@ -370,6 +370,7 @@ class Configuration( object ): self.screencasts_url = kwargs.get( 'screencasts_url', None ) self.library_import_dir = kwargs.get( 'library_import_dir', None ) self.user_library_import_dir = kwargs.get( 'user_library_import_dir', None ) + self.user_library_import_symlink_whitelist = listify( kwargs.get( 'user_library_import_symlink_whitelist', []), do_strip=True ) # Searching data libraries self.enable_lucene_library_search = string_as_bool( kwargs.get( 'enable_lucene_library_search', False ) ) self.enable_whoosh_library_search = string_as_bool( kwargs.get( 'enable_whoosh_library_search', False ) ) diff --git a/lib/galaxy/managers/folders.py b/lib/galaxy/managers/folders.py index cbb4ad3e9b7..b835ae76e50 100644 --- a/lib/galaxy/managers/folders.py +++ b/lib/galaxy/managers/folders.py @@ -132,7 +132,7 @@ class FolderManager( object ): """ parent_folder = self.get( trans, parent_folder_id ) current_user_roles = trans.get_current_user_roles() - if not ( trans.user_is_admin or trans.app.security_agent.can_add_library_item( current_user_roles, parent_folder ) ): + if not ( trans.user_is_admin() or trans.app.security_agent.can_add_library_item( current_user_roles, parent_folder ) ): raise InsufficientPermissionsException( 'You do not have proper permission to create folders under given folder.' ) new_folder = trans.app.model.LibraryFolder( name=new_folder_name, description=new_folder_description ) # We are associating the last used genome build with folders, so we will always @@ -230,7 +230,7 @@ class FolderManager( object ): """ Return true if the user has permissions to add item to the given folder. """ - if trans.user_is_admin: + if trans.user_is_admin(): return True current_user_roles = trans.get_current_user_roles() add_roles = set( trans.app.security_agent.get_roles_for_action( folder, trans.app.security_agent.permitted_actions.LIBRARY_ADD ) ) diff --git a/lib/galaxy/managers/libraries.py b/lib/galaxy/managers/libraries.py index 9bcb4d82fd6..337cb3ac807 100644 --- a/lib/galaxy/managers/libraries.py +++ b/lib/galaxy/managers/libraries.py @@ -50,7 +50,7 @@ class LibraryManager( object ): """ Create a new library. """ - if not trans.user_is_admin: + if not trans.user_is_admin(): raise exceptions.ItemAccessibilityException( 'Only administrators can create libraries.' ) else: library = trans.app.model.Library( name=name, description=description, synopsis=synopsis ) diff --git a/lib/galaxy/tools/parameters/grouping.py b/lib/galaxy/tools/parameters/grouping.py index f83d6649050..b14b440f8fb 100644 --- a/lib/galaxy/tools/parameters/grouping.py +++ b/lib/galaxy/tools/parameters/grouping.py @@ -360,6 +360,7 @@ class UploadDataset( Group ): warnings.append( "All FTP uploaded file selections were ignored." ) elif ftp_files is not None and trans.user is not None: # look for files uploaded via FTP user_ftp_dir = trans.user_ftp_dir + assert not os.path.islink(user_ftp_dir), "User FTP directory cannot be a symbolic link" for ( dirpath, dirnames, filenames ) in os.walk( user_ftp_dir ): for filename in filenames: for ftp_filename in ftp_files: @@ -423,6 +424,7 @@ class UploadDataset( Group ): # TODO: warning to the user (could happen if session has become invalid) else: user_ftp_dir = trans.user_ftp_dir + assert not os.path.islink(user_ftp_dir), "User FTP directory cannot be a symbolic link" for ( dirpath, dirnames, filenames ) in os.walk( user_ftp_dir ): for filename in filenames: path = relpath( os.path.join( dirpath, filename ), user_ftp_dir ) diff --git a/lib/galaxy/util/__init__.py b/lib/galaxy/util/__init__.py index 02055f4963d..df32a3d2923 100644 --- a/lib/galaxy/util/__init__.py +++ b/lib/galaxy/util/__init__.py @@ -8,6 +8,7 @@ from __future__ import absolute_import import binascii import collections import errno +import importlib import json import logging import os @@ -29,7 +30,7 @@ except ImportError: from datetime import datetime from hashlib import md5 -from os.path import normpath, relpath +from os.path import relpath from xml.etree import ElementInclude, ElementTree from xml.etree.ElementTree import ParseError @@ -49,6 +50,7 @@ except ImportError: docutils_html4css1 = None from .inflection import English, Inflector +from .path import safe_contains, safe_makedirs, safe_relpath # noqa: F401 inflector = Inflector(English) @@ -604,31 +606,19 @@ def which(file): return None -def safe_makedirs(path): - """ Safely make a directory, do not fail if it already exist or - is created during execution. - """ - if not os.path.exists(path): - try: - os.makedirs(path) - except OSError as e: - # review source for Python 2.7 this would only ever happen - # for the last path anyway so need to recurse - this exception - # means the last part of the path was already in existence. - if e.errno != errno.EEXIST: - raise - - 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 - """ - # Make both absolute. - directory = local_path_module.abspath(directory) - file = local_path_module.abspath(file) - return local_path_module.commonprefix([file, directory]) == directory + local_path_module is used by Pulsar to check Windows paths while running on + a POSIX-like system. + """ + if local_path_module != os.path: + _safe_contains = importlib.import_module('galaxy.util.path.%s' % local_path_module.__name__).safe_contains + else: + _safe_contains = safe_contains + return _safe_contains(directory, file) def merge_sorted_iterables( operator, *iterables ): @@ -1503,22 +1493,6 @@ def download_to_file(url, dest_file_path, timeout=30, chunk_size=2 ** 20): f.write(chunk) -def safe_relpath(path): - """ - Given what we expect to be a relative path, determine whether the path - would exist inside the current directory. - - :type path: string - :param path: a path to check - :rtype: bool - :returns: ``True`` if path is relative and does not reference a path - in a parent directory, ``False`` otherwise. - """ - if path.startswith(os.sep) or normpath(path).startswith(os.pardir): - return False - return True - - class ExecutionTimer(object): def __init__(self): diff --git a/lib/galaxy/util/path/__init__.py b/lib/galaxy/util/path/__init__.py new file mode 100644 index 00000000000..48a8355ad79 --- /dev/null +++ b/lib/galaxy/util/path/__init__.py @@ -0,0 +1,185 @@ +"""Path manipulation functions. +""" +from __future__ import absolute_import + +import errno +import imp +from functools import partial +from itertools import starmap +from operator import getitem +from os import ( + makedirs, + walk, +) +from os.path import ( + abspath, + exists, + isabs, + join, + normpath, + pardir, + realpath, + relpath, +) + +from six import string_types +from six.moves import filterfalse, map, zip + + +def safe_contains(prefix, path, whitelist=None): + """Ensure a path is contained within another path. + + 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 + may also point inside a ``whitelist`` path. + + The ``path`` is checked against ``whitelist`` using either its absolute pathname (if passed in as absolute) or + relative to ``prefix`` and canonicalized (if applicable). It is *not* ``os.path.join()``ed with each ``whitelist`` + directory. + + :type prefix: string + :param prefix: a directory under which ``path`` is to be checked + :type path: string + :param path: a filename to check + :type whitelist: list of strings + :param whitelist: list of additional paths under which ``path`` may be located + :rtype: bool + :returns: ``True`` if ``path`` is contained within ``prefix`` or ``whitelist``, ``False`` otherwise. + """ + return any(__contains(prefix, path, whitelist=whitelist)) + + +def safe_makedirs(path): + """Safely make a directory, do not fail if it already exists or is created during execution. + + :type path: string + :param path: a directory to create + """ + # prechecking for existence is faster than try/except + if not exists(path): + try: + makedirs(path) + except OSError as e: + # reviewing the source for Python 2.7, this would only ever happen for the last path element anyway so no + # need to recurse - this exception means the last part of the path was already in existence. + if e.errno != errno.EEXIST: + raise + + +def safe_relpath(path): + """Determine whether a relative path references a path outside its root. + + This is a path computation: the filesystem is not accessed to confirm the existence or nature of ``path``. + + :type path: string + :param path: a path to check + :rtype: bool + :returns: ``True`` if path is relative and does not reference a path in a parent directory, ``False`` + otherwise. + """ + return not (isabs(path) or normpath(path).startswith(pardir)) + + +def unsafe_walk(path, whitelist=None): + """Walk a path and ensure that none of its contents are symlinks outside the path. + + It is assumed that ``path`` itself has already been validated e.g. with :func:`safe_relpath` or + :func:`safe_contains`. + + :type path: string + :param path: a directory to check for unsafe contents + :type whitelist: list of strings + :param whitelist: list of additional paths under which contents may be located + :rtype: iterator + :returns: Iterator of "bad" files found under ``path`` + """ + return filterfalse(partial(safe_contains, path, whitelist=whitelist), __walk(abspath(path))) + + +def __listify(item): + """A non-splitting version of :func:`galaxy.util.listify`. + """ + if not item: + return [] + elif isinstance(item, list) or isinstance(item, tuple): + return item + else: + return [item] + + +# helpers + + +def __walk(path): + for dirpath, dirnames, filenames in walk(path): + for name in dirnames + filenames: + yield join(dirpath, name) + + +def __contains(prefix, path, whitelist=None): + real = realpath(join(prefix, path)) + yield not relpath(real, prefix).startswith(pardir) + for wldir in whitelist or []: + yield not relpath(real, wldir).startswith(pardir) + + +# cross-platform support + + +def _build_self(target, path_module): + """Populate a module with the same exported functions as this module, but using the given os.path module. + + :type target: module + :param target: module on which to set ``galaxy.util.path`` functions + :type path_module: ``ntpath`` or ``posixpath`` module + :param path_module: module implementing ``os.path`` API to use for path functions + """ + __copy_self().__set_fxns_on(target, path_module) + + +def __copy_self(names=__name__, parent=None): + """Returns a copy of this module that can be modified without modifying `galaxy.util.path`` in ``sys.modules``. + """ + if isinstance(names, string_types): + names = iter(names.split('.')) + try: + name = names.next() + except StopIteration: + return parent + path = parent and parent.__path__ + parent = imp.load_module(name, *imp.find_module(name, path)) + return __copy_self(names, parent) + + +def __set_fxns_on(target, path_module): + """Overrides imported os.path functions with the ones from path_module and populates target with the global + functions from this module. + """ + for name in __pathfxns__: + globals()[name] = getattr(path_module, name) + __get = partial(getitem, globals()) + __set = partial(setattr, target) + # this is actually izip(..., imap(...)) + __fxns = zip(__all__, map(__get, __all__)) + # list() to execute + list(starmap(__set, __fxns)) + + +__pathfxns__ = ( + 'abspath', + 'exists', + 'isabs', + 'join', + 'normpath', + 'pardir', + 'realpath', + 'relpath', +) + +__all__ = ( + 'safe_contains', + 'safe_makedirs', + 'safe_relpath', + 'unsafe_walk', +) diff --git a/lib/galaxy/util/path/ntpath.py b/lib/galaxy/util/path/ntpath.py new file mode 100644 index 00000000000..5363409b3af --- /dev/null +++ b/lib/galaxy/util/path/ntpath.py @@ -0,0 +1,11 @@ +"""Galaxy "safe" path functions forced to work with Windows-style paths regardless of current platform +""" +from __future__ import absolute_import + +import ntpath +import sys + +from . import _build_self + + +_build_self(sys.modules[__name__], ntpath) diff --git a/lib/galaxy/util/path/posixpath.py b/lib/galaxy/util/path/posixpath.py new file mode 100644 index 00000000000..28232649b57 --- /dev/null +++ b/lib/galaxy/util/path/posixpath.py @@ -0,0 +1,11 @@ +"""Galaxy "safe" path functions forced to work with POSIX-style paths regardless of current platform +""" +from __future__ import absolute_import + +import posixpath +import sys + +from . import _build_self + + +_build_self(sys.modules[__name__], posixpath) diff --git a/lib/galaxy/webapps/galaxy/api/lda_datasets.py b/lib/galaxy/webapps/galaxy/api/lda_datasets.py index 14dc75158b7..b7574301577 100644 --- a/lib/galaxy/webapps/galaxy/api/lda_datasets.py +++ b/lib/galaxy/webapps/galaxy/api/lda_datasets.py @@ -19,6 +19,7 @@ from galaxy import web from galaxy.exceptions import ObjectNotFound from galaxy.managers import folders, roles from galaxy.tools.actions import upload_common +from galaxy.util.path import safe_contains, safe_relpath, unsafe_walk from galaxy.util.streamball import StreamBall from galaxy.web import _future_expose_api as expose_api from galaxy.web import _future_expose_api_anonymous as expose_api_anonymous @@ -422,29 +423,45 @@ class LibraryDatasetsController( BaseAPIController, UsesVisualizationMixin ): source = kwd.get( 'source', None ) if source not in [ 'userdir_file', 'userdir_folder', 'importdir_file', 'importdir_folder', 'admin_path' ]: raise exceptions.RequestParameterMissingException( 'You have to specify "source" parameter. Possible values are "userdir_file", "userdir_folder", "admin_path", "importdir_file" and "importdir_folder". ') - if source in [ 'importdir_file', 'importdir_folder' ]: - if not trans.user_is_admin: + elif source in [ 'importdir_file', 'importdir_folder' ]: + if not trans.user_is_admin(): raise exceptions.AdminRequiredException( 'Only admins can import from importdir.' ) if not trans.app.config.library_import_dir: raise exceptions.ConfigDoesNotAllowException( 'The configuration of this Galaxy instance does not allow admins to import into library from importdir.' ) import_base_dir = trans.app.config.library_import_dir + if not safe_relpath(path): + # admins shouldn't be able to explicitly specify a path outside server_dir, but symlinks are allowed. + # the reasoning here is that galaxy admins may not have direct filesystem access or can only access + # library_import_dir via FTP (which cannot create symlinks), and may rely on sysadmins to set up the + # import directory. if they have filesystem access, all bets are off. + raise exceptions.RequestParameterInvalidException( 'The given path is invalid.' ) path = os.path.join( import_base_dir, path ) - if source in [ 'userdir_file', 'userdir_folder' ]: + elif source in [ 'userdir_file', 'userdir_folder' ]: + unsafe = None user_login = trans.user.email user_base_dir = trans.app.config.user_library_import_dir if user_base_dir is None: raise exceptions.ConfigDoesNotAllowException( 'The configuration of this Galaxy instance does not allow upload from user directories.' ) full_dir = os.path.join( user_base_dir, user_login ) - if not path.lower().startswith( full_dir.lower() ): + if not safe_contains( full_dir, path, whitelist=trans.app.config.user_library_import_symlink_whitelist ): + # the path is a symlink outside the user dir path = os.path.join( full_dir, path ) + log.error( 'User attempted to import a path that resolves to a path outside of their import dir: %s -> %s', path, os.path.realpath(path) ) + raise exceptions.RequestParameterInvalidException( 'The given path is invalid.' ) + path = os.path.join( full_dir, path ) + for unsafe in unsafe_walk( path, whitelist=[full_dir] + trans.app.config.user_library_import_symlink_whitelist ): + # the path is a dir and contains files that symlink outside the user dir + log.error( 'User attempted to import a directory containing a path that resolves to a path outside of their import dir: %s -> %s', unsafe, os.path.realpath(unsafe) ) + if unsafe: + raise exceptions.RequestParameterInvalidException( 'The given path is invalid.' ) if not os.path.exists( path ): raise exceptions.RequestParameterInvalidException( 'Given path does not exist on the host.' ) if not self.folder_manager.can_add_item( trans, folder ): raise exceptions.InsufficientPermissionsException( 'You do not have proper permission to add items to the given folder.' ) - if source == 'admin_path': + elif source == 'admin_path': if not trans.app.config.allow_library_path_paste: raise exceptions.ConfigDoesNotAllowException( 'The configuration of this Galaxy instance does not allow admins to import into library from path.' ) - if not trans.user_is_admin: + if not trans.user_is_admin(): raise exceptions.AdminRequiredException( 'Only admins can import from path.' ) # Set up the traditional tool state/params @@ -463,12 +480,12 @@ class LibraryDatasetsController( BaseAPIController, UsesVisualizationMixin ): if source in [ 'importdir_folder' ]: kwd[ 'filesystem_paths' ] = os.path.join( import_base_dir, path ) # user wants to import one file only - if source in [ "userdir_file", "importdir_file" ]: + elif source in [ "userdir_file", "importdir_file" ]: file = os.path.abspath( path ) abspath_datasets.append( trans.webapp.controllers[ 'library_common' ].make_library_uploaded_dataset( trans, 'api', kwd, os.path.basename( file ), file, 'server_dir', library_bunch ) ) # user wants to import whole folder - if source == "userdir_folder": + elif source == "userdir_folder": uploaded_datasets_bunch = trans.webapp.controllers[ 'library_common' ].get_path_paste_uploaded_datasets( trans, 'api', kwd, library_bunch, 200, '' ) uploaded_datasets = uploaded_datasets_bunch[ 0 ] diff --git a/lib/galaxy/webapps/galaxy/api/remote_files.py b/lib/galaxy/webapps/galaxy/api/remote_files.py index 28d42cd45c0..904c186e275 100644 --- a/lib/galaxy/webapps/galaxy/api/remote_files.py +++ b/lib/galaxy/webapps/galaxy/api/remote_files.py @@ -107,7 +107,7 @@ class RemoteFilesAPIController( BaseAPIController ): subfolders and returns a flat list. """ response = [] - if os.path.exists( directory ): + if os.path.exists( directory ) and not os.path.islink( directory ): for ( dirpath, dirnames, filenames ) in os.walk( directory ): for filename in filenames: path = os.path.relpath( os.path.join( dirpath, filename ), directory ) @@ -130,7 +130,7 @@ class RemoteFilesAPIController( BaseAPIController ): """ userdir_jstree = None jstree_paths = [] - if os.path.exists( directory ): + if os.path.exists( directory ) and not os.path.islink( directory ): for ( dirpath, dirnames, filenames ) in os.walk( directory ): for dirname in dirnames: dir_path = os.path.relpath( os.path.join( dirpath, dirname ), directory ) diff --git a/lib/galaxy/webapps/galaxy/controllers/library.py b/lib/galaxy/webapps/galaxy/controllers/library.py index 3df63fa6584..245410bd6e5 100644 --- a/lib/galaxy/webapps/galaxy/controllers/library.py +++ b/lib/galaxy/webapps/galaxy/controllers/library.py @@ -91,6 +91,7 @@ class Library( BaseUIController ): 'app': app } ) @web.expose + @web.require_admin def index( self, trans, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = escape( kwd.get( 'status', 'done' ) ) @@ -101,6 +102,7 @@ class Library( BaseUIController ): status=status ) @web.expose + @web.require_admin def browse_libraries( self, trans, **kwd ): if 'operation' in kwd: operation = kwd['operation'].lower() diff --git a/lib/galaxy/webapps/galaxy/controllers/library_common.py b/lib/galaxy/webapps/galaxy/controllers/library_common.py index 87252bd143b..a05b9c97647 100644 --- a/lib/galaxy/webapps/galaxy/controllers/library_common.py +++ b/lib/galaxy/webapps/galaxy/controllers/library_common.py @@ -22,6 +22,7 @@ from galaxy import util, web from galaxy.security import Action from galaxy.tools.actions import upload_common from galaxy.util import inflector, unicodify, FILENAME_VALID_CHARS +from galaxy.util.path import safe_contains, safe_relpath, unsafe_walk from galaxy.util.streamball import StreamBall from galaxy.web.base.controller import BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMetadataMixin, UsesLibraryMixinItems from galaxy.web.form_builder import AddressField, CheckboxField, SelectField, build_select_field @@ -71,6 +72,7 @@ except: class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMetadataMixin, UsesLibraryMixinItems ): @web.json + @web.require_admin def library_item_updates( self, trans, ids=None, states=None ): # Avoid caching trans.response.headers['Pragma'] = 'no-cache' @@ -94,6 +96,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet return rval @web.expose + @web.require_admin def browse_library( self, trans, cntrller='library', **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -166,6 +169,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=status ) ) @web.expose + @web.require_admin def library_info( self, trans, cntrller, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -225,6 +229,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def library_permissions( self, trans, cntrller, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -272,6 +277,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet 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' ) @@ -347,6 +353,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def folder_info( self, trans, cntrller, id, library_id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -403,6 +410,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def folder_permissions( self, trans, cntrller, id, library_id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -453,6 +461,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def ldda_edit_info( self, trans, cntrller, library_id, folder_id, id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -604,6 +613,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def ldda_info( self, trans, cntrller, library_id, folder_id, id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -653,6 +663,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def ldda_permissions( self, trans, cntrller, library_id, folder_id, id, **kwd ): message = str( escape( kwd.get( 'message', '' ) ) ) status = kwd.get( 'status', 'done' ) @@ -791,6 +802,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet 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' ) @@ -850,6 +862,9 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet elif upload_option == 'upload_paths' and not is_admin: error = True message = 'Uploading files via filesystem paths can only be performed by administrators' + elif upload_option not in ('upload_file', 'upload_directory', 'upload_paths'): + error = True + message = 'Invalid upload_option' elif roles: # Check to see if the user selected roles to associate with the DATASET_ACCESS permission # on the dataset that would cause accessibility issues. @@ -1065,17 +1080,26 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet if upload_option == 'upload_directory': if server_dir in [ None, 'None', '' ]: response_code = 400 - if cntrller == 'library_admin' or ( cntrller == 'api' and trans.user_is_admin ): + if trans.user_is_admin() and cntrller in ( 'library_admin', 'api' ): import_dir = trans.app.config.library_import_dir import_dir_desc = 'library_import_dir' - full_dir = os.path.join( import_dir, server_dir ) else: import_dir = trans.app.config.user_library_import_dir + if server_dir != trans.user.email: + import_dir = os.path.join( import_dir, trans.user.email ) import_dir_desc = 'user_library_import_dir' - if server_dir == trans.user.email: - full_dir = os.path.join( import_dir, server_dir ) - else: - full_dir = os.path.join( import_dir, trans.user.email, server_dir ) + full_dir = os.path.join( import_dir, server_dir ) + unsafe = None + if safe_relpath( server_dir ): + if import_dir_desc == 'user_library_import_dir' and safe_contains( import_dir, full_dir, whitelist=trans.app.config.user_library_import_symlink_whitelist ): + for unsafe in unsafe_walk( full_dir, whitelist=[import_dir] + trans.app.config.user_library_import_symlink_whitelist ): + log.error( 'User attempted to import a path that resolves to a path outside of their import dir: %s -> %s', unsafe, os.path.realpath(unsafe) ) + else: + log.error( 'User attempted to import a directory path that resolves to a path outside of their import dir: %s -> %s', server_dir, os.path.realpath(full_dir) ) + unsafe = True + if unsafe: + response_code = 403 + message = 'Invalid server_dir' if import_dir: message = 'Select a directory' else: @@ -1268,6 +1292,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet return None @web.expose + @web.require_admin def add_history_datasets_to_library( self, trans, cntrller, library_id, folder_id, hda_ids='', **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -1510,6 +1535,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet return upload_option_select_list @web.expose + @web.require_admin def download_dataset_from_folder( self, trans, cntrller, id, library_id=None, **kwd ): """Catches the dataset id and displays file contents as directed""" show_deleted = util.string_as_bool( kwd.get( 'show_deleted', False ) ) @@ -1549,6 +1575,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status='error' ) ) @web.expose + @web.require_admin def library_dataset_info( self, trans, cntrller, id, library_id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -1597,6 +1624,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def library_dataset_permissions( self, trans, cntrller, id, library_id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -1645,6 +1673,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def make_library_item_public( self, trans, cntrller, library_id, item_type, id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -1687,6 +1716,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=status ) ) @web.expose + @web.require_admin def act_on_multiple_datasets( self, trans, cntrller, library_id=None, ldda_ids='', **kwd ): # This method is called from 1 of 3 places: # - this controller's download_dataset_from_folder() method @@ -1979,6 +2009,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def import_datasets_to_histories( self, trans, cntrller, library_id='', folder_id='', ldda_ids='', target_history_id='', target_history_ids='', new_history_name='', **kwd ): # This method is called from one of the following places: # - a menu option for a library dataset ( ldda_ids is a single ldda id ) @@ -2109,6 +2140,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def manage_template_inheritance( self, trans, cntrller, item_type, library_id, folder_id=None, ldda_id=None, **kwd ): show_deleted = util.string_as_bool( kwd.get( 'show_deleted', False ) ) use_panels = util.string_as_bool( kwd.get( 'use_panels', False ) ) @@ -2154,6 +2186,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status='done' ) ) @web.expose + @web.require_admin def move_library_item( self, trans, cntrller, item_type, item_id, source_library_id='', make_target_current=True, **kwd ): # This method is called from one of the following places: # - a menu option for a library dataset ( item_type is 'ldda' and item_id is a single ldda id ) @@ -2371,6 +2404,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def delete_library_item( self, trans, cntrller, library_id, item_id, item_type, **kwd ): # This action will handle deleting all types of library items. State is saved for libraries and # folders ( i.e., if undeleted, the state of contents of the library or folder will remain, so previously @@ -2439,6 +2473,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=status ) ) @web.expose + @web.require_admin def undelete_library_item( self, trans, cntrller, library_id, item_id, item_type, **kwd ): # This action will handle undeleting all types of library items status = kwd.get( 'status', 'done' ) diff --git a/lib/galaxy/webapps/galaxy/controllers/user.py b/lib/galaxy/webapps/galaxy/controllers/user.py index 471b20dee7c..a530bead0f3 100644 --- a/lib/galaxy/webapps/galaxy/controllers/user.py +++ b/lib/galaxy/webapps/galaxy/controllers/user.py @@ -673,7 +673,7 @@ class User( BaseUIController, UsesFormDefinitionsMixin, CreatesUsersMixin, Creat subscribe_checked = CheckboxField.is_checked( subscribe ) referer = trans.request.referer or '' redirect = kwd.get( 'redirect', referer ).strip() - is_admin = cntrller == 'admin' and trans.user_is_admin + is_admin = cntrller == 'admin' and trans.user_is_admin() if not trans.app.config.allow_user_creation and not trans.user_is_admin(): message = 'User registration is disabled. Please contact your local Galaxy administrator for an account.' if trans.app.config.error_email_to is not None: diff --git a/lib/tool_shed/managers/groups.py b/lib/tool_shed/managers/groups.py index a85fde86959..81b069e9c79 100644 --- a/lib/tool_shed/managers/groups.py +++ b/lib/tool_shed/managers/groups.py @@ -56,7 +56,7 @@ class GroupManager( object ): """ Create a new group. """ - if not trans.user_is_admin: + if not trans.user_is_admin(): raise ItemAccessibilityException( 'Only administrators can create groups.' ) else: if self.get( trans, name=name ): From 658f126e7a387c11df9b5f0a01e0b75e4d84410b Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Thu, 19 Oct 2017 09:29:00 -0400 Subject: [PATCH 06/13] [GX-2017-0003]: Fix for the reported issue, only allow http, https, and ftp schemes in the data_source tool. --- tools/data_source/data_source.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/data_source/data_source.py b/tools/data_source/data_source.py index 53121f1376c..f513cec5495 100644 --- a/tools/data_source/data_source.py +++ b/tools/data_source/data_source.py @@ -6,7 +6,7 @@ import socket import sys from json import dumps, loads -from six.moves.urllib.parse import urlencode +from six.moves.urllib.parse import urlencode, urlparse from six.moves.urllib.request import urlopen from galaxy.datatypes import sniff @@ -80,7 +80,7 @@ def __main__(): for data_dict in job_params[ 'output_data' ]: cur_filename = data_dict.get( 'file_name', filename ) cur_URL = params.get( '%s|%s|URL' % ( GALAXY_PARAM_PREFIX, data_dict[ 'out_data_name' ] ), URL ) - if not cur_URL: + if not cur_URL or urlparse(cur_URL).scheme not in ('http', 'https', 'ftp'): open( cur_filename, 'w' ).write( "" ) stop_err( 'The remote data source application has not sent back a URL parameter in the request.' ) From e8a72252effa5fdd888abd0e3462dde3b5c70f6e Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Tue, 12 Sep 2017 15:06:30 -0400 Subject: [PATCH 07/13] Security: Fix issues with path handling in libraries and in general. --- config/galaxy.ini.sample | 8 + lib/galaxy/config.py | 1 + lib/galaxy/managers/folders.py | 4 +- lib/galaxy/managers/libraries.py | 2 +- lib/galaxy/tools/parameters/grouping.py | 2 + lib/galaxy/util/__init__.py | 48 ++--- lib/galaxy/util/path/__init__.py | 185 ++++++++++++++++++ lib/galaxy/util/path/ntpath.py | 11 ++ lib/galaxy/util/path/posixpath.py | 11 ++ lib/galaxy/webapps/galaxy/api/lda_datasets.py | 33 +++- lib/galaxy/webapps/galaxy/api/remote_files.py | 4 +- .../webapps/galaxy/controllers/library.py | 2 + .../galaxy/controllers/library_common.py | 47 ++++- lib/galaxy/webapps/galaxy/controllers/user.py | 2 +- lib/tool_shed/managers/groups.py | 2 +- 15 files changed, 304 insertions(+), 58 deletions(-) create mode 100644 lib/galaxy/util/path/__init__.py create mode 100644 lib/galaxy/util/path/ntpath.py create mode 100644 lib/galaxy/util/path/posixpath.py diff --git a/config/galaxy.ini.sample b/config/galaxy.ini.sample index 3c0ea4283e1..b54b94457fc 100644 --- a/config/galaxy.ini.sample +++ b/config/galaxy.ini.sample @@ -907,6 +907,14 @@ use_interactive = True # sub-directories of files contained in their directory. #user_library_import_dir = None +# For security reasons, users may not import any files that actually lie +# outside of their `user_library_import_dir` (e.g. using symbolic links). A +# list of directories can be allowed by setting the following option (the list +# is comma-separated). Be aware that *any* user with library import permissions +# can import from anywhere in these directories (assuming they are able to +# create symlinks to them). +#user_library_import_symlink_whitelist = None + # Add an option to the admin library upload tool allowing admins to paste # filesystem paths to files and directories in a box, and these paths will be # added to a library. Set to True to enable. Please note the security diff --git a/lib/galaxy/config.py b/lib/galaxy/config.py index ab91f0d64af..94d5e456a38 100644 --- a/lib/galaxy/config.py +++ b/lib/galaxy/config.py @@ -377,6 +377,7 @@ class Configuration( object ): self.screencasts_url = kwargs.get( 'screencasts_url', None ) self.library_import_dir = kwargs.get( 'library_import_dir', None ) self.user_library_import_dir = kwargs.get( 'user_library_import_dir', None ) + self.user_library_import_symlink_whitelist = listify( kwargs.get( 'user_library_import_symlink_whitelist', []), do_strip=True ) # Searching data libraries self.enable_lucene_library_search = string_as_bool( kwargs.get( 'enable_lucene_library_search', False ) ) self.enable_whoosh_library_search = string_as_bool( kwargs.get( 'enable_whoosh_library_search', False ) ) diff --git a/lib/galaxy/managers/folders.py b/lib/galaxy/managers/folders.py index cbb4ad3e9b7..b835ae76e50 100644 --- a/lib/galaxy/managers/folders.py +++ b/lib/galaxy/managers/folders.py @@ -132,7 +132,7 @@ class FolderManager( object ): """ parent_folder = self.get( trans, parent_folder_id ) current_user_roles = trans.get_current_user_roles() - if not ( trans.user_is_admin or trans.app.security_agent.can_add_library_item( current_user_roles, parent_folder ) ): + if not ( trans.user_is_admin() or trans.app.security_agent.can_add_library_item( current_user_roles, parent_folder ) ): raise InsufficientPermissionsException( 'You do not have proper permission to create folders under given folder.' ) new_folder = trans.app.model.LibraryFolder( name=new_folder_name, description=new_folder_description ) # We are associating the last used genome build with folders, so we will always @@ -230,7 +230,7 @@ class FolderManager( object ): """ Return true if the user has permissions to add item to the given folder. """ - if trans.user_is_admin: + if trans.user_is_admin(): return True current_user_roles = trans.get_current_user_roles() add_roles = set( trans.app.security_agent.get_roles_for_action( folder, trans.app.security_agent.permitted_actions.LIBRARY_ADD ) ) diff --git a/lib/galaxy/managers/libraries.py b/lib/galaxy/managers/libraries.py index 9bcb4d82fd6..337cb3ac807 100644 --- a/lib/galaxy/managers/libraries.py +++ b/lib/galaxy/managers/libraries.py @@ -50,7 +50,7 @@ class LibraryManager( object ): """ Create a new library. """ - if not trans.user_is_admin: + if not trans.user_is_admin(): raise exceptions.ItemAccessibilityException( 'Only administrators can create libraries.' ) else: library = trans.app.model.Library( name=name, description=description, synopsis=synopsis ) diff --git a/lib/galaxy/tools/parameters/grouping.py b/lib/galaxy/tools/parameters/grouping.py index f83d6649050..b14b440f8fb 100644 --- a/lib/galaxy/tools/parameters/grouping.py +++ b/lib/galaxy/tools/parameters/grouping.py @@ -360,6 +360,7 @@ class UploadDataset( Group ): warnings.append( "All FTP uploaded file selections were ignored." ) elif ftp_files is not None and trans.user is not None: # look for files uploaded via FTP user_ftp_dir = trans.user_ftp_dir + assert not os.path.islink(user_ftp_dir), "User FTP directory cannot be a symbolic link" for ( dirpath, dirnames, filenames ) in os.walk( user_ftp_dir ): for filename in filenames: for ftp_filename in ftp_files: @@ -423,6 +424,7 @@ class UploadDataset( Group ): # TODO: warning to the user (could happen if session has become invalid) else: user_ftp_dir = trans.user_ftp_dir + assert not os.path.islink(user_ftp_dir), "User FTP directory cannot be a symbolic link" for ( dirpath, dirnames, filenames ) in os.walk( user_ftp_dir ): for filename in filenames: path = relpath( os.path.join( dirpath, filename ), user_ftp_dir ) diff --git a/lib/galaxy/util/__init__.py b/lib/galaxy/util/__init__.py index 5f8881a8872..061b32f62fa 100644 --- a/lib/galaxy/util/__init__.py +++ b/lib/galaxy/util/__init__.py @@ -8,6 +8,7 @@ from __future__ import absolute_import import binascii import collections import errno +import importlib import json import logging import os @@ -29,7 +30,7 @@ except ImportError: from datetime import datetime from hashlib import md5 -from os.path import normpath, relpath +from os.path import relpath from xml.etree import ElementInclude, ElementTree from xml.etree.ElementTree import ParseError @@ -49,6 +50,7 @@ except ImportError: docutils_html4css1 = None from .inflection import English, Inflector +from .path import safe_contains, safe_makedirs, safe_relpath # noqa: F401 inflector = Inflector(English) @@ -604,31 +606,19 @@ def which(file): return None -def safe_makedirs(path): - """ Safely make a directory, do not fail if it already exist or - is created during execution. - """ - if not os.path.exists(path): - try: - os.makedirs(path) - except OSError as e: - # review source for Python 2.7 this would only ever happen - # for the last path anyway so need to recurse - this exception - # means the last part of the path was already in existence. - if e.errno != errno.EEXIST: - raise - - 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 - """ - # Make both absolute. - directory = local_path_module.abspath(directory) - file = local_path_module.abspath(file) - return local_path_module.commonprefix([file, directory]) == directory + local_path_module is used by Pulsar to check Windows paths while running on + a POSIX-like system. + """ + if local_path_module != os.path: + _safe_contains = importlib.import_module('galaxy.util.path.%s' % local_path_module.__name__).safe_contains + else: + _safe_contains = safe_contains + return _safe_contains(directory, file) def merge_sorted_iterables( operator, *iterables ): @@ -1503,22 +1493,6 @@ def download_to_file(url, dest_file_path, timeout=30, chunk_size=2 ** 20): f.write(chunk) -def safe_relpath(path): - """ - Given what we expect to be a relative path, determine whether the path - would exist inside the current directory. - - :type path: string - :param path: a path to check - :rtype: bool - :returns: ``True`` if path is relative and does not reference a path - in a parent directory, ``False`` otherwise. - """ - if path.startswith(os.sep) or normpath(path).startswith(os.pardir): - return False - return True - - class ExecutionTimer(object): def __init__(self): diff --git a/lib/galaxy/util/path/__init__.py b/lib/galaxy/util/path/__init__.py new file mode 100644 index 00000000000..48a8355ad79 --- /dev/null +++ b/lib/galaxy/util/path/__init__.py @@ -0,0 +1,185 @@ +"""Path manipulation functions. +""" +from __future__ import absolute_import + +import errno +import imp +from functools import partial +from itertools import starmap +from operator import getitem +from os import ( + makedirs, + walk, +) +from os.path import ( + abspath, + exists, + isabs, + join, + normpath, + pardir, + realpath, + relpath, +) + +from six import string_types +from six.moves import filterfalse, map, zip + + +def safe_contains(prefix, path, whitelist=None): + """Ensure a path is contained within another path. + + 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 + may also point inside a ``whitelist`` path. + + The ``path`` is checked against ``whitelist`` using either its absolute pathname (if passed in as absolute) or + relative to ``prefix`` and canonicalized (if applicable). It is *not* ``os.path.join()``ed with each ``whitelist`` + directory. + + :type prefix: string + :param prefix: a directory under which ``path`` is to be checked + :type path: string + :param path: a filename to check + :type whitelist: list of strings + :param whitelist: list of additional paths under which ``path`` may be located + :rtype: bool + :returns: ``True`` if ``path`` is contained within ``prefix`` or ``whitelist``, ``False`` otherwise. + """ + return any(__contains(prefix, path, whitelist=whitelist)) + + +def safe_makedirs(path): + """Safely make a directory, do not fail if it already exists or is created during execution. + + :type path: string + :param path: a directory to create + """ + # prechecking for existence is faster than try/except + if not exists(path): + try: + makedirs(path) + except OSError as e: + # reviewing the source for Python 2.7, this would only ever happen for the last path element anyway so no + # need to recurse - this exception means the last part of the path was already in existence. + if e.errno != errno.EEXIST: + raise + + +def safe_relpath(path): + """Determine whether a relative path references a path outside its root. + + This is a path computation: the filesystem is not accessed to confirm the existence or nature of ``path``. + + :type path: string + :param path: a path to check + :rtype: bool + :returns: ``True`` if path is relative and does not reference a path in a parent directory, ``False`` + otherwise. + """ + return not (isabs(path) or normpath(path).startswith(pardir)) + + +def unsafe_walk(path, whitelist=None): + """Walk a path and ensure that none of its contents are symlinks outside the path. + + It is assumed that ``path`` itself has already been validated e.g. with :func:`safe_relpath` or + :func:`safe_contains`. + + :type path: string + :param path: a directory to check for unsafe contents + :type whitelist: list of strings + :param whitelist: list of additional paths under which contents may be located + :rtype: iterator + :returns: Iterator of "bad" files found under ``path`` + """ + return filterfalse(partial(safe_contains, path, whitelist=whitelist), __walk(abspath(path))) + + +def __listify(item): + """A non-splitting version of :func:`galaxy.util.listify`. + """ + if not item: + return [] + elif isinstance(item, list) or isinstance(item, tuple): + return item + else: + return [item] + + +# helpers + + +def __walk(path): + for dirpath, dirnames, filenames in walk(path): + for name in dirnames + filenames: + yield join(dirpath, name) + + +def __contains(prefix, path, whitelist=None): + real = realpath(join(prefix, path)) + yield not relpath(real, prefix).startswith(pardir) + for wldir in whitelist or []: + yield not relpath(real, wldir).startswith(pardir) + + +# cross-platform support + + +def _build_self(target, path_module): + """Populate a module with the same exported functions as this module, but using the given os.path module. + + :type target: module + :param target: module on which to set ``galaxy.util.path`` functions + :type path_module: ``ntpath`` or ``posixpath`` module + :param path_module: module implementing ``os.path`` API to use for path functions + """ + __copy_self().__set_fxns_on(target, path_module) + + +def __copy_self(names=__name__, parent=None): + """Returns a copy of this module that can be modified without modifying `galaxy.util.path`` in ``sys.modules``. + """ + if isinstance(names, string_types): + names = iter(names.split('.')) + try: + name = names.next() + except StopIteration: + return parent + path = parent and parent.__path__ + parent = imp.load_module(name, *imp.find_module(name, path)) + return __copy_self(names, parent) + + +def __set_fxns_on(target, path_module): + """Overrides imported os.path functions with the ones from path_module and populates target with the global + functions from this module. + """ + for name in __pathfxns__: + globals()[name] = getattr(path_module, name) + __get = partial(getitem, globals()) + __set = partial(setattr, target) + # this is actually izip(..., imap(...)) + __fxns = zip(__all__, map(__get, __all__)) + # list() to execute + list(starmap(__set, __fxns)) + + +__pathfxns__ = ( + 'abspath', + 'exists', + 'isabs', + 'join', + 'normpath', + 'pardir', + 'realpath', + 'relpath', +) + +__all__ = ( + 'safe_contains', + 'safe_makedirs', + 'safe_relpath', + 'unsafe_walk', +) diff --git a/lib/galaxy/util/path/ntpath.py b/lib/galaxy/util/path/ntpath.py new file mode 100644 index 00000000000..5363409b3af --- /dev/null +++ b/lib/galaxy/util/path/ntpath.py @@ -0,0 +1,11 @@ +"""Galaxy "safe" path functions forced to work with Windows-style paths regardless of current platform +""" +from __future__ import absolute_import + +import ntpath +import sys + +from . import _build_self + + +_build_self(sys.modules[__name__], ntpath) diff --git a/lib/galaxy/util/path/posixpath.py b/lib/galaxy/util/path/posixpath.py new file mode 100644 index 00000000000..28232649b57 --- /dev/null +++ b/lib/galaxy/util/path/posixpath.py @@ -0,0 +1,11 @@ +"""Galaxy "safe" path functions forced to work with POSIX-style paths regardless of current platform +""" +from __future__ import absolute_import + +import posixpath +import sys + +from . import _build_self + + +_build_self(sys.modules[__name__], posixpath) diff --git a/lib/galaxy/webapps/galaxy/api/lda_datasets.py b/lib/galaxy/webapps/galaxy/api/lda_datasets.py index 384baeac819..5a2b499467b 100644 --- a/lib/galaxy/webapps/galaxy/api/lda_datasets.py +++ b/lib/galaxy/webapps/galaxy/api/lda_datasets.py @@ -19,6 +19,7 @@ from galaxy.exceptions import ObjectNotFound from galaxy.managers import folders, roles from galaxy.tools.actions import upload_common from galaxy.tools.parameters import populate_state +from galaxy.util.path import safe_contains, safe_relpath, unsafe_walk from galaxy.util.streamball import StreamBall from galaxy.web import _future_expose_api as expose_api from galaxy.web import _future_expose_api_anonymous as expose_api_anonymous @@ -422,29 +423,45 @@ class LibraryDatasetsController( BaseAPIController, UsesVisualizationMixin ): source = kwd.get( 'source', None ) if source not in [ 'userdir_file', 'userdir_folder', 'importdir_file', 'importdir_folder', 'admin_path' ]: raise exceptions.RequestParameterMissingException( 'You have to specify "source" parameter. Possible values are "userdir_file", "userdir_folder", "admin_path", "importdir_file" and "importdir_folder". ') - if source in [ 'importdir_file', 'importdir_folder' ]: - if not trans.user_is_admin: + elif source in [ 'importdir_file', 'importdir_folder' ]: + if not trans.user_is_admin(): raise exceptions.AdminRequiredException( 'Only admins can import from importdir.' ) if not trans.app.config.library_import_dir: raise exceptions.ConfigDoesNotAllowException( 'The configuration of this Galaxy instance does not allow admins to import into library from importdir.' ) import_base_dir = trans.app.config.library_import_dir + if not safe_relpath(path): + # admins shouldn't be able to explicitly specify a path outside server_dir, but symlinks are allowed. + # the reasoning here is that galaxy admins may not have direct filesystem access or can only access + # library_import_dir via FTP (which cannot create symlinks), and may rely on sysadmins to set up the + # import directory. if they have filesystem access, all bets are off. + raise exceptions.RequestParameterInvalidException( 'The given path is invalid.' ) path = os.path.join( import_base_dir, path ) - if source in [ 'userdir_file', 'userdir_folder' ]: + elif source in [ 'userdir_file', 'userdir_folder' ]: + unsafe = None user_login = trans.user.email user_base_dir = trans.app.config.user_library_import_dir if user_base_dir is None: raise exceptions.ConfigDoesNotAllowException( 'The configuration of this Galaxy instance does not allow upload from user directories.' ) full_dir = os.path.join( user_base_dir, user_login ) - if not path.lower().startswith( full_dir.lower() ): + if not safe_contains( full_dir, path, whitelist=trans.app.config.user_library_import_symlink_whitelist ): + # the path is a symlink outside the user dir path = os.path.join( full_dir, path ) + log.error( 'User attempted to import a path that resolves to a path outside of their import dir: %s -> %s', path, os.path.realpath(path) ) + raise exceptions.RequestParameterInvalidException( 'The given path is invalid.' ) + path = os.path.join( full_dir, path ) + for unsafe in unsafe_walk( path, whitelist=[full_dir] + trans.app.config.user_library_import_symlink_whitelist ): + # the path is a dir and contains files that symlink outside the user dir + log.error( 'User attempted to import a directory containing a path that resolves to a path outside of their import dir: %s -> %s', unsafe, os.path.realpath(unsafe) ) + if unsafe: + raise exceptions.RequestParameterInvalidException( 'The given path is invalid.' ) if not os.path.exists( path ): raise exceptions.RequestParameterInvalidException( 'Given path does not exist on the host.' ) if not self.folder_manager.can_add_item( trans, folder ): raise exceptions.InsufficientPermissionsException( 'You do not have proper permission to add items to the given folder.' ) - if source == 'admin_path': + elif source == 'admin_path': if not trans.app.config.allow_library_path_paste: raise exceptions.ConfigDoesNotAllowException( 'The configuration of this Galaxy instance does not allow admins to import into library from path.' ) - if not trans.user_is_admin: + if not trans.user_is_admin(): raise exceptions.AdminRequiredException( 'Only admins can import from path.' ) # Set up the traditional tool state/params @@ -463,12 +480,12 @@ class LibraryDatasetsController( BaseAPIController, UsesVisualizationMixin ): if source in [ 'importdir_folder' ]: kwd[ 'filesystem_paths' ] = os.path.join( import_base_dir, path ) # user wants to import one file only - if source in [ "userdir_file", "importdir_file" ]: + elif source in [ "userdir_file", "importdir_file" ]: file = os.path.abspath( path ) abspath_datasets.append( trans.webapp.controllers[ 'library_common' ].make_library_uploaded_dataset( trans, 'api', kwd, os.path.basename( file ), file, 'server_dir', library_bunch ) ) # user wants to import whole folder - if source == "userdir_folder": + elif source == "userdir_folder": uploaded_datasets_bunch = trans.webapp.controllers[ 'library_common' ].get_path_paste_uploaded_datasets( trans, 'api', kwd, library_bunch, 200, '' ) uploaded_datasets = uploaded_datasets_bunch[ 0 ] diff --git a/lib/galaxy/webapps/galaxy/api/remote_files.py b/lib/galaxy/webapps/galaxy/api/remote_files.py index 28d42cd45c0..904c186e275 100644 --- a/lib/galaxy/webapps/galaxy/api/remote_files.py +++ b/lib/galaxy/webapps/galaxy/api/remote_files.py @@ -107,7 +107,7 @@ class RemoteFilesAPIController( BaseAPIController ): subfolders and returns a flat list. """ response = [] - if os.path.exists( directory ): + if os.path.exists( directory ) and not os.path.islink( directory ): for ( dirpath, dirnames, filenames ) in os.walk( directory ): for filename in filenames: path = os.path.relpath( os.path.join( dirpath, filename ), directory ) @@ -130,7 +130,7 @@ class RemoteFilesAPIController( BaseAPIController ): """ userdir_jstree = None jstree_paths = [] - if os.path.exists( directory ): + if os.path.exists( directory ) and not os.path.islink( directory ): for ( dirpath, dirnames, filenames ) in os.walk( directory ): for dirname in dirnames: dir_path = os.path.relpath( os.path.join( dirpath, dirname ), directory ) diff --git a/lib/galaxy/webapps/galaxy/controllers/library.py b/lib/galaxy/webapps/galaxy/controllers/library.py index 3df63fa6584..245410bd6e5 100644 --- a/lib/galaxy/webapps/galaxy/controllers/library.py +++ b/lib/galaxy/webapps/galaxy/controllers/library.py @@ -91,6 +91,7 @@ class Library( BaseUIController ): 'app': app } ) @web.expose + @web.require_admin def index( self, trans, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = escape( kwd.get( 'status', 'done' ) ) @@ -101,6 +102,7 @@ class Library( BaseUIController ): status=status ) @web.expose + @web.require_admin def browse_libraries( self, trans, **kwd ): if 'operation' in kwd: operation = kwd['operation'].lower() diff --git a/lib/galaxy/webapps/galaxy/controllers/library_common.py b/lib/galaxy/webapps/galaxy/controllers/library_common.py index 9416aa37a4f..5ac56968e39 100644 --- a/lib/galaxy/webapps/galaxy/controllers/library_common.py +++ b/lib/galaxy/webapps/galaxy/controllers/library_common.py @@ -23,6 +23,7 @@ from galaxy.security import Action from galaxy.tools.actions import upload_common from galaxy.tools.parameters import populate_state from galaxy.util import inflector, unicodify, FILENAME_VALID_CHARS +from galaxy.util.path import safe_contains, safe_relpath, unsafe_walk from galaxy.util.streamball import StreamBall from galaxy.web.base.controller import BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMetadataMixin, UsesLibraryMixinItems from galaxy.web.form_builder import AddressField, CheckboxField, SelectField, build_select_field @@ -72,6 +73,7 @@ except: class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMetadataMixin, UsesLibraryMixinItems ): @web.json + @web.require_admin def library_item_updates( self, trans, ids=None, states=None ): # Avoid caching trans.response.headers['Pragma'] = 'no-cache' @@ -95,6 +97,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet return rval @web.expose + @web.require_admin def browse_library( self, trans, cntrller='library', **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -167,6 +170,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=status ) ) @web.expose + @web.require_admin def library_info( self, trans, cntrller, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -226,6 +230,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def library_permissions( self, trans, cntrller, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -273,6 +278,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet 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' ) @@ -348,6 +354,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def folder_info( self, trans, cntrller, id, library_id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -404,6 +411,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def folder_permissions( self, trans, cntrller, id, library_id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -454,6 +462,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def ldda_edit_info( self, trans, cntrller, library_id, folder_id, id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -605,6 +614,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def ldda_info( self, trans, cntrller, library_id, folder_id, id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -654,6 +664,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def ldda_permissions( self, trans, cntrller, library_id, folder_id, id, **kwd ): message = str( escape( kwd.get( 'message', '' ) ) ) status = kwd.get( 'status', 'done' ) @@ -792,6 +803,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet 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' ) @@ -851,6 +863,9 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet elif upload_option == 'upload_paths' and not is_admin: error = True message = 'Uploading files via filesystem paths can only be performed by administrators' + elif upload_option not in ('upload_file', 'upload_directory', 'upload_paths'): + error = True + message = 'Invalid upload_option' elif roles: # Check to see if the user selected roles to associate with the DATASET_ACCESS permission # on the dataset that would cause accessibility issues. @@ -1066,17 +1081,26 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet if upload_option == 'upload_directory': if server_dir in [ None, 'None', '' ]: response_code = 400 - if cntrller == 'library_admin' or ( cntrller == 'api' and trans.user_is_admin ): + if trans.user_is_admin() and cntrller in ( 'library_admin', 'api' ): import_dir = trans.app.config.library_import_dir import_dir_desc = 'library_import_dir' - full_dir = os.path.join( import_dir, server_dir ) else: import_dir = trans.app.config.user_library_import_dir + if server_dir != trans.user.email: + import_dir = os.path.join( import_dir, trans.user.email ) import_dir_desc = 'user_library_import_dir' - if server_dir == trans.user.email: - full_dir = os.path.join( import_dir, server_dir ) - else: - full_dir = os.path.join( import_dir, trans.user.email, server_dir ) + full_dir = os.path.join( import_dir, server_dir ) + unsafe = None + if safe_relpath( server_dir ): + if import_dir_desc == 'user_library_import_dir' and safe_contains( import_dir, full_dir, whitelist=trans.app.config.user_library_import_symlink_whitelist ): + for unsafe in unsafe_walk( full_dir, whitelist=[import_dir] + trans.app.config.user_library_import_symlink_whitelist ): + log.error( 'User attempted to import a path that resolves to a path outside of their import dir: %s -> %s', unsafe, os.path.realpath(unsafe) ) + else: + log.error( 'User attempted to import a directory path that resolves to a path outside of their import dir: %s -> %s', server_dir, os.path.realpath(full_dir) ) + unsafe = True + if unsafe: + response_code = 403 + message = 'Invalid server_dir' if import_dir: message = 'Select a directory' else: @@ -1269,6 +1293,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet return None @web.expose + @web.require_admin def add_history_datasets_to_library( self, trans, cntrller, library_id, folder_id, hda_ids='', **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -1511,6 +1536,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet return upload_option_select_list @web.expose + @web.require_admin def download_dataset_from_folder( self, trans, cntrller, id, library_id=None, **kwd ): """Catches the dataset id and displays file contents as directed""" show_deleted = util.string_as_bool( kwd.get( 'show_deleted', False ) ) @@ -1550,6 +1576,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status='error' ) ) @web.expose + @web.require_admin def library_dataset_info( self, trans, cntrller, id, library_id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -1598,6 +1625,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def library_dataset_permissions( self, trans, cntrller, id, library_id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -1646,6 +1674,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def make_library_item_public( self, trans, cntrller, library_id, item_type, id, **kwd ): message = escape( kwd.get( 'message', '' ) ) status = kwd.get( 'status', 'done' ) @@ -1688,6 +1717,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=status ) ) @web.expose + @web.require_admin def act_on_multiple_datasets( self, trans, cntrller, library_id=None, ldda_ids='', **kwd ): # This method is called from 1 of 3 places: # - this controller's download_dataset_from_folder() method @@ -1980,6 +2010,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def import_datasets_to_histories( self, trans, cntrller, library_id='', folder_id='', ldda_ids='', target_history_id='', target_history_ids='', new_history_name='', **kwd ): # This method is called from one of the following places: # - a menu option for a library dataset ( ldda_ids is a single ldda id ) @@ -2110,6 +2141,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def manage_template_inheritance( self, trans, cntrller, item_type, library_id, folder_id=None, ldda_id=None, **kwd ): show_deleted = util.string_as_bool( kwd.get( 'show_deleted', False ) ) use_panels = util.string_as_bool( kwd.get( 'use_panels', False ) ) @@ -2155,6 +2187,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status='done' ) ) @web.expose + @web.require_admin def move_library_item( self, trans, cntrller, item_type, item_id, source_library_id='', make_target_current=True, **kwd ): # This method is called from one of the following places: # - a menu option for a library dataset ( item_type is 'ldda' and item_id is a single ldda id ) @@ -2372,6 +2405,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=escape( status ) ) @web.expose + @web.require_admin def delete_library_item( self, trans, cntrller, library_id, item_id, item_type, **kwd ): # This action will handle deleting all types of library items. State is saved for libraries and # folders ( i.e., if undeleted, the state of contents of the library or folder will remain, so previously @@ -2440,6 +2474,7 @@ class LibraryCommon( BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMet status=status ) ) @web.expose + @web.require_admin def undelete_library_item( self, trans, cntrller, library_id, item_id, item_type, **kwd ): # This action will handle undeleting all types of library items status = kwd.get( 'status', 'done' ) diff --git a/lib/galaxy/webapps/galaxy/controllers/user.py b/lib/galaxy/webapps/galaxy/controllers/user.py index 9310661c3fa..d5434b6e0bc 100644 --- a/lib/galaxy/webapps/galaxy/controllers/user.py +++ b/lib/galaxy/webapps/galaxy/controllers/user.py @@ -669,7 +669,7 @@ class User( BaseUIController, UsesFormDefinitionsMixin, CreatesUsersMixin, Creat subscribe_checked = CheckboxField.is_checked( subscribe ) referer = trans.request.referer or '' redirect = kwd.get( 'redirect', referer ).strip() - is_admin = cntrller == 'admin' and trans.user_is_admin + is_admin = cntrller == 'admin' and trans.user_is_admin() if not trans.app.config.allow_user_creation and not trans.user_is_admin(): message = 'User registration is disabled. Please contact your local Galaxy administrator for an account.' if trans.app.config.error_email_to is not None: diff --git a/lib/tool_shed/managers/groups.py b/lib/tool_shed/managers/groups.py index a85fde86959..81b069e9c79 100644 --- a/lib/tool_shed/managers/groups.py +++ b/lib/tool_shed/managers/groups.py @@ -56,7 +56,7 @@ class GroupManager( object ): """ Create a new group. """ - if not trans.user_is_admin: + if not trans.user_is_admin(): raise ItemAccessibilityException( 'Only administrators can create groups.' ) else: if self.get( trans, name=name ): From 9f8d3ee444ad10038add204ed1c1dc11e636dd9d Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Fri, 29 Sep 2017 14:00:46 -0400 Subject: [PATCH 08/13] Security: Switch GIE Popen() calls to run without shell=True to fix an ACE vulnerability. --- .../web/base/interactive_environments.py | 68 ++++++++++--------- 1 file changed, 35 insertions(+), 33 deletions(-) diff --git a/lib/galaxy/web/base/interactive_environments.py b/lib/galaxy/web/base/interactive_environments.py index ca29bd9e043..78804aebd20 100644 --- a/lib/galaxy/web/base/interactive_environments.py +++ b/lib/galaxy/web/base/interactive_environments.py @@ -3,14 +3,17 @@ import json import logging import os import random +import shlex import stat import tempfile import uuid +from itertools import product from subprocess import PIPE, Popen from sys import platform as _platform import yaml +from six.moves import shlex_quote from galaxy import model, web from galaxy.containers import build_container_interfaces @@ -126,7 +129,7 @@ class InteractiveEnvironmentRequest(object): # their defaults dictionary instead. default_dict = { 'container_interface': None, - 'command': 'docker {docker_args}', + 'command': 'docker', 'command_inject': '-e DEBUG=false -e DEFAULT_CONTAINER_RUNTIME=120', 'docker_hostname': 'localhost', 'wx_tempdir': 'False', @@ -263,40 +266,40 @@ class InteractiveEnvironmentRequest(object): def _get_name_for_run(self): return CONTAINER_NAME_PREFIX + uuid.uuid4().hex + def base_docker_cmd(self, subcmd=None): + # This is the basic docker command such as "sudo -u docker docker" or just "docker" + # Previously, {docker_args} was required to be in the string, this is no longer the case + base = shlex.split(self.attr.viz_config.get("docker", "command").format(docker_args='').strip()) + if subcmd: + base.append(subcmd) + return base + def docker_cmd(self, image, env_override=None, volumes=None): """ Generate and return the docker command to execute """ - if volumes is None: - volumes = [] - env = self._get_env_for_run(env_override) - import_volume_def = self._get_import_volume_for_run() - env_str = ' '.join(['-e "%s=%s"' % (key, item) for key, item in env.items()]) - volume_str = ' '.join(['-v "%s"' % volume for volume in volumes]) if self.use_volumes else '' - import_volume_str = '-v "{import_volume}"'.format(import_volume=import_volume_def) if import_volume_def else '' - name = None - # This is the basic docker command such as "sudo -u docker docker {docker_args}" - # or just "docker {docker_args}" - command = self.attr.viz_config.get("docker", "command") - # Then we format in the entire docker command in place of - # {docker_args}, so as to let the admin not worry about which args are - # getting passed + def _flag_opts(flag, opts): + return [arg for pair in product((flag,), opts) for arg in pair] + command_inject = self.attr.viz_config.get("docker", "command_inject") # --name should really not be set, but we'll try to honor it anyway - if '--name' not in command_inject: - name = self._get_name_for_run() - command = command.format(docker_args='run {command_inject} {name} {environment} -d -P {import_volume_str} {volume_str} {image}') + name = ['--name=%s' % self._get_name_for_run()] if '--name' not in command_inject else [] + env = self._get_env_for_run(env_override) + import_volume_def = self._get_import_volume_for_run() + if volumes is None: + volumes = [] + if import_volume_def: + volumes.insert(0, import_volume_def) - # Once that's available, we format again with all of our arguments - command = command.format( - command_inject=command_inject, - name='--name=%s' % name if name is not None else '', - environment=env_str, - import_volume_str=import_volume_str, - volume_str=volume_str, - image=image, + return ( + self.base_docker_cmd('run') + + shlex.split(command_inject) + + name + + _flag_opts('-e', ['='.join(map(str, t)) for t in env.items()]) + + ['-d', '-P'] + + _flag_opts('-v', map(str, volumes)) + + [image] ) - return command @property def use_volumes(self): @@ -368,9 +371,9 @@ class InteractiveEnvironmentRequest(object): log.info("Starting docker container for IE {0} with command [{1}]".format( self.attr.viz_id, - raw_cmd + ' '.join([shlex_quote(x) for x in raw_cmd]) )) - p = Popen( raw_cmd, stdout=PIPE, stderr=PIPE, close_fds=True, shell=True) + p = Popen( raw_cmd, stdout=PIPE, stderr=PIPE, close_fds=True) stdout, stderr = p.communicate() if p.returncode != 0: log.error( "%s\n%s" % (stdout, stderr) ) @@ -474,14 +477,13 @@ class InteractiveEnvironmentRequest(object): :returns: inspect_data, a dict of docker inspect output """ - command = self.attr.viz_config.get("docker", "command") - command = command.format(docker_args="inspect %s" % container_id) + raw_cmd = self.base_docker_cmd('inspect') + [container_id] log.info("Inspecting docker container {0} with command [{1}]".format( container_id, - command + ' '.join([shlex_quote(x) for x in raw_cmd]) )) - p = Popen(command, stdout=PIPE, stderr=PIPE, close_fds=True, shell=True) + p = Popen(raw_cmd, stdout=PIPE, stderr=PIPE, close_fds=True) stdout, stderr = p.communicate() if p.returncode != 0: log.error( "%s\n%s" % (stdout, stderr) ) From cded49354e5fba4b93432294a4518c93b51f259f Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Thu, 19 Oct 2017 09:29:00 -0400 Subject: [PATCH 09/13] [GX-2017-0003]: Fix for the reported issue, only allow http, https, and ftp schemes in the data_source tool. --- tools/data_source/data_source.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/data_source/data_source.py b/tools/data_source/data_source.py index 53121f1376c..f513cec5495 100644 --- a/tools/data_source/data_source.py +++ b/tools/data_source/data_source.py @@ -6,7 +6,7 @@ import socket import sys from json import dumps, loads -from six.moves.urllib.parse import urlencode +from six.moves.urllib.parse import urlencode, urlparse from six.moves.urllib.request import urlopen from galaxy.datatypes import sniff @@ -80,7 +80,7 @@ def __main__(): for data_dict in job_params[ 'output_data' ]: cur_filename = data_dict.get( 'file_name', filename ) cur_URL = params.get( '%s|%s|URL' % ( GALAXY_PARAM_PREFIX, data_dict[ 'out_data_name' ] ), URL ) - if not cur_URL: + if not cur_URL or urlparse(cur_URL).scheme not in ('http', 'https', 'ftp'): open( cur_filename, 'w' ).write( "" ) stop_err( 'The remote data source application has not sent back a URL parameter in the request.' ) From 646a8d1092541c797238e540d071c4346cc5885b Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Fri, 20 Oct 2017 12:43:09 -0400 Subject: [PATCH 10/13] Support a log directory argument for pgcleanup.py --- scripts/cleanup_datasets/pgcleanup.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/cleanup_datasets/pgcleanup.py b/scripts/cleanup_datasets/pgcleanup.py index 2d50a94c817..ef17e66e3df 100755 --- a/scripts/cleanup_datasets/pgcleanup.py +++ b/scripts/cleanup_datasets/pgcleanup.py @@ -72,6 +72,7 @@ class Cleanup(object): parser.add_option('-U', '--no-update-time', action='store_false', dest='update_time', help="Don't set update_time on updated objects", default=True) parser.add_option('-s', '--sequence', dest='sequence', help='Comma-separated sequence of actions, chosen from: %s' % self.action_names, default='') parser.add_option('-w', '--work-mem', dest='work_mem', help='Set PostgreSQL work_mem for this connection', default=None) + parser.add_option('-l', '--log-dir', dest='log_dir', help='Log file directory', default=os.path.join(galaxy_root, 'scripts', 'cleanup_datasets')) (self.options, self.args) = parser.parse_args() self.options.sequence = [x.strip() for x in self.options.sequence.split(',')] @@ -117,7 +118,7 @@ class Cleanup(object): def _open_logfile(self): action_name = inspect.stack()[1][3] - logname = os.path.join(galaxy_root, 'scripts', 'cleanup_datasets', action_name + '.log') + logname = os.path.join(self.options.log_dir, action_name + '.log') if self.options.dry_run: log.debug('--dry-run specified, logging changes to stdout instead of log file: %s' % logname) From f02cd7ddb6cc5ae3c9fe5d1b78abcff471ffc461 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Tue, 12 Sep 2017 15:06:30 -0400 Subject: [PATCH 11/13] Security: Fix issues with path handling in libraries and in general. --- config/galaxy.ini.sample | 8 + lib/galaxy/config.py | 1 + lib/galaxy/managers/folders.py | 4 +- lib/galaxy/managers/libraries.py | 2 +- lib/galaxy/tools/parameters/grouping.py | 2 + lib/galaxy/util/__init__.py | 48 ++--- lib/galaxy/util/path/__init__.py | 185 ++++++++++++++++++ lib/galaxy/util/path/ntpath.py | 11 ++ lib/galaxy/util/path/posixpath.py | 11 ++ .../webapps/galaxy/api/library_datasets.py | 33 +++- lib/galaxy/webapps/galaxy/api/remote_files.py | 4 +- .../webapps/galaxy/controllers/library.py | 2 + .../galaxy/controllers/library_common.py | 47 ++++- lib/tool_shed/managers/groups.py | 2 +- 14 files changed, 303 insertions(+), 57 deletions(-) create mode 100644 lib/galaxy/util/path/__init__.py create mode 100644 lib/galaxy/util/path/ntpath.py create mode 100644 lib/galaxy/util/path/posixpath.py diff --git a/config/galaxy.ini.sample b/config/galaxy.ini.sample index a72084bcb7a..77b673ffbe5 100644 --- a/config/galaxy.ini.sample +++ b/config/galaxy.ini.sample @@ -911,6 +911,14 @@ use_interactive = True # sub-directories of files contained in their directory. #user_library_import_dir = None +# For security reasons, users may not import any files that actually lie +# outside of their `user_library_import_dir` (e.g. using symbolic links). A +# list of directories can be allowed by setting the following option (the list +# is comma-separated). Be aware that *any* user with library import permissions +# can import from anywhere in these directories (assuming they are able to +# create symlinks to them). +#user_library_import_symlink_whitelist = None + # Allow admins to paste filesystem paths during upload. For libraries this # adds an option to the admin library upload tool allowing admins to paste # filesystem paths to files and directories in a box, and these paths will be diff --git a/lib/galaxy/config.py b/lib/galaxy/config.py index 756cc8f0a13..4514fdbed9f 100644 --- a/lib/galaxy/config.py +++ b/lib/galaxy/config.py @@ -394,6 +394,7 @@ class Configuration(object): self.genomespace_ui_url = kwargs.get('genomespace_ui_url', 'https://gsui.genomespace.org/jsui/') self.library_import_dir = kwargs.get('library_import_dir', None) self.user_library_import_dir = kwargs.get('user_library_import_dir', None) + self.user_library_import_symlink_whitelist = listify(kwargs.get('user_library_import_symlink_whitelist', []), do_strip=True) # Searching data libraries self.enable_lucene_library_search = string_as_bool(kwargs.get('enable_lucene_library_search', False)) self.enable_whoosh_library_search = string_as_bool(kwargs.get('enable_whoosh_library_search', False)) diff --git a/lib/galaxy/managers/folders.py b/lib/galaxy/managers/folders.py index d681e6a78da..506518b0b2e 100644 --- a/lib/galaxy/managers/folders.py +++ b/lib/galaxy/managers/folders.py @@ -132,7 +132,7 @@ class FolderManager(object): """ parent_folder = self.get(trans, parent_folder_id) current_user_roles = trans.get_current_user_roles() - if not (trans.user_is_admin or trans.app.security_agent.can_add_library_item(current_user_roles, parent_folder)): + if not (trans.user_is_admin() or trans.app.security_agent.can_add_library_item(current_user_roles, parent_folder)): raise InsufficientPermissionsException('You do not have proper permission to create folders under given folder.') new_folder = trans.app.model.LibraryFolder(name=new_folder_name, description=new_folder_description) # We are associating the last used genome build with folders, so we will always @@ -230,7 +230,7 @@ class FolderManager(object): """ Return true if the user has permissions to add item to the given folder. """ - if trans.user_is_admin: + if trans.user_is_admin(): return True current_user_roles = trans.get_current_user_roles() add_roles = set(trans.app.security_agent.get_roles_for_action(folder, trans.app.security_agent.permitted_actions.LIBRARY_ADD)) diff --git a/lib/galaxy/managers/libraries.py b/lib/galaxy/managers/libraries.py index b94217ee53f..ed7ef58c39b 100644 --- a/lib/galaxy/managers/libraries.py +++ b/lib/galaxy/managers/libraries.py @@ -50,7 +50,7 @@ class LibraryManager(object): """ Create a new library. """ - if not trans.user_is_admin: + if not trans.user_is_admin(): raise exceptions.ItemAccessibilityException('Only administrators can create libraries.') else: library = trans.app.model.Library(name=name, description=description, synopsis=synopsis) diff --git a/lib/galaxy/tools/parameters/grouping.py b/lib/galaxy/tools/parameters/grouping.py index 1531bcef598..85558f2a7de 100644 --- a/lib/galaxy/tools/parameters/grouping.py +++ b/lib/galaxy/tools/parameters/grouping.py @@ -380,6 +380,7 @@ class UploadDataset(Group): warnings.append("All FTP uploaded file selections were ignored.") elif ftp_files is not None and trans.user is not None: # look for files uploaded via FTP user_ftp_dir = trans.user_ftp_dir + assert not os.path.islink(user_ftp_dir), "User FTP directory cannot be a symbolic link" for (dirpath, dirnames, filenames) in os.walk(user_ftp_dir): for filename in filenames: for ftp_filename in ftp_files: @@ -449,6 +450,7 @@ class UploadDataset(Group): # TODO: warning to the user (could happen if session has become invalid) else: user_ftp_dir = trans.user_ftp_dir + assert not os.path.islink(user_ftp_dir), "User FTP directory cannot be a symbolic link" for (dirpath, dirnames, filenames) in os.walk(user_ftp_dir): for filename in filenames: path = relpath(os.path.join(dirpath, filename), user_ftp_dir) diff --git a/lib/galaxy/util/__init__.py b/lib/galaxy/util/__init__.py index e3f2eb6e4a5..062bc4b216f 100644 --- a/lib/galaxy/util/__init__.py +++ b/lib/galaxy/util/__init__.py @@ -8,6 +8,7 @@ from __future__ import absolute_import import binascii import collections import errno +import importlib import json import logging import os @@ -29,7 +30,7 @@ except ImportError: from datetime import datetime from hashlib import md5 -from os.path import normpath, relpath +from os.path import relpath from xml.etree import ElementInclude, ElementTree from xml.etree.ElementTree import ParseError @@ -49,6 +50,7 @@ except ImportError: docutils_html4css1 = None from .inflection import English, Inflector +from .path import safe_contains, safe_makedirs, safe_relpath # noqa: F401 inflector = Inflector(English) @@ -604,31 +606,19 @@ def which(file): return None -def safe_makedirs(path): - """ Safely make a directory, do not fail if it already exist or - is created during execution. - """ - if not os.path.exists(path): - try: - os.makedirs(path) - except OSError as e: - # review source for Python 2.7 this would only ever happen - # for the last path anyway so need to recurse - this exception - # means the last part of the path was already in existence. - if e.errno != errno.EEXIST: - raise - - 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 - """ - # Make both absolute. - directory = local_path_module.abspath(directory) - file = local_path_module.abspath(file) - return local_path_module.commonprefix([file, directory]) == directory + local_path_module is used by Pulsar to check Windows paths while running on + a POSIX-like system. + """ + if local_path_module != os.path: + _safe_contains = importlib.import_module('galaxy.util.path.%s' % local_path_module.__name__).safe_contains + else: + _safe_contains = safe_contains + return _safe_contains(directory, file) def merge_sorted_iterables(operator, *iterables): @@ -1505,22 +1495,6 @@ def download_to_file(url, dest_file_path, timeout=30, chunk_size=2 ** 20): f.write(chunk) -def safe_relpath(path): - """ - Given what we expect to be a relative path, determine whether the path - would exist inside the current directory. - - :type path: string - :param path: a path to check - :rtype: bool - :returns: ``True`` if path is relative and does not reference a path - in a parent directory, ``False`` otherwise. - """ - if path.startswith(os.sep) or normpath(path).startswith(os.pardir): - return False - return True - - class ExecutionTimer(object): def __init__(self): diff --git a/lib/galaxy/util/path/__init__.py b/lib/galaxy/util/path/__init__.py new file mode 100644 index 00000000000..48a8355ad79 --- /dev/null +++ b/lib/galaxy/util/path/__init__.py @@ -0,0 +1,185 @@ +"""Path manipulation functions. +""" +from __future__ import absolute_import + +import errno +import imp +from functools import partial +from itertools import starmap +from operator import getitem +from os import ( + makedirs, + walk, +) +from os.path import ( + abspath, + exists, + isabs, + join, + normpath, + pardir, + realpath, + relpath, +) + +from six import string_types +from six.moves import filterfalse, map, zip + + +def safe_contains(prefix, path, whitelist=None): + """Ensure a path is contained within another path. + + 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 + may also point inside a ``whitelist`` path. + + The ``path`` is checked against ``whitelist`` using either its absolute pathname (if passed in as absolute) or + relative to ``prefix`` and canonicalized (if applicable). It is *not* ``os.path.join()``ed with each ``whitelist`` + directory. + + :type prefix: string + :param prefix: a directory under which ``path`` is to be checked + :type path: string + :param path: a filename to check + :type whitelist: list of strings + :param whitelist: list of additional paths under which ``path`` may be located + :rtype: bool + :returns: ``True`` if ``path`` is contained within ``prefix`` or ``whitelist``, ``False`` otherwise. + """ + return any(__contains(prefix, path, whitelist=whitelist)) + + +def safe_makedirs(path): + """Safely make a directory, do not fail if it already exists or is created during execution. + + :type path: string + :param path: a directory to create + """ + # prechecking for existence is faster than try/except + if not exists(path): + try: + makedirs(path) + except OSError as e: + # reviewing the source for Python 2.7, this would only ever happen for the last path element anyway so no + # need to recurse - this exception means the last part of the path was already in existence. + if e.errno != errno.EEXIST: + raise + + +def safe_relpath(path): + """Determine whether a relative path references a path outside its root. + + This is a path computation: the filesystem is not accessed to confirm the existence or nature of ``path``. + + :type path: string + :param path: a path to check + :rtype: bool + :returns: ``True`` if path is relative and does not reference a path in a parent directory, ``False`` + otherwise. + """ + return not (isabs(path) or normpath(path).startswith(pardir)) + + +def unsafe_walk(path, whitelist=None): + """Walk a path and ensure that none of its contents are symlinks outside the path. + + It is assumed that ``path`` itself has already been validated e.g. with :func:`safe_relpath` or + :func:`safe_contains`. + + :type path: string + :param path: a directory to check for unsafe contents + :type whitelist: list of strings + :param whitelist: list of additional paths under which contents may be located + :rtype: iterator + :returns: Iterator of "bad" files found under ``path`` + """ + return filterfalse(partial(safe_contains, path, whitelist=whitelist), __walk(abspath(path))) + + +def __listify(item): + """A non-splitting version of :func:`galaxy.util.listify`. + """ + if not item: + return [] + elif isinstance(item, list) or isinstance(item, tuple): + return item + else: + return [item] + + +# helpers + + +def __walk(path): + for dirpath, dirnames, filenames in walk(path): + for name in dirnames + filenames: + yield join(dirpath, name) + + +def __contains(prefix, path, whitelist=None): + real = realpath(join(prefix, path)) + yield not relpath(real, prefix).startswith(pardir) + for wldir in whitelist or []: + yield not relpath(real, wldir).startswith(pardir) + + +# cross-platform support + + +def _build_self(target, path_module): + """Populate a module with the same exported functions as this module, but using the given os.path module. + + :type target: module + :param target: module on which to set ``galaxy.util.path`` functions + :type path_module: ``ntpath`` or ``posixpath`` module + :param path_module: module implementing ``os.path`` API to use for path functions + """ + __copy_self().__set_fxns_on(target, path_module) + + +def __copy_self(names=__name__, parent=None): + """Returns a copy of this module that can be modified without modifying `galaxy.util.path`` in ``sys.modules``. + """ + if isinstance(names, string_types): + names = iter(names.split('.')) + try: + name = names.next() + except StopIteration: + return parent + path = parent and parent.__path__ + parent = imp.load_module(name, *imp.find_module(name, path)) + return __copy_self(names, parent) + + +def __set_fxns_on(target, path_module): + """Overrides imported os.path functions with the ones from path_module and populates target with the global + functions from this module. + """ + for name in __pathfxns__: + globals()[name] = getattr(path_module, name) + __get = partial(getitem, globals()) + __set = partial(setattr, target) + # this is actually izip(..., imap(...)) + __fxns = zip(__all__, map(__get, __all__)) + # list() to execute + list(starmap(__set, __fxns)) + + +__pathfxns__ = ( + 'abspath', + 'exists', + 'isabs', + 'join', + 'normpath', + 'pardir', + 'realpath', + 'relpath', +) + +__all__ = ( + 'safe_contains', + 'safe_makedirs', + 'safe_relpath', + 'unsafe_walk', +) diff --git a/lib/galaxy/util/path/ntpath.py b/lib/galaxy/util/path/ntpath.py new file mode 100644 index 00000000000..5363409b3af --- /dev/null +++ b/lib/galaxy/util/path/ntpath.py @@ -0,0 +1,11 @@ +"""Galaxy "safe" path functions forced to work with Windows-style paths regardless of current platform +""" +from __future__ import absolute_import + +import ntpath +import sys + +from . import _build_self + + +_build_self(sys.modules[__name__], ntpath) diff --git a/lib/galaxy/util/path/posixpath.py b/lib/galaxy/util/path/posixpath.py new file mode 100644 index 00000000000..28232649b57 --- /dev/null +++ b/lib/galaxy/util/path/posixpath.py @@ -0,0 +1,11 @@ +"""Galaxy "safe" path functions forced to work with POSIX-style paths regardless of current platform +""" +from __future__ import absolute_import + +import posixpath +import sys + +from . import _build_self + + +_build_self(sys.modules[__name__], posixpath) diff --git a/lib/galaxy/webapps/galaxy/api/library_datasets.py b/lib/galaxy/webapps/galaxy/api/library_datasets.py index b2030e0038e..0b1ba0c0c3a 100644 --- a/lib/galaxy/webapps/galaxy/api/library_datasets.py +++ b/lib/galaxy/webapps/galaxy/api/library_datasets.py @@ -18,6 +18,7 @@ from galaxy.managers import base as managers_base from galaxy.managers import folders, library_datasets, roles from galaxy.tools.actions import upload_common from galaxy.tools.parameters import populate_state +from galaxy.util.path import safe_contains, safe_relpath, unsafe_walk from galaxy.util.streamball import StreamBall from galaxy.web import _future_expose_api as expose_api from galaxy.web import _future_expose_api_anonymous as expose_api_anonymous @@ -408,29 +409,45 @@ class LibraryDatasetsController(BaseAPIController, UsesVisualizationMixin): source = kwd.get('source', None) if source not in ['userdir_file', 'userdir_folder', 'importdir_file', 'importdir_folder', 'admin_path']: raise exceptions.RequestParameterMissingException('You have to specify "source" parameter. Possible values are "userdir_file", "userdir_folder", "admin_path", "importdir_file" and "importdir_folder". ') - if source in ['importdir_file', 'importdir_folder']: - if not trans.user_is_admin: + elif source in ['importdir_file', 'importdir_folder']: + if not trans.user_is_admin(): raise exceptions.AdminRequiredException('Only admins can import from importdir.') if not trans.app.config.library_import_dir: raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow admins to import into library from importdir.') import_base_dir = trans.app.config.library_import_dir + if not safe_relpath(path): + # admins shouldn't be able to explicitly specify a path outside server_dir, but symlinks are allowed. + # the reasoning here is that galaxy admins may not have direct filesystem access or can only access + # library_import_dir via FTP (which cannot create symlinks), and may rely on sysadmins to set up the + # import directory. if they have filesystem access, all bets are off. + raise exceptions.RequestParameterInvalidException( 'The given path is invalid.' ) path = os.path.join(import_base_dir, path) - if source in ['userdir_file', 'userdir_folder']: + elif source in ['userdir_file', 'userdir_folder']: + unsafe = None user_login = trans.user.email user_base_dir = trans.app.config.user_library_import_dir if user_base_dir is None: raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow upload from user directories.') full_dir = os.path.join(user_base_dir, user_login) - if not path.lower().startswith(full_dir.lower()): + if not safe_contains(full_dir, path, whitelist=trans.app.config.user_library_import_symlink_whitelist): + # the path is a symlink outside the user dir path = os.path.join(full_dir, path) + log.error('User attempted to import a path that resolves to a path outside of their import dir: %s -> %s', path, os.path.realpath(path)) + raise exceptions.RequestParameterInvalidException('The given path is invalid.') + path = os.path.join(full_dir, path) + for unsafe in unsafe_walk(path, whitelist=[full_dir] + trans.app.config.user_library_import_symlink_whitelist): + # the path is a dir and contains files that symlink outside the user dir + log.error('User attempted to import a directory containing a path that resolves to a path outside of their import dir: %s -> %s', unsafe, os.path.realpath(unsafe)) + if unsafe: + raise exceptions.RequestParameterInvalidException('The given path is invalid.') if not os.path.exists(path): raise exceptions.RequestParameterInvalidException('Given path does not exist on the host.') if not self.folder_manager.can_add_item(trans, folder): raise exceptions.InsufficientPermissionsException('You do not have proper permission to add items to the given folder.') - if source == 'admin_path': + elif source == 'admin_path': if not trans.app.config.allow_library_path_paste: raise exceptions.ConfigDoesNotAllowException('The configuration of this Galaxy instance does not allow admins to import into library from path.') - if not trans.user_is_admin: + if not trans.user_is_admin(): raise exceptions.AdminRequiredException('Only admins can import from path.') # Set up the traditional tool state/params @@ -449,12 +466,12 @@ class LibraryDatasetsController(BaseAPIController, UsesVisualizationMixin): if source in ['importdir_folder']: kwd['filesystem_paths'] = os.path.join(import_base_dir, path) # user wants to import one file only - if source in ["userdir_file", "importdir_file"]: + elif source in ["userdir_file", "importdir_file"]: file = os.path.abspath(path) abspath_datasets.append(trans.webapp.controllers['library_common'].make_library_uploaded_dataset( trans, 'api', kwd, os.path.basename(file), file, 'server_dir', library_bunch)) # user wants to import whole folder - if source == "userdir_folder": + elif source == "userdir_folder": uploaded_datasets_bunch = trans.webapp.controllers['library_common'].get_path_paste_uploaded_datasets( trans, 'api', kwd, library_bunch, 200, '') uploaded_datasets = uploaded_datasets_bunch[0] diff --git a/lib/galaxy/webapps/galaxy/api/remote_files.py b/lib/galaxy/webapps/galaxy/api/remote_files.py index 9dc39fc8222..7360f8ec12d 100644 --- a/lib/galaxy/webapps/galaxy/api/remote_files.py +++ b/lib/galaxy/webapps/galaxy/api/remote_files.py @@ -107,7 +107,7 @@ class RemoteFilesAPIController(BaseAPIController): subfolders and returns a flat list. """ response = [] - if os.path.exists(directory): + if os.path.exists(directory) and not os.path.islink(directory): for (dirpath, dirnames, filenames) in os.walk(directory): for filename in filenames: path = os.path.relpath(os.path.join(dirpath, filename), directory) @@ -130,7 +130,7 @@ class RemoteFilesAPIController(BaseAPIController): """ userdir_jstree = None jstree_paths = [] - if os.path.exists(directory): + if os.path.exists(directory) and not os.path.islink(directory): for (dirpath, dirnames, filenames) in os.walk(directory): for dirname in dirnames: dir_path = os.path.relpath(os.path.join(dirpath, dirname), directory) diff --git a/lib/galaxy/webapps/galaxy/controllers/library.py b/lib/galaxy/webapps/galaxy/controllers/library.py index 7dbac8ca561..dacb9e8c6bf 100644 --- a/lib/galaxy/webapps/galaxy/controllers/library.py +++ b/lib/galaxy/webapps/galaxy/controllers/library.py @@ -91,6 +91,7 @@ class Library(BaseUIController): 'app': app}) @web.expose + @web.require_admin def index(self, trans, **kwd): message = escape(kwd.get('message', '')) status = escape(kwd.get('status', 'done')) @@ -101,6 +102,7 @@ class Library(BaseUIController): status=status) @web.expose + @web.require_admin def browse_libraries(self, trans, **kwd): if 'operation' in kwd: operation = kwd['operation'].lower() diff --git a/lib/galaxy/webapps/galaxy/controllers/library_common.py b/lib/galaxy/webapps/galaxy/controllers/library_common.py index 955fede10f0..44ad3e2e4ec 100644 --- a/lib/galaxy/webapps/galaxy/controllers/library_common.py +++ b/lib/galaxy/webapps/galaxy/controllers/library_common.py @@ -23,6 +23,7 @@ from galaxy.security import Action from galaxy.tools.actions import upload_common from galaxy.tools.parameters import populate_state from galaxy.util import inflector, unicodify, FILENAME_VALID_CHARS +from galaxy.util.path import safe_contains, safe_relpath, unsafe_walk from galaxy.util.streamball import StreamBall from galaxy.web.base.controller import BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMetadataMixin, UsesLibraryMixinItems from galaxy.web.form_builder import AddressField, CheckboxField, SelectField, build_select_field @@ -72,6 +73,7 @@ except: class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMetadataMixin, UsesLibraryMixinItems): @web.json + @web.require_admin def library_item_updates(self, trans, ids=None, states=None): # Avoid caching trans.response.headers['Pragma'] = 'no-cache' @@ -95,6 +97,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta return rval @web.expose + @web.require_admin def browse_library(self, trans, cntrller='library', **kwd): message = escape(kwd.get('message', '')) status = kwd.get('status', 'done') @@ -167,6 +170,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta status=status)) @web.expose + @web.require_admin def library_info(self, trans, cntrller, **kwd): message = escape(kwd.get('message', '')) status = kwd.get('status', 'done') @@ -226,6 +230,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta status=escape(status)) @web.expose + @web.require_admin def library_permissions(self, trans, cntrller, **kwd): message = escape(kwd.get('message', '')) status = kwd.get('status', 'done') @@ -273,6 +278,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta 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') @@ -348,6 +354,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta status=escape(status)) @web.expose + @web.require_admin def folder_info(self, trans, cntrller, id, library_id, **kwd): message = escape(kwd.get('message', '')) status = kwd.get('status', 'done') @@ -404,6 +411,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta status=escape(status)) @web.expose + @web.require_admin def folder_permissions(self, trans, cntrller, id, library_id, **kwd): message = escape(kwd.get('message', '')) status = kwd.get('status', 'done') @@ -454,6 +462,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta status=escape(status)) @web.expose + @web.require_admin def ldda_edit_info(self, trans, cntrller, library_id, folder_id, id, **kwd): message = escape(kwd.get('message', '')) status = kwd.get('status', 'done') @@ -605,6 +614,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta status=escape(status)) @web.expose + @web.require_admin def ldda_info(self, trans, cntrller, library_id, folder_id, id, **kwd): message = escape(kwd.get('message', '')) status = kwd.get('status', 'done') @@ -654,6 +664,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta status=escape(status)) @web.expose + @web.require_admin def ldda_permissions(self, trans, cntrller, library_id, folder_id, id, **kwd): message = str(escape(kwd.get('message', ''))) status = kwd.get('status', 'done') @@ -792,6 +803,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta 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') @@ -851,6 +863,9 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta elif upload_option == 'upload_paths' and not is_admin: error = True message = 'Uploading files via filesystem paths can only be performed by administrators' + elif upload_option not in ('upload_file', 'upload_directory', 'upload_paths'): + error = True + message = 'Invalid upload_option' elif roles: # Check to see if the user selected roles to associate with the DATASET_ACCESS permission # on the dataset that would cause accessibility issues. @@ -1066,17 +1081,26 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta if upload_option == 'upload_directory': if server_dir in [None, 'None', '']: response_code = 400 - if cntrller == 'library_admin' or (cntrller == 'api' and trans.user_is_admin): + if trans.user_is_admin() and cntrller in ('library_admin', 'api'): import_dir = trans.app.config.library_import_dir import_dir_desc = 'library_import_dir' - full_dir = os.path.join(import_dir, server_dir) else: import_dir = trans.app.config.user_library_import_dir + if server_dir != trans.user.email: + import_dir = os.path.join(import_dir, trans.user.email) import_dir_desc = 'user_library_import_dir' - if server_dir == trans.user.email: - full_dir = os.path.join(import_dir, server_dir) - else: - full_dir = os.path.join(import_dir, trans.user.email, server_dir) + full_dir = os.path.join(import_dir, server_dir) + unsafe = None + if safe_relpath(server_dir): + if import_dir_desc == 'user_library_import_dir' and safe_contains(import_dir, full_dir, whitelist=trans.app.config.user_library_import_symlink_whitelist): + for unsafe in unsafe_walk(full_dir, whitelist=[import_dir] + trans.app.config.user_library_import_symlink_whitelist): + log.error('User attempted to import a path that resolves to a path outside of their import dir: %s -> %s', unsafe, os.path.realpath(unsafe)) + else: + log.error('User attempted to import a directory path that resolves to a path outside of their import dir: %s -> %s', server_dir, os.path.realpath(full_dir)) + unsafe = True + if unsafe: + response_code = 403 + message = 'Invalid server_dir' if import_dir: message = 'Select a directory' else: @@ -1270,6 +1294,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta return None @web.expose + @web.require_admin def add_history_datasets_to_library(self, trans, cntrller, library_id, folder_id, hda_ids='', **kwd): message = escape(kwd.get('message', '')) status = kwd.get('status', 'done') @@ -1512,6 +1537,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta return upload_option_select_list @web.expose + @web.require_admin def download_dataset_from_folder(self, trans, cntrller, id, library_id=None, **kwd): """Catches the dataset id and displays file contents as directed""" show_deleted = util.string_as_bool(kwd.get('show_deleted', False)) @@ -1551,6 +1577,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta status='error')) @web.expose + @web.require_admin def library_dataset_info(self, trans, cntrller, id, library_id, **kwd): message = escape(kwd.get('message', '')) status = kwd.get('status', 'done') @@ -1599,6 +1626,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta status=escape(status)) @web.expose + @web.require_admin def library_dataset_permissions(self, trans, cntrller, id, library_id, **kwd): message = escape(kwd.get('message', '')) status = kwd.get('status', 'done') @@ -1647,6 +1675,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta status=escape(status)) @web.expose + @web.require_admin def make_library_item_public(self, trans, cntrller, library_id, item_type, id, **kwd): message = escape(kwd.get('message', '')) status = kwd.get('status', 'done') @@ -1689,6 +1718,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta status=status)) @web.expose + @web.require_admin def act_on_multiple_datasets(self, trans, cntrller, library_id=None, ldda_ids='', **kwd): # This method is called from 1 of 3 places: # - this controller's download_dataset_from_folder() method @@ -1981,6 +2011,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta status=escape(status)) @web.expose + @web.require_admin def import_datasets_to_histories(self, trans, cntrller, library_id='', folder_id='', ldda_ids='', target_history_id='', target_history_ids='', new_history_name='', **kwd): # This method is called from one of the following places: # - a menu option for a library dataset ( ldda_ids is a single ldda id ) @@ -2111,6 +2142,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta status=escape(status)) @web.expose + @web.require_admin def manage_template_inheritance(self, trans, cntrller, item_type, library_id, folder_id=None, ldda_id=None, **kwd): show_deleted = util.string_as_bool(kwd.get('show_deleted', False)) use_panels = util.string_as_bool(kwd.get('use_panels', False)) @@ -2156,6 +2188,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta status='done')) @web.expose + @web.require_admin def move_library_item(self, trans, cntrller, item_type, item_id, source_library_id='', make_target_current=True, **kwd): # This method is called from one of the following places: # - a menu option for a library dataset ( item_type is 'ldda' and item_id is a single ldda id ) @@ -2373,6 +2406,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta status=escape(status)) @web.expose + @web.require_admin def delete_library_item(self, trans, cntrller, library_id, item_id, item_type, **kwd): # This action will handle deleting all types of library items. State is saved for libraries and # folders ( i.e., if undeleted, the state of contents of the library or folder will remain, so previously @@ -2441,6 +2475,7 @@ class LibraryCommon(BaseUIController, UsesFormDefinitionsMixin, UsesExtendedMeta status=status)) @web.expose + @web.require_admin def undelete_library_item(self, trans, cntrller, library_id, item_id, item_type, **kwd): # This action will handle undeleting all types of library items status = kwd.get('status', 'done') diff --git a/lib/tool_shed/managers/groups.py b/lib/tool_shed/managers/groups.py index f590850aeed..117a192169a 100644 --- a/lib/tool_shed/managers/groups.py +++ b/lib/tool_shed/managers/groups.py @@ -56,7 +56,7 @@ class GroupManager(object): """ Create a new group. """ - if not trans.user_is_admin: + if not trans.user_is_admin(): raise ItemAccessibilityException('Only administrators can create groups.') else: if self.get(trans, name=name): From c3fd90a36dfa0468447b20a63affbd7a6853f0f7 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Fri, 29 Sep 2017 14:00:46 -0400 Subject: [PATCH 12/13] Security: Switch GIE Popen() calls to run without shell=True to fix an ACE vulnerability. --- .../web/base/interactive_environments.py | 69 ++++++++++--------- 1 file changed, 35 insertions(+), 34 deletions(-) diff --git a/lib/galaxy/web/base/interactive_environments.py b/lib/galaxy/web/base/interactive_environments.py index 7623c9d1345..cb45a8b47db 100644 --- a/lib/galaxy/web/base/interactive_environments.py +++ b/lib/galaxy/web/base/interactive_environments.py @@ -2,15 +2,17 @@ import json import logging import os import random +import shlex import stat import string import tempfile import uuid +from itertools import product from subprocess import PIPE, Popen from sys import platform as _platform import yaml -from six.moves import configparser +from six.moves import configparser, shlex_quote from galaxy import model, web from galaxy.containers import build_container_interfaces @@ -132,7 +134,7 @@ class InteractiveEnvironmentRequest(object): # their defaults dictionary instead. default_dict = { 'container_interface': None, - 'command': 'docker {docker_args}', + 'command': 'docker', 'command_inject': '-e DEBUG=false -e DEFAULT_CONTAINER_RUNTIME=120', 'docker_hostname': 'localhost', 'wx_tempdir': 'False', @@ -273,40 +275,40 @@ class InteractiveEnvironmentRequest(object): def _get_name_for_run(self): return CONTAINER_NAME_PREFIX + uuid.uuid4().hex + def base_docker_cmd(self, subcmd=None): + # This is the basic docker command such as "sudo -u docker docker" or just "docker" + # Previously, {docker_args} was required to be in the string, this is no longer the case + base = shlex.split(self.attr.viz_config.get("docker", "command").format(docker_args='').strip()) + if subcmd: + base.append(subcmd) + return base + def docker_cmd(self, image, env_override=None, volumes=None): """ Generate and return the docker command to execute """ - if volumes is None: - volumes = [] - env = self._get_env_for_run(env_override) - import_volume_def = self._get_import_volume_for_run() - env_str = ' '.join('-e "%s=%s"' % (key, item) for key, item in env.items()) - volume_str = ' '.join('-v "%s"' % volume for volume in volumes) if self.use_volumes else '' - import_volume_str = '-v "{import_volume}"'.format(import_volume=import_volume_def) if import_volume_def else '' - name = None - # This is the basic docker command such as "sudo -u docker docker {docker_args}" - # or just "docker {docker_args}" - command = self.attr.viz_config.get("docker", "command") - # Then we format in the entire docker command in place of - # {docker_args}, so as to let the admin not worry about which args are - # getting passed + def _flag_opts(flag, opts): + return [arg for pair in product((flag,), opts) for arg in pair] + command_inject = self.attr.viz_config.get("docker", "command_inject") # --name should really not be set, but we'll try to honor it anyway - if '--name' not in command_inject: - name = self._get_name_for_run() - command = command.format(docker_args='run {command_inject} {name} {environment} -d -P {import_volume_str} {volume_str} {image}') + name = ['--name=%s' % self._get_name_for_run()] if '--name' not in command_inject else [] + env = self._get_env_for_run(env_override) + import_volume_def = self._get_import_volume_for_run() + if volumes is None: + volumes = [] + if import_volume_def: + volumes.insert(0, import_volume_def) - # Once that's available, we format again with all of our arguments - command = command.format( - command_inject=command_inject, - name='--name=%s' % name if name is not None else '', - environment=env_str, - import_volume_str=import_volume_str, - volume_str=volume_str, - image=image, + return ( + self.base_docker_cmd('run') + + shlex.split(command_inject) + + name + + _flag_opts('-e', ['='.join(map(str, t)) for t in env.items()]) + + ['-d', '-P'] + + _flag_opts('-v', map(str, volumes)) + + [image] ) - return command @property def use_volumes(self): @@ -398,9 +400,9 @@ class InteractiveEnvironmentRequest(object): log.info("Starting docker container for IE {0} with command [{1}]".format( self.attr.viz_id, - raw_cmd + ' '.join([shlex_quote(x) for x in raw_cmd]) )) - p = Popen(raw_cmd, stdout=PIPE, stderr=PIPE, close_fds=True, shell=True) + p = Popen(raw_cmd, stdout=PIPE, stderr=PIPE, close_fds=True) stdout, stderr = p.communicate() if p.returncode != 0: log.error("%s\n%s" % (stdout, stderr)) @@ -504,14 +506,13 @@ class InteractiveEnvironmentRequest(object): :returns: inspect_data, a dict of docker inspect output """ - command = self.attr.viz_config.get("docker", "command") - command = command.format(docker_args="inspect %s" % container_id) + raw_cmd = self.base_docker_cmd('inspect') + [container_id] log.info("Inspecting docker container {0} with command [{1}]".format( container_id, - command + ' '.join([shlex_quote(x) for x in raw_cmd]) )) - p = Popen(command, stdout=PIPE, stderr=PIPE, close_fds=True, shell=True) + p = Popen(raw_cmd, stdout=PIPE, stderr=PIPE, close_fds=True) stdout, stderr = p.communicate() if p.returncode != 0: log.error("%s\n%s" % (stdout, stderr)) From 391586ba01fc1a01443962d137e9df3fce707434 Mon Sep 17 00:00:00 2001 From: Nate Coraor Date: Thu, 19 Oct 2017 09:29:00 -0400 Subject: [PATCH 13/13] [GX-2017-0003]: Fix for the reported issue, only allow http, https, and ftp schemes in the data_source tool. --- tools/data_source/data_source.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/data_source/data_source.py b/tools/data_source/data_source.py index 0417be67477..aa113ffa5e1 100644 --- a/tools/data_source/data_source.py +++ b/tools/data_source/data_source.py @@ -6,7 +6,7 @@ import socket import sys from json import dumps, loads -from six.moves.urllib.parse import urlencode +from six.moves.urllib.parse import urlencode, urlparse from six.moves.urllib.request import urlopen from galaxy.datatypes import sniff @@ -80,7 +80,7 @@ def __main__(): for data_dict in job_params['output_data']: cur_filename = data_dict.get('file_name', filename) cur_URL = params.get('%s|%s|URL' % (GALAXY_PARAM_PREFIX, data_dict['out_data_name']), URL) - if not cur_URL: + if not cur_URL or urlparse(cur_URL).scheme not in ('http', 'https', 'ftp'): open(cur_filename, 'w').write("") stop_err('The remote data source application has not sent back a URL parameter in the request.')