From: Trond Myklebust <trond.myklebust@fys.uio.no>
Subject: Re: [PATCH] SUNRPC: Fix xdr_decode_string_inplace()
	mixed	sign comparison
Date: Wed, 31 Oct 2007 15:00:19 -0400
Message-ID: <1193857219.7454.70.camel@heimdal.trondhjem.org>
References: <20071031165045.5861.52308.stgit@manray.1015granger.net>
	<EXNANE01v6EybqfAj810000085a@exnane01.hq.netapp.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Cc: nfs@lists.sourceforge.net
To: "Talpey, Thomas" <Thomas.Talpey@netapp.com>
In-Reply-To: <EXNANE01v6EybqfAj810000085a@exnane01.hq.netapp.com>
Sender: nfs-bounces@lists.sourceforge.net
Errors-To: nfs-bounces@lists.sourceforge.net


On Wed, 2007-10-31 at 13:06 -0400, Talpey, Thomas wrote:
> This is a serious vunerability! A huge string length will always be
> accepted by this code, right? Security/integrity bug, not a minor
> sign cleanup IOW.

Wrong! The current code is quite correct.

It trusts that the caller is setting a reasonable value for maxlen, and
assumes that 'len' is the untrusted value (since it comes from the
network).

in the comparison

  ((len = ntohl(*p++)) < maxlen)

then the trusted value maxlen is the one that gets cast to an unsigned
value since 'len' and 'maxlen' are both integers of the same rank (see
the description of the usual binary conversions in section 6.3.4 in
Harbison and Steele).

Chuck's patch _introduces_ the bug, since it causes the above comparison
to become a signed comparison. In that case, you also need to test for
len<0.

Trond

> Tom.
> 
> At 12:50 PM 10/31/2007, Chuck Lever wrote:
> >xdr_decode_string_inplace() compares an incoming length to a maximum length
> >allowed by the protocol.  Make sure both sides of the comparison have the
> >same sign.
> >
> >A better fix for this would be always to use unsigned 32-bit integers for
> >string lengths.  To wit, RFC 4506 says:
> >
> >4.2.  Unsigned Integer
> >
> >   An XDR unsigned integer is a 32-bit datum that encodes a non-negative
> >   integer in the range [0,4294967295].
> >
> > ...
> >
> >4.11.  String
> >
> >   The standard defines a string of n (numbered 0 through n-1) ASCII
> >   bytes to be the number n encoded as an unsigned integer (as described
> >   above), and followed by the n bytes of the string.
> >
> >This would mean fixing up the callers of xdr_decode_string_inplace, which
> >include the NFS server's filename handling functions (including
> >decode_filename, decode_pathname, and nfsd_lookup), and lockd's nlm_lock
> >structure.
> >
> >Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> >---
> >
> > net/sunrpc/xdr.c |    2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> >diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> >index 3d1f7cd..db80a77 100644
> >--- a/net/sunrpc/xdr.c
> >+++ b/net/sunrpc/xdr.c
> >@@ -95,7 +95,7 @@ xdr_encode_string(__be32 *p, const char *string)
> > __be32 *
> > xdr_decode_string_inplace(__be32 *p, char **sp, int *lenp, int maxlen)
> > {
> >-	unsigned int	len;
> >+	int len;
> > 
> > 	if ((len = ntohl(*p++)) > maxlen)
> > 		return NULL;
> >
> >
> >-------------------------------------------------------------------------
> >This SF.net email is sponsored by: Splunk Inc.
> >Still grepping through log files to find problems?  Stop.
> >Now Search log events and configuration files using AJAX and a browser.
> >Download your FREE copy of Splunk now >> http://get.splunk.com/
> >_______________________________________________
> >NFS maillist  -  NFS@lists.sourceforge.net
> >https://lists.sourceforge.net/lists/listinfo/nfs


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs