Re: Taking strlen of buffers copied from userspace

["Randy.Dunlap" <rddunlap@osdl.org>]

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