Bug 459830

So the patch has this bit:

--- include/libxml/parser.h     (revision 3771)
+++ include/libxml/parser.h     (working copy)
@@ -297,6 +297,7 @@ struct _xmlParserCtxt {
      */
     xmlError          lastError;
     xmlParserMode     parseMode;    /* the parser mode */
+    unsigned long    nbentities;    /* number of entities references */
 };
 
 /**
--- include/libxml/entities.h.orig      2005-01-04 15:49:49.000000000 +0100
+++ include/libxml/entities.h   2008-08-11 17:56:53.000000000 +0200
@@ -56,6 +56,7 @@ struct _xmlEntity {
     struct _xmlEntity     *nexte;      /* unused */
     const xmlChar           *URI;      /* the full URI as computed */
     int                    owner;      /* does the entity own the childrens */
+    unsigned long     nbentities;      /* the number of entities references */
 };
 
Which is growing a structure that's in a public header file.

librsvg is doing some iffy code:

       entity = xmlMalloc (sizeof(xmlEntity));
       memset(entity, 0, sizeof(xmlEntity));
       entity->type = XML_ENTITY_DECL;
       dupname = (xmlChar *) xmlMemStrdup ((const char *)name);
       entity->name = dupname; 
       
       entity->etype = type;

in a function called rsvg_entity_decl

It should probably be using some constructor api instead (xmlAddEntity?), but we can't break ABI in RHEL. It might break existing customers systems.

I'd recommend this errata getting fixed a different way that doesn't change abi.

Oh crap !!!
there are 
  xmlAddDocEntity() and xmlAddDtdEntity() which do this kind of things
in the API.
Creation of entities really should be done only by an XML parser in 
the DTD parsing stage, no application code should ever create some
unless they buid a document with a DTD, and those entry points are for this.

Daniel

Really this is a code abuse, there are constructor for those,
and growing a structure is usually safe, the patch also grows the
parser structure which is also public. And there was no way to make
a clean handling of that DoS without growing the structures to add
additional informations.
I first tried a simpler, less intrusive approach, but this still would
not catch some of the entities recursions exposed.
Absolute ABI maintainance for libxml2 is near impossible for the simple
reason that (nearly) everything is exposed.
In a nutshell I don't have a solution which will work reliably without
extending those 2 structures. I have tried, really,

Daniel

Looks like this is restricted to the following icon set:
/usr/share/icons/HighContrast-SVG


backtrace from gdb 'eog HighContrast.svg' (Thanks dmalcolm!):
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1114208576 (LWP 5615)]
0x00000035c4871033 in _int_free () from /lib64/libc.so.6
(gdb) bt
#0  0x00000035c4871033 in _int_free () from /lib64/libc.so.6
#1  0x00000035c4874c5c in free () from /lib64/libc.so.6
#2  0x00002ab9acd4da4c in xmlParseEntityDecl () from /usr/lib64/libxml2.so.2
#3  0x00002ab9acd4e973 in xmlParseMarkupDecl () from /usr/lib64/libxml2.so.2
#4  0x00002ab9acd4ea0e in xmlParseMarkupDecl () from /usr/lib64/libxml2.so.2
#5  0x00002ab9acd53118 in xmlParseChunk () from /usr/lib64/libxml2.so.2
#6  0x00002aaaaacfe8e0 in rsvg_error_quark () from /usr/lib64/librsvg-2.so.2
#7  0x00002aaaaaab8d3c in fill_info () from
/usr/lib64/gtk-2.0/2.10.0/loaders/svg_loader.so
#8  0x00000035c94098d4 in gdk_pixbuf_loader_write () from
/usr/lib64/libgdk_pixbuf-2.0.so.0
#9  0x000000000041ef1b in g_cclosure_marshal_VOID__VOID ()
#10 0x000000000041a156 in g_cclosure_marshal_VOID__VOID ()
#11 0x00000000004238f2 in g_cclosure_marshal_VOID__VOID ()
#12 0x000000000042345a in g_cclosure_marshal_VOID__VOID ()
#13 0x00000035c6048f24 in g_thread_create_full () from /lib64/libglib-2.0.so.0
#14 0x00000035c54062f7 in start_thread () from /lib64/libpthread.so.0
#15 0x00000035c48d1b6d in clone () from /lib64/libc.so.6

Okay I have tried to make a patch where the struct _xmlEntity
is not changed, the nbentities is piggy backed on the checked int
in the same structure which is currently used only as a boolean.
It still grows the parser context structure, but really I don't see how
user code could hand duplicate it or try to embed it in a structure !
patch against upstream available as a proof of concept. But this would
need to be backported to RHEL 5 (and possibly 4).
I will look at this again tomorrow.

Daniel

> backtrace from gdb 'eog HighContrast.svg'
what I meant to say was "eog $(any icon from the HighContrast-SVG set)"

Created attachment 314849 [details]
Example of patch for upstream

w.r.t. comment #7, I'm afraid any icon containing an entity declaration
in their XML SVG will hit this. No idea what the rscvg had in mind, probably
they though that SAX parsing and doing entities their own way was a good idea.
Ben could you check if RHEL4 is affected too ?

  thanks,

Daniel

Damn the patch won't work for RHEL5, the struct  _xmlEntity don't
have a 'checked' field either. I don't see any way to piggy back the
information on another field, unless trying to reuse 'owner' which is
also used as a boolean, but its use is very distinct, and that could
really break programs which actually used the API correctly.

Daniel

Created attachment 314860 [details]
another patch related to upstream but using the owner field of the entity structure

that new patch reuses the owner field of the entity structure to count
the entities replacements needed to expand the current entity. The 
owner field was previously used only as 0 or 1 to indicate if the entity
subtree was owned by that entity node or a copy from another reference.
This makes that code even harder to follow but I think I checked all the 
cases possible, ran upstream regression tests and checked against memory 
leaks and it looks okay.
The patch still need to add a field to the parser context, but I think 
that can't be avoided, and this should be safe.
I note that the entity structures and the parser context have been grown
previously in the libxml2-2.6.x releases and that librsvg problem didn't
surfaced, I wonder if their upstream version has cleaned up that code.

I will now backport that new patch to RHEL-5 and RHEL-4 that should be 
relatively easy.

Daniel

Created attachment 314862 [details]
New patch for RHEL-5 reusing the owner field

Created attachment 314863 [details]
New patch for RHEL-4 reusing the owner field

Severity/Priority = medium/medium

GNOME:
This will crash all Gnome apps that attempt to render an SVG with an entity declaration.  For example gnome-panel will crash if the default icon set is "HighContrast-SVG"; Nautilus will crash if thumbnails are enabled and a customer navigates to a directory with an SVG containing an entity declaration.

Customers experiencing problems with the latest libmxl2 can unset the HighContrast-SVG icon set by running the following command from a console:

gconftool-2 --unset /desktop/gnome/interface/icon_theme

If the customer's custom application launchers are impacted, they can work around this bug by specifying a new icon in the relevant .desktop files found here:

~/.gnome2/panel2.d/default/launchers/

Additionally, thumbnails can be disabled in Nautilus with the following command:

gconftool-2 --type string -s /apps/nautilus/preferences/show_image_thumbnails "never"

or by setting "Show Thumbnails" to "Never" in the following control panel:
System -> Preferences -> File Management -> Preview

KDE:
Unable to reproduce this bug in KDE.

(In reply to comment #9)
> Ben could you check if RHEL4 is affected too ?

I can't reproduce this on RHEL-4 (verified on i386 and x86_64)

Created attachment 314908 [details]
New patch for RHEL-3 reusing the owner field

Runnning:
find /usr/share/icons -name \*.svg | xargs grep ENTITY | wc -l
on my RHEL 5 workstation shows 751 references to XML entities within svg files; all of these are within /usr/share/icons/HighContrast-SVG

Not all svg files within /usr/share/icons/HighContrast-SVG contain XML entities.

This is with gnome-themes-2.16.0-1.fc6

(In reply to comment #16)
> (...)
> KDE:
> Unable to reproduce this bug in KDE.

It seems Dolphin (in KDE 4.1) crashes, too (while navigating a directory icon.) The new patch solves this issue, though.

Correct, Dolphin / Strigi was mentioned in the Debian bug report:

http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=496125#99