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 <[email protected]>
---
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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
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 <[email protected]>
>---
>
> 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 - [email protected]
>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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
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 <[email protected]>
>> >---
>> >
>> > 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 - [email protected]
>> >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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
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 <[email protected]>
> >---
> >
> > 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 - [email protected]
> >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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
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 <[email protected]>
>>> ---
>>>
>>> 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 - [email protected]
>>> 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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
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 - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs