[patch] isdn: make sure strings are null terminated

[patch] isdn: make sure strings are null terminated

[Dan Carpenter]

These strings come from the user.  We strcpy() them inside
cf_command() so we should check that they are NULL terminated and
return an error if not.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/isdn/divert/divert_procfs.c b/drivers/isdn/divert/divert_procfs.c
index 33ec9e4..0c16687 100644
--- a/drivers/isdn/divert/divert_procfs.c
+++ b/drivers/isdn/divert/divert_procfs.c
@@ -242,6 +242,10 @@ static int isdn_divert_ioctl_unlocked(struct file *file, uint cmd, ulong arg)
 		case IIOCDOCFINT:
 			if (!divert_if.drv_to_name(dioctl.cf_ctrl.drvid))
 				return (-EINVAL);	/* invalid driver */
+			if (strlen(dioctl.cf_ctrl.msn) >= sizeof(dioctl.cf_ctrl.msn))
+				return -EINVAL;
+			if (strlen(dioctl.cf_ctrl.fwd_nr) >= sizeof(dioctl.cf_ctrl.fwd_nr))
+				return -EINVAL;
 			if ((i = cf_command(dioctl.cf_ctrl.drvid,
 					    (cmd == IIOCDOCFACT) ? 1 : (cmd == IIOCDOCFDIS) ? 0 : 2,
 					    dioctl.cf_ctrl.cfproc,

Re: [patch] isdn: make sure strings are null terminated

[Eric Dumazet]

Le mercredi 23 novembre 2011 à 09:42 +0300, Dan Carpenter a écrit :
> These strings come from the user.  We strcpy() them inside
> cf_command() so we should check that they are NULL terminated and
> return an error if not.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/isdn/divert/divert_procfs.c b/drivers/isdn/divert/divert_procfs.c
> index 33ec9e4..0c16687 100644
> --- a/drivers/isdn/divert/divert_procfs.c
> +++ b/drivers/isdn/divert/divert_procfs.c
> @@ -242,6 +242,10 @@ static int isdn_divert_ioctl_unlocked(struct file *file, uint cmd, ulong arg)
>  		case IIOCDOCFINT:
>  			if (!divert_if.drv_to_name(dioctl.cf_ctrl.drvid))
>  				return (-EINVAL);	/* invalid driver */
> +			if (strlen(dioctl.cf_ctrl.msn) >= sizeof(dioctl.cf_ctrl.msn))
> +				return -EINVAL;

This looks buggy.

If string is not null terminated, how strlen() will stop you from going
out of bounds, and trigger some run time checker ?

strnlen() would be more effective...

> +			if (strlen(dioctl.cf_ctrl.fwd_nr) >= sizeof(dioctl.cf_ctrl.fwd_nr))
> +				return -EINVAL;
>  			if ((i = cf_command(dioctl.cf_ctrl.drvid,
>  					    (cmd == IIOCDOCFACT) ? 1 : (cmd == IIOCDOCFDIS) ? 0 : 2,
>  					    dioctl.cf_ctrl.cfproc,
> --

Re: [patch] isdn: make sure strings are null terminated

[Dan Carpenter]

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

On Wed, Nov 23, 2011 at 08:03:31AM +0100, Eric Dumazet wrote:
> > +			if (strlen(dioctl.cf_ctrl.msn) >= sizeof(dioctl.cf_ctrl.msn))
> > +				return -EINVAL;
> 
> This looks buggy.
> 
> If string is not null terminated, how strlen() will stop you from going
> out of bounds, and trigger some run time checker ?
> 
> strnlen() would be more effective...
> 

Aw crap.  My first version used strnlen() and I redid it to be
simpler.  I just figured that it doesn't take long to hit a zeroed
u8.

I'll resend all three strlen() patches to use strnlen().

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [patch] isdn: make sure strings are null terminated

[walter harms]

Am 23.11.2011 07:42, schrieb Dan Carpenter:
> These strings come from the user.  We strcpy() them inside
> cf_command() so we should check that they are NULL terminated and
> return an error if not.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/isdn/divert/divert_procfs.c b/drivers/isdn/divert/divert_procfs.c
> index 33ec9e4..0c16687 100644
> --- a/drivers/isdn/divert/divert_procfs.c
> +++ b/drivers/isdn/divert/divert_procfs.c
> @@ -242,6 +242,10 @@ static int isdn_divert_ioctl_unlocked(struct file *file, uint cmd, ulong arg)
>  		case IIOCDOCFINT:
>  			if (!divert_if.drv_to_name(dioctl.cf_ctrl.drvid))
>  				return (-EINVAL);	/* invalid driver */
> +			if (strlen(dioctl.cf_ctrl.msn) >= sizeof(dioctl.cf_ctrl.msn))
> +				return -EINVAL;
> +			if (strlen(dioctl.cf_ctrl.fwd_nr) >= sizeof(dioctl.cf_ctrl.fwd_nr))
> +				return -EINVAL;

forcing the last field to be zero seems more easy.
dioctl.cf_ctrl.fwd_nr[sizeof(dioctl.cf_ctrl.fwd_nr))-1]=0;

re,
 wh

>  			if ((i = cf_command(dioctl.cf_ctrl.drvid,
>  					    (cmd == IIOCDOCFACT) ? 1 : (cmd == IIOCDOCFDIS) ? 0 : 2,
>  					    dioctl.cf_ctrl.cfproc,
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [patch] isdn: make sure strings are null terminated

[Dan Carpenter]

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

On Wed, Nov 23, 2011 at 09:25:56AM +0100, walter harms wrote:
> 
> 
> Am 23.11.2011 07:42, schrieb Dan Carpenter:
> > These strings come from the user.  We strcpy() them inside
> > cf_command() so we should check that they are NULL terminated and
> > return an error if not.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/drivers/isdn/divert/divert_procfs.c b/drivers/isdn/divert/divert_procfs.c
> > index 33ec9e4..0c16687 100644
> > --- a/drivers/isdn/divert/divert_procfs.c
> > +++ b/drivers/isdn/divert/divert_procfs.c
> > @@ -242,6 +242,10 @@ static int isdn_divert_ioctl_unlocked(struct file *file, uint cmd, ulong arg)
> >  		case IIOCDOCFINT:
> >  			if (!divert_if.drv_to_name(dioctl.cf_ctrl.drvid))
> >  				return (-EINVAL);	/* invalid driver */
> > +			if (strlen(dioctl.cf_ctrl.msn) >= sizeof(dioctl.cf_ctrl.msn))
> > +				return -EINVAL;
> > +			if (strlen(dioctl.cf_ctrl.fwd_nr) >= sizeof(dioctl.cf_ctrl.fwd_nr))
> > +				return -EINVAL;
> 
> forcing the last field to be zero seems more easy.
> dioctl.cf_ctrl.fwd_nr[sizeof(dioctl.cf_ctrl.fwd_nr))-1]=0;
> 

That's a valid option to use, but I'd prefer to return an error code
here because that's what we do on the line before.  Passing a too
long string is clearly invalid.

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [patch] isdn: make sure strings are null terminated

[walter harms]

Am 24.11.2011 12:34, schrieb Dan Carpenter:
> On Wed, Nov 23, 2011 at 09:25:56AM +0100, walter harms wrote:
>>
>>
>> Am 23.11.2011 07:42, schrieb Dan Carpenter:
>>> These strings come from the user.  We strcpy() them inside
>>> cf_command() so we should check that they are NULL terminated and
>>> return an error if not.
>>>
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> diff --git a/drivers/isdn/divert/divert_procfs.c b/drivers/isdn/divert/divert_procfs.c
>>> index 33ec9e4..0c16687 100644
>>> --- a/drivers/isdn/divert/divert_procfs.c
>>> +++ b/drivers/isdn/divert/divert_procfs.c
>>> @@ -242,6 +242,10 @@ static int isdn_divert_ioctl_unlocked(struct file *file, uint cmd, ulong arg)
>>>  		case IIOCDOCFINT:
>>>  			if (!divert_if.drv_to_name(dioctl.cf_ctrl.drvid))
>>>  				return (-EINVAL);	/* invalid driver */
>>> +			if (strlen(dioctl.cf_ctrl.msn) >= sizeof(dioctl.cf_ctrl.msn))
>>> +				return -EINVAL;
>>> +			if (strlen(dioctl.cf_ctrl.fwd_nr) >= sizeof(dioctl.cf_ctrl.fwd_nr))
>>> +				return -EINVAL;
>>
>> forcing the last field to be zero seems more easy.
>> dioctl.cf_ctrl.fwd_nr[sizeof(dioctl.cf_ctrl.fwd_nr))-1]=0;
>>
> 
> That's a valid option to use, but I'd prefer to return an error code
> here because that's what we do on the line before.  Passing a too
> long string is clearly invalid.
> 

the line before has the same problem, of cause.

So far i see you do not get a string, you get a structure. An it will hard
to validate the element is a useful string. I thing my (sledgehammer) method
is ok here because you make sure that all later calls (strcmp,strcpy) will succeed.
If someone supplies a bad string the later calls will catch by failing to identify
and return a proper code from there (at least i hope so).

re,
 wh

RE: [patch] isdn: make sure strings are null terminated

[David Laight]

 
> >>> +			if (strlen(dioctl.cf_ctrl.msn) >=
sizeof(dioctl.cf_ctrl.msn))
> >>> +				return -EINVAL;
...
> So far i see you do not get a string, you get a structure. And 
> it will hard to validate the element is a useful string.
> I think my (sledgehammer) method is ok here because you
> make sure that all later calls (strcmp,strcpy) will succeed.
> If someone supplies a bad string the later calls will catch 
> by failing to identify and return a proper code from there
> (at least i hope so).
> 
> re,
>  wh

Except that the strlen() can run right off the end
of the structure - and might eventually fault.
You need to use something like strnlen().

	David

[patch v2] isdn: make sure strings are null terminated

[Dan Carpenter]

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

These strings come from the user.  We strcpy() them inside
cf_command() so we should check that they are NULL terminated and
return an error if not.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: use strnlen() instead of strlen().

diff --git a/drivers/isdn/divert/divert_procfs.c b/drivers/isdn/divert/divert_procfs.c
index 33ec9e4..9021182 100644
--- a/drivers/isdn/divert/divert_procfs.c
+++ b/drivers/isdn/divert/divert_procfs.c
@@ -242,6 +242,12 @@ static int isdn_divert_ioctl_unlocked(struct file *file, uint cmd, ulong arg)
 		case IIOCDOCFINT:
 			if (!divert_if.drv_to_name(dioctl.cf_ctrl.drvid))
 				return (-EINVAL);	/* invalid driver */
+			if (strnlen(dioctl.cf_ctrl.msn, sizeof(dioctl.cf_ctrl.msn)) ==
+					sizeof(dioctl.cf_ctrl.msn))
+				return -EINVAL;
+			if (strnlen(dioctl.cf_ctrl.fwd_nr, sizeof(dioctl.cf_ctrl.fwd_nr)) ==
+					sizeof(dioctl.cf_ctrl.fwd_nr))
+				return -EINVAL;
 			if ((i = cf_command(dioctl.cf_ctrl.drvid,
 					    (cmd == IIOCDOCFACT) ? 1 : (cmd == IIOCDOCFDIS) ? 0 : 2,
 					    dioctl.cf_ctrl.cfproc,

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [patch] isdn: make sure strings are null terminated

[Karsten Keil]

On Thu, Nov 24, 2011 at 01:21:42PM +0100, walter harms wrote:
> 
> 
> Am 24.11.2011 12:34, schrieb Dan Carpenter:
> > On Wed, Nov 23, 2011 at 09:25:56AM +0100, walter harms wrote:
> >>
> >>
> >> Am 23.11.2011 07:42, schrieb Dan Carpenter:
> >>> These strings come from the user.  We strcpy() them inside
> >>> cf_command() so we should check that they are NULL terminated and
> >>> return an error if not.
> >>>
> >>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >>>
> >>> diff --git a/drivers/isdn/divert/divert_procfs.c b/drivers/isdn/divert/divert_procfs.c
> >>> index 33ec9e4..0c16687 100644
> >>> --- a/drivers/isdn/divert/divert_procfs.c
> >>> +++ b/drivers/isdn/divert/divert_procfs.c
> >>> @@ -242,6 +242,10 @@ static int isdn_divert_ioctl_unlocked(struct file *file, uint cmd, ulong arg)
> >>>  		case IIOCDOCFINT:
> >>>  			if (!divert_if.drv_to_name(dioctl.cf_ctrl.drvid))
> >>>  				return (-EINVAL);	/* invalid driver */
> >>> +			if (strlen(dioctl.cf_ctrl.msn) >= sizeof(dioctl.cf_ctrl.msn))
> >>> +				return -EINVAL;
> >>> +			if (strlen(dioctl.cf_ctrl.fwd_nr) >= sizeof(dioctl.cf_ctrl.fwd_nr))
> >>> +				return -EINVAL;
> >>
> >> forcing the last field to be zero seems more easy.
> >> dioctl.cf_ctrl.fwd_nr[sizeof(dioctl.cf_ctrl.fwd_nr))-1]=0;
> >>
> > 
> > That's a valid option to use, but I'd prefer to return an error code
> > here because that's what we do on the line before.  Passing a too
> > long string is clearly invalid.
> > 
> 
> the line before has the same problem, of cause.
> 
> So far i see you do not get a string, you get a structure. An it will hard
> to validate the element is a useful string. I thing my (sledgehammer) method
> is ok here because you make sure that all later calls (strcmp,strcpy) will succeed.
> If someone supplies a bad string the later calls will catch by failing to identify
> and return a proper code from there (at least i hope so).
> 

Since the proper userspace programs should use the correct size, these valid
fixes (Thanks !) will prevent bad programs to use this for some bad purpose.
I think both methods are OK, I tend to the sledgehammer, because it is the
lowest CPU consuming method - but on the other side the valid length is
small enough.

Thanks
Karsten

Re: [patch v2] isdn: make sure strings are null terminated

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Thu, 24 Nov 2011 15:41:49 +0300

> These strings come from the user.  We strcpy() them inside
> cf_command() so we should check that they are NULL terminated and
> return an error if not.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: use strnlen() instead of strlen().

Applied.