Bug 698704 - [MuPDF] The fix for CVE-2017-15587 (bug 698605) is incorrect
Summary: [MuPDF] The fix for CVE-2017-15587 (bug 698605) is incorrect
Status: RESOLVED FIXED
Alias: None
Product: MuPDF
Classification: Unclassified
Component: mupdf (show other bugs)
Version: unspecified
Hardware: PC Linux
: P4 normal
Assignee: MuPDF bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-26 08:27 UTC by Kan-Ru Chen
Modified: 2019-05-08 13:50 UTC (History)
1 user (show)

See Also:
Customer:
Word Size: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Kan-Ru Chen 2017-10-26 08:27:02 UTC
https://git.ghostscript.com/?p=mupdf.git;a=commit;h=82df2631d7d0446b206ea6b434ea609b6c28b0e8

The original fix contains this change:

-       if (i0 < 0 || i1 < 0)
+       if (i0 < 0 || i1 < 0 || (i0+i1) < 0)

Since signed integer overflow is undefined behavior, compiler has the right to assume it won't happen. The previous i0 <0 || i1 < 0 check makes sure that i0 and i1 are both greater than zero, the last check then considered always true.

That's what has happened when I were preparing update for Debian. If compiled with build=debug the reproducer file no longer crashes but if compiled with build=release, the reproducer still crashes.

I ended up using this patch in Debian:


-       if (i0 < 0 || i1 < 0)
+       if (i0 < 0 || i1 < 0 || __builtin_add_overflow_p(i0, i1, (int)0))
Comment 1 Kan-Ru Chen 2017-10-26 08:28:08 UTC
(In reply to Kan-Ru Chen from comment #0)
> https://git.ghostscript.com/?p=mupdf.git;a=commit;
> h=82df2631d7d0446b206ea6b434ea609b6c28b0e8
> 
> The original fix contains this change:
> 
> -       if (i0 < 0 || i1 < 0)
> +       if (i0 < 0 || i1 < 0 || (i0+i1) < 0)
> 
> Since signed integer overflow is undefined behavior, compiler has the right
> to assume it won't happen. The previous i0 <0 || i1 < 0 check makes sure
> that i0 and i1 are both greater than zero, the last check then considered
> always true.

I mean, always *false*.
Comment 2 Robin Watts 2017-10-26 09:20:41 UTC
So instead of

 || ((i0+i1) < 0

use:

 || ((i0+i1) & (1<<(8*sizeof(int)-1))

maybe? Yuck.
Comment 3 Joseph Heenan 2017-10-26 09:35:33 UTC
Perhaps: if (i0 < 0 || i1 < 0 || i0 > (INT_MAX - i1))
Comment 4 Robin Watts 2017-10-26 09:44:30 UTC
(In reply to Joseph Heenan from comment #3)
> Perhaps: if (i0 < 0 || i1 < 0 || i0 > (INT_MAX - i1))

Ah, yes, that's nicer. Thanks.
Comment 5 Robin Watts 2017-11-07 10:29:38 UTC
I have a fix awaiting review.
Comment 6 Robin Watts 2017-11-08 04:32:32 UTC
Fixed in:

commit d18bc728e46c5a5708f14d27c2b6c44e1d0c3232
Author: Robin Watts <robin.watts@artifex.com>
Date:   Tue Nov 7 17:53:40 2017 +0000

    Bug 698704: Fix for overflow check failing due to 'clever' compiler.

    Adopt Josephs suggested fix for arithmetic overflow.

    Thanks to Kan-Ru Chen for spotting the problem.