[PATCH] SUNRPC: Fix xdr_decode_string_inplace() mixed sign comparison

[PATCH] SUNRPC: Fix xdr_decode_string_inplace() mixed sign comparison

[Chuck Lever]

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

Re: [PATCH] SUNRPC: Fix xdr_decode_string_inplace() mixed sign comparison

[Talpey, Thomas]

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.

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

Re: [PATCH] SUNRPC: Fix xdr_decode_string_inplace() mixed sign comparison

[Chuck Lever]

[-- Attachment #1: Type: text/plain, Size: 2717 bytes --]

Talpey, Thomas wrote:
> This is a serious vunerability! A huge string length will always be
> accepted by this code, right?

In the current code, a sufficiently large length coming from the wire 
will be treated as a negative value, thus will not be detected by the 
maximum length check in xdr_decode_string_inplace.

> Security/integrity bug, not a minor
> sign cleanup IOW.

My proposal is to make all the variables in xdr_decode_string_inplace of 
type u32, and then work backwards into the ULPs, changing the length 
variables of type int to type u32.

Note however that we also have to worry about open-coded string 
decoding, and the lengths of variable-length opaques.  I haven't even 
looked at those yet.

> 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

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 259 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 314 bytes --]

-------------------------------------------------------------------------
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/

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] SUNRPC: Fix xdr_decode_string_inplace() mixed sign comparison

[Talpey, Thomas]

At 01:29 PM 10/31/2007, Chuck Lever wrote:
>My proposal is to make all the variables in xdr_decode_string_inplace of 
>type u32, and then work backwards into the ULPs, changing the length 
>variables of type int to type u32.

I'd suggest a slight variation - using u32 where the variables shadow
objects which are decoded/encoded off the wire, but using a native
type (unsigned int/int) after they are actually passed outside xdr.

For example, the length here is u32 within the xdr routine, but after
the it's decoded, it's really not something we'd want to pass to
memcpy. IOW, it seems ok to me to have the "maxlen" argument
remain an int (maybe unsigned), but the internal "len" is naturally u32.

>Note however that we also have to worry about open-coded string 
>decoding, and the lengths of variable-length opaques.  I haven't even 
>looked at those yet.

Can't happen too soon, by the look of this. :-)

Tom.

-------------------------------------------------------------------------
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

Re: [PATCH] SUNRPC: Fix xdr_decode_string_inplace() mixed sign comparison

[Chuck Lever]

[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]

Talpey, Thomas wrote:
> At 01:29 PM 10/31/2007, Chuck Lever wrote:
>> My proposal is to make all the variables in xdr_decode_string_inplace of 
>> type u32, and then work backwards into the ULPs, changing the length 
>> variables of type int to type u32.
> 
> I'd suggest a slight variation - using u32 where the variables shadow
> objects which are decoded/encoded off the wire, but using a native
> type (unsigned int/int) after they are actually passed outside xdr.

Converting XDR types to native types makes sense, I guess.

> For example, the length here is u32 within the xdr routine, but after
> it's decoded, it's really not something we'd want to pass to
> memcpy. IOW, it seems ok to me to have the "maxlen" argument
> remain an int (maybe unsigned), but the internal "len" is naturally u32.

I can't see why we need to handle negative string length values in most 
places, so the length variables should be made unsigned, at least. 
Trond insisted that *lenp and maxlen must be same type here.

However, it seems to me that native string lengths should be size_t. 
(ie the same type that strlen() returns).

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 259 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 314 bytes --]

-------------------------------------------------------------------------
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/

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] SUNRPC: Fix xdr_decode_string_inplace() mixed sign comparison

[Talpey, Thomas]

At 01:56 PM 10/31/2007, Chuck Lever wrote:
>However, it seems to me that native string lengths should be size_t. 
>(ie the same type that strlen() returns).

Sounds right (if a tad huge for xdr purposes).

If anyone ever builds a machine without a native 32-bit type, I'm sure
a lot of this code will need fixing. :-) As it is, the xdr_hyper support is
the only truly portable integer marshaling w.r.t. non-native object sizing.

Tom.

-------------------------------------------------------------------------
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

Re: [PATCH] SUNRPC: Fix xdr_decode_string_inplace() mixed sign comparison

[Trond Myklebust]

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

Re: [PATCH] SUNRPC: Fix xdr_decode_string_inplace() mixed sign comparison

[Talpey, Thomas]

At 03:00 PM 10/31/2007, Trond Myklebust wrote:
>
>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).

Hmm - okay right you are, the unsigned is wider and therefore maxlen
is promoted. So the current code is safe, pfew.

>
>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.

Actually, shouldn't it be len <= 0 ? ;-) If len == 0 then there isn't any
data to point to inline since XDR doesn't marshal zero bytes.

Tom.

>
>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

Re: [PATCH] SUNRPC: Fix xdr_decode_string_inplace() mixed sign comparison

[Chuck Lever]

On Oct 31, 2007, at 3:00 PM, Trond Myklebust wrote:
> 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).

Whatever H&S says, the compiler flags this as a mixed sign  
comparison.  Thus something is not working the way you assume it is.

[cel@ingres NFS_ALL]$ make net/sunrpc/xdr.o
   Using /home/cel/src/linux/NFS_ALL as source for kernel
   GEN     /u/cel/obj/Makefile
   CHK     include/linux/version.h
   CHK     include/linux/utsrelease.h
   UPD     include/linux/utsrelease.h
   CALL    /home/cel/src/linux/NFS_ALL/scripts/checksyscalls.sh
   CC      net/sunrpc/xdr.o
/home/cel/src/linux/NFS_ALL/net/sunrpc/xdr.c: In function  
xdr_decode_string_inplace:
/home/cel/src/linux/NFS_ALL/net/sunrpc/xdr.c:100: warning: comparison  
between signed and unsigned
[cel@ingres NFS_ALL]$

Line 100 is precisely:

	if ((len = ntohl(*p++)) > maxlen)

My gcc is the latest available for Fedora 7:

gcc version 4.1.2 20070925 (Red Hat 4.1.2-27)

I rather prefer spelling this out completely so that neither the  
compiler nor humans can mistake the intent of this logic.

> 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.

Yes, the second version of the patch is incorrect.

> 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
>

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com




-------------------------------------------------------------------------
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

Re: [PATCH] SUNRPC: Fix xdr_decode_string_inplace() mixed sign comparison

[Trond Myklebust]

On Wed, 2007-10-31 at 21:53 -0400, Chuck Lever wrote:
> On Oct 31, 2007, at 3:00 PM, Trond Myklebust wrote:
> > 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).
> 
> Whatever H&S says, the compiler flags this as a mixed sign  
> comparison.  Thus something is not working the way you assume it is.
> 
> [cel@ingres NFS_ALL]$ make net/sunrpc/xdr.o
>    Using /home/cel/src/linux/NFS_ALL as source for kernel
>    GEN     /u/cel/obj/Makefile
>    CHK     include/linux/version.h
>    CHK     include/linux/utsrelease.h
>    UPD     include/linux/utsrelease.h
>    CALL    /home/cel/src/linux/NFS_ALL/scripts/checksyscalls.sh
>    CC      net/sunrpc/xdr.o
> /home/cel/src/linux/NFS_ALL/net/sunrpc/xdr.c: In function  
> xdr_decode_string_inplace:
> /home/cel/src/linux/NFS_ALL/net/sunrpc/xdr.c:100: warning: comparison  
> between signed and unsigned
> [cel@ingres NFS_ALL]$
> 
> Line 100 is precisely:
> 
> 	if ((len = ntohl(*p++)) > maxlen)

Which is still correct according to both the old and new C standards. I
know you've got that book at home...

> My gcc is the latest available for Fedora 7:
> 
> gcc version 4.1.2 20070925 (Red Hat 4.1.2-27)
> 
> I rather prefer spelling this out completely so that neither the  
> compiler nor humans can mistake the intent of this logic.

That's fine, but please do not change the logic. The correct change is
to replace the maxlen parameter with an unsigned int.



-------------------------------------------------------------------------
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

Re: [PATCH] SUNRPC: Fix xdr_decode_string_inplace() mixed sign comparison

[Chuck Lever]

[-- Attachment #1: Type: text/plain, Size: 3015 bytes --]

Trond Myklebust wrote:
> On Wed, 2007-10-31 at 21:53 -0400, Chuck Lever wrote:
>> On Oct 31, 2007, at 3:00 PM, Trond Myklebust wrote:
>>> 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).
>> Whatever H&S says, the compiler flags this as a mixed sign  
>> comparison.  Thus something is not working the way you assume it is.
>>
>> [cel@ingres NFS_ALL]$ make net/sunrpc/xdr.o
>>    Using /home/cel/src/linux/NFS_ALL as source for kernel
>>    GEN     /u/cel/obj/Makefile
>>    CHK     include/linux/version.h
>>    CHK     include/linux/utsrelease.h
>>    UPD     include/linux/utsrelease.h
>>    CALL    /home/cel/src/linux/NFS_ALL/scripts/checksyscalls.sh
>>    CC      net/sunrpc/xdr.o
>> /home/cel/src/linux/NFS_ALL/net/sunrpc/xdr.c: In function  
>> xdr_decode_string_inplace:
>> /home/cel/src/linux/NFS_ALL/net/sunrpc/xdr.c:100: warning: comparison  
>> between signed and unsigned
>> [cel@ingres NFS_ALL]$
>>
>> Line 100 is precisely:
>>
>> 	if ((len = ntohl(*p++)) > maxlen)
> 
> Which is still correct according to both the old and new C standards. I
> know you've got that book at home...
> 
>> My gcc is the latest available for Fedora 7:
>>
>> gcc version 4.1.2 20070925 (Red Hat 4.1.2-27)
>>
>> I rather prefer spelling this out completely so that neither the  
>> compiler nor humans can mistake the intent of this logic.
> 
> That's fine, but please do not change the logic. The correct change is
> to replace the maxlen parameter with an unsigned int.

That's what I sent you originally.  You rejected it:

On October 26, 2007 at 14:24 -0400, Trond Myklebust said:
>> diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
>> index 3d1f7cd..ff16bab 100644
>> --- a/net/sunrpc/xdr.c
>> +++ b/net/sunrpc/xdr.c
>> @@ -93,7 +93,7 @@ xdr_encode_string(__be32 *p, const char *string)
>>  }
>>  
>>  __be32 *
>> -xdr_decode_string_inplace(__be32 *p, char **sp, int *lenp, int maxlen)
>> +xdr_decode_string_inplace(__be32 *p, char **sp, int *lenp, unsigned int maxlen)
>>  {
>>  	unsigned int	len;
 >
> Nope. maxlen should be of the same type as *lenp.
> 
> Trond

Thus I now argue that both *lenp and maxlen should either be unsigned 
integers or size_t.  Negative string lengths make no sense whatsoever.

If we change both arguments, then we should also change the callers, at 
least to be consistent.

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 259 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 314 bytes --]

-------------------------------------------------------------------------
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/

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] SUNRPC: Fix xdr_decode_string_inplace() mixed sign comparison

[Trond Myklebust]

On Thu, 2007-11-01 at 11:37 -0400, Chuck Lever wrote:
> >> -xdr_decode_string_inplace(__be32 *p, char **sp, int *lenp, int maxlen)
> >> +xdr_decode_string_inplace(__be32 *p, char **sp, int *lenp, unsigned int maxlen)
> >>  {
> >>  	unsigned int	len;
>  >
> > Nope. maxlen should be of the same type as *lenp.
> > 
> > Trond
> 
> Thus I now argue that both *lenp and maxlen should either be unsigned 
> integers or size_t.  Negative string lengths make no sense whatsoever.
> 
> If we change both arguments, then we should also change the callers, at 
> least to be consistent.

That would be fine by me.


-------------------------------------------------------------------------
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