Re: CVE-2016-8685 in potrace
Hi Hugo
I have looked at the correction in combination with the new and old source code.
The short summary is that this could very well be optimized away by
the optimizer, at least if it is a buggy optimizer. If you see a
difference in behavior between wheezy and later debian versions that
points towards a buggy optimizer. It could also be what I describe
below in "side-note".
I'll let you analyze this further. I do not have a conclusive answer,
but I think I have at least something. Read on.
The code in wheezy and sid are almost identical. In fact there are
only three changes to this file:
1) Copyright statement
2) An include of config.h
3) This patch that you refer to.
In fact if you compare the entire src directory of wheezy and sid you
can see that the total difference is very small. It can be summarized
in:
- the above
- support for gejson
- something for color shift option.
- src/trace.c (alpha > changed to alpha >= )
- build changes
There is nothing more that I can see. So my conclusion is that if the
wheezy version is the only one having this problem with the optimizer
optimizing away the fix, then that is most likely due to a bug in
wheezy version of gcc.
In any case I have analyzed the patch file and the corresponding code
and found the following.
So what is the change that the patch file do?
- for (x=x0; x<bm->w; x+=BM_WORDBITS) {
+ for (x=x0; x<bm->w && x>=0; x+=(unsigned)BM_WORDBITS) {
Let us take each change at a time:
Addition of && x >= 0:
--------------------------------
If we look at the code x can never be negative without data corruption.
x can be:
- x0 at first call of this function. The function calling findnext
start with x as 0. Even if the in-data is a negative value that will
be taken away due to the masking.
- 0 at the end of each x loop
- x0 + N * BM_WORDBITS
- 0 + N * BM_WORDBITS
In theory this means that it can not be zero. However we could have an
integer overflow if N is large enough. N in this case is bm->w.
Ah wait a second. If w is the largest possible value w can be, then at
+= BM_WORDBITS it can actually cause an overflow.
So if you ask me, the introduction of a x>=0 check could be seen as
superflous and maybe an optimizer can actually optimize that away as x
should not be negative with this code. However it has a meaning, but
it is very subtle.
I think it would be better to have a check to validate if w is too large.
As a side-note. We probably have a similar issue if something is
matched at this very end. Then x++ is done a number of times, and that
could also lead to integer overflow. Maybe this is what you see in
wheezy?
Addition of (unsigned):
---------------------------------
This is clearly superflous. First of all it will be converted back to
(int) as x is an integer. Secondly BM_WORDBITS is
8*(int)sizeof(potrace_word) = 8*(int)sizeof(unsigned long) and that is
definitely a positive number always.
I think this is definitely optimized away, if it ever had a meaning.
Best regards
// Ola
On 2 April 2017 at 11:06, Hugo Lefeuvre <hle@debian.org> wrote:
> Hi Ola,
>
>> I do not have any objection on marking it as no-dsa, especially since it is
>> that already for jessie.
>>
>> However I thought I should have a check but I can not find a patch. The
>> patch mentioned here, gives a 404.
>> https://blogs.gentoo.org/ago/2016/08/29/potrace-invalid-memory-access-in-findnext-decompose-c/
>>
>> Q1: What is the patch you have used?
>>
>> Q2: Is the problem still there for Stretch as well?
>
> No, the issue has already been fixed in Stretch, and the patch got
> integrated in 1.14. You can still find it here[0].
>
> It would be helpful if you could have a check, indeed ! I'd like to know
> why the patch only "fixes" the issue for -O0 and -O1.
>
> I briefly asked myself whether it could be a good idea to upload the
> package with a lower optimization level, but actually I think it would
> be a very bad solution.
>
> If the problem still affects potrace with higher optimization levels, then
> it means probably that something is still going wrong.
>
> Cheers,
> Hugo
>
> [0] https://sources.debian.net/src/potrace/1.13-3/debian/patches/cve-2016-8685.patch/
>
> --
> Hugo Lefeuvre (hle) | www.owl.eu.com
> 4096/ ACB7 B67F 197F 9B32 1533 431C AC90 AC3E C524 065E
--
--- Inguza Technology AB --- MSc in Information Technology ----
/ ola@inguza.com Folkebogatan 26 \
| opal@debian.org 654 68 KARLSTAD |
| http://inguza.com/ Mobile: +46 (0)70-332 1551 |
\ gpg/f.p.: 7090 A92B 18FE 7994 0C36 4FE4 18A1 B1CF 0FE5 3DD9 /
---------------------------------------------------------------
Reply to: