Bug 13519

Summary: EVI Extension Integer Overflow Vulnerability
Product: xorg Reporter: Matthieu Herrb <matthieu.herrb>
Component: SecurityAssignee: X.Org Security <xorg_security>
Status: RESOLVED FIXED QA Contact: X.Org Security <xorg_security>
Severity: normal    
Priority: medium CC: jcristau, sndirsch
Version: 7.3 (2007.09)   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments:
Description Flags
Draft advisory
none
Proposed fix
none
updated proposed patch, adds include for scrnintstr.h
none
Testcase for Request Size overflow
none
updated patch after iDefense comments
none
patch with correct UINT32 definition none

Description Matthieu Herrb 2007-12-04 12:54:22 UTC
iDefense has sent us the attached draft advisory
Comment 1 Matthieu Herrb 2007-12-04 12:55:03 UTC
Created attachment 12940 [details]
Draft advisory
Comment 2 Adam Jackson 2007-12-11 11:50:59 UTC
Created attachment 13033 [details] [review]
Proposed fix
Comment 3 Dave Airlie 2007-12-13 23:13:05 UTC
Created attachment 13101 [details] [review]
updated proposed patch, adds include for scrnintstr.h
Comment 4 Alan Coopersmith 2007-12-21 18:08:42 UTC
Created attachment 13300 [details]
Testcase for Request Size overflow

This tests for the simple overflow of stuff->n_visual * sz_VisualID32
in the REQUEST_FIXED_SIZE check.
Comment 5 Matthieu Herrb 2008-01-04 14:02:07 UTC
Created attachment 13536 [details] [review]
updated patch after iDefense comments 

Here's the comment from iDefense: 

Matthew / X.Org,

We reviewed the patches and found that most of them appear to be
sufficient.  However, we did see some minor issues with them.

The patch for the EVI Integer Overflow bug does not appear to be
sufficient to prevent the integer overflow resulting from multiplcation.

     int max_sz_evi = n_visual * sz_xExtendedVisualInfo *
screenInfo.numScreens;
...
+
+    if (max_sz_evi < n_visual || max_sz_evi < screenInfo.numScreens)
+	return BadAlloc;
+

It is still possible for max_sz_evi to be greater than both n_visual and
screenInfo.numScreens even though an integer overflow has occurred.  For
example, let's assume there are 8 screens.

0x204F000 * 0x10 * 0x8 = 0x102780000

Due to 32-bit integer wrap, the resulting value stored is 0x2780000
which is larger than 0x204F000.  However, the resulting copy loop will
try to copy 0x102780000 bytes.  More care should be taken to ensure that
no overflow occurs for any of the multiplication operations.

===
unsigned long max_sz_evi = sz_xExtendedVisualInfo;

if (n_visual >= ULONG_MAX / max_sz_evi)
  return BadAlloc;
max_sz_evi *= n_visual;

if (screenInfo.numScreens >= ULONG_MAX / max_sz_evi)
  return BadAlloc;
max_sz_evi *= screenInfo.numScreens;
===

The other, similar integer overflow check suffers from the same ill
assumption.

Also, it appears that n_conflict is never set to anything except zero
rendering the loop setting the temp_conflict array entries unreached.
We're not sure of the intended use of this code, perhaps it should be
corrected or removed?

Please let us know if we should still expect a release next Tuesday.

Joshua J. Drake
VeriSign iDefense Labs
Comment 6 Alan Coopersmith 2008-01-07 16:25:04 UTC
(In reply to comment #5)

In the latest revision of the patch you have:

+#if HAVE_STDINT_H
+#include <stdint.h>
+#elif !defined(INT_MAX)
+#define INT_MAX 0x7fffffff
+#endif

but later on in that file, you use UINT32_MAX, not INT_MAX - shouldn't the above
be:
#elif !defined(UINT32_MAX)
#define UINT32_MAX 0xffffffff
?
Comment 7 Matthieu Herrb 2008-01-08 00:41:03 UTC
(In reply to comment #6)
> (In reply to comment #5)
> 
> In the latest revision of the patch you have:
> 
> +#if HAVE_STDINT_H
> +#include <stdint.h>
> +#elif !defined(INT_MAX)
> +#define INT_MAX 0x7fffffff
> +#endif
> 
> but later on in that file, you use UINT32_MAX, not INT_MAX - shouldn't the
> above
> be:
> #elif !defined(UINT32_MAX)
> #define UINT32_MAX 0xffffffff
> ?
> 

Sure... even #define UINT32_MAX 0xffffffffU
Comment 8 Matthieu Herrb 2008-01-08 00:46:17 UTC
Created attachment 13581 [details] [review]
patch with correct UINT32 definition
Comment 9 Matthieu Herrb 2008-01-17 08:25:15 UTC
This is public now. 
Comment 10 Matthieu Herrb 2008-01-17 08:26:42 UTC
Patch has been committed

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.