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

Ensure that PathAccess(...).accessFS ⊆ cfg.handledAccessFS #18

Closed
gnoack opened this issue Sep 8, 2021 · 3 comments
Closed

Ensure that PathAccess(...).accessFS ⊆ cfg.handledAccessFS #18

gnoack opened this issue Sep 8, 2021 · 3 comments

Comments

@gnoack
Copy link
Collaborator

gnoack commented Sep 8, 2021

Ensure that

PathAccess(...).accessFS ⊆ cfg.handledAccessFS

for all path options constructed with PathAccess().

This is a conservative choice, but it's easier to enforce that way.

RODirs() and friends are more "magic" - they let you give broad access permissions on entire directories, and that is supposed to work even when the user specifies a smaller AccessFSSet in their config. On the other hand, when users pass a custom AccessFSSet to PathAccess(), they should have a clear understanding of the configuration that they will use it in, and it can be expected that they ensure it is a subset.

Examples

This is a good example. On library upgrade, it should be enough to bump the version number, in most cases. Note that a Go-landlock library that supports V3 at some point still needs to do the exact same thing in the V2 case though:

landlock.V2.BestEffort().RestrictPaths(
  landlock.RODirs("/bin", "/usr", "/etc"),
  landlock.RWDirs("/tmp"),
)

This is a good example as well. Making and removing directories is forbidden everywhere except in /tmp.

landlock.MustConfig(landlock.AccessFSSet(ll.AccessFSMakeDir|ll.AccessFSRemoveDir)).BestEffort().RestrictPaths(
  landlock.RWDirs("/tmp"),
)

This is a good example as well. Making and removing directories is forbidden, except in /tmp and creating them in /home/x/tmp. It's verbose, but the author knows exactly what is being restricted and what is the scope of each exception:

landlock.MustConfig(landlock.AccessFSSet(ll.AccessFSMakeDir|ll.AccessFSRemoveDir)).BestEffort().RestrictPaths(
  landlock.PathAccess(landlock.AccessFSSet(ll.AccessFSMakeDir|ll.AccessFSRemoveDir), "/tmp"),
  landlock.PathAccess(landlock.AccessFSSet(ll.AccessFSMakeDir), "/home/x/tmp"),
)

The following example is on the fence:

  • It's a good example as long as the author has checked that "transmogrify" is captured in Landlock ABI V99.
  • It's a bad example if the author has not checked that. If "transmogrify" is not part of Landlock ABI V99, it's very likely to be a programmer mistake, and it would be better to give an error in that case.
landlock.V99.BestEffort().RestrictPaths(
  landlock.PathAccess(landlock.AccessFSSet(ll.AccessTransmogrifyFile), "/tmp"),
)

This last case is the one that the bug is about.

@gnoack gnoack closed this as completed in c567107 Sep 8, 2021
@gnoack
Copy link
Collaborator Author

gnoack commented Sep 8, 2021

I should note that this is a breaking change. It's unlikely that there are existing users making this mistake. But with runc-like tools delegating the configuration of AccessFSSets to users completely, it's likely that people will eventually make the mistake accidentally.

@l0kod
Copy link
Member

l0kod commented Sep 9, 2021

Nice! Does it check the subset consistency even when Landlock is not enabled at runtime?

@gnoack
Copy link
Collaborator Author

gnoack commented Sep 9, 2021

Nice! Does it check the subset consistency even when Landlock is not enabled at runtime?

Yes, it does.

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

No branches or pull requests

2 participants