Bug 698704

Summary: [MuPDF] The fix for CVE-2017-15587 (bug 698605) is incorrect
Product: MuPDF Reporter: Kan-Ru Chen <koster>
Component: mupdfAssignee: MuPDF bugs <mupdf-bugs>
Status: RESOLVED FIXED    
Severity: normal CC: robin.watts
Priority: P4    
Version: unspecified   
Hardware: PC   
OS: Linux   
Customer: Word Size: ---

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.