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

Clarify default access status #17

Open
cd-work opened this issue Aug 26, 2022 · 2 comments
Open

Clarify default access status #17

cd-work opened this issue Aug 26, 2022 · 2 comments
Labels
enhancement New feature or request

Comments

@cd-work
Copy link
Contributor

cd-work commented Aug 26, 2022

Currently, the handle_access method states the following:

Attempts to add a set of access rights that will be supported by this ruleset.
By default, all actions requiring these access rights will be denied.

This implies that if something is handled, without adding exceptions as rules, access to it will be denied. However I haven't found any place that explicitly states how things behave when something isn't handled. I think it would be beneficial to have this pointed out on handle_access or Ruleset::new in a rustdoc # Safety block, just to ensure people don't make assumptions that lead to security issues.

Right now the default behavior seems to be that things which aren't handled are allowed. So creating a ruleset with handle_access(AccessFs::from_write(abi)) without restricting read, would allow full read access. I could easily see how this might trip someone up when they're just trying to allow some read access without allowing any write access for example.

I think it might also be worth considering if this is the right default. I personally think this library would be much harder to use incorrectly when just Ruleset::new().create().unwrap().restrict_self().unwrap() was enough to deny all filesystem access. Especially when taking possible future changes into account (you wouldn't want new stuff to be allowed by default).

@l0kod
Copy link
Member

l0kod commented Aug 29, 2022

Right now the default behavior seems to be that things which aren't handled are allowed. So creating a ruleset with handle_access(AccessFs::from_write(abi)) without restricting read, would allow full read access. I could easily see how this might trip someone up when they're just trying to allow some read access without allowing any write access for example.

You're correct. I think it should be safe to require Ruleset::handle_access() to take ABI instead of AccessFs. Moving that to Ruleset::new() would not enable to adjust the compatibility level before setting the handled access rights, but that could still be overloaded later and might be a better alternative for most use cases anyway. 🤔

I think it might also be worth considering if this is the right default. I personally think this library would be much harder to use incorrectly when just Ruleset::new().create().unwrap().restrict_self().unwrap() was enough to deny all filesystem access. Especially when taking possible future changes into account (you wouldn't want new stuff to be allowed by default).

Hmm, using Ruleset::new(ABI) would solve this potential issue (not only with FS access rights but also future access types).

This would replace:

let abi = ABI::V1;
Ruleset::new()
    .handle_access(AccessFs::from_all(abi))?
    .create()?
    .add_rules(path_beneath_rules(&["/usr", "/etc", "/dev"], AccessFs::from_read(abi)))?
    .restrict_self()?

…with something like this:

Ruleset::new(ABI::V1)
    .create()?
    .add_rules(path_beneath_rules(&["/usr", "/etc", "/dev"], Access::Read))?
    .restrict_self()?

The current way do to it could still be usable, but for very custom needs.

I need to merge #12 before any other changes though.

@cd-work
Copy link
Contributor Author

cd-work commented Aug 29, 2022

Hmm, using Ruleset::new(ABI) would solve this potential issue (not only with FS access rights but also future access types).

I don't think passing ABI to Ruleset::new would be a problem. And if that would allow a deny-all approach by default I think that would be a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants