RFC/PATCH: avoid metafilevirus problems

[Marcus Meissner]

Hi,

requesting comments...

This patch reduces the attack vector on metafiles.

I originally wanted to filter only SETABORTPROC,
but there are a lot of things that might be used
to inject code.

Comments?

Ciao, Marcus

Changelog:
    Only allow whitelisted escape codes when playing metafiles.

Index: dlls/gdi/metafile.c
===================================================================
RCS file: /home/wine/wine/dlls/gdi/metafile.c,v
retrieving revision 1.10
diff -u -r1.10 metafile.c
--- dlls/gdi/metafile.c	5 Nov 2005 10:45:02 -0000	1.10
+++ dlls/gdi/metafile.c	2 Jan 2006 20:52:42 -0000
@@ -1121,10 +1121,57 @@
         GDIRealizePalette(hdc);
         break;

-    case META_ESCAPE:
+    case META_ESCAPE: {
+        BOOL passdown = FALSE;
+
+        switch (mr->rdParm[0]) {
+        case SETABORTPROC:
+             FIXME("NOTE: Suppressing SETABORTPROC in metafile, possible exploit.\n");
+             break;
+        case STARTDOC:
+        case ABORTDOC:
+        case ENDDOC:
+        case NEWFRAME:
+        case NEXTBAND:
+        case SETCOPYCOUNT:
+        case SETCOLORTABLE:
+        case FLUSHOUTPUT:
+        case DRAFTMODE:
+        case SELECTPAPERSOURCE:
+        case SETLINECAP:
+        case SETLINEJOIN:
+        case SETMITERLIMIT:
+        case DRAWPATTERNRECT:
+        case ENABLEDUPLEX:
+        case EPSPRINTING:
+        case SETDIBSCALING:
+        case EXTTEXTOUT:
+        case ENABLEPAIRKERNING:
+        case SETCHARSET:
+        case SETKERNTRACK:
+        case SETALLJUSTVALUES:
+        case STRETCHBLT:
+        case BEGIN_PATH:
+        case CLIP_TO_PATH:
+        case END_PATH:
+        case SET_ARC_DIRECTION:
+        case SET_BACKGROUND_COLOR:
+        case SET_POLY_MODE:
+        case SET_SCREEN_ANGLE:
+        case SET_SPREAD:
+        case TRANSFORM_CTM:
+        case SET_CLIP_BOX:
+        case SET_BOUNDS:
+        case SET_MIRROR_MODE:
+             passdown = TRUE;
+             break;
+        default:
+             FIXME("Ignoring strange Escape code %d in Metafile.\n");
+             break;
+        }
         Escape(hdc, mr->rdParm[0], mr->rdParm[1], (LPCSTR)&mr->rdParm[2], NULL);
         break;
-
+    }
     case META_EXTTEXTOUT:
         MF_Play_MetaExtTextOut( hdc, mr );
         break;

[Walt Ogburn]

On Mon, 2 Jan 2006, Marcus Meissner wrote:
> Hi,
>
> requesting comments...
>
> This patch reduces the attack vector on metafiles.
>
> I originally wanted to filter only SETABORTPROC,
> but there are a lot of things that might be used
> to inject code.
>
> Comments?

Would it not be better to block it in the gdi Escape function?  Or is
SETABORTPROC legitimitely needed in some cases outside of metafiles?

[Peter Beutner]

Walt Ogburn schrieb:
> On Mon, 2 Jan 2006, Marcus Meissner wrote:
> > Hi,
> >
> > requesting comments...
> >
> > This patch reduces the attack vector on metafiles.
> >
> > I originally wanted to filter only SETABORTPROC,
> > but there are a lot of things that might be used
> > to inject code.
> >
> > Comments?
>
> Would it not be better to block it in the gdi Escape function?  Or is
> SETABORTPROC legitimitely needed in some cases outside of metafiles?

IMO there is no reason to forbid SETABORTPROC outside of metafiles.I think the whole point 
of this is only to prevent that a wmf picture can be used to execute code, i.e. prevent 
handling of SETABORTPROC records in metafiles. But there should be nothing dangerous with 
an application calling directly Escape(SETABORTPROC, ...).

@Marcus
Are you sure your patch is right? You still pass every type of record to the Escape 
function. Shouldn't it be something like the attached patch?
(I changed the FIXME() -> WARN(). IMO it's not really something that can be "fixed".)

Btw. you said that there are more things that allow injecting code, but you still filter 
only SETABORTROC?

diff --git a/dlls/gdi/metafile.c b/dlls/gdi/metafile.c
index 1219888..dca5c05 100644
--- a/dlls/gdi/metafile.c
+++ b/dlls/gdi/metafile.c
@@ -1122,6 +1122,50 @@ BOOL WINAPI PlayMetaFileRecord( HDC hdc,
         break;

case META_ESCAPE:
+        switch (mr->rdParm[0]) {
+        case SETABORTPROC:
+             WARN("NOTE: Suppressing SETABORTPROC in metafile, possible exploit.\n");
+             return FALSE;
+        case STARTDOC:
+        case ABORTDOC:
+        case ENDDOC:
+        case NEWFRAME:
+        case NEXTBAND:
+        case SETCOPYCOUNT:
+        case SETCOLORTABLE:
+        case FLUSHOUTPUT:
+        case DRAFTMODE:
+        case SELECTPAPERSOURCE:
+        case SETLINECAP:
+        case SETLINEJOIN:
+        case SETMITERLIMIT:
+        case DRAWPATTERNRECT:
+        case ENABLEDUPLEX:
+        case EPSPRINTING:
+        case SETDIBSCALING:
+        case EXTTEXTOUT:
+        case ENABLEPAIRKERNING:
+        case SETCHARSET:
+        case SETKERNTRACK:
+        case SETALLJUSTVALUES:
+        case STRETCHBLT:
+        case BEGIN_PATH:
+        case CLIP_TO_PATH:
+        case END_PATH:
+        case SET_ARC_DIRECTION:
+        case SET_BACKGROUND_COLOR:
+        case SET_POLY_MODE:
+        case SET_SCREEN_ANGLE:
+        case SET_SPREAD:
+        case TRANSFORM_CTM:
+        case SET_CLIP_BOX:
+        case SET_BOUNDS:
+        case SET_MIRROR_MODE:
+             break;
+        default:
+             WARN("Ignoring strange Escape code %d in Metafile.\n");
+             return FALSE;
+        }
         Escape(hdc, mr->rdParm[0], mr->rdParm[1], (LPCSTR)&mr->rdParm[2], NULL);
         break;