Skip to content
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

What exactly does it mean to have --allow-write but not --allow-read? #4423

Closed
dubiousjim opened this issue Mar 18, 2020 · 4 comments
Closed
Labels

Comments

@dubiousjim
Copy link
Contributor

dubiousjim commented Mar 18, 2020

Say I have a directory /writeonly to which I have --allow-write permissions but not --allow-read permissions. Some of the things I might want to do inside that directory should clearly be out of bounds (and this is already enforced). For instance, I can't create a file with read-write access there, or open a file for reading. Nor can I stat a path inside that directory to get any information about the existence, mtime, size, etc of its files.

However, there are other things I might try to do that arguably also should be out of bounds, but which aren't currently enforced. For example, although I can't stat a path there to see if a file exists, I'm not prevented from telling whether the file exists by other means. I could for instance try to create a file there with open mode x (or {createNew: true, read:false}), and if the attempt fails, then I know there's a file there. There are other similar possibilities, and the possible exploits (if this is in fact undesirable) will be multiplied when some of the commits I have queued for #4017 are merged.

It's easy enough to block these attempts: one just has to add an extra state.check_read(&path)?; in cli/ops/fs.rs in the right circumstances. The tricky thing is to decide what those circumstances are. So I'm raising this issue for discussion.

Here's a proposal: if I don't have --allow-read access to a path, then I shouldn't be able to open a file at that path with {create:false} or {createNew:true} options, as that involves checking for the file's existence. The same goes for analogous options for writeFile, copyFile, truncate, and rename (most of these are in the queued commits for #4017). Nor should I be able to mkdir at that path without {recursive:true}.

Still have to review/think about other ops (symlink, link, ...).

What about chdir? Should I be permitted to chdir to /writable/subdir if I don't have --allow-read access to /writable?

@dubiousjim
Copy link
Contributor Author

Here's a systematic list of the ops in cli/ops/fs.rs for which these questions arise.

  • op_stat(path, lstat: boolean)
  • op_realpath(path)
  • op_read_dir(path)
  • op_read_link(path)

The preceding already require --allow-read

  • op_chmod(path, ...)
  • op_chown(path, ...)
  • op_utime(path, ...)

These currently only require --allow-write. Perhaps they should also require --allow-read, else they provide a way to tell whether a file already exists at path...

  • op_open(path, {...})
  • op_truncate(path, {...})
  • op_copy_file(from, to, {...})
  • op_rename(oldpath, newpath, {...})

These are covered by my proposal above.

  • op_link(oldpath, newpath)
  • op_symlink(oldpath, newpath)

Perhaps these should also require --allow-read access to newpath.

Note that link does currently require --allow-read on oldpath (which makes sense, else you could make a link targeting some file you lack --allow-read access to, and place it in a location where you have --allow-read access). But it doesn't require --allow-read on newpath. Yet it will fail if there's already a file there. So this gives a way to tell whether there's a file at newpath without having --allow-read access to it.

Note also that symlink doesn't currently require --allow-read access to oldpath. For some use cases, that seems reasonable. We don't want to require that oldpath even exists right now (and it may be inside directories that also don't exist). But on the other hand, does our permission system prevent me from creating a symlink from /unreadable_dir/file to /readable_dir/symlink, and then opening /readable_dir/symlink for reading, or applying stat (not lstat) to it, etc?

  • op_mkdir(path, {recursive, mode})

Currently only requires --allow-write, perhaps it should also require --allow-read if recursive is false.

  • op_make_temp_dir(dir, prefix, suffix)
  • op_make_temp_file(dir, prefix, suffix)
  • op_remove(path, {recursive})

Not sure about these. Currently they only require --allow-write.

  • op_chdir(directory)

Currently doesn't require any permissions to directory.

@dubiousjim
Copy link
Contributor Author

It's very unclear to me that this is a security hole worth trying to close. Especially if it's not feasible to close it completely, as I suspect it may not be.

Consider mkdir(/root/middle/target, {recursive:true}), invoked by a user who only has --allow-write access to /root. This won't give the user information about whether /root, /root/middle, or /root/middle/target already exist as directories. But it will complain if any of them are files. The implementation of mkdir with {recurse:true} could be changed to check for --allow-read access to target. But it wouldn't be feasible to change it to check for --allow-read access to middle (which may be complex). If one went down that path, one might as well give up having a specialized fast-path for recursive mkdirs.

So there'd always at least be this way to tell whether there's a file at /root/middle.

Given that, maybe the best thing to do is to make no attempts to close this security leakage, and just document that someone with --allow-write but not --allow-read access to a path has the means (in many different ways) to determine whether a file or directory exist at that path or anywhere below it. They just can't read that file or directory's contents.

@dubiousjim
Copy link
Contributor Author

Note also that symlink doesn't currently require --allow-read access to oldpath. For some use cases, that seems reasonable. We don't want to require that oldpath even exists right now (and it may be inside directories that also don't exist). But on the other hand, does our permission system prevent me from creating a symlink from /unreadable_dir/file to /readable_dir/symlink, and then opening /readable_dir/symlink for reading, or applying stat (not lstat) to it, etc?

I think this is an error. Raising a new issue #4437 for it.

@stale
Copy link

stale bot commented Jan 6, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 6, 2021
@stale stale bot closed this as completed Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant