My favorites | Sign in
Project Home Downloads Wiki Issues Code Search
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 93472: Yet another double-free caused by malformed XPath expression in XSLT
2 people starred this issue and may be notified of changes. Back to list
Status:  Fixed
Owner:  cev...@chromium.org
Closed:  Aug 2011
Cc:  veill...@gmail.com, ddkilzer@gmail.com, happyaro...@gmail.com, ddkil...@apple.com

Restricted
  • Only users with EditIssue permission may comment.


Sign in to add a comment
 
Reported by yangding...@gmail.com, Aug 18, 2011
This bug is quite similar in nature to  issue 63444  and 89402. When the XPath engine processes some malformed expression, say //book[hello:world()][1], it first evaluates the //book part, resulting in a node set filled with book elements. After that, each of these elements is set as the new context node where subsequent predicate [hello:world()] is evaluated. This evaluation happens in function xmlXPathCompOpEvalPositionalPredicate() of xpath.c, and after a series of calls from that function, control finally moves into xmlXPathCompOpEval(), where the following code is executed:

13392                     if (op->value5 == NULL)
13393                         func =
13394                             xmlXPathFunctionLookup(ctxt->context,
13395                                                    op->value4);
13396                     else {
13397                         URI = xmlXPathNsLookup(ctxt->context, op->value5);
13398                         if (URI == NULL) {
13399                             xmlGenericError(xmlGenericErrorContext,
13400             "xmlXPathCompOpEval: function %s bound to undefined prefix %s\n",
13401                                     (char *)op->value4, (char *)op->value5);
13402                             return (total);
13403                         }
13404                         func = xmlXPathFunctionLookupNS(ctxt->context,
13405                                                         op->value4, URI);
13406                     }
13407                     if (func == NULL) {
13408                         xmlGenericError(xmlGenericErrorContext,
13409                                 "xmlXPathCompOpEval: function %s not found\n",
13410                                         (char *)op->value4);
13411                         XP_ERROR0(XPATH_UNKNOWN_FUNC_ERROR);
13412                     }

The code first calls xmlXPathNsLookup() to look for the namespace prefix 'hello', which obviously would fail and return NULL. Then this failure is signaled by a call to xmlGenericError() and control returns afterwards without setting any flags to indicate this error, like the code in line 13411 does.

14036             xmlXPathCompOpEval(ctxt, op);
14037             if (ctxt->error != XPATH_EXPRESSION_OK)
14038                 return(-1);
14039 
14040             resObj = valuePop(ctxt);
14041             if (resObj == NULL)
14042                 return(-1);
14043             break;
14044     }
14045 
14046     if (resObj) {
14047         int res;
14048 
14049         if (resObj->type == XPATH_BOOLEAN) {
14050             res = resObj->boolval;
14051         } else if (isPredicate) {
14052             /*
14053             * For predicates a result of type "number" is handled
14054             * differently:
14055             * SPEC XPath 1.0:
14056             * "If the result is a number, the result will be converted
14057             *  to true if the number is equal to the context position
14058             *  and will be converted to false otherwise;"
14059             */
14060             res = xmlXPathEvaluatePredicateResult(ctxt, resObj);
14061         } else {
14062             res = xmlXPathCastToBoolean(resObj);
14063         }
14064         xmlXPathReleaseObject(ctxt->context, resObj);
14065         return(res);
14066     }

Now we've returned from xmlXPathCompOpEval() at line 14036. Since the error check in line 14037 doesn't reveal anything exceptional, the code assumes the XPath function was found and its result had been placed in the value table. So this 'resObj' is popped up from the value table (line 14040), converted to a boolean value (line 14060), and finally released by xmlXPathReleaseObject (line 14064). Actually this 'resObj' is the context object, wrapping one of the book elements from the aforementioned node set. This context object would be explicitly released afterwards, resulting in a double-free bug.

VERSION
Chrome Version: 13.0.782.112 (Official Build 95650) / 15.0.857.0 (Developer Build 97295)
Operating System: Windows XP SP3

REPRODUCTION CASE
Please see the attached files (repro.html, demo.xml and demo.xsl). Put them in the same web directory, load repro.html in the browser and a sad tab would show up.

repro.html
77 bytes   View   Download
demo.xml
293 bytes   Download
demo.xsl
321 bytes   Download
Aug 19, 2011
#1 scarybea...@gmail.com
Let me see if my patch fixes this one too or not...
Owner: cev...@chromium.org
Aug 24, 2011
#2 scarybea...@gmail.com
Nope, definitely a new one. How many of these do you have, Yang?? :)
Status: Assigned
Labels: reward-topanel Mstone-14
Aug 24, 2011
#3 yangding...@gmail.com
Actually, none. I've completed the new round of test where the XPath engine has gone through a suite of test cases that sum up to 3GB+ in size. This is the only issue it turned out. So I doubt if there would be a new bug report titled 'Yet yet another double-free...' :-D
Aug 25, 2011
#4 scarybea...@gmail.com
(No comment was entered for this change.)
Cc: veill...@gmail.com
Aug 25, 2011
#5 scarybea...@gmail.com
Thanks for all your work on this Yang! Glad that things are looking more solid.
I've landed a simple fix to Chromium which simply sets the error flag in a couple of cases involving unknown prefix names.
I don't think any web pages should be depending on execution somehow continuing after using an invalid namespace, so the change should be safe for Chromium :D
I've pinged upstream libxml about whether it's a correct fix or not.

Status: FixUnreleased
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Aug 25, 2011
#6 veill...@gmail.com
Okay, I will look quickly. Do you have a CVE handy for it ?

  thanks,

Daniel
Aug 25, 2011
#7 scarybea...@gmail.com
Sure, use CVE-2011-2834
Labels: CVE-2011-2834 Merge-Approved
Aug 29, 2011
#8 scarybea...@gmail.com
Merged r98359 to M14 at r98648
Labels: -Merge-Approved Merge-Merged
Sep 8, 2011
#9 scarybea...@gmail.com
@yangdingning: thanks again for all your work on XPath! My pleasure to offer another $1000 Chromium Security Reward.

----
Boilerplate text:
Please do NOT publicly disclose details until a fix has been released to all our
users. Early public disclosure may cancel the provisional reward.
Also, please be considerate about disclosure when the bug affects a core library
that may be used by other products.
Please do NOT share this information with third parties who are not directly
involved in fixing the bug. Doing so may cancel the provisional reward.
Please be honest if you have already disclosed anything publicly or to third parties.
----
Labels: -reward-topanel reward-1000 reward-unpaid
Sep 16, 2011
#10 scarybea...@gmail.com
(No comment was entered for this change.)
Cc: ddkilzer@gmail.com
Sep 23, 2011
#11 scarybea...@gmail.com
Payment in system.
Labels: -reward-unpaid
Oct 4, 2011
#12 jschuh@chromium.org
Batch update.
Labels: SecImpacts-Stable
Oct 11, 2011
#13 veill...@gmail.com
BTW I commited upstream a fix which augments Chris patch with
a few other places where the interpretor was forgetting to set the
context error :-) I don't think they are big issues, one is a case of out
of memory error and the other one if the XPath compiled representation
is somehow broken, unlikely to happen:

http://git.gnome.org/browse/libxml2/commit/?id=1d4526f6f4ec8d18c40e2a09b387652a6c1aa2cd

  thanks !

Daniel
May 15, 2012
#14 cdn@chromium.org
Marking old security bugs Fixed..
Status: Fixed
Jul 13, 2012
#15 jschuh@chromium.org
CC'ing Debian libxml maintainer.
Cc: happyaro...@gmail.com
Oct 13, 2012
#16 bugdro...@chromium.org
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.
Labels: Restrict-AddIssueComment-Commit
Mar 9, 2013
#17 bugdro...@chromium.org
(No comment was entered for this change.)
Labels: -Type-Security -Mstone-14 -SecImpacts-Stable Security-Impact-Stable Type-Bug-Security M-14
Mar 10, 2013
#18 bugdro...@chromium.org
(No comment was entered for this change.)
Labels: -Area-Undefined
Mar 13, 2013
#19 bugdro...@chromium.org
(No comment was entered for this change.)
Labels: Restrict-View-EditIssue
Mar 13, 2013
#20 bugdro...@chromium.org
(No comment was entered for this change.)
Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue
Mar 21, 2013
#21 scarybea...@gmail.com
(No comment was entered for this change.)
Labels: -Restrict-View-SecurityNotify -Restrict-View-EditIssue
Mar 21, 2013
#22 bugdro...@chromium.org
(No comment was entered for this change.)
Labels: -Security-Impact-Stable Security_Impact-Stable
Dec 19, 2013
#23 par...@chromium.org
(No comment was entered for this change.)
Cc: ddkil...@apple.com
Sign in to add a comment

Powered by Google Project Hosting