Security: Fix issues with path handling in libraries and in general.

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

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 ) )

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 ) )

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 )

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 )

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):

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',
+)

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)

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)

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 ]

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 )

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()

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' )

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:

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 ):