Make the XML entity recursion check more precise. libxml doesn't detect entity recursion specifically but has a variety of related checks, such as entities not expanding too deeply or producing exponential blow-ups in content. Because entity declarations are parsed in a separate context with their own element recursion budget, a recursive entity can overflow the stack using a lot of open elements (but within the per-context limit) as it slowly consumes (but does not exhaust) the entity depth budget. This adds a specific, precise check for recursive entities that detects entity recursion specifically and fails immediately. The existing entity expansion depth checks are still relevant for long chains of different entities. BUG=628581 Review-Url: https://codereview.chromium.org/2539003002 Cr-Commit-Position: refs/heads/master@{#436899} Index: libxml2-2.9.4/entities.c =================================================================== --- libxml2-2.9.4.orig/entities.c +++ libxml2-2.9.4/entities.c @@ -159,6 +159,7 @@ xmlCreateEntity(xmlDictPtr dict, const x memset(ret, 0, sizeof(xmlEntity)); ret->type = XML_ENTITY_DECL; ret->checked = 0; + ret->guard = XML_ENTITY_NOT_BEING_CHECKED; /* * fill the structure. @@ -931,6 +932,7 @@ xmlCopyEntity(xmlEntityPtr ent) { cur->orig = xmlStrdup(ent->orig); if (ent->URI != NULL) cur->URI = xmlStrdup(ent->URI); + cur->guard = 0; return(cur); } Index: libxml2-2.9.4/include/libxml/entities.h =================================================================== --- libxml2-2.9.4.orig/include/libxml/entities.h +++ libxml2-2.9.4/include/libxml/entities.h @@ -30,6 +30,11 @@ typedef enum { XML_INTERNAL_PREDEFINED_ENTITY = 6 } xmlEntityType; +typedef enum { + XML_ENTITY_NOT_BEING_CHECKED, + XML_ENTITY_BEING_CHECKED /* entity check is in progress */ +} xmlEntityRecursionGuard; + /* * An unit of storage for an entity, contains the string, the value * and the linkind data needed for the linking in the hash table. @@ -60,6 +65,7 @@ struct _xmlEntity { /* this is also used to count entities * references done from that entity * and if it contains '<' */ + xmlEntityRecursionGuard guard; }; /* Index: libxml2-2.9.4/parser.c =================================================================== --- libxml2-2.9.4.orig/parser.c +++ libxml2-2.9.4/parser.c @@ -133,6 +133,10 @@ xmlParserEntityCheck(xmlParserCtxtPtr ct if (ctxt->lastError.code == XML_ERR_ENTITY_LOOP) return (1); + if ((ent != NULL) && (ent->guard == XML_ENTITY_BEING_CHECKED)) { + xmlFatalErr(ctxt, XML_ERR_ENTITY_LOOP, NULL); + return (1); + } /* * This may look absurd but is needed to detect * entities problems @@ -143,12 +147,14 @@ xmlParserEntityCheck(xmlParserCtxtPtr ct unsigned long oldnbent = ctxt->nbentities; xmlChar *rep; + ent->guard = XML_ENTITY_BEING_CHECKED; ent->checked = 1; ++ctxt->depth; rep = xmlStringDecodeEntities(ctxt, ent->content, XML_SUBSTITUTE_REF, 0, 0, 0); --ctxt->depth; + ent->guard = XML_ENTITY_NOT_BEING_CHECKED; if (ctxt->errNo == XML_ERR_ENTITY_LOOP) { ent->content[0] = 0; } @@ -7337,23 +7343,28 @@ xmlParseReference(xmlParserCtxtPtr ctxt) * if its replacement text matches the production labeled * content. */ - if (ent->etype == XML_INTERNAL_GENERAL_ENTITY) { - ctxt->depth++; - ret = xmlParseBalancedChunkMemoryInternal(ctxt, ent->content, - user_data, &list); - ctxt->depth--; - - } else if (ent->etype == XML_EXTERNAL_GENERAL_PARSED_ENTITY) { - ctxt->depth++; - ret = xmlParseExternalEntityPrivate(ctxt->myDoc, ctxt, ctxt->sax, - user_data, ctxt->depth, ent->URI, - ent->ExternalID, &list); - ctxt->depth--; - } else { - ret = XML_ERR_ENTITY_PE_INTERNAL; - xmlErrMsgStr(ctxt, XML_ERR_INTERNAL_ERROR, - "invalid entity type found\n", NULL); - } + if (ent->guard == XML_ENTITY_BEING_CHECKED) { + ret = XML_ERR_ENTITY_LOOP; + } else { + ent->guard = XML_ENTITY_BEING_CHECKED; + if (ent->etype == XML_INTERNAL_GENERAL_ENTITY) { + ctxt->depth++; + ret = xmlParseBalancedChunkMemoryInternal(ctxt, ent->content, + user_data, &list); + ctxt->depth--; + } else if (ent->etype == XML_EXTERNAL_GENERAL_PARSED_ENTITY) { + ctxt->depth++; + ret = xmlParseExternalEntityPrivate(ctxt->myDoc, ctxt, ctxt->sax, + user_data, ctxt->depth, ent->URI, + ent->ExternalID, &list); + ctxt->depth--; + } else { + ret = XML_ERR_ENTITY_PE_INTERNAL; + xmlErrMsgStr(ctxt, XML_ERR_INTERNAL_ERROR, + "invalid entity type found\n", NULL); + } + ent->guard = XML_ENTITY_NOT_BEING_CHECKED; + } /* * Store the number of entities needing parsing for this entity @@ -7456,23 +7467,29 @@ xmlParseReference(xmlParserCtxtPtr ctxt) else user_data = ctxt->userData; - if (ent->etype == XML_INTERNAL_GENERAL_ENTITY) { - ctxt->depth++; - ret = xmlParseBalancedChunkMemoryInternal(ctxt, - ent->content, user_data, NULL); - ctxt->depth--; - } else if (ent->etype == - XML_EXTERNAL_GENERAL_PARSED_ENTITY) { - ctxt->depth++; - ret = xmlParseExternalEntityPrivate(ctxt->myDoc, ctxt, - ctxt->sax, user_data, ctxt->depth, - ent->URI, ent->ExternalID, NULL); - ctxt->depth--; - } else { - ret = XML_ERR_ENTITY_PE_INTERNAL; - xmlErrMsgStr(ctxt, XML_ERR_INTERNAL_ERROR, - "invalid entity type found\n", NULL); - } + if (ent->guard == XML_ENTITY_BEING_CHECKED) { + ret = XML_ERR_ENTITY_LOOP; + } else { + ent->guard = XML_ENTITY_BEING_CHECKED; + if (ent->etype == XML_INTERNAL_GENERAL_ENTITY) { + ctxt->depth++; + ret = xmlParseBalancedChunkMemoryInternal(ctxt, + ent->content, user_data, NULL); + ctxt->depth--; + } else if (ent->etype == + XML_EXTERNAL_GENERAL_PARSED_ENTITY) { + ctxt->depth++; + ret = xmlParseExternalEntityPrivate(ctxt->myDoc, ctxt, + ctxt->sax, user_data, ctxt->depth, + ent->URI, ent->ExternalID, NULL); + ctxt->depth--; + } else { + ret = XML_ERR_ENTITY_PE_INTERNAL; + xmlErrMsgStr(ctxt, XML_ERR_INTERNAL_ERROR, + "invalid entity type found\n", NULL); + } + ent->guard = XML_ENTITY_NOT_BEING_CHECKED; + } if (ret == XML_ERR_ENTITY_LOOP) { xmlFatalErr(ctxt, XML_ERR_ENTITY_LOOP, NULL); return;