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

fuse: pass default_options #3903

Closed
blueyed opened this issue Jun 13, 2018 · 30 comments
Closed

fuse: pass default_options #3903

blueyed opened this issue Jun 13, 2018 · 30 comments

Comments

@blueyed
Copy link

blueyed commented Jun 13, 2018

llfuse's documentation recommends using llfuse.default_options.

I've tried this, and it appears to work.

In particular "default_permissions" might be useful for performance reasons.

I'd be happy to provide a PR for this, if you agree that it makes sense.

@ThomasWaldmann
Copy link
Member

>>> import llfuse
>>> llfuse.default_options
frozenset({'default_permissions', 'no_splice_read', 'nonempty', 'splice_move', 'splice_write', 'big_writes'}) 

Not sure what they all mean, whether they make sense for us or since when default_options is available in llfuse.

So, double check that please and then do a PR. :)

@blueyed
Copy link
Author

blueyed commented Jun 13, 2018

default_options is there since release-0.42~61(1d8c8e0): python-llfuse/python-llfuse@1d8c8e0. Do you want to bump the required version, or should be duplicate it for older versions?

See python-llfuse/python-llfuse#12 about the documentation for its values.

default_permissions seems to be crucial: without it, I can access a mount with -o uid=1001,umask=077 as user 1000!

@ThomasWaldmann
Copy link
Member

ThomasWaldmann commented Jun 14, 2018

Hmm, somehow embarassing that the default_options must be explicitly given and are not the default(*)...

Same for the default_permissions.

(*) in fact, they are the default value for the init argument. but that's not practically helpful as in many cases one needs to pass other options there, so the argument default is not used.

@ThomasWaldmann
Copy link
Member

ThomasWaldmann commented Jun 14, 2018

@blueyed can you evaluate whether this means a security or performance issue for:

  • borg 1.1.x (x < 6) or borg 1.0.y - no uid, gid, umask options implemented there
  • borg >= 1.1.6 (with new uid, gid, umask options)

@ThomasWaldmann
Copy link
Member

ThomasWaldmann commented Jun 14, 2018

related PRs: #3769 (#3843 is the fwd port to master, no backport to 1.0 yet)

@ThomasWaldmann
Copy link
Member

@blueyed do not bump the required version, but use something like:

default_options = frozenset(...)  # copied from llfuse, to support pre-0.42 llfuse
default_options = getattr(llfuse, 'default_options', default_options)
...

@ThomasWaldmann
Copy link
Member

@blueyed you said In particular "default_permissions" might be useful for performance reasons..

Did you mean security reasons? If not, can you explain the performance impact?

@blueyed
Copy link
Author

blueyed commented Jun 14, 2018

By now I would say "security reasons", but initially I've read something along that otherwise the program would have to do checks, which might be slower then.
borg's fuse driver does not do any checks however, so it would be slower probably, but for good reasons. The other options should improve performance though I guess.

@ThomasWaldmann
Copy link
Member

considering the PRs have been merged into master and 1.1-maint, guess we can close this?

@ThomasWaldmann
Copy link
Member

ehrm, looks like I misinterpreted my own comment #3903 (comment) - "related" did not mean this issue has been fixed there, just related stuff...

reopening...

@ThomasWaldmann ThomasWaldmann reopened this Feb 3, 2019
@ThomasWaldmann ThomasWaldmann added this to the 1.1.9 milestone Feb 3, 2019
@ThomasWaldmann
Copy link
Member

ThomasWaldmann commented Feb 7, 2019

ok, looks like we need a security fix here, I am working on this currently and will release 1.1.9 ASAP.

@LocutusOfBorg can you help with getting a CVE for this?

A) When borg mount -o allow_other ... is used (for this to work, it needs to be allowed via /etc/fuse.conf and user_allow_other in there) the FUSE subsystem principally allows access by other UIDs than the uid which did the borg mount.

B) If allow_other is not used, FUSE does not allow any access by other users, so no problem here. If allow_root is used, there is also no problem as root is able to access everything anyway.

C) As unfixed borg versions did not give default_permissions to llfuse.init(), it did not request that UID/GID/mode-based permissions are checked / enforced by the kernel.

A) and C) together mean that uid/gid/mode as present in a borg archive are visible (e.g. ls -l), but are not enforced, which is unsecure and unexpected(*).

(*) This is especially the case if the UIDs/GIDs stored in the borg archive match the system executing the mount - then users would likely expect it behaving like a local filesystem, enforcing permissions perfectly. If UIDs/GIDs do not match between system and archive, there is a fundamental problem with that mismatch, of course (this is not borgfs specific, but happens with all filesystems).

@ThomasWaldmann
Copy link
Member

@blueyed could you review the PRs?

@ThomasWaldmann
Copy link
Member

ThomasWaldmann commented Feb 8, 2019

Bah, this is a mess. :-(

From mount.fuse(8):

       default_permissions
              By  default  FUSE  doesn't check file access permissions,
              the filesystem is free to implement it's access policy or
              leave it to the underlying file access mechanism (e.g. in
              case of network filesystems).
              This option enables permission checking, restricting access based on file mode.
              This is option is usually useful together with the allow_other mount option.

So, the llfuse docs recommend llfuse users (== [borg] developers) use default_options (which sets default_permissions) and give that to llfuse.init().

OTOH, the mount.fuse(8) man page says checking permissions is NOT the default and users need to give that mount option if they want checking.

OTOH, the name default_permissions somehow seems to indicate that this should be on by default.

A related problem is: if we give some options by default, how does one get rid of them in case one wants different behaviour? It looks like llfuse implements some stuff like no_read_splice, but there is no no_default_permissions...

@ThomasWaldmann
Copy link
Member

nitpick: the filesystem is free to implement **it's** access policy - note the wrong apostrophe.

@ThomasWaldmann
Copy link
Member

related: libfuse/libfuse#15

@ThomasWaldmann
Copy link
Member

any comments about how to deal with this?

@Nikratio
Copy link
Contributor

Nikratio commented Feb 8, 2019

default_options has to be passed explicitly so that you can filter out options that you do not want.

@ThomasWaldmann
Copy link
Member

@Nikratio passed where by whom? do you mean by the end user to mount.fuse on the shell?

@ThomasWaldmann
Copy link
Member

I updated PR #4331 according to these ideas:

  • have secure internal default in borg mount (== enforce default_permissions)
  • if a user does not want that, revoking is possible with a custom ignore_permissions mount option
  • keep out of the way of / do no magic with all other fuse options, the user who mounts gets what they specify
  • document OUR options, point to mount.fuse docs for everything else.
  • leave cleaning up this mess to llfuse / fuse / kernel people.

@ThomasWaldmann
Copy link
Member

@LocutusOfBorg considering that mount.fuse documents that there is no permissions checking, I guess borg does not need a CVE, sorry for the noise.

Not sure if that is a sane situation in fuse/kernel, but that's not our problem.

After the PR we will at least keep users from falling into that pitfall accidentally.

@ThomasWaldmann ThomasWaldmann removed the bug label Feb 9, 2019
@Nikratio
Copy link
Contributor

Nikratio commented Feb 9, 2019

Passed to llfuse.init() by the program that uses python-llfuse. As http://www.rath.org/llfuse-docs/data.html says, "This is a recommended set of options that should be passed to llfuse.init to get reasonable behavior and performance. ".

@ThomasWaldmann
Copy link
Member

@Nikratio ah. well, as you've maybe read above, that has its issues.

what i did not talk about yet are potential platform issues (like some platform not supporting some option or some platform requiring some option). i didn't feel like inventing a negated (or negated negated) option just for the case somebody running into such an issue for all the options in llfuse.default_options. i did that for default_permissions.

what (ll)fuse could do better here is at least have all options in negated and non-negated form.

@Nikratio
Copy link
Contributor

Nikratio commented Feb 9, 2019

The default options are platform-dependent. If a platform doesn't support an option, then on that platform it will never be included in default_options. For negation, I don't understand why
llfuse.main(default_options + { 'no_foo'}) is better than llfuse.main(default_options - {'foo'}). Actually, the former raises the question why no_foo should take precedence over foo.

@ThomasWaldmann
Copy link
Member

platform dependency: ok, sounds good. i thought i have seen some code removing some options for macOS, but I don't remember it exactly.

assuming that 1) a software calls llfuse.init() and uses llfuse.default_options as a starting point to build the options list, but that 2) these options are not usable "as is" for all thinkable scenarios.

then, if you have "foo" in default_options, the software needs to offer "nofoo" to the cli user to override that. if you have "nobar" in default_options, the software needs to offer "bar" to the user to override that.

of course the software needs to give the options to llfuse.init in the way so it has the right effect (so e.g. if "foo" is in options, "nofoo" would just remove it from the list. if "nobar" is in options, "bar" would remove it from there). of course this also depends what the real internal defaults are.

ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this issue Feb 10, 2019
…ckup#3903

"default_permissions" is now enforced by borg by default to let the
kernel check uid/gid/mode based permissions.

"ignore_permissions" can be given to not enforce "default_permissions".

note: man mount.fuse explicitly tells about the security issue:

    default_permissions
	By  default FUSE doesn't check file access permissions, ...
	This option enables permission checking, restricting access
	based on file mode.
	This option is usually useful together with the allow_other
	mount option.

We consider this a pitfall waiting for someone to fall into and this is
why we chose to change the default behaviour for borg.
ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this issue Feb 10, 2019
…ckup#3903

"default_permissions" is now enforced by borg by default to let the
kernel check uid/gid/mode based permissions.

"ignore_permissions" can be given to not enforce "default_permissions".

note: man mount.fuse explicitly tells about the security issue:

    default_permissions
	By  default FUSE doesn't check file access permissions, ...
	This option enables permission checking, restricting access
	based on file mode.
	This option is usually useful together with the allow_other
	mount option.

We consider this a pitfall waiting for someone to fall into and this is
why we chose to change the default behaviour for borg.
@ThomasWaldmann
Copy link
Member

#4331 fixed 1.1-maint.

@ThomasWaldmann ThomasWaldmann modified the milestones: 1.1.9, hydrogen Feb 10, 2019
@ThomasWaldmann
Copy link
Member

ThomasWaldmann commented Feb 11, 2019

@LocutusOfBorg is it already too late or can this (borg 1.1.9) still get into next debian?

@LocutusOfBorg
Copy link
Contributor

You mean the one that is already ongoing in unstable?
I guess it is fine, if it builds correctly and nobody reports regressions

@ThomasWaldmann
Copy link
Member

as far as i could see on packages.debian.org, unstable (sid) has 1.1.8 still (1.1.9 is brand new, just released it).

@LocutusOfBorg
Copy link
Contributor

https://packages.qa.debian.org/b/borgbackup/news/20190211T093407Z.html :)

ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this issue Feb 11, 2019
…ckup#3903

"default_permissions" is now enforced by borg by default to let the
kernel check uid/gid/mode based permissions.

"ignore_permissions" can be given to not enforce "default_permissions".

note: man mount.fuse explicitly tells about the security issue:

    default_permissions
	By  default FUSE doesn't check file access permissions, ...
	This option enables permission checking, restricting access
	based on file mode.
	This option is usually useful together with the allow_other
	mount option.

We consider this a pitfall waiting for someone to fall into and this is
why we chose to change the default behaviour for borg.

(cherry picked from commit 1b277cb)
ThomasWaldmann added a commit to ThomasWaldmann/borg that referenced this issue Feb 11, 2019
…ckup#3903

"default_permissions" is now enforced by borg by default to let the
kernel check uid/gid/mode based permissions.

"ignore_permissions" can be given to not enforce "default_permissions".

note: man mount.fuse explicitly tells about the security issue:

    default_permissions
	By  default FUSE doesn't check file access permissions, ...
	This option enables permission checking, restricting access
	based on file mode.
	This option is usually useful together with the allow_other
	mount option.

We consider this a pitfall waiting for someone to fall into and this is
why we chose to change the default behaviour for borg.

(cherry picked from commit 1b277cb)
ThomasWaldmann added a commit that referenced this issue Feb 11, 2019
security fix: configure FUSE with "default_permissions", fixes #3903
ThomasWaldmann added a commit that referenced this issue Feb 11, 2019
security fix: configure FUSE with "default_permissions", fixes #3903
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

4 participants