New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add option to check file permissions for the user library imports #5154

Merged
merged 4 commits into from Dec 19, 2017

Conversation

5 participants
@scholtalbers
Contributor

scholtalbers commented Dec 7, 2017

related to #4937

Existing:

# 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

Addition:

# In conjunction or alternatively, Galaxy can restrict user library imports to
# those files that the user can read (by checking basic unix permissions).
# For this to work, the username has to match the username on the filesystem.
#user_library_import_check_permissions = False

@galaxybot galaxybot added the triage label Dec 7, 2017

@galaxybot galaxybot added this to the 18.01 milestone Dec 7, 2017

@martenson martenson changed the title from related to #4937 - add option to check file permissions for the user library imports to add option to check file permissions for the user library imports Dec 7, 2017

@martenson

This comment has been minimized.

Member

martenson commented Dec 7, 2017

@galaxybot test this

@dannon

This comment has been minimized.

Member

dannon commented Dec 19, 2017

Thanks @scholtalbers!

@dannon dannon merged commit 1880230 into galaxyproject:dev Dec 19, 2017

6 checks passed

api test Build finished. 334 tests run, 4 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 165 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 59 tests run, 0 skipped, 0 failed.
Details
selenium test Build finished. 117 tests run, 3 skipped, 0 failed.
Details
toolshed test Build finished. 577 tests run, 0 skipped, 0 failed.
Details
@@ -82,7 +89,7 @@ def safe_relpath(path):
return not (isabs(path) or normpath(path).startswith(pardir))
def unsafe_walk(path, whitelist=None):
def unsafe_walk(path, whitelist=None, username=None):

This comment has been minimized.

@martenson

martenson Dec 19, 2017

Member

@natefoo afaik you spent a lot of time on these recently so you might want to have a look

This comment has been minimized.

@natefoo

natefoo Jan 19, 2018

Member

Looks ok from a security perspective although in practice you can't assume the filesystem will actually honor the traditional unix ownership and permissions, especially with ACLs. The only way to know for sure is to attempt to perform the desired operation as the desired user, but that's not an option here. However, as long as the admin is aware of this (perhaps we should put a stronger warning next to the config option) this method is ok.

I would prefer to keep unsafe_walk as an iterator for performance reasons, though, so we should probably add a new function like unsafe_user_walk that does this.

This comment has been minimized.

@natefoo

natefoo Jan 19, 2018

Member

@martenson thanks for #5350 so I don't lose track of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment