Created attachment 12940 [details]
Draft advisory

Created attachment 13033 [details] [review]
Proposed fix

Created attachment 13101 [details] [review]
updated proposed patch, adds include for scrnintstr.h

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.

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

(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
?

(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

Created attachment 13581 [details] [review]
patch with correct UINT32 definition

This is public now. 

Patch has been committed