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

[Alexander Strasser]

On 2017-03-01 01:48 +0100, Bálint Réczey wrote:
> 2017-03-01 0:54 GMT+01:00 Alexander Strasser <eclipse7 at gmx.net>:
> > 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,

That depends on the signedness of char. AFAIU it can return either
-128..127 or 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.

OK, I only wanted to point out that this solution does not support strings
longer than 127 bytes if char is signed.

> 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.

Yeah, I see that. I would not recommend changing the behavior of
quicktime_read_char without examination of all call sites. At least
they should only be inside of libquicktime IIUC.

One could also change it to return 0 in case the byte could not be
read and 0..255 if it could read the character. Yes, that interface
would be ambiguous when returning 0, but it could probably be used
without explicit error checking. I am not saying this is the way
to go, it's just one possibility and likely not the best.

> >
> > 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.

Sorry, I mixed up the function name.

> > 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.

Probably yes.


  Alexander