[SCM] libquicktime/master: Fix integer overflow in the quicktime_read_pascal function (CVE-2016-2399)

[Bálint Réczey]

Hi Alexander,

2017-03-01 0:54 GMT+01:00 Alexander Strasser <eclipse7 at gmx.net>:
> Hi!
>
> On 2017-02-27 22:54 +0000, rbalint at users.alioth.debian.org wrote:
>> The following commit has been merged in the master branch:
>> commit f6bb364af6a2bc65eab832614afb578c5740a0cc
>> Author: Balint Reczey <balint at balintreczey.hu>
>> Date:   Mon Feb 27 23:13:47 2017 +0100
>>
>>     Fix integer overflow in the quicktime_read_pascal function (CVE-2016-2399)
>>
>>     Closes: #855099
>>
>> diff --git a/debian/patches/CVE-2016-2399.patch b/debian/patches/CVE-2016-2399.patch
>> new file mode 100644
>> index 0000000..dfa8180
>> --- /dev/null
>> +++ b/debian/patches/CVE-2016-2399.patch
>> @@ -0,0 +1,22 @@
>> +diff --git a/src/util.c b/src/util.c
>> +index d8dc3c3..9422fc5 100644
>> +--- a/src/util.c
>> ++++ b/src/util.c
>> +@@ -340,9 +340,14 @@ int64_t quicktime_byte_position(quicktime_t *file)
>> +
>> + void quicktime_read_pascal(quicktime_t *file, char *data)
>> + {
>> +-    char len = quicktime_read_char(file);
>> +-    quicktime_read_data(file, (uint8_t*)data, len);
>> +-    data[(int)len] = 0;
>> ++    int len = quicktime_read_char(file);
>> ++    if ((len > 0) && (len < 256)) {
>> ++          /* data[] is expected to be 256 bytes long */
>> ++          quicktime_read_data(file, (uint8_t*)data, len);
>> ++          data[len] = 0;
>> ++        } else {
>> ++          data[0] = 0;
>> ++        }
>> + }
>
>
> I might be missing something, so please check my words and the code carefully.
>
> Could the len ever get bigger than 255 ?

First, thank you for checking the patch.

You are right, at the moment quicktime_read_char() does return values 0..255,
in int which type can hold values from a much bigger range including
negative values.
To clearly check for those I have added the range check which makes sure that
the code indexes the valid indices of data[].

>
> If my assumption is true and that security issue only happens with signed char
> and the problem is therefore writing in front of the array, I would think that
> there is a simpler fix while still supporting strings larger than 127 bytes.
>
> E.g. by changing the type of the variable at src/util.c : 820 to uint8_t
> and the type of len at src/util.c : 343 to int or uint8_t .

This would be a possible solution, but I don't like narrowing conversions
without clear markers and I like storing return values to the same type
because it leaves less opportunities for hidden problems.
Sometimes the return type's range is wider than the valid values in order
to signal error using a value which is not 'valid'.

See fgetc() for example which returns 0..255, or EOF (-1) in case of an error.

If quicktime_read_char() ever starts returning -1 on error this part
of code stays
correct in a sense that 0..255 values will be handled correctly, but simply
using uint8_t would then convert -1 to 255 which may mess up other things.

>
> I am sorry if I am misunderstanding the issue, the fix just made me curious to
> dig a little deeper. However I can't see how your commit would not fix the
> security issue or introduce new issues.
>
>
>
> Apart from this issue in particular I think there can be more trouble with
> quicktime_read_char and for example quicktime_read_pascal :
>
> In quicktime_read_pascal (src/util.c : 820) the variable output is not

This would be quicktime_read_char(), I guess.

> initialized and following call to quicktime_read_data could return without
> writing into output and thus another line below we would return an
> uninitialized value.
>
> For example in quicktime_read_pascal this could lead to making up strings
> full strings (max size 256 bytes) of uninitialized data.
>
> If and how this can become a security problem is a different story and
> I did not investigate the code any further.

This could cause an information leak, but this is much less severe than
CVE-2016-2399 IMO. You are right, there are other issues in the code
and I actually spotted this one and started fixing it, but I found new
problems everywhere I looked and had to focus on this CVE.

The article about the original issue contains this part:
http://www.nemux.org/2016/02/23/libquicktime-1-2-4/
...
 Disclosure part

 Let me make a little introduction... i'm sure this is not the only
issue of this library. I suppose libquicktime needs refactoring
process (IMHO)
...

I would say libquicktime could use an audit and it would benefit from
more extensive error checking at several places.

Cheers,
Balint