On Wed 27-08-25 21:43:09, Amir Goldstein wrote:
> Commit 620c266f39493 ("fhandle: relax open_by_handle_at() permission
> checks") relaxed the coditions for decoding a file handle from non init
> userns.
>
> The conditions are that that decoded dentry is accessible from the user
> provided mountfd (or to fs root) and that all the ancestors along the
> path have a valid id mapping in the userns.
>
> These conditions are intentionally more strict than the condition that
> the decoded dentry should be "lookable" by path from the mountfd.
>
> For example, the path /home/amir/dir/subdir is lookable by path from
> unpriv userns of user amir, because /home perms is 755, but the owner of
> /home does not have a valid id mapping in unpriv userns of user amir.
>
> The current code did not check that the decoded dentry itself has a
> valid id mapping in the userns. There is no security risk in that,
> because that final open still performs the needed permission checks,
> but this is inconsistent with the checks performed on the ancestors,
> so the behavior can be a bit confusing.
>
> Add the check for the decoded dentry itself, so that the entire path,
> including the last component has a valid id mapping in the userns.
>
> Fixes: 620c266f39493 ("fhandle: relax open_by_handle_at() permission checks")
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
Yeah, probably it's less surprising this way. Feel free to add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
Honza
> ---
> fs/fhandle.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 68a7d2861c58f..a907ddfac4d51 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -207,6 +207,14 @@ static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
> if (!ctx->flags)
> return 1;
>
> + /*
> + * Verify that the decoded dentry itself has a valid id mapping.
> + * In case the decoded dentry is the mountfd root itself, this
> + * verifies that the mountfd inode itself has a valid id mapping.
> + */
> + if (!privileged_wrt_inode_uidgid(user_ns, idmap, d_inode(dentry)))
> + return 0;
> +
> /*
> * It's racy as we're not taking rename_lock but we're able to ignore
> * permissions and we just need an approximation whether we were able
> --
> 2.50.1
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
