Re: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL

[Michael Ellerman]

NeilBrown  writes:
> On Thu, Jun 15 2017, Andrew Morton wrote:
>> On Wed, 07 Jun 2017 12:08:38 +1000 NeilBrown  wrote:
>>> --- a/fs/autofs4/dev-ioctl.c
>>> +++ b/fs/autofs4/dev-ioctl.c
>>> @@ -344,7 +344,7 @@ static int autofs_dev_ioctl_fail(struct file *fp,
>>> int status;
>>>  
>>> token = (autofs_wqt_t) param->fail.token;
>>> -   status = param->fail.status ? param->fail.status : -ENOENT;
>>> +   status = param->fail.status < 0 ? param->fail.status : -ENOENT;
>>> return autofs4_wait_release(sbi, token, status);
>>>  }
>>
>> Sounds serious.  Was the absence of a cc:stable deliberate?
>
> You need CAP_SYS_ADMIN to  get the ioctl even looked at.  Doesn't that
> mean the bug can only be triggered by a process that could easily do
> worse?
>
> Or do containers allow admins to give out CAP_SYS_ADMIN to untrusted
> people??  I haven't been keeping up.

Yep. They can be configured individually in fact, eg:

  $ docker run --cap-drop=ALL --cap-add=sys_admin -it debian:jessie /bin/bash
  root@aedebe8c46e0:/# capsh --print
  Current: = cap_sys_admin+eip
  Bounding set =cap_sys_admin
  Securebits: 00/0x0/1'b0
   secure-noroot: no (unlocked)
   secure-no-suid-fixup: no (unlocked)
   secure-keep-caps: no (unlocked)
  uid=0(root)
  gid=0(root)
  groups=


cheers

Re: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL

[Michael Ellerman]

NeilBrown  writes:
> On Thu, Jun 15 2017, Andrew Morton wrote:
>> On Wed, 07 Jun 2017 12:08:38 +1000 NeilBrown  wrote:
>>> --- a/fs/autofs4/dev-ioctl.c
>>> +++ b/fs/autofs4/dev-ioctl.c
>>> @@ -344,7 +344,7 @@ static int autofs_dev_ioctl_fail(struct file *fp,
>>> int status;
>>>  
>>> token = (autofs_wqt_t) param->fail.token;
>>> -   status = param->fail.status ? param->fail.status : -ENOENT;
>>> +   status = param->fail.status < 0 ? param->fail.status : -ENOENT;
>>> return autofs4_wait_release(sbi, token, status);
>>>  }
>>
>> Sounds serious.  Was the absence of a cc:stable deliberate?
>
> You need CAP_SYS_ADMIN to  get the ioctl even looked at.  Doesn't that
> mean the bug can only be triggered by a process that could easily do
> worse?
>
> Or do containers allow admins to give out CAP_SYS_ADMIN to untrusted
> people??  I haven't been keeping up.

Yep. They can be configured individually in fact, eg:

  $ docker run --cap-drop=ALL --cap-add=sys_admin -it debian:jessie /bin/bash
  root@aedebe8c46e0:/# capsh --print
  Current: = cap_sys_admin+eip
  Bounding set =cap_sys_admin
  Securebits: 00/0x0/1'b0
   secure-noroot: no (unlocked)
   secure-no-suid-fixup: no (unlocked)
   secure-keep-caps: no (unlocked)
  uid=0(root)
  gid=0(root)
  groups=


cheers

Re: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL

[Ian Kent]

On Fri, 2017-06-16 at 12:13 +1000, NeilBrown wrote:
> On Thu, Jun 15 2017, Andrew Morton wrote:
> 
> > On Wed, 07 Jun 2017 12:08:38 +1000 NeilBrown  wrote:
> > 
> > > 
> > > If a positive status is passed with the AUTOFS_DEV_IOCTL_FAIL
> > > ioctl, autofs4_d_automount() will return
> > >    ERR_PTR(status)
> > > with that status to follow_automount(), which will then
> > > dereference an invalid pointer.
> > > 
> > > So treat a positive status the same as zero, and map
> > > to ENOENT.
> > > 
> > > See comment in systemd src/core/automount.c::automount_send_ready().
> > > 
> > > ...
> > > 
> > > --- a/fs/autofs4/dev-ioctl.c
> > > +++ b/fs/autofs4/dev-ioctl.c
> > > @@ -344,7 +344,7 @@ static int autofs_dev_ioctl_fail(struct file *fp,
> > >   int status;
> > >  
> > >   token = (autofs_wqt_t) param->fail.token;
> > > - status = param->fail.status ? param->fail.status : -ENOENT;
> > > + status = param->fail.status < 0 ? param->fail.status : -ENOENT;
> > >   return autofs4_wait_release(sbi, token, status);
> > >  }
> > 
> > Sounds serious.  Was the absence of a cc:stable deliberate?
> 
> You need CAP_SYS_ADMIN to  get the ioctl even looked at.  Doesn't that
> mean the bug can only be triggered by a process that could easily do
> worse?

Think so, yes.

> 
> Or do containers allow admins to give out CAP_SYS_ADMIN to untrusted
> people??  I haven't been keeping up.

Maybe, with docker root can start a container with --privileged to give the
container admin capabilities. It may be the case that capabilities can be used
now I don't know.

> 
> Given how simple the patch is, it probably makes sense to add a
> cc:stable, just in case.

IMHO it needs to be applied to stable as well.

> 
> Thanks,
> NeilBrown

Re: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL

[Ian Kent]

On Fri, 2017-06-16 at 12:13 +1000, NeilBrown wrote:
> On Thu, Jun 15 2017, Andrew Morton wrote:
> 
> > On Wed, 07 Jun 2017 12:08:38 +1000 NeilBrown  wrote:
> > 
> > > 
> > > If a positive status is passed with the AUTOFS_DEV_IOCTL_FAIL
> > > ioctl, autofs4_d_automount() will return
> > >    ERR_PTR(status)
> > > with that status to follow_automount(), which will then
> > > dereference an invalid pointer.
> > > 
> > > So treat a positive status the same as zero, and map
> > > to ENOENT.
> > > 
> > > See comment in systemd src/core/automount.c::automount_send_ready().
> > > 
> > > ...
> > > 
> > > --- a/fs/autofs4/dev-ioctl.c
> > > +++ b/fs/autofs4/dev-ioctl.c
> > > @@ -344,7 +344,7 @@ static int autofs_dev_ioctl_fail(struct file *fp,
> > >   int status;
> > >  
> > >   token = (autofs_wqt_t) param->fail.token;
> > > - status = param->fail.status ? param->fail.status : -ENOENT;
> > > + status = param->fail.status < 0 ? param->fail.status : -ENOENT;
> > >   return autofs4_wait_release(sbi, token, status);
> > >  }
> > 
> > Sounds serious.  Was the absence of a cc:stable deliberate?
> 
> You need CAP_SYS_ADMIN to  get the ioctl even looked at.  Doesn't that
> mean the bug can only be triggered by a process that could easily do
> worse?

Think so, yes.

> 
> Or do containers allow admins to give out CAP_SYS_ADMIN to untrusted
> people??  I haven't been keeping up.

Maybe, with docker root can start a container with --privileged to give the
container admin capabilities. It may be the case that capabilities can be used
now I don't know.

> 
> Given how simple the patch is, it probably makes sense to add a
> cc:stable, just in case.

IMHO it needs to be applied to stable as well.

> 
> Thanks,
> NeilBrown

Re: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL

[NeilBrown]

On Thu, Jun 15 2017, Andrew Morton wrote:

> On Wed, 07 Jun 2017 12:08:38 +1000 NeilBrown  wrote:
>
>> 
>> If a positive status is passed with the AUTOFS_DEV_IOCTL_FAIL
>> ioctl, autofs4_d_automount() will return
>>ERR_PTR(status)
>> with that status to follow_automount(), which will then
>> dereference an invalid pointer.
>> 
>> So treat a positive status the same as zero, and map
>> to ENOENT.
>> 
>> See comment in systemd src/core/automount.c::automount_send_ready().
>> 
>> ...
>>
>> --- a/fs/autofs4/dev-ioctl.c
>> +++ b/fs/autofs4/dev-ioctl.c
>> @@ -344,7 +344,7 @@ static int autofs_dev_ioctl_fail(struct file *fp,
>>  int status;
>>  
>>  token = (autofs_wqt_t) param->fail.token;
>> -status = param->fail.status ? param->fail.status : -ENOENT;
>> +status = param->fail.status < 0 ? param->fail.status : -ENOENT;
>>  return autofs4_wait_release(sbi, token, status);
>>  }
>
> Sounds serious.  Was the absence of a cc:stable deliberate?

You need CAP_SYS_ADMIN to  get the ioctl even looked at.  Doesn't that
mean the bug can only be triggered by a process that could easily do
worse?

Or do containers allow admins to give out CAP_SYS_ADMIN to untrusted
people??  I haven't been keeping up.

Given how simple the patch is, it probably makes sense to add a
cc:stable, just in case.

Thanks,
NeilBrown


signature.asc
Description: PGP signature

Re: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL

[NeilBrown]

On Thu, Jun 15 2017, Andrew Morton wrote:

> On Wed, 07 Jun 2017 12:08:38 +1000 NeilBrown  wrote:
>
>> 
>> If a positive status is passed with the AUTOFS_DEV_IOCTL_FAIL
>> ioctl, autofs4_d_automount() will return
>>ERR_PTR(status)
>> with that status to follow_automount(), which will then
>> dereference an invalid pointer.
>> 
>> So treat a positive status the same as zero, and map
>> to ENOENT.
>> 
>> See comment in systemd src/core/automount.c::automount_send_ready().
>> 
>> ...
>>
>> --- a/fs/autofs4/dev-ioctl.c
>> +++ b/fs/autofs4/dev-ioctl.c
>> @@ -344,7 +344,7 @@ static int autofs_dev_ioctl_fail(struct file *fp,
>>  int status;
>>  
>>  token = (autofs_wqt_t) param->fail.token;
>> -status = param->fail.status ? param->fail.status : -ENOENT;
>> +status = param->fail.status < 0 ? param->fail.status : -ENOENT;
>>  return autofs4_wait_release(sbi, token, status);
>>  }
>
> Sounds serious.  Was the absence of a cc:stable deliberate?

You need CAP_SYS_ADMIN to  get the ioctl even looked at.  Doesn't that
mean the bug can only be triggered by a process that could easily do
worse?

Or do containers allow admins to give out CAP_SYS_ADMIN to untrusted
people??  I haven't been keeping up.

Given how simple the patch is, it probably makes sense to add a
cc:stable, just in case.

Thanks,
NeilBrown


signature.asc
Description: PGP signature

Re: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL

[Andrew Morton]

On Wed, 07 Jun 2017 12:08:38 +1000 NeilBrown  wrote:

> 
> If a positive status is passed with the AUTOFS_DEV_IOCTL_FAIL
> ioctl, autofs4_d_automount() will return
>ERR_PTR(status)
> with that status to follow_automount(), which will then
> dereference an invalid pointer.
> 
> So treat a positive status the same as zero, and map
> to ENOENT.
> 
> See comment in systemd src/core/automount.c::automount_send_ready().
> 
> ...
>
> --- a/fs/autofs4/dev-ioctl.c
> +++ b/fs/autofs4/dev-ioctl.c
> @@ -344,7 +344,7 @@ static int autofs_dev_ioctl_fail(struct file *fp,
>   int status;
>  
>   token = (autofs_wqt_t) param->fail.token;
> - status = param->fail.status ? param->fail.status : -ENOENT;
> + status = param->fail.status < 0 ? param->fail.status : -ENOENT;
>   return autofs4_wait_release(sbi, token, status);
>  }

Sounds serious.  Was the absence of a cc:stable deliberate?

Re: [PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL

[Andrew Morton]

On Wed, 07 Jun 2017 12:08:38 +1000 NeilBrown  wrote:

> 
> If a positive status is passed with the AUTOFS_DEV_IOCTL_FAIL
> ioctl, autofs4_d_automount() will return
>ERR_PTR(status)
> with that status to follow_automount(), which will then
> dereference an invalid pointer.
> 
> So treat a positive status the same as zero, and map
> to ENOENT.
> 
> See comment in systemd src/core/automount.c::automount_send_ready().
> 
> ...
>
> --- a/fs/autofs4/dev-ioctl.c
> +++ b/fs/autofs4/dev-ioctl.c
> @@ -344,7 +344,7 @@ static int autofs_dev_ioctl_fail(struct file *fp,
>   int status;
>  
>   token = (autofs_wqt_t) param->fail.token;
> - status = param->fail.status ? param->fail.status : -ENOENT;
> + status = param->fail.status < 0 ? param->fail.status : -ENOENT;
>   return autofs4_wait_release(sbi, token, status);
>  }

Sounds serious.  Was the absence of a cc:stable deliberate?

[PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL

[NeilBrown]

If a positive status is passed with the AUTOFS_DEV_IOCTL_FAIL
ioctl, autofs4_d_automount() will return
   ERR_PTR(status)
with that status to follow_automount(), which will then
dereference an invalid pointer.

So treat a positive status the same as zero, and map
to ENOENT.

See comment in systemd src/core/automount.c::automount_send_ready().

Signed-off-by: NeilBrown 
---
 fs/autofs4/dev-ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index 734cbf8d9676..dd9f1bebb5a3 100644
--- a/fs/autofs4/dev-ioctl.c
+++ b/fs/autofs4/dev-ioctl.c
@@ -344,7 +344,7 @@ static int autofs_dev_ioctl_fail(struct file *fp,
int status;
 
token = (autofs_wqt_t) param->fail.token;
-   status = param->fail.status ? param->fail.status : -ENOENT;
+   status = param->fail.status < 0 ? param->fail.status : -ENOENT;
return autofs4_wait_release(sbi, token, status);
 }
 
-- 
2.12.2



signature.asc
Description: PGP signature

[PATCH] autofs: sanity check status reported with AUTOFS_DEV_IOCTL_FAIL

[NeilBrown]

If a positive status is passed with the AUTOFS_DEV_IOCTL_FAIL
ioctl, autofs4_d_automount() will return
   ERR_PTR(status)
with that status to follow_automount(), which will then
dereference an invalid pointer.

So treat a positive status the same as zero, and map
to ENOENT.

See comment in systemd src/core/automount.c::automount_send_ready().

Signed-off-by: NeilBrown 
---
 fs/autofs4/dev-ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/autofs4/dev-ioctl.c b/fs/autofs4/dev-ioctl.c
index 734cbf8d9676..dd9f1bebb5a3 100644
--- a/fs/autofs4/dev-ioctl.c
+++ b/fs/autofs4/dev-ioctl.c
@@ -344,7 +344,7 @@ static int autofs_dev_ioctl_fail(struct file *fp,
int status;
 
token = (autofs_wqt_t) param->fail.token;
-   status = param->fail.status ? param->fail.status : -ENOENT;
+   status = param->fail.status < 0 ? param->fail.status : -ENOENT;
return autofs4_wait_release(sbi, token, status);
 }
 
-- 
2.12.2



signature.asc
Description: PGP signature