iDefense has sent us the attached draft advisory
Created attachment 12941 [details] Draft advisory
Created attachment 13034 [details] [review] Proposed fix
okay please drop the first hunk of ajax's patch, its buggy as size is replaced in the derference.. further down ProcPanoramiXShmCreatePixmap contains the same code does this also require a fix?
Created attachment 13099 [details] [review] proposed fix - for both functions.. Okay this fixes both functions non-panoramix and panoramix..
Created attachment 13178 [details] Testcase This is a test for the simplest case (width & height of 32768, depth of 32 bit = 32768 * 32768 * 4 = INT_MAX+1), which is allowed to pass before the fix, and now caught by the simple width > 32767 & depth > 32767 checks - better tests for the cases in the more complex code paths would be good to have too.
This is public now
Patch has beem committed: 6de61f82728df22ea01f9659df6581b87f33f11d
Created attachment 13779 [details] mitshm-testcase.c Enhanced testcase that also verifies that 1bpp pixmaps continue to work, ie, that: http://cgit.freedesktop.org/xorg/xserver/commit/?id=e9fa7c1c88a8130a48f772c92b186b8b777986b5 does the right thing.
+ size = PixmapBytePad(width, depth) * height; + if (sizeof(size) == 4 && BitsPerPixel(depth) > 8) { ^^^ IMHO this should read >= Currently this *doesn't* test 8bpp pixmaps. + if (size < width * height) + return BadAlloc; I *really* don't understand this test. When can this ever hit true (with depth >=8, that is)? size, width, and height are unsigned 32bit, and 32767*32767 perfectly fits 32bit. + /* thankfully, offset is unsigned */ + if (stuff->offset + size < size) + return BadAlloc; This overwrap test (which I assume was the original fix) can (and should) always be done, independently of the machine size and pixel depth. Agreed?
(In reply to comment #9) > + size = PixmapBytePad(width, depth) * height; > + if (sizeof(size) == 4 && BitsPerPixel(depth) > 8) { > ^^^ > IMHO this should read >= > Currently this *doesn't* test 8bpp pixmaps. Sure, but everybody stores depth 8 pixmaps as 8bpp, and as you note below, 32767*32767 < 2^32-1. It'd be harmless to test 8bpp too, I suppose. > + if (size < width * height) > + return BadAlloc; > > I *really* don't understand this test. When can this ever hit true (with depth > >=8, that is)? size, width, and height are unsigned 32bit, and 32767*32767 > perfectly fits 32bit. Yeah, for <=32bpp it's fine. I suppose it's just paranoia in case anyone starts doing >32bpp, but that day is soon. > + /* thankfully, offset is unsigned */ > + if (stuff->offset + size < size) > + return BadAlloc; > > This overwrap test (which I assume was the original fix) can (and should) > always be done, independently of the machine size and pixel depth. > > Agreed? Agreed.
> > + if (size < width * height) > > + return BadAlloc; > > > > I *really* don't understand this test. When can this ever hit true (with depth > > >=8, that is)? size, width, and height are unsigned 32bit, and 32767*32767 > > perfectly fits 32bit. > > Yeah, for <=32bpp it's fine. I suppose it's just paranoia in case anyone > starts doing >32bpp, but that day is soon. Hm. Yes, sounds true. But wouldn't be the proper fix to just change size to CARD64? AFAICS the structure is used in this file only... Pushed the offset wrapping in be6c17fcf9e.
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.