Taking strlen of buffers copied from userspace

Taking strlen of buffers copied from userspace

[Artem Frolov]

Hello,

I am in the process of testing static defect analyzer on a Linux
kernel source code (see disclosure below).

I found some potential array bounds violations. The pattern is as
follows: bytes are copied from the user space and then buffer is
accessed on index strlen(buf)-1. This is a defect if user data start
from 0. So the question is: can we make any assumptions what data may
be received from the user or it could be arbitrary?

For example, in ./drivers/block/cciss.c, function cciss_proc_write
(line numbers are taken form 2.6.11.3):
   ....
   293          if (count > sizeof(cmd)-1) return -EINVAL;
   294          if (copy_from_user(cmd, buffer, count)) return -EFAULT;
   295          cmd[count] = '\0';
   296          len = strlen(cmd);      // above 3 lines ensure safety
   297          if (cmd[len-1] == '\n')
   298                  cmd[--len] = '\0';
   .....

Another example is arch/i386/kernel/cpu/mtrr/if.c, function mtrr_write:
   ....
   107          if (copy_from_user(line, buf, len - 1))
   108                  return -EFAULT;
   109          ptr = line + strlen(line) - 1;
   110          if (*ptr == '\n')
   111                  *ptr = '\0';
    ....


Full disclosure: I am working for Klocwork (http://www.klocwork.com/),
which is a vendor of commercial closed-source proprietary products,
static analyzer for C/C++ is part of its products

Best regards
--
Artem Frolov
Senior software engineer
Klocwork inc

Re: Taking strlen of buffers copied from userspace

[Robert Hancock]

Artem Frolov wrote:
> Hello,
> 
> I am in the process of testing static defect analyzer on a Linux
> kernel source code (see disclosure below).
> 
> I found some potential array bounds violations. The pattern is as
> follows: bytes are copied from the user space and then buffer is
> accessed on index strlen(buf)-1. This is a defect if user data start
> from 0. So the question is: can we make any assumptions what data may
> be received from the user or it could be arbitrary?

In general I don't think any such assumptions should be made. In the 
case of the two below I'm assuming that root access is required to write 
those files, preventing any serious security hole, but it shouldn't 
really be permitted to corrupt kernel memory like this, as would likely 
happen if somebody wrote some data that contained a null as the first 
character.

> 
> For example, in ./drivers/block/cciss.c, function cciss_proc_write
> (line numbers are taken form 2.6.11.3):
>    ....
>    293          if (count > sizeof(cmd)-1) return -EINVAL;
>    294          if (copy_from_user(cmd, buffer, count)) return -EFAULT;
>    295          cmd[count] = '\0';
>    296          len = strlen(cmd);      // above 3 lines ensure safety
>    297          if (cmd[len-1] == '\n')
>    298                  cmd[--len] = '\0';
>    .....
> 
> Another example is arch/i386/kernel/cpu/mtrr/if.c, function mtrr_write:
>    ....
>    107          if (copy_from_user(line, buf, len - 1))
>    108                  return -EFAULT;
>    109          ptr = line + strlen(line) - 1;
>    110          if (*ptr == '\n')
>    111                  *ptr = '\0';
>     ....
> 

This one is also unsafe if somebody writes some data which is not 
null-terminated (assuming that that's possible), since strlen will run 
off the end of the buffer. The first example doesn't have that problem.

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Re: Taking strlen of buffers copied from userspace

[Randy.Dunlap]

Robert Hancock wrote:
> Artem Frolov wrote:
> 
>> Hello,
>>
>> I am in the process of testing static defect analyzer on a Linux
>> kernel source code (see disclosure below).
>>
>> I found some potential array bounds violations. The pattern is as
>> follows: bytes are copied from the user space and then buffer is
>> accessed on index strlen(buf)-1. This is a defect if user data start
>> from 0. So the question is: can we make any assumptions what data may
>> be received from the user or it could be arbitrary?
> 
> 
> In general I don't think any such assumptions should be made. In the 
> case of the two below I'm assuming that root access is required to write 
> those files, preventing any serious security hole, but it shouldn't 
> really be permitted to corrupt kernel memory like this, as would likely 
> happen if somebody wrote some data that contained a null as the first 
> character.
> 
>>
>> For example, in ./drivers/block/cciss.c, function cciss_proc_write
>> (line numbers are taken form 2.6.11.3):
>>    ....
>>    293          if (count > sizeof(cmd)-1) return -EINVAL;
>>    294          if (copy_from_user(cmd, buffer, count)) return -EFAULT;
>>    295          cmd[count] = '\0';
>>    296          len = strlen(cmd);      // above 3 lines ensure safety
>>    297          if (cmd[len-1] == '\n')
>>    298                  cmd[--len] = '\0';
>>    .....
>>
>> Another example is arch/i386/kernel/cpu/mtrr/if.c, function mtrr_write:
>>    ....
>>    107          if (copy_from_user(line, buf, len - 1))
>>    108                  return -EFAULT;
>>    109          ptr = line + strlen(line) - 1;
>>    110          if (*ptr == '\n')
>>    111                  *ptr = '\0';
>>     ....
>>
> 
> This one is also unsafe if somebody writes some data which is not 
> null-terminated (assuming that that's possible), since strlen will run 
> off the end of the buffer. The first example doesn't have that problem.

The latter one does (before the listed code):

	memset(line, 0, LINE_SIZE);
	if (len > LINE_SIZE)
		len = LINE_SIZE;
	if (copy_from_user(line, buf, len - 1))
		return -EFAULT;

so isn't line[LINE_SIZE - 1] always 0 ?

-- 
~Randy

Re: Taking strlen of buffers copied from userspace

[Robert Hancock]

Randy.Dunlap wrote:
> The latter one does (before the listed code):
> 
>     memset(line, 0, LINE_SIZE);
>     if (len > LINE_SIZE)
>         len = LINE_SIZE;
>     if (copy_from_user(line, buf, len - 1))
>         return -EFAULT;
> 
> so isn't line[LINE_SIZE - 1] always 0 ?

In that case, yes (I hadn't looked at the surrounding code). Rather an 
odd way of doing it, but shouldn't have that problem. Could still be 
subject to problems if buf contains a null at the first character, 
unless they're somehow preventing that too..

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

Re: Taking strlen of buffers copied from userspace

[Randy.Dunlap]

Robert Hancock wrote:
> Randy.Dunlap wrote:
> 
>> The latter one does (before the listed code):
>>
>>     memset(line, 0, LINE_SIZE);
>>     if (len > LINE_SIZE)
>>         len = LINE_SIZE;
>>     if (copy_from_user(line, buf, len - 1))
>>         return -EFAULT;
>>
>> so isn't line[LINE_SIZE - 1] always 0 ?
> 
> 
> In that case, yes (I hadn't looked at the surrounding code). Rather an 
> odd way of doing it, but shouldn't have that problem. Could still be 
> subject to problems if buf contains a null at the first character, 
> unless they're somehow preventing that too..

Yes, that's still a problem.

-- 
~Randy

[PATCH] Taking strlen of buffers copied from userspace

[Randy.Dunlap]

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

Artem Frolov wrote:
> Hello,
> 
> I am in the process of testing static defect analyzer on a Linux
> kernel source code (see disclosure below).
> 
> I found some potential array bounds violations. The pattern is as
> follows: bytes are copied from the user space and then buffer is
> accessed on index strlen(buf)-1. This is a defect if user data start
> from 0. So the question is: can we make any assumptions what data may
> be received from the user or it could be arbitrary?

Both are potential problems for someone with CAP_SYS_ADMIN
capabilties.  Attached are patches for them.


> Full disclosure: I am working for Klocwork (http://www.klocwork.com/),
> which is a vendor of commercial closed-source proprietary products,
> static analyzer for C/C++ is part of its products


-- 
~Randy

[-- Attachment #2: mtrr_strlen_v2.patch --]
[-- Type: text/x-patch, Size: 1122 bytes --]


mtrr: prevent copy_from_user(to, from, -1) or (if that should
  succeed somehow) write to line[-1] (on stack);

Signed-off-by: Randy Dunlap <rddunlap@osdl.org>

diffstat:=
 arch/i386/kernel/cpu/mtrr/if.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff -Naurp ./arch/i386/kernel/cpu/mtrr/if.c~mtrr_strlen ./arch/i386/kernel/cpu/mtrr/if.c
--- ./arch/i386/kernel/cpu/mtrr/if.c~mtrr_strlen	2005-03-01 23:37:50.000000000 -0800
+++ ./arch/i386/kernel/cpu/mtrr/if.c	2005-03-15 20:02:35.000000000 -0800
@@ -98,16 +98,20 @@ mtrr_write(struct file *file, const char
 	unsigned long long base, size;
 	char *ptr;
 	char line[LINE_SIZE];
+	size_t linelen;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
+	if (!len)
+		return -EINVAL;
 	memset(line, 0, LINE_SIZE);
 	if (len > LINE_SIZE)
 		len = LINE_SIZE;
 	if (copy_from_user(line, buf, len - 1))
 		return -EFAULT;
-	ptr = line + strlen(line) - 1;
-	if (*ptr == '\n')
+	linelen = strlen(line);
+	ptr = line + linelen - 1;
+	if (linelen && *ptr == '\n')
 		*ptr = '\0';
 	if (!strncmp(line, "disable=", 8)) {
 		reg = simple_strtoul(line + 8, &ptr, 0);

[-- Attachment #3: cciss_strlen.patch --]
[-- Type: text/x-patch, Size: 748 bytes --]


cciss: prevent write to cmd[-1] (on stack);

Signed-off-by: Randy Dunlap <rddunlap@osdl.org>

diffstat:=
 drivers/block/cciss.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

diff -Naurp ./drivers/block/cciss.c~cciss_strlen ./drivers/block/cciss.c
--- ./drivers/block/cciss.c~cciss_strlen	2005-03-14 15:28:18.000000000 -0800
+++ ./drivers/block/cciss.c	2005-03-15 14:53:52.000000000 -0800
@@ -304,7 +304,7 @@ cciss_proc_write(struct file *file, cons
 	if (copy_from_user(cmd, buffer, count)) return -EFAULT;
 	cmd[count] = '\0';
 	len = strlen(cmd);	// above 3 lines ensure safety
-	if (cmd[len-1] == '\n') 
+	if (len && cmd[len-1] == '\n') 
 		cmd[--len] = '\0';
 #	ifdef CONFIG_CISS_SCSI_TAPE
 		if (strcmp("engage scsi", cmd)==0) {