diff mbox series

cifs: do not allow creating sockets in SMB1

Message ID 20180419005237.11404-2-lsahlber@redhat.com
State New
Headers show
Series cifs: fix kernel NULL deref | expand

Commit Message

Ronnie Sahlberg April 19, 2018, 12:52 a.m. UTC 
RHBZ: 1453123

Since at least the 3.10 kernel and likely a lot earlier we have
not been able to create unix domain sockets in a cifs share.
Trying to create a socket, for example using the af_unix command from
xfstests will cause :
BUG: unable to handle kernel NULL pointer dereference at 00000000
00000040

Since no one uses or depends on being able to create unix domains sockets
on a cifs share the easiest fix to stop this vulnerability is to simply
disallow creation of such sockets.

Reported-by: Eryu Guan <eguan@redhat.com>
Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
---
 fs/cifs/dir.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Steve French April 19, 2018, 1:42 a.m. UTC | #1 
I think the safer patch is to catch all invalid special file types
(not just S_ISSOCK) but putting in the missing "else" clause (see
below) - but when I tested it I got a different return code than
expected mapped by the caller ... so perhaps we need to return a
different return code other than EOPNOTSUPP

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 81ba6e0d88d8..ebb40ad7ec7f 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -742,7 +742,8 @@ int cifs_mknod(struct inode *inode, struct dentry
*direntry, umode_t mode,
                pdev->minor = cpu_to_le64(MINOR(device_number));
                rc = tcon->ses->server->ops->sync_write(xid, &fid, &io_parms,
                                                        &bytes_written, iov, 1);
-       } /* else if (S_ISFIFO) */
+       } else
+               rc = -EOPNOTSUPP;
        tcon->ses->server->ops->close(xid, tcon, &fid);
        d_drop(direntry);


On Wed, Apr 18, 2018 at 7:52 PM, Ronnie Sahlberg <lsahlber@redhat.com> wrote:
> RHBZ: 1453123
>
> Since at least the 3.10 kernel and likely a lot earlier we have
> not been able to create unix domain sockets in a cifs share.
> Trying to create a socket, for example using the af_unix command from
> xfstests will cause :
> BUG: unable to handle kernel NULL pointer dereference at 00000000
> 00000040
>
> Since no one uses or depends on being able to create unix domains sockets
> on a cifs share the easiest fix to stop this vulnerability is to simply
> disallow creation of such sockets.
>
> Reported-by: Eryu Guan <eguan@redhat.com>
> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> ---
>  fs/cifs/dir.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index 81ba6e0d88d8..632a35be85ea 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -684,6 +684,9 @@ int cifs_mknod(struct inode *inode, struct dentry *direntry, umode_t mode,
>                 goto mknod_out;
>         }
>
> +       if (S_ISSOCK(mode))
> +               return -EINVAL;
> +
>         if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL))
>                 goto mknod_out;
>
> --
> 2.13.3
>
Steve French April 19, 2018, 1:50 a.m. UTC | #2 
changing it to EPERM worked - but I think the check for the valid
types needs to be moved earlier so we don't leave the newly created
file around on failure.  My revised patch (changing EOPNOTSUPP to
EPERM) does work, but ... better if it failed before creating the
file.

# python -c "import socket as s; sock = s.socket(s.AF_UNIX);
sock.bind('/mnt1/somesoc3ket')"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/usr/lib/python2.7/socket.py", line 228, in meth
    return getattr(self._sock,name)(*args)
socket.error: [Errno 1] Operation not permitted

On Wed, Apr 18, 2018 at 8:42 PM, Steve French <smfrench@gmail.com> wrote:
> I think the safer patch is to catch all invalid special file types
> (not just S_ISSOCK) but putting in the missing "else" clause (see
> below) - but when I tested it I got a different return code than
> expected mapped by the caller ... so perhaps we need to return a
> different return code other than EOPNOTSUPP
>
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index 81ba6e0d88d8..ebb40ad7ec7f 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -742,7 +742,8 @@ int cifs_mknod(struct inode *inode, struct dentry
> *direntry, umode_t mode,
>                 pdev->minor = cpu_to_le64(MINOR(device_number));
>                 rc = tcon->ses->server->ops->sync_write(xid, &fid, &io_parms,
>                                                         &bytes_written, iov, 1);
> -       } /* else if (S_ISFIFO) */
> +       } else
> +               rc = -EOPNOTSUPP;
>         tcon->ses->server->ops->close(xid, tcon, &fid);
>         d_drop(direntry);
>
>
> On Wed, Apr 18, 2018 at 7:52 PM, Ronnie Sahlberg <lsahlber@redhat.com> wrote:
>> RHBZ: 1453123
>>
>> Since at least the 3.10 kernel and likely a lot earlier we have
>> not been able to create unix domain sockets in a cifs share.
>> Trying to create a socket, for example using the af_unix command from
>> xfstests will cause :
>> BUG: unable to handle kernel NULL pointer dereference at 00000000
>> 00000040
>>
>> Since no one uses or depends on being able to create unix domains sockets
>> on a cifs share the easiest fix to stop this vulnerability is to simply
>> disallow creation of such sockets.
>>
>> Reported-by: Eryu Guan <eguan@redhat.com>
>> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
>> ---
>>  fs/cifs/dir.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
>> index 81ba6e0d88d8..632a35be85ea 100644
>> --- a/fs/cifs/dir.c
>> +++ b/fs/cifs/dir.c
>> @@ -684,6 +684,9 @@ int cifs_mknod(struct inode *inode, struct dentry *direntry, umode_t mode,
>>                 goto mknod_out;
>>         }
>>
>> +       if (S_ISSOCK(mode))
>> +               return -EINVAL;
>> +
>>         if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL))
>>                 goto mknod_out;
>>
>> --
>> 2.13.3
>>
>
>
>
> --
> Thanks,
>
> Steve
Ronnie Sahlberg April 19, 2018, 2:10 a.m. UTC | #3 
----- Original Message -----
> From: "Steve French" <smfrench@gmail.com>
> To: "Ronnie Sahlberg" <lsahlber@redhat.com>
> Cc: "linux-cifs" <linux-cifs@vger.kernel.org>
> Sent: Thursday, 19 April, 2018 11:50:35 AM
> Subject: Re: [PATCH] cifs: do not allow creating sockets in SMB1
> 
> changing it to EPERM worked - but I think the check for the valid
> types needs to be moved earlier so we don't leave the newly created
> file around on failure.  My revised patch (changing EOPNOTSUPP to
> EPERM) does work, but ... better if it failed before creating the
> file.

Yes, we want it to fail before it creates the file.

> 
> # python -c "import socket as s; sock = s.socket(s.AF_UNIX);
> sock.bind('/mnt1/somesoc3ket')"
> Traceback (most recent call last):
>   File "<string>", line 1, in <module>
>   File "/usr/lib/python2.7/socket.py", line 228, in meth
>     return getattr(self._sock,name)(*args)
> socket.error: [Errno 1] Operation not permitted
> 
> On Wed, Apr 18, 2018 at 8:42 PM, Steve French <smfrench@gmail.com> wrote:
> > I think the safer patch is to catch all invalid special file types
> > (not just S_ISSOCK) but putting in the missing "else" clause (see
> > below) - but when I tested it I got a different return code than
> > expected mapped by the caller ... so perhaps we need to return a
> > different return code other than EOPNOTSUPP
> >
> > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> > index 81ba6e0d88d8..ebb40ad7ec7f 100644
> > --- a/fs/cifs/dir.c
> > +++ b/fs/cifs/dir.c
> > @@ -742,7 +742,8 @@ int cifs_mknod(struct inode *inode, struct dentry
> > *direntry, umode_t mode,
> >                 pdev->minor = cpu_to_le64(MINOR(device_number));
> >                 rc = tcon->ses->server->ops->sync_write(xid, &fid,
> >                 &io_parms,
> >                                                         &bytes_written,
> >                                                         iov, 1);
> > -       } /* else if (S_ISFIFO) */
> > +       } else
> > +               rc = -EOPNOTSUPP;
> >         tcon->ses->server->ops->close(xid, tcon, &fid);
> >         d_drop(direntry);
> >
> >
> > On Wed, Apr 18, 2018 at 7:52 PM, Ronnie Sahlberg <lsahlber@redhat.com>
> > wrote:
> >> RHBZ: 1453123
> >>
> >> Since at least the 3.10 kernel and likely a lot earlier we have
> >> not been able to create unix domain sockets in a cifs share.
> >> Trying to create a socket, for example using the af_unix command from
> >> xfstests will cause :
> >> BUG: unable to handle kernel NULL pointer dereference at 00000000
> >> 00000040
> >>
> >> Since no one uses or depends on being able to create unix domains sockets
> >> on a cifs share the easiest fix to stop this vulnerability is to simply
> >> disallow creation of such sockets.
> >>
> >> Reported-by: Eryu Guan <eguan@redhat.com>
> >> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com>
> >> ---
> >>  fs/cifs/dir.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> >> index 81ba6e0d88d8..632a35be85ea 100644
> >> --- a/fs/cifs/dir.c
> >> +++ b/fs/cifs/dir.c
> >> @@ -684,6 +684,9 @@ int cifs_mknod(struct inode *inode, struct dentry
> >> *direntry, umode_t mode,
> >>                 goto mknod_out;
> >>         }
> >>
> >> +       if (S_ISSOCK(mode))
> >> +               return -EINVAL;
> >> +
> >>         if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL))
> >>                 goto mknod_out;
> >>
> >> --
> >> 2.13.3
> >>
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
> 
> 
> 
> --
> Thanks,
> 
> Steve
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 81ba6e0d88d8..632a35be85ea 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -684,6 +684,9 @@  int cifs_mknod(struct inode *inode, struct dentry *direntry, umode_t mode,
 		goto mknod_out;
 	}
 
+	if (S_ISSOCK(mode))
+		return -EINVAL;
+
 	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL))
 		goto mknod_out;