Bug 1264813 - ggz-base-libs: does not disable internal XML entity expansion
Summary: ggz-base-libs: does not disable internal XML entity expansion
Keywords:
Status: NEW
Alias: None
Product: Fedora
Classification: Fedora
Component: ggz-base-libs
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Pete Walter
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 888729
TreeView+ depends on / blocked
 
Reported: 2015-09-21 09:33 UTC by Florian Weimer
Modified: 2019-08-08 15:22 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Enhancement
Doc Text:
Clone Of: 889142
Environment:
Last Closed:
Type: Bug
Embargoed:


Attachments (Terms of Use)

Description Florian Weimer 2015-09-21 09:33:07 UTC
+++ This bug was initially created as a clone of Bug #889142 +++

ggz-base-libs 0.99.5 contains the following code in ggzcore/netxml.c:

        if (!(net->parser = XML_ParserCreate("UTF-8")))
                ggz_error_sys_exit
                    ("Couldn't allocate memory for XML parser");

        /* Setup handlers for tags */
        XML_SetElementHandler(net->parser, (XML_StartElementHandler)
                              _ggzcore_net_parse_start_tag,
                              (XML_EndElementHandler)
                              _ggzcore_net_parse_end_tag);
        XML_SetCharacterDataHandler(net->parser, _ggzcore_net_parse_text);
        XML_SetUserData(net->parser, net);

This does not disable expansion of XML entities in the internal DTD subset, making the code subject to denial-of-service attacks ("billion laughs").  It seems the data comes from the network, so a trust boundary is crossed, making this a (low impact) security issue.

Adding the following handler using

  XML_SetEntityDeclHandler(expat, EntityDeclHandler);

should be sufficient to address this issue.

// Stop the parser when an entity declaration is encountered.
static void
EntityDeclHandler(void *userData,
		  const XML_Char *entityName, int is_parameter_entity,
		  const XML_Char *value, int value_length,
		  const XML_Char *base, const XML_Char *systemId,
		  const XML_Char *publicId, const XML_Char *notationName)
{
  XML_StopParser((XML_Parser)userData, XML_FALSE);
}

Comment 1 Jan Kurik 2016-02-24 13:46:49 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 24 development cycle.
Changing version to '24'.

More information and reason for this action is here:
https://fedoraproject.org/wiki/Fedora_Program_Management/HouseKeeping/Fedora24#Rawhide_Rebase

Comment 2 Fedora Admin XMLRPC Client 2016-04-21 19:17:10 UTC
This package has changed ownership in the Fedora Package Database.  Reassigning to the new owner of this component.

Comment 3 Fedora Admin XMLRPC Client 2016-04-26 07:38:41 UTC
This package has changed ownership in the Fedora Package Database.  Reassigning to the new owner of this component.

Comment 4 Hans de Goede 2016-04-26 07:42:36 UTC
ggzcore's xml parsing is actually intended to parse xml coming from the network, so I'm afraid that disallowing network based dtd-s may break things ...

Comment 5 Florian Weimer 2016-04-26 08:13:21 UTC
(In reply to Hans de Goede from comment #4)
> ggzcore's xml parsing is actually intended to parse xml coming from the
> network, so I'm afraid that disallowing network based dtd-s may break things
> ...

Do you expect these XML documents contain an internal DTD subset?

Comment 6 Hans de Goede 2016-04-26 08:38:51 UTC
(In reply to Florian Weimer from comment #5)
> (In reply to Hans de Goede from comment #4)
> > ggzcore's xml parsing is actually intended to parse xml coming from the
> > network, so I'm afraid that disallowing network based dtd-s may break things
> > ...
> 
> Do you expect these XML documents contain an internal DTD subset?

XML is not really my forte so I do not know, I just picked up ggz because it is a dep of a couple of games I [co-]maintain. Looking at the code / protocol it does not seem to use DTDs at all, the code in question is for parsing a streaming XML protocol with messages like this:

http://www.ggzgamingzone.org/docs/spec/html/req_login.html

So I think that the fix you suggested is safe and should not lead to regressions, if you think the same I'll go ahead and add your fix.


Note You need to log in before you can comment on or make changes to this bug.