Ensure user-supplied string null terminated before kstrdup()
Subsystems (2)
PATCH 1/1 Ensure user-supplied string null terminated before kstrdup()
From: Rob Landley <[email protected]>
Make sure user string is null terminated before copying it.
---
fs/nfs/super.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
fs/nfs/super.c +4 -1
@@ -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 )
Quoted 18 lines · Rob Landley
From: Rob Landley <[email protected]>
Make sure user string is null terminated before copying it.
Signed-off-by Rob Landley <[email protected]>---
fs/nfs/super.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)fs/nfs/super.c +0
-0@@ -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 ;
fs/nfs/super.c static int nfs_validate_mount_data(void *options, · line 1892
Quoted 10 lines
fs/nfs/super.c +4
-1+/* 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
Signature · 6 lines
Trond Myklebust Linux NFS client maintainer NetApp [email protected] www.netapp.com
Quoted 28 lines · Trond Myklebust
Quoted 27 lines · Rob Landley
From: Rob Landley <[email protected]>
Make sure user string is null terminated before copying it.
Signed-off-by Rob Landley <[email protected]>---
fs/nfs/super.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)fs/nfs/super.c +4
-1@@ -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;
Quoted 4 lines
fs/nfs/super.c static int nfs_validate_mount_data(void *options, · line 1892
Quoted 1 line
fs/nfs/super.c +0
-0if (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
Quoted 29 lines · Rob Landley
Quoted 28 lines · Trond Myklebust
Quoted 27 lines · Rob Landley
From: Rob Landley <[email protected]>
Make sure user string is null terminated before copying it.
Signed-off-by Rob Landley <[email protected]>---
fs/nfs/super.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)fs/nfs/super.c +4
-1@@ -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;
Quoted 8 lines
Quoted 4 lines
fs/nfs/super.c static int nfs_validate_mount_data(void *options, · line 1892
Quoted 1 line
fs/nfs/super.c +0
-0if (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->hostnameis 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....
Quoted 6 lines
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
Signature · 6 lines
Trond Myklebust Linux NFS client maintainer NetApp [email protected] www.netapp.com