[patch] isdn: fix information leak

[patch] isdn: fix information leak

[Dan Carpenter]

The main motivation of this patch changing strcpy() to strlcpy().  
We strcpy() to copy a 48 byte buffers into a 49 byte buffers.  So at
best the last byte has leaked information, or maybe there is an
overflow?  Anyway, this patch closes the information leaks by zeroing
the memory and the calls to strlcpy() prevent overflows. 

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/drivers/isdn/sc/ioctl.c b/drivers/isdn/sc/ioctl.c
index 1081091..2655e3a 100644
--- a/drivers/isdn/sc/ioctl.c
+++ b/drivers/isdn/sc/ioctl.c
@@ -174,7 +174,7 @@ int sc_ioctl(int card, scs_ioctl *data)
 		pr_debug("%s: SCIOGETSPID: ioctl received\n",
 				sc_adapter[card]->devicename);
 
-		spid = kmalloc(SCIOC_SPIDSIZE, GFP_KERNEL);
+		spid = kzalloc(SCIOC_SPIDSIZE, GFP_KERNEL);
 		if (!spid) {
 			kfree(rcvmsg);
 			return -ENOMEM;
@@ -194,7 +194,7 @@ int sc_ioctl(int card, scs_ioctl *data)
 			kfree(rcvmsg);
 			return status;
 		}
-		strcpy(spid, rcvmsg->msg_data.byte_array);
+		strlcpy(spid, rcvmsg->msg_data.byte_array, SCIOC_SPIDSIZE);
 
 		/*
 		 * Package the switch type and send to user space
@@ -272,12 +272,12 @@ int sc_ioctl(int card, scs_ioctl *data)
 			return status;
 		}
 
-		dn = kmalloc(SCIOC_DNSIZE, GFP_KERNEL);
+		dn = kzalloc(SCIOC_DNSIZE, GFP_KERNEL);
 		if (!dn) {
 			kfree(rcvmsg);
 			return -ENOMEM;
 		}
-		strcpy(dn, rcvmsg->msg_data.byte_array);
+		strlcpy(dn, rcvmsg->msg_data.byte_array, SCIOC_DNSIZE);
 		kfree(rcvmsg);
 
 		/*
@@ -348,7 +348,7 @@ int sc_ioctl(int card, scs_ioctl *data)
 		pr_debug("%s: SCIOSTAT: ioctl received\n",
 				sc_adapter[card]->devicename);
 
-		bi = kmalloc (sizeof(boardInfo), GFP_KERNEL);
+		bi = kzalloc(sizeof(boardInfo), GFP_KERNEL);
 		if (!bi) {
 			kfree(rcvmsg);
 			return -ENOMEM;

Re: [patch] isdn: fix information leak

[Changli Gao]

On Thu, Aug 5, 2010 at 5:38 PM, Dan Carpenter <error27@gmail.com> wrote:
> The main motivation of this patch changing strcpy() to strlcpy().
> We strcpy() to copy a 48 byte buffers into a 49 byte buffers.  So at
> best the last byte has leaked information, or maybe there is an
> overflow?  Anyway, this patch closes the information leaks by zeroing
> the memory and the calls to strlcpy() prevent overflows.

strlcpy() can handle the terminator NUL. so you don't need to zero it.

>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
>
> diff --git a/drivers/isdn/sc/ioctl.c b/drivers/isdn/sc/ioctl.c
> index 1081091..2655e3a 100644
> --- a/drivers/isdn/sc/ioctl.c
> +++ b/drivers/isdn/sc/ioctl.c
> @@ -174,7 +174,7 @@ int sc_ioctl(int card, scs_ioctl *data)
>                pr_debug("%s: SCIOGETSPID: ioctl received\n",
>                                sc_adapter[card]->devicename);
>
> -               spid = kmalloc(SCIOC_SPIDSIZE, GFP_KERNEL);
> +               spid = kzalloc(SCIOC_SPIDSIZE, GFP_KERNEL);
>                if (!spid) {
>                        kfree(rcvmsg);
>                        return -ENOMEM;
> @@ -194,7 +194,7 @@ int sc_ioctl(int card, scs_ioctl *data)
>                        kfree(rcvmsg);
>                        return status;
>                }
> -               strcpy(spid, rcvmsg->msg_data.byte_array);
> +               strlcpy(spid, rcvmsg->msg_data.byte_array, SCIOC_SPIDSIZE);
>
>                /*
>                 * Package the switch type and send to user space
> @@ -272,12 +272,12 @@ int sc_ioctl(int card, scs_ioctl *data)
>                        return status;
>                }
>
> -               dn = kmalloc(SCIOC_DNSIZE, GFP_KERNEL);
> +               dn = kzalloc(SCIOC_DNSIZE, GFP_KERNEL);
>                if (!dn) {
>                        kfree(rcvmsg);
>                        return -ENOMEM;
>                }
> -               strcpy(dn, rcvmsg->msg_data.byte_array);
> +               strlcpy(dn, rcvmsg->msg_data.byte_array, SCIOC_DNSIZE);
>                kfree(rcvmsg);
>
>                /*
> @@ -348,7 +348,7 @@ int sc_ioctl(int card, scs_ioctl *data)
>                pr_debug("%s: SCIOSTAT: ioctl received\n",
>                                sc_adapter[card]->devicename);
>
> -               bi = kmalloc (sizeof(boardInfo), GFP_KERNEL);
> +               bi = kzalloc(sizeof(boardInfo), GFP_KERNEL);
>                if (!bi) {
>                        kfree(rcvmsg);
>                        return -ENOMEM;


-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [patch] isdn: fix information leak

[Dan Carpenter]

On Thu, Aug 05, 2010 at 06:08:08PM +0800, Changli Gao wrote:
> On Thu, Aug 5, 2010 at 5:38 PM, Dan Carpenter <error27@gmail.com> wrote:
> > The main motivation of this patch changing strcpy() to strlcpy().
> > We strcpy() to copy a 48 byte buffers into a 49 byte buffers.  So at
> > best the last byte has leaked information, or maybe there is an
> > overflow?  Anyway, this patch closes the information leaks by zeroing
> > the memory and the calls to strlcpy() prevent overflows.
> 
> strlcpy() can handle the terminator NUL. so you don't need to zero it.

If there are no NUL chars in "rcvmsg->msg_data.byte_array" then strlcpy()
is sufficient, but if there is a NUL character then you need to zero the
memory.  The patch handles both possibilities.

regards,
dan carpenter

Re: [patch] isdn: fix information leak

[Changli Gao]

On Thu, Aug 5, 2010 at 6:19 PM, Dan Carpenter <error27@gmail.com> wrote:
> On Thu, Aug 05, 2010 at 06:08:08PM +0800, Changli Gao wrote:
>> On Thu, Aug 5, 2010 at 5:38 PM, Dan Carpenter <error27@gmail.com> wrote:
>> > The main motivation of this patch changing strcpy() to strlcpy().
>> > We strcpy() to copy a 48 byte buffers into a 49 byte buffers.  So at
>> > best the last byte has leaked information, or maybe there is an
>> > overflow?  Anyway, this patch closes the information leaks by zeroing
>> > the memory and the calls to strlcpy() prevent overflows.
>>
>> strlcpy() can handle the terminator NUL. so you don't need to zero it.
>
> If there are no NUL chars in "rcvmsg->msg_data.byte_array" then strlcpy()
> is sufficient, but if there is a NUL character then you need to zero the
> memory.  The patch handles both possibilities.
>

the second parameter of strlcpy() must a NUL terminated C string. I
think you means strncpy().

FYI:
http://lxr.linux.no/#linux+v2.6.35/lib/string.c#L146
http://lxr.linux.no/#linux+v2.6.35/lib/string.c#L119


-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [patch] isdn: fix information leak

[Dan Carpenter]

On Thu, Aug 05, 2010 at 07:02:06PM +0800, Changli Gao wrote:
> 
> the second parameter of strlcpy() must a NUL terminated C string. I
> think you means strncpy().
> 

Both strncpy() and strlcpy() take a limitter.  The difference is that
strlcpy() always takes on a terminator and strncpy() only adds a
terminator if there is space.

strlcpy() is a BSD function that never caught on in Linux.  The glibc
maintainers think that if you accidentally chop off the last part of a
word that makes you an idiot.  They think you should known the length of
your data at all times and use memcpy() or a proper string library.

I prefer strlcpy() to strncpy().  Some people do stuff like:
	strncpy(bar, foo, n);
	bar[n] = '\0';
You have to read through the code to find if n is "sizeof(bar)" or
"sizeof(bar) - 1".  Which is a pain in the arse.  strlcpy() is explicit
and it's just one line of code instead of two.

The other tricky thing you should remember about strncpy() is that the
posix version writes NUL chars from the end of the string to the
limitter but the kernel version only copies one NUL character.

regards,
dan carpenter

> FYI:
> http://lxr.linux.no/#linux+v2.6.35/lib/string.c#L146
> http://lxr.linux.no/#linux+v2.6.35/lib/string.c#L119
> 

Re: [patch] isdn: fix information leak

[Changli Gao]

On Thu, Aug 5, 2010 at 7:37 PM, Dan Carpenter <error27@gmail.com> wrote:
>
> Both strncpy() and strlcpy() take a limitter.  The difference is that
> strlcpy() always takes on a terminator and strncpy() only adds a
> terminator if there is space.
>
> strlcpy() is a BSD function that never caught on in Linux.  The glibc
> maintainers think that if you accidentally chop off the last part of a
> word that makes you an idiot.  They think you should known the length of
> your data at all times and use memcpy() or a proper string library.
>
> I prefer strlcpy() to strncpy().  Some people do stuff like:
>        strncpy(bar, foo, n);
>        bar[n] = '\0';
> You have to read through the code to find if n is "sizeof(bar)" or
> "sizeof(bar) - 1".  Which is a pain in the arse.  strlcpy() is explicit
> and it's just one line of code instead of two.
>
> The other tricky thing you should remember about strncpy() is that the
> posix version writes NUL chars from the end of the string to the
> limitter but the kernel version only copies one NUL character.
>

You should spend some time on reading the source code of strlcpy() and
strncpy().

the example use of them is:

char dst[24];
char *src = "test";

strncpy(dst, src, sizeof(dst) - 1);
strlcpy(dst, src, sizeof(dst));

both of them don't need to zero dst, and they don't need to pad zero
at then end of the dst.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [patch] isdn: fix information leak

[Changli Gao]

On Thu, Aug 5, 2010 at 9:18 PM, Changli Gao <xiaosuo@gmail.com> wrote:
> On Thu, Aug 5, 2010 at 7:37 PM, Dan Carpenter <error27@gmail.com> wrote:
>>
>> Both strncpy() and strlcpy() take a limitter.  The difference is that
>> strlcpy() always takes on a terminator and strncpy() only adds a
>> terminator if there is space.
>>
>> strlcpy() is a BSD function that never caught on in Linux.  The glibc
>> maintainers think that if you accidentally chop off the last part of a
>> word that makes you an idiot.  They think you should known the length of
>> your data at all times and use memcpy() or a proper string library.
>>
>> I prefer strlcpy() to strncpy().  Some people do stuff like:
>>        strncpy(bar, foo, n);
>>        bar[n] = '\0';
>> You have to read through the code to find if n is "sizeof(bar)" or
>> "sizeof(bar) - 1".  Which is a pain in the arse.  strlcpy() is explicit
>> and it's just one line of code instead of two.
>>
>> The other tricky thing you should remember about strncpy() is that the
>> posix version writes NUL chars from the end of the string to the
>> limitter but the kernel version only copies one NUL character.
>>
>
> You should spend some time on reading the source code of strlcpy() and
> strncpy().
>
> the example use of them is:
>
> char dst[24];
> char *src = "test";
>
> strncpy(dst, src, sizeof(dst) - 1);

Oh, Sorry, I made a mistake here. As you said, the code should be
 strncpy(dst, src, sizeof(dst));
 dst[sizeof(dst) - 1] = '\0';

However, if you use strlcpy(), you really don't need to zero the dst buffer.

> strlcpy(dst, src, sizeof(dst));
>
> both of them don't need to zero dst, and they don't need to pad zero
> at then end of the dst.
>



-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [patch] isdn: fix information leak

[Dan Carpenter]

First of all, I'm happy that you're reviewing these patches.  A lot of
learner, often untested, patches go through kernel-janitors and the
entire purpose of the list is to check each other's work.

Here is what I meant by an information leak.  The original code did:
	spid = kmalloc(SCIOC_SPIDSIZE, GFP_KERNEL);
	...
	strcpy(spid, rcvmsg->msg_data.byte_array);
	...
	if (copy_to_user(data->dataptr, spid, SCIOC_SPIDSIZE)) {

When you allocate memory with kmalloc() it doesn't clear that memory.  It
could have anything in there including passwords etc.  If there is a
short string in "rcvmsg->msg_data.byte_array" like "123" then the
strcpy() puts that in the first 4 bytes of "spid", but the rest of the
buffer is still full of passwords.

The copy_to_user() sends it to the user, and normally the user only reads
as far as the terminator, but if they read past that then they would see
all the passwords etc that were saved there.

This function is in the ioctl() probably only root has open() permission
on the device so it's not a big deal because root can already find your
password.  But I didn't dig that far.  It's just simpler to zero out the
memory instead of worrying about if it really is exploitable or not.

Also it's quite possible that the strcpy can't overflow.  But it's weird
for the code to leave space for the terminator and then not use it.  In
the end, I decided to do the cautious thing and be done with it.

regards,
dan carpenter

Re: [patch] isdn: fix information leak

[Changli Gao]

On Thu, Aug 5, 2010 at 9:55 PM, Dan Carpenter <error27@gmail.com> wrote:
> First of all, I'm happy that you're reviewing these patches.  A lot of
> learner, often untested, patches go through kernel-janitors and the
> entire purpose of the list is to check each other's work.
>
> Here is what I meant by an information leak.  The original code did:
>        spid = kmalloc(SCIOC_SPIDSIZE, GFP_KERNEL);
>        ...
>        strcpy(spid, rcvmsg->msg_data.byte_array);
>        ...
>        if (copy_to_user(data->dataptr, spid, SCIOC_SPIDSIZE)) {
>
> When you allocate memory with kmalloc() it doesn't clear that memory.  It
> could have anything in there including passwords etc.  If there is a
> short string in "rcvmsg->msg_data.byte_array" like "123" then the
> strcpy() puts that in the first 4 bytes of "spid", but the rest of the
> buffer is still full of passwords.
>
> The copy_to_user() sends it to the user, and normally the user only reads
> as far as the terminator, but if they read past that then they would see
> all the passwords etc that were saved there.
>
> This function is in the ioctl() probably only root has open() permission
> on the device so it's not a big deal because root can already find your
> password.  But I didn't dig that far.  It's just simpler to zero out the
> memory instead of worrying about if it really is exploitable or not.
>
> Also it's quite possible that the strcpy can't overflow.  But it's weird
> for the code to leave space for the terminator and then not use it.  In
> the end, I decided to do the cautious thing and be done with it.
>

Thanks, Dan. I got it.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [patch] isdn: fix information leak

[David Miller]

From: Dan Carpenter <error27@gmail.com>
Date: Thu, 5 Aug 2010 11:38:06 +0200

> The main motivation of this patch changing strcpy() to strlcpy().  
> We strcpy() to copy a 48 byte buffers into a 49 byte buffers.  So at
> best the last byte has leaked information, or maybe there is an
> overflow?  Anyway, this patch closes the information leaks by zeroing
> the memory and the calls to strlcpy() prevent overflows. 
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>

Applied.