isdn: fix information leak

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 <[email protected]>

drivers/isdn/sc/ioctl.c +5 -5

index 1081091..2655e3a 100644
@@ -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;

Quoted 5 lines · 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.

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

Quoted 7 lines · Changli Gao

Quoted 5 lines · 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.

Quoted 1 line

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

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.

Quoted 17 lines · Dan Carpenter

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.

Quoted 4 lines

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.

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.

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 <[email protected]>