Re: [PATCH v2] fuse: Add module param for non-descendant userns access to allow_other

[Dave Marchevsky <davemarchevsky@fb.com>]

On 6/16/22 4:01 AM, Christian Brauner wrote:   
> On Wed, Jun 15, 2022 at 04:36:35PM -0700, Andrii Nakryiko wrote:
>> On Tue, Jun 14, 2022 at 7:33 AM Christian Brauner <brauner@kernel.org> wrote:
>>>
>>> On Mon, Jun 13, 2022 at 11:21:24AM -0700, Andrii Nakryiko wrote:
>>>> On Mon, Jun 13, 2022 at 3:34 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>>>>>
>>>>> On Mon, 13 Jun 2022 at 11:37, Christian Brauner <brauner@kernel.org> wrote:
>>>>>>
>>>>>> On Mon, Jun 13, 2022 at 10:23:47AM +0200, Miklos Szeredi wrote:
>>>>>>> On Fri, 10 Jun 2022 at 23:39, Andrii Nakryiko <andriin@fb.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 6/7/22 1:47 AM, Christian Brauner wrote:
>>>>>>>>> On Wed, Jun 01, 2022 at 11:44:07AM -0700, Dave Marchevsky wrote:
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>>>> +static bool __read_mostly allow_other_parent_userns;
>>>>>>>>>> +module_param(allow_other_parent_userns, bool, 0644);
>>>>>>>>>> +MODULE_PARM_DESC(allow_other_parent_userns,
>>>>>>>>>> + "Allow users not in mounting or descendant userns "
>>>>>>>>>> + "to access FUSE with allow_other set");
>>>>>>>>>
>>>>>>>>> The name of the parameter also suggests that access is granted to parent
>>>>>>>>> userns tasks whereas the change seems to me to allows every task access
>>>>>>>>> to that fuse filesystem independent of what userns they are in.
>>>>>>>>>
>>>>>>>>> So even a task in a sibling userns could - probably with rather
>>>>>>>>> elaborate mount propagation trickery - access that fuse filesystem.
>>>>>>>>>
>>>>>>>>> AFaict, either the module parameter is misnamed or the patch doesn't
>>>>>>>>> implement the behavior expressed in the name.
>>>>>>>>>
>>>>>>>>> The original patch restricted access to a CAP_SYS_ADMIN capable task.
>>>>>>>>> Did we agree that it was a good idea to weaken it to all tasks?
>>>>>>>>> Shouldn't we still just restrict this to CAP_SYS_ADMIN capable tasks in
>>>>>>>>> the initial userns?
>>>>>>>>
>>>>>>>> I think it's fine to allow for CAP_SYS_ADMIN only, but can we then
>>>>>>>> ignore the allow_other mount option in such case? The idea is that
>>>>>>>> CAP_SYS_ADMIN allows you to read FUSE-backed contents no matter what, so
>>>>>>>> user not mounting with allow_other preventing root from reading contents
>>>>>>>> defeats the purpose at least partially.
>>>>>>>
>>>>>>> If we want to be compatible with "user_allow_other", then it should be
>>>>>>> checking if the uid/gid of the current task is mapped in the
>>>>>>> filesystems user_ns (fsuidgid_has_mapping()).  Right?
>>>>>>
>>>>>> I think that's doable. So assuming we're still talking about requiring
>>>>>> cap_sys_admin then we'd roughly have sm like:
>>>>>>
>>>>>>         if (fc->allow_other)
>>>>>>                 return current_in_userns(fc->user_ns) ||
>>>>>>                         (capable(CAP_SYS_ADMIN) &&
>>>>>>                         fsuidgid_has_mapping(..., &init_user_ns));
>>>>>
>>>>> No, I meant this:
>>>>>
>>>>>         if (fc->allow_other)
>>>>>                 return current_in_userns(fc->user_ns) ||
>>>>>                         (userns_allow_other &&
>>>>>                         fsuidgid_has_mapping(..., &init_user_ns));
>>>>>
>>>>> But I think the OP wanted to allow real root to access the fs, which
>>>>> this doesn't allow (since 0 will have no mapping in the user ns), so
>>>>> I'm not sure what's the right solution...
>>>>
>>>> Right, so I was basically asking why not do something like this:
>>>>
>>>> $ git diff
>>>> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
>>>> index 74303d6e987b..8c04955eb26e 100644
>>>> --- a/fs/fuse/dir.c
>>>> +++ b/fs/fuse/dir.c
>>>> @@ -1224,6 +1224,9 @@ int fuse_allow_current_process(struct fuse_conn *fc)
>>>>  {
>>>>         const struct cred *cred;
>>>>
>>>> +       if (fuse_allow_sys_admin_access && capable(CAP_SYS_ADMIN))
>>>> +               return 1;
>>>> +
>>>>         if (fc->allow_other)
>>>>                 return current_in_userns(fc->user_ns);
>>>>
>>>>
>>>> where fuse_allow_sys_admin_access is module param which has to be
>>>> opted into through sysfs?
>>>
>>> You can either do this or do what I suggested in:
>>> https://lore.kernel.org/linux-fsdevel/20220613104604.t5ptuhrl2d4l7kbl@wittgenstein
>>> which is a bit more lax.
>>
>> My logic was that given we require opt-in and we are root, we
>> shouldn't be prevented from reading contents just because someone
>> didn't know about allow_other mount option. So I'd go with a simple
>> check before we even check fc-allow_other.
> 
> I don't see a problem with this but it other than that it subverts the
> allow_other mount option a bit tbh...

Will send v3 doing this shortly.

>>
>>>
>>> If you make it module load parameter only it has the advantage that it
>>> can't be changed after fuse has been loaded which in this case might be
>>> an advantage. It's likely that users might not be too happy if module
>>> semantics can be changed that drastically at runtime. But I have no
>>> strong opinions here.
>>>
>>
>> I'm not too familiar with this, whatever Dave was doing with
>> MODULE_PARM_DESC seems to be working fine? Did you have some other
>> preference for a specific param mechanism?
> 
> Nope, that one seems fine.

For our usecase, changing the behavior after FUSE module has been loaded is
preferable. Otherwise perf - or our internal tool - would have to emit some
message like "can't symbolicate these paths, please reload FUSE and try again"