Changes with v2 version: when unexpected fs_locations occurs:
1. Throw print message "unexpected fs_locations"
2. Stop parsing xdr and return EIO error
----
There is a remote vulnerability for nfs4-client. That allows
a remote NFSv4 server or attacker in middle rewrite memory on
NFSv4 client.
Of course if encryption is used then only server can do corruption.
1. Client send GETATTR request to server, but there are not
checks for bitmask in reply from server. Therefore server may return
more attributes than were asked.
2. Pointer to struct nfs_fattr is passed to decode_getfattr(), but
that parameter is considered as pointer to struct nfs4_fs_locations
in case set FATTR4_WORD0_FS_LOCATIONS in reply from server.
But there is no check for pointer type that was passed!
3. Remote side can set FATTR4_WORD0_FS_LOCATIONS and fill
datas fsattr4_fs_locations in reply to client.
4. sizeof(nfs4_fs_locations) is about 92080 bytes in kernel 2.6.38
So ~ 90 Kbytes can be overwritten in memory of NFSv4 client.
Fortunately, nfs_fattr is allocated via kmalloc and
is not stored on stack. So generally kmalloc-192 is corrupted.
Affected kernels: all from v2.6.18
Commit 683b57b435326eb512c7305892683b6205669448
"NFSv4: Implement the fs_locations function call"
---
fs/nfs/nfs4xdr.c | 39 +++++++++++++++++++++++++++++----------
1 files changed, 29 insertions(+), 10 deletions(-)
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index c87e543..6fea9c9 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -3451,7 +3451,8 @@ out_overflow:
return -EIO;
}
-static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, struct nfs4_fs_locations *res)
+static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, struct nfs4_fs_locations *res,
+ uint32_t *deny_bitmap)
{
int n;
__be32 *p;
@@ -3462,6 +3463,13 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
status = 0;
if (unlikely(!(bitmap[0] & FATTR4_WORD0_FS_LOCATIONS)))
goto out;
+
+ if (unlikely(deny_bitmap[0] & FATTR4_WORD0_FS_LOCATIONS)) {
+ status = -EIO;
+ printk(KERN_WARNING "%s: Unexpected fs_locations\n", __func__);
+ goto out;
+ }
+
dprintk("%s: fsroot ", __func__);
status = decode_pathname(xdr, &res->fs_path);
if (unlikely(status != 0))
@@ -4186,7 +4194,8 @@ xdr_error:
static int decode_getfattr_attrs(struct xdr_stream *xdr, uint32_t *bitmap,
struct nfs_fattr *fattr, struct nfs_fh *fh,
- const struct nfs_server *server, int may_sleep)
+ const struct nfs_server *server, int may_sleep,
+ uint32_t *deny_bitmap)
{
int status;
umode_t fmode = 0;
@@ -4232,7 +4241,7 @@ static int decode_getfattr_attrs(struct xdr_stream *xdr, uint32_t *bitmap,
status = decode_attr_fs_locations(xdr, bitmap, container_of(fattr,
struct nfs4_fs_locations,
- fattr));
+ fattr), deny_bitmap);
if (status < 0)
goto xdr_error;
fattr->valid |= status;
@@ -4301,11 +4310,13 @@ xdr_error:
}
static int decode_getfattr_generic(struct xdr_stream *xdr, struct nfs_fattr *fattr,
- struct nfs_fh *fh, const struct nfs_server *server, int may_sleep)
+ struct nfs_fh *fh, const struct nfs_server *server, int may_sleep,
+ int expect_fsloc)
{
__be32 *savep;
uint32_t attrlen,
bitmap[3] = {0};
+ uint32_t deny_bitmap[3] = {0};
int status;
status = decode_op_hdr(xdr, OP_GETATTR);
@@ -4316,11 +4327,15 @@ static int decode_getfattr_generic(struct xdr_stream *xdr, struct nfs_fattr *fat
if (status < 0)
goto xdr_error;
+ if (!expect_fsloc)
+ deny_bitmap[0] |= FATTR4_WORD0_FS_LOCATIONS;
+
status = decode_attr_length(xdr, &attrlen, &savep);
if (status < 0)
goto xdr_error;
- status = decode_getfattr_attrs(xdr, bitmap, fattr, fh, server, may_sleep);
+ status = decode_getfattr_attrs(xdr, bitmap, fattr, fh, server, may_sleep,
+ deny_bitmap);
if (status < 0)
goto xdr_error;
@@ -4333,7 +4348,7 @@ xdr_error:
static int decode_getfattr(struct xdr_stream *xdr, struct nfs_fattr *fattr,
const struct nfs_server *server, int may_sleep)
{
- return decode_getfattr_generic(xdr, fattr, NULL, server, may_sleep);
+ return decode_getfattr_generic(xdr, fattr, NULL, server, may_sleep, 0);
}
/*
@@ -6338,9 +6353,11 @@ static int nfs4_xdr_dec_fs_locations(struct rpc_rqst *req,
if (status)
goto out;
xdr_enter_page(xdr, PAGE_SIZE);
- status = decode_getfattr(xdr, &res->fs_locations->fattr,
+ status = decode_getfattr_generic(xdr, &res->fs_locations->fattr,
+ NULL,
res->fs_locations->server,
- !RPC_IS_ASYNC(req->rq_task));
+ !RPC_IS_ASYNC(req->rq_task),
+ 1);
out:
return status;
}
@@ -6639,7 +6656,7 @@ out:
int nfs4_decode_dirent(struct xdr_stream *xdr, struct nfs_entry *entry,
int plus)
{
- uint32_t bitmap[3] = {0};
+ uint32_t bitmap[3] = {0}, deny_bitmap[3] = {0};
uint32_t len;
__be32 *p = xdr_inline_decode(xdr, 4);
if (unlikely(!p))
@@ -6680,8 +6697,10 @@ int nfs4_decode_dirent(struct xdr_stream *xdr, struct nfs_entry *entry,
if (decode_attr_length(xdr, &len, &p) < 0)
goto out_overflow;
+ deny_bitmap[0] |= FATTR4_WORD0_FS_LOCATIONS;
+
if (decode_getfattr_attrs(xdr, bitmap, entry->fattr, entry->fh,
- entry->server, 1) < 0)
+ entry->server, 1, deny_bitmap) < 0)
goto out_overflow;
if (entry->fattr->valid & NFS_ATTR_FATTR_FILEID)
entry->ino = entry->fattr->fileid;
--
1.7.1
On Tue, 2011-03-22 at 17:46 -0400, Trond Myklebust wrote:
> Why are you limiting this to fs_locations?
Sorry, I haven't seen any other attribute that can cause memory
corruption.
> As I believe I said earlier,
> any attribute that we didn't explicitly request is an error and can
> cause corruption in the client.
There are checks on each decode attr function. For instance,
decode_attr_filehandle:
if (unlikely(bitmap[0] & (FATTR4_WORD0_FILEHANDLE - 1U)))
return -EIO;
So any non handled attribute raise EIO error.
>
> If we're going to fix this, we should fix all potential occurrences once
> and for all.
Yes, I agree. Could you direct that I missed ?
>
--
Thanks,
Vitaliy Gusev
On Wed, 2011-03-23 at 01:39 +0300, Vitaliy Gusev wrote:
> On Tue, 2011-03-22 at 17:46 -0400, Trond Myklebust wrote:
>
> > Why are you limiting this to fs_locations?
>
> Sorry, I haven't seen any other attribute that can cause memory
> corruption.
decode_attr_filehandle() will certainly cause an Oops if someone inserts one.
There may be more occurrences in the future if/when we need to add
support for more attributes.
> > As I believe I said earlier,
> > any attribute that we didn't explicitly request is an error and can
> > cause corruption in the client.
>
> There are checks on each decode attr function. For instance,
> decode_attr_filehandle:
>
> if (unlikely(bitmap[0] & (FATTR4_WORD0_FILEHANDLE - 1U)))
> return -EIO;
>
> So any non handled attribute raise EIO error.
If we're going to do this, then I suggest we add an 'expected bitmask'
argument to 'decode_attr_bitmap()'. If the server sets any bit that is
not part of this expected bitmask, then we can immediately return an EIO
without having to decode any further.
That will allow us to get rid of the 'if (unlikely(bitmap[] & ....)'
tests altogether.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://www.netapp.com
On Tue, 2011-03-22 at 19:10 -0400, Trond Myklebust wrote:
> On Wed, 2011-03-23 at 01:39 +0300, Vitaliy Gusev wrote:
> > On Tue, 2011-03-22 at 17:46 -0400, Trond Myklebust wrote:
> >
> > > Why are you limiting this to fs_locations?
> >
> > Sorry, I haven't seen any other attribute that can cause memory
> > corruption.
>
> decode_attr_filehandle() will certainly cause an Oops if someone inserts one.
Hmm, Thanks! But it seems that here plain check on NULL is enough...
>
> There may be more occurrences in the future if/when we need to add
> support for more attributes.
>
> > > As I believe I said earlier,
> > > any attribute that we didn't explicitly request is an error and can
> > > cause corruption in the client.
> >
> > There are checks on each decode attr function. For instance,
> > decode_attr_filehandle:
> >
> > if (unlikely(bitmap[0] & (FATTR4_WORD0_FILEHANDLE - 1U)))
> > return -EIO;
> >
> > So any non handled attribute raise EIO error.
>
> If we're going to do this, then I suggest we add an 'expected bitmask'
> argument to 'decode_attr_bitmap()'. If the server sets any bit that is
> not part of this expected bitmask, then we can immediately return an EIO
> without having to decode any further.
>
> That will allow us to get rid of the 'if (unlikely(bitmap[] & ....)'
> tests altogether.
You are right. I'll do as you said.
--
Thanks,
Vitaliy Gusev
On Wed, 2011-03-23 at 00:36 +0300, Vitaliy Gusev wrote:
> Changes with v2 version: when unexpected fs_locations occurs:
>
> 1. Throw print message "unexpected fs_locations"
> 2. Stop parsing xdr and return EIO error
>
> ----
>
> There is a remote vulnerability for nfs4-client. That allows
> a remote NFSv4 server or attacker in middle rewrite memory on
> NFSv4 client.
> Of course if encryption is used then only server can do corruption.
>
> 1. Client send GETATTR request to server, but there are not
> checks for bitmask in reply from server. Therefore server may return
> more attributes than were asked.
>
> 2. Pointer to struct nfs_fattr is passed to decode_getfattr(), but
> that parameter is considered as pointer to struct nfs4_fs_locations
> in case set FATTR4_WORD0_FS_LOCATIONS in reply from server.
>
> But there is no check for pointer type that was passed!
>
> 3. Remote side can set FATTR4_WORD0_FS_LOCATIONS and fill
> datas fsattr4_fs_locations in reply to client.
>
> 4. sizeof(nfs4_fs_locations) is about 92080 bytes in kernel 2.6.38
>
> So ~ 90 Kbytes can be overwritten in memory of NFSv4 client.
>
> Fortunately, nfs_fattr is allocated via kmalloc and
> is not stored on stack. So generally kmalloc-192 is corrupted.
>
> Affected kernels: all from v2.6.18
> Commit 683b57b435326eb512c7305892683b6205669448
> "NFSv4: Implement the fs_locations function call"
> ---
> fs/nfs/nfs4xdr.c | 39 +++++++++++++++++++++++++++++----------
> 1 files changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
> index c87e543..6fea9c9 100644
> --- a/fs/nfs/nfs4xdr.c
> +++ b/fs/nfs/nfs4xdr.c
> @@ -3451,7 +3451,8 @@ out_overflow:
> return -EIO;
> }
>
> -static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, struct nfs4_fs_locations *res)
> +static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, struct nfs4_fs_locations *res,
> + uint32_t *deny_bitmap)
> {
> int n;
> __be32 *p;
> @@ -3462,6 +3463,13 @@ static int decode_attr_fs_locations(struct xdr_stream *xdr, uint32_t *bitmap, st
> status = 0;
> if (unlikely(!(bitmap[0] & FATTR4_WORD0_FS_LOCATIONS)))
> goto out;
> +
> + if (unlikely(deny_bitmap[0] & FATTR4_WORD0_FS_LOCATIONS)) {
> + status = -EIO;
> + printk(KERN_WARNING "%s: Unexpected fs_locations\n", __func__);
> + goto out;
> + }
Why are you limiting this to fs_locations? As I believe I said earlier,
any attribute that we didn't explicitly request is an error and can
cause corruption in the client.
If we're going to fix this, we should fix all potential occurrences once
and for all.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
[email protected]
http://www.netapp.com