[PATCH 0/2] userns: automount cleanups

[PATCH 0/2] userns: automount cleanups

[Eric W. Biederman]

While reviewing some code I realized that in getting d_automount working
with s_user_ns I had left behind some unnecessary relics of the blind
path I started down.  Here are two patches that remove those relics.

Unless someone has another preference I will drop them in my userns tree
and merge them that way.

Eric W. Biederman (2):
      userns: Don't fail follow_automount based on s_user_ns
      autofs4: Modify autofs_wait to use current_uid() and current_gid()

 fs/autofs4/waitq.c | 4 ++--
 fs/namei.c         | 3 ---
 2 files changed, 2 insertions(+), 5 deletions(-)

[PATCH 1/2] userns: Don't fail follow_automount based on s_user_ns

[Eric W. Biederman]

When vfs_submount was added the test to limit automounts from
filesystems that with s_user_ns != &init_user_ns accidentially left
in follow_automount.  The test was never about any security concerns
and was always about how do we implement this for filesystems whose
s_user_ns != &init_user_ns.

At the moment this check makes no difference as there are no
filesystems that both set FS_USERNS_MOUNT and implement d_automount.

Remove this check now while I am thinking about it so there will not
be odd booby traps for someone who does want to make this combination
work.

vfs_submount still needs improvements to allow this combination to work,
and vfs_submount contains a check that presents a warning.

The autofs4 filesystem could be modified to set FS_USERNS_MOUNT and it would
need not work on this code path, as userspace performs the mounts.

Fixes: 93faccbbfa95 ("fs: Better permission checking for submounts")
Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems creds")
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 fs/namei.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index f0c7a7b9b6ca..f47118ed36e7 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1142,9 +1142,6 @@ static int follow_automount(struct path *path, struct nameidata *nd,
 			return -ENOENT;
 	}
 
-	if (path->dentry->d_sb->s_user_ns != &init_user_ns)
-		return -EACCES;
-
 	nd->total_link_count++;
 	if (nd->total_link_count >= 40)
 		return -ELOOP;
-- 
2.14.1

[PATCH 2/2] autofs4: Modify autofs_wait to use current_uid() and current_gid()

[Eric W. Biederman]

The code used to do that and then I mucked with it and never quite put
the code back.  Today the code references current_cred()->uid and
current_cred()->gid which is equivalent but more wordy, and not
idiomatic.

Fixes: 93faccbbfa95 ("fs: Better permission checking for submounts")
Fixes: 069d5ac9ae0d ("autofs:  Fix automounts by using current_real_cred()->uid")
Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 fs/autofs4/waitq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index 8fc41705c7cd..9908ecf7fce0 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -443,8 +443,8 @@ int autofs4_wait(struct autofs_sb_info *sbi,
 		memcpy(&wq->name, &qstr, sizeof(struct qstr));
 		wq->dev = autofs4_get_dev(sbi);
 		wq->ino = autofs4_get_ino(sbi);
-		wq->uid = current_cred()->uid;
-		wq->gid = current_cred()->gid;
+		wq->uid = current_uid();
+		wq->gid = current_gid();
 		wq->pid = pid;
 		wq->tgid = tgid;
 		wq->status = -EINTR; /* Status return if interrupted */
-- 
2.14.1

[PATCH 2/2] autofs4: Modify autofs_wait to use current_uid() and current_gid()

[Eric W. Biederman]

The code used to do that and then I mucked with it and never quite put
the code back.  Today the code references current_cred()->uid and
current_cred()->gid which is equivalent but more wordy, and not
idiomatic.

Fixes: 93faccbbfa95 ("fs: Better permission checking for submounts")
Fixes: 069d5ac9ae0d ("autofs:  Fix automounts by using current_real_cred()->uid")
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/autofs4/waitq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/autofs4/waitq.c b/fs/autofs4/waitq.c
index 8fc41705c7cd..9908ecf7fce0 100644
--- a/fs/autofs4/waitq.c
+++ b/fs/autofs4/waitq.c
@@ -443,8 +443,8 @@ int autofs4_wait(struct autofs_sb_info *sbi,
 		memcpy(&wq->name, &qstr, sizeof(struct qstr));
 		wq->dev = autofs4_get_dev(sbi);
 		wq->ino = autofs4_get_ino(sbi);
-		wq->uid = current_cred()->uid;
-		wq->gid = current_cred()->gid;
+		wq->uid = current_uid();
+		wq->gid = current_gid();
 		wq->pid = pid;
 		wq->tgid = tgid;
 		wq->status = -EINTR; /* Status return if interrupted */
-- 
2.14.1

Re: [PATCH 0/2] userns: automount cleanups

[Ian Kent]

On 30/11/17 08:01, Eric W. Biederman wrote:
> 
> While reviewing some code I realized that in getting d_automount working
> with s_user_ns I had left behind some unnecessary relics of the blind
> path I started down.  Here are two patches that remove those relics.
> 
> Unless someone has another preference I will drop them in my userns tree
> and merge them that way.

I saw the "<etc>->s_user_ns != &init_user_ns" and wondered if that would
trigger for automount(8) run entirely with a container (eg. docker)?

Anyway, it's gone now, so ACK to these two, thanks Eric.

> 
> Eric W. Biederman (2):
>       userns: Don't fail follow_automount based on s_user_ns
>       autofs4: Modify autofs_wait to use current_uid() and current_gid()
> 
>  fs/autofs4/waitq.c | 4 ++--
>  fs/namei.c         | 3 ---
>  2 files changed, 2 insertions(+), 5 deletions(-)
> 
> 

[PATCH 1/2] userns: Don't fail follow_automount based on s_user_ns

[Eric W. Biederman]

When vfs_submount was added the test to limit automounts from
filesystems that with s_user_ns != &init_user_ns accidentially left
in follow_automount.  The test was never about any security concerns
and was always about how do we implement this for filesystems whose
s_user_ns != &init_user_ns.

At the moment this check makes no difference as there are no
filesystems that both set FS_USERNS_MOUNT and implement d_automount.

Remove this check now while I am thinking about it so there will not
be odd booby traps for someone who does want to make this combination
work.

vfs_submount still needs improvements to allow this combination to work,
and vfs_submount contains a check that presents a warning.

The autofs4 filesystem could be modified to set FS_USERNS_MOUNT and it would
need not work on this code path, as userspace performs the mounts.

Fixes: 93faccbbfa95 ("fs: Better permission checking for submounts")
Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems creds")
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/namei.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index f0c7a7b9b6ca..f47118ed36e7 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1142,9 +1142,6 @@ static int follow_automount(struct path *path, struct nameidata *nd,
 			return -ENOENT;
 	}
 
-	if (path->dentry->d_sb->s_user_ns != &init_user_ns)
-		return -EACCES;
-
 	nd->total_link_count++;
 	if (nd->total_link_count >= 40)
 		return -ELOOP;
-- 
2.14.1

Re: [PATCH 0/2] userns: automount cleanups

[Ian Kent]

On 30/11/17 08:01, Eric W. Biederman wrote:
> 
> While reviewing some code I realized that in getting d_automount working
> with s_user_ns I had left behind some unnecessary relics of the blind
> path I started down.  Here are two patches that remove those relics.
> 
> Unless someone has another preference I will drop them in my userns tree
> and merge them that way.

I saw the "<etc>->s_user_ns != &init_user_ns" and wondered if that would
trigger for automount(8) run entirely with a container (eg. docker)?

Anyway, it's gone now, so ACK to these two, thanks Eric.

> 
> Eric W. Biederman (2):
>       userns: Don't fail follow_automount based on s_user_ns
>       autofs4: Modify autofs_wait to use current_uid() and current_gid()
> 
>  fs/autofs4/waitq.c | 4 ++--
>  fs/namei.c         | 3 ---
>  2 files changed, 2 insertions(+), 5 deletions(-)
> 
> 

Re: [PATCH 0/2] userns: automount cleanups

[Eric W. Biederman]

Ian Kent <raven@themaw.net> writes:

> On 30/11/17 08:01, Eric W. Biederman wrote:
>> 
>> While reviewing some code I realized that in getting d_automount working
>> with s_user_ns I had left behind some unnecessary relics of the blind
>> path I started down.  Here are two patches that remove those relics.
>> 
>> Unless someone has another preference I will drop them in my userns tree
>> and merge them that way.
>
> I saw the "<etc>->s_user_ns != &init_user_ns" and wondered if that would
> trigger for automount(8) run entirely with a container (eg. docker)?

autofs still needs FS_USERNS_MOUNT before you can reach that point.  But
docker does have a mode ?--userns-remap? where it sets up the containers
mounts that way.

I think in principle that should work and be safe.  I don't know how
robust autofs is against malicious users.  Which is the question to ask
before actually adding FS_USERNS_MOUNT in struct file_system_type.

> Anyway, it's gone now, so ACK to these two, thanks Eric.

Eric

Re: [PATCH 0/2] userns: automount cleanups

[Ian Kent]

On 30/11/17 13:21, Eric W. Biederman wrote:
> Ian Kent <raven-PKsaG3nR2I+sTnJN9+BGXg@public.gmane.org> writes:
> 
>> On 30/11/17 08:01, Eric W. Biederman wrote:
>>>
>>> While reviewing some code I realized that in getting d_automount working
>>> with s_user_ns I had left behind some unnecessary relics of the blind
>>> path I started down.  Here are two patches that remove those relics.
>>>
>>> Unless someone has another preference I will drop them in my userns tree
>>> and merge them that way.
>>
>> I saw the "<etc>->s_user_ns != &init_user_ns" and wondered if that would
>> trigger for automount(8) run entirely with a container (eg. docker)?
> 
> autofs still needs FS_USERNS_MOUNT before you can reach that point.  But
> docker does have a mode ?--userns-remap? where it sets up the containers
> mounts that way.
> 
> I think in principle that should work and be safe.  I don't know how
> robust autofs is against malicious users.  Which is the question to ask
> before actually adding FS_USERNS_MOUNT in struct file_system_type.

automount(8) thinks it is running as uid 0 (which it is in the container)
so this would require a bit of thought since automount(8) doesn't take
special precautions.

The restrictions, in the case of running entirely within a container, are
those of the container itself. For example, if there are no granular
capabilities available then the admin needs to run the container as
privileged in order to be able to mount NFS file systems, which is a
common usage case. 

Ian

Re: [PATCH 0/2] userns: automount cleanups

[Ian Kent]

On 30/11/17 13:21, Eric W. Biederman wrote:
> Ian Kent <raven@themaw.net> writes:
> 
>> On 30/11/17 08:01, Eric W. Biederman wrote:
>>>
>>> While reviewing some code I realized that in getting d_automount working
>>> with s_user_ns I had left behind some unnecessary relics of the blind
>>> path I started down.  Here are two patches that remove those relics.
>>>
>>> Unless someone has another preference I will drop them in my userns tree
>>> and merge them that way.
>>
>> I saw the "<etc>->s_user_ns != &init_user_ns" and wondered if that would
>> trigger for automount(8) run entirely with a container (eg. docker)?
> 
> autofs still needs FS_USERNS_MOUNT before you can reach that point.  But
> docker does have a mode ?--userns-remap? where it sets up the containers
> mounts that way.
> 
> I think in principle that should work and be safe.  I don't know how
> robust autofs is against malicious users.  Which is the question to ask
> before actually adding FS_USERNS_MOUNT in struct file_system_type.

automount(8) thinks it is running as uid 0 (which it is in the container)
so this would require a bit of thought since automount(8) doesn't take
special precautions.

The restrictions, in the case of running entirely within a container, are
those of the container itself. For example, if there are no granular
capabilities available then the admin needs to run the container as
privileged in order to be able to mount NFS file systems, which is a
common usage case. 

Ian

Re: [PATCH 0/2] userns: automount cleanups

[Eric W. Biederman]

Ian Kent <raven-PKsaG3nR2I+sTnJN9+BGXg@public.gmane.org> writes:

> On 30/11/17 08:01, Eric W. Biederman wrote:
>> 
>> While reviewing some code I realized that in getting d_automount working
>> with s_user_ns I had left behind some unnecessary relics of the blind
>> path I started down.  Here are two patches that remove those relics.
>> 
>> Unless someone has another preference I will drop them in my userns tree
>> and merge them that way.
>
> I saw the "<etc>->s_user_ns != &init_user_ns" and wondered if that would
> trigger for automount(8) run entirely with a container (eg. docker)?

autofs still needs FS_USERNS_MOUNT before you can reach that point.  But
docker does have a mode ?--userns-remap? where it sets up the containers
mounts that way.

I think in principle that should work and be safe.  I don't know how
robust autofs is against malicious users.  Which is the question to ask
before actually adding FS_USERNS_MOUNT in struct file_system_type.

> Anyway, it's gone now, so ACK to these two, thanks Eric.

Eric

[PATCH 0/2] userns: automount cleanups

[Eric W. Biederman]

While reviewing some code I realized that in getting d_automount working
with s_user_ns I had left behind some unnecessary relics of the blind
path I started down.  Here are two patches that remove those relics.

Unless someone has another preference I will drop them in my userns tree
and merge them that way.

Eric W. Biederman (2):
      userns: Don't fail follow_automount based on s_user_ns
      autofs4: Modify autofs_wait to use current_uid() and current_gid()

 fs/autofs4/waitq.c | 4 ++--
 fs/namei.c         | 3 ---
 2 files changed, 2 insertions(+), 5 deletions(-)