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

[Alexander Strasser]

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 ?

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 .

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


  Alexander