Re: Taking strlen of buffers copied from userspace

Randy.Dunlap
2005-03-16T04:21:44Z

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