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

[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) {