Possible NFS 4.1 client vulnerability: uninitialized/garbage kfree() in decode_cb_sequence_args()

Commit Message

Trond Myklebust Feb. 11, 2015, 10:42 p.m. UTC

Hi David,

On Wed, 2015-02-11 at 13:39 -0800, David Ramos wrote:
> Hello,
> 
> Our UC-KLEE tool found a kfree() of an uninitialized pointer in decode_cb_sequence_args (fs/nfs/callback_xdr.c) that may be remotely exploitable. The bug affects Linux kernel 3.16.3, but it appears to date back to commit 4aece6a19cf7f474f15eb861ba74db4479884ce3 (4/1/2009), which first implemented the CB_SEQUENCE operation from NFS 4.1.
> 
> Here is some of the relevant code:
>  458        if (args->csa_nrclists) {
>  459                args->csa_rclists = kmalloc_array(args->csa_nrclists,
>  460                                                  sizeof(*args->csa_rclists),
>  461                                                  GFP_KERNEL);
>  ...
>  465                for (i = 0; i < args->csa_nrclists; i++) {
>  466                        status = decode_rc_list(xdr, &args->csa_rclists[i]);
>  467                        if (status)
>  468                                goto out_free;
>  469                }
>  470        }
>  …
>  487out_free:
>  488        for (i = 0; i < args->csa_nrclists; i++)
>  489                kfree(args->csa_rclists[i].rcl_refcalls);
> 
> If a call to decode_rc_list() on line 466 returns non-zero during iteration ‘i', the kfree() call at line 489 will attempt to free uninitialized (heap garbage) pointers for all indices in [i, args->csa_nrclists).
> 
> I’m not familiar enough with the NFS internals to understand whether an attacker can cause decode_rc_list() to fail (i.e., by causing read_buf() to fail), but it seems plausible?
> 

Thanks for reporting this!

I can't see this issue as being exploitable without a fair amount of
trouble because the above RPC request would be incoming on a TCP
connection that was initiated by the NFSv4.1 client. If someone can do
that level of spoofing, then they can cause all sorts of mischief for
the client.

How about the following fix?

Cheers
  Trond

8<------------------------------------------------------------
From 5d4f299811799ec4731e27983939ea83186be939 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <[e-mail]>
Date: Wed, 11 Feb 2015 17:27:55 -0500
Subject: [PATCH] NFSv4.1: Fix a kfree() of uninitialised pointers in
 decode_cb_sequence_args

If the call to decode_rc_list() fails due to a memory allocation error,
then we need to truncate the array size to ensure that we only call
kfree() on those pointer that were allocated.

Reported-by: David Ramos <[e-mail]>
Fixes: 4aece6a19cf7f ("nfs41: cb_sequence xdr implementation")
Cc: [e-mail]
Signed-off-by: Trond Myklebust <[e-mail]>
---
 fs/nfs/callback_xdr.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

David Ramos Feb. 11, 2015, 10:58 p.m. UTC | #1

Hi Trond,

Thanks for the quick response. Your patch looks good to me.

On Feb 11, 2015, at 2:42 PM, Trond Myklebust <[e-mail]> wrote:

> I can't see this issue as being exploitable without a fair amount of
> trouble because the above RPC request would be incoming on a TCP
> connection that was initiated by the NFSv4.1 client. If someone can do
> that level of spoofing, then they can cause all sorts of mischief for
> the client.

That’s a fair point as far as spoofing. The attack vector I had in mind was one in which a client is induced to mount an NFS volume on a malicious server (either directly, through some DNS trickery, or via automount). Even if the client shouldn’t trust the “mischievous" files being from the server, we shouldn’t let the server crash the client machine. This is clearly not as compelling as a client attacking a server, but I think it’s worth considering.

Best,
-David



--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Trond Myklebust Feb. 11, 2015, 11:08 p.m. UTC | #2

Hi David,

On Wed, Feb 11, 2015 at 5:56 PM, David Ramos <[e-mail]> wrote:
> Hi Trond,
>
> Thanks for the quick response. Your patch looks good to me.
>
> On Feb 11, 2015, at 2:42 PM, Trond Myklebust
> <[e-mail]> wrote:
>
>
> I can't see this issue as being exploitable without a fair amount of
>
> trouble because the above RPC request would be incoming on a TCP
> connection that was initiated by the NFSv4.1 client. If someone can do
> that level of spoofing, then they can cause all sorts of mischief for
> the client.
>
>
> That’s a fair point as far as spoofing. The attack vector I had in mind was
> one in which a client is induced to mount an NFS volume on a malicious
> server (either directly, through some DNS trickery, or via automount). Even
> if the client shouldn’t trust the “mischievous" files being from the server,
> we shouldn’t let the server crash the client machine. This is clearly not as
> compelling as a client attacking a server, but I think it’s worth
> considering.

If you are operating in that kind of hostile environment, then you
really should be using RPCSEC_GSS with krb5i or krb5p to protect your
payloads. I'll admit that we don't yet have proper support for
RPCSEC_GSS protection on the NFSv4.1 backchannel RPC requests, so the
particular spoofing scenario described above is still possible,
however the server and client will mutually identify each other via
RPCSEC_GSS when the TCP connection is being set up.

Cheers
  Trond

Patch

diff --git a/fs/nfs/callback_xdr.c b/fs/nfs/callback_xdr.c
index f4ccfe6521ec..02f8d09e119f 100644
--- a/fs/nfs/callback_xdr.c
+++ b/fs/nfs/callback_xdr.c
@@ -464,8 +464,10 @@  static __be32 decode_cb_sequence_args(struct svc_rqst *rqstp,
 
 		for (i = 0; i < args->csa_nrclists; i++) {
 			status = decode_rc_list(xdr, &args->csa_rclists[i]);
-			if (status)
+			if (status) {
+				args->csa_nrclists = i;
 				goto out_free;
+			}
 		}
 	}
 	status = 0;