[PATCH] Ensure user-supplied string null terminated before kstrdup()

[PATCH] Ensure user-supplied string null terminated before kstrdup()

[Rob Landley]

From: Rob Landley <rlandley@parallels.com>

Make sure user string is null terminated before copying it.

Signed-off-by: Rob Landley <rlandley@parallels.com>
---

 fs/nfs/super.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index b68c860..0ad1255 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1881,9 +1881,12 @@ static int nfs_validate_mount_data(void *options,
 
 		if (!(data->flags & NFS_MOUNT_TCP))
 			args->nfs_server.protocol = XPRT_TRANSPORT_UDP;
+		/* Force null termination of data->hostname no matter what
+		   user passed in. */
+		args->namlen		= data->namlen;
+		data->namlen = 0;
 		/* N.B. caller will free nfs_server.hostname in all cases */
 		args->nfs_server.hostname = kstrdup(data->hostname, GFP_KERNEL);
-		args->namlen		= data->namlen;
 		args->bsize		= data->bsize;
 
 		if (data->flags & NFS_MOUNT_SECFLAVOUR)

Re: [PATCH] Ensure user-supplied string null terminated before kstrdup()

[Trond Myklebust]

On Thu, 2011-02-03 at 16:13 -0600, Rob Landley wrote: 
> From: Rob Landley <rlandley@parallels.com>
> 
> Make sure user string is null terminated before copying it.
> 
> Signed-off-by: Rob Landley <rlandley@parallels.com>
> ---
> 
>  fs/nfs/super.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index b68c860..0ad1255 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -1881,9 +1881,12 @@ static int nfs_validate_mount_data(void *options,
>  
>  		if (!(data->flags & NFS_MOUNT_TCP))
>  			args->nfs_server.protocol = XPRT_TRANSPORT_UDP;
> +		/* Force null termination of data->hostname no matter what
> +		   user passed in. */
> +		args->namlen		= data->namlen;
> +		data->namlen = 0;
>  		/* N.B. caller will free nfs_server.hostname in all cases */
>  		args->nfs_server.hostname = kstrdup(data->hostname, GFP_KERNEL);
> -		args->namlen		= data->namlen;
>  		args->bsize		= data->bsize;
>  
>  		if (data->flags & NFS_MOUNT_SECFLAVOUR)

The "namlen" mount option bears absolutely no relation to the server
hostname AFAIK. I can't see how this patch makes sense...

Cheers
  Trond
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH] Ensure user-supplied string null terminated before kstrdup()

[Rob Landley]

On 02/03/2011 08:59 PM, Trond Myklebust wrote:
> On Thu, 2011-02-03 at 16:13 -0600, Rob Landley wrote: 
>> From: Rob Landley <rlandley@parallels.com>
>>
>> Make sure user string is null terminated before copying it.
>>
>> Signed-off-by: Rob Landley <rlandley@parallels.com>
>> ---
>>
>>  fs/nfs/super.c |    5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>> index b68c860..0ad1255 100644
>> --- a/fs/nfs/super.c
>> +++ b/fs/nfs/super.c
>> @@ -1881,9 +1881,12 @@ static int nfs_validate_mount_data(void *options,
>>  
>>  		if (!(data->flags & NFS_MOUNT_TCP))
>>  			args->nfs_server.protocol = XPRT_TRANSPORT_UDP;
>> +		/* Force null termination of data->hostname no matter what
>> +		   user passed in. */
>> +		args->namlen		= data->namlen;
>> +		data->namlen = 0;
>>  		/* N.B. caller will free nfs_server.hostname in all cases */
>>  		args->nfs_server.hostname = kstrdup(data->hostname, GFP_KERNEL);
>> -		args->namlen		= data->namlen;
>>  		args->bsize		= data->bsize;
>>  
>>  		if (data->flags & NFS_MOUNT_SECFLAVOUR)
> 
> The "namlen" mount option bears absolutely no relation to the server
> hostname AFAIK. I can't see how this patch makes sense...

That field is right after the hostname in the struct, therefore if it's
zeroed the data->hostname is guaranteed to be null terminated at the end
of its nominal range.  (A bit subtle, hence the comment.)

This isn't necessarily the proper fix, but I don't see why the user
can't feed in a "data" blob with an arbitrarily long hostname.  This is
user supplied data we're arbitrarily kstrdup()ing without checking it,
right?  (Is there a size limit on the option string?  Does the range
check on userspace pointers prevent it from being right _before_
something we shouldn't read and padding up to it?)

Entirely possible this is benign and irrelevant, and you probably have
to be root to trigger it, but it's the kind of security "huh" I thought
I'd ping the list about rather than assuming my inability to figure out
how to exploit it of the top of my head means there's nothing lurking.

Rob

Re: [PATCH] Ensure user-supplied string null terminated before kstrdup()

[Trond Myklebust]

On Thu, 2011-02-03 at 21:48 -0600, Rob Landley wrote: 
> On 02/03/2011 08:59 PM, Trond Myklebust wrote:
> > On Thu, 2011-02-03 at 16:13 -0600, Rob Landley wrote: 
> >> From: Rob Landley <rlandley@parallels.com>
> >>
> >> Make sure user string is null terminated before copying it.
> >>
> >> Signed-off-by: Rob Landley <rlandley@parallels.com>
> >> ---
> >>
> >>  fs/nfs/super.c |    5 ++++-
> >>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> >> index b68c860..0ad1255 100644
> >> --- a/fs/nfs/super.c
> >> +++ b/fs/nfs/super.c
> >> @@ -1881,9 +1881,12 @@ static int nfs_validate_mount_data(void *options,
> >>  
> >>  		if (!(data->flags & NFS_MOUNT_TCP))
> >>  			args->nfs_server.protocol = XPRT_TRANSPORT_UDP;
> >> +		/* Force null termination of data->hostname no matter what
> >> +		   user passed in. */
> >> +		args->namlen		= data->namlen;
> >> +		data->namlen = 0;
> >>  		/* N.B. caller will free nfs_server.hostname in all cases */
> >>  		args->nfs_server.hostname = kstrdup(data->hostname, GFP_KERNEL);
> >> -		args->namlen		= data->namlen;
> >>  		args->bsize		= data->bsize;
> >>  
> >>  		if (data->flags & NFS_MOUNT_SECFLAVOUR)
> > 
> > The "namlen" mount option bears absolutely no relation to the server
> > hostname AFAIK. I can't see how this patch makes sense...
> 
> That field is right after the hostname in the struct, therefore if it's
> zeroed the data->hostname is guaranteed to be null terminated at the end
> of its nominal range.  (A bit subtle, hence the comment.)

No. Changing a user specified mount option in order to zero the previous
field is completely unacceptable....

> This isn't necessarily the proper fix, but I don't see why the user
> can't feed in a "data" blob with an arbitrarily long hostname.  This is
> user supplied data we're arbitrarily kstrdup()ing without checking it,
> right?  (Is there a size limit on the option string?  Does the range
> check on userspace pointers prevent it from being right _before_
> something we shouldn't read and padding up to it?)

The right thing to do, is to check the value of strlen(data->hostname)
and to return -EINVAL if it exceeds (sizeof(data->hostname) - 1).

This is a (legacy) _binary_  interface for mounts. Adjusting the value
of the next field in the structure is never going to prevent an attack
that relies on an overflow condition. What about the values of the
nfs_mount_data's root filehandle or the security context fields? If they
have been screwed up due to an overflow attack on the hostname field,
then you are still compromised...

Trond
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com