[Gluster-devel] Help needed with Coverity - How to remove tainted_data_argument?

[Xavier Hernandez]

On 12/17/2014 12:46 PM, Krishnan Parthasarathi wrote:
>
>
> ----- Original Message -----
>> On 12/17/2014 11:29 AM, Niels de Vos wrote:
>>> On Wed, Dec 17, 2014 at 10:24:46AM +0100, Xavier Hernandez wrote:
>>>> I think the root cause of this particular problem is a pattern like this:
>>>>
>>>>       GF_ASSERT(type <= x);
>>>>
>>>>       array[type] = y
>>>>
>>>> I think there are several places where this pattern is used. GF_ASSERT()
>>>> is
>>>> there to detect bugs, however it does not stop the execution, it simply
>>>> logs
>>>> the problem. So if there's really a bug and 'type' > x, we will do an
>>>> incorrect access into the array. Additionally, if 'type' is obtained from
>>>> a
>>>> tainted pointer, coverity considers this a potential security issue.
>>>>
>>>> This is also applicable to NULL pointer checks and some others.
>
> This issue is because we are reading contents of a file into a fixed
> sized buffer. Coverity considers external sources like I/O as taint by
> default. My original question was if there are ways by which we can
> programmatically let Coverity know that the data is _not_ tainted. This could
> be a Coverity primitive, like explained here https://communities.coverity.com/thread/2303,
> or refactoring our code. AFAICS, this pattern is not the same as boundary checks.
> (NB. buffer in question is fixed length array and fgets (I/O) was performed
> with sizeof (buffer) as the limit.)

This problem derives to a boundary check as a side effect.

fgets() is safe (at least to avoid buffer overruns), what could not be 
safe is the data it reads. The problem here is that gluster allocates 
buffers with a header for memory accounting. The sequence is this:

1. fgets() gets data into a buffer. The buffer pointer is marked as tainted.
2. A buffer is allocated to store a copy of the string, so the resulting 
pointer is also marked as tainted.
3. The string is assigned to an array of strings. This seems to mark the 
array of strings as tainted (maybe this shouldn't happen).
4. The array of strings is redimensioned. This generates an access to 
the memory area just before the pointer to read memory block header 
info. Since this access has been made using a tainted pointer, the data 
read is also marked as tainted. Basically this means that 'type' is 
considered unsafe.
5. 'type' is used to access an array. Since 'type' is considered unsafe, 
the access itself could be unsafe.

Anyway I agree that this isn't a bug. It seems more a limitation of the 
tracking of what is really tainted and what not in coverity scan.

Xavi