Thread: pgxml bug (crash) in xslt_proc.c

pgxml bug (crash) in xslt_proc.c

From

Mark Simonetti

Date:

11 October 2014, 18:27:17

Hi,
I think I've found a bug in xslt_proc.c in the pgxml contrib module that
I've also fixed.  It is a serious bug as it crashes the entire database
backend.  Do I just describe it here, or is there somewhere else to
report that, or is there a way for me to submit the actual bug fix?

Apologies as I'm not sure of the protocol here.

Regards,
Mark.
--

Re: pgxml bug (crash) in xslt_proc.c

From

Tom Lane

Date:

11 October 2014, 18:33:36

Mark Simonetti <marks@opalsoftware.co.uk> writes:
> I think I've found a bug in xslt_proc.c in the pgxml contrib module that
> I've also fixed.  It is a serious bug as it crashes the entire database
> backend.  Do I just describe it here, or is there somewhere else to
> report that, or is there a way for me to submit the actual bug fix?

If you want you can report it to pgsql-security at postgresql.org
instead of the regular bugs list.  We'd probably only treat it as
a security issue if the bug seems exploitable for more than a mere
crash (eg, if it could lead to privilege escalation or arbitrary
code execution).  If you're not sure about the possible consequences
probably best to let the -security list see it first.

Whichever way you report it, please include your proposed fix along
with the bug report.

            regards, tom lane

Re: pgxml bug (crash) in xslt_proc.c

From

Tom Lane

Date:

11 October 2014, 19:39:40

Mark Simonetti <marks@opalsoftware.co.uk> writes:
> I hadn't really thought of it as a security issue, it came about from
> just trying to use it normally while developing software for one of my
> clients.  At first I found it hard to repeat, but I eventually found a
> query to repeat the problem 100% of the time. Unfortunately the XML I
> used to repeat it is vast and generated from lots of database data so it
> would be hard to submit that as a test case (though I can if it would
> help by capturing the XML data into a file and sending it along with the
> XSLT file).

Well, it would be nice to have a test case ...

> It seems to be to do with the order in which resources are
> freed:

> I changed this (xslt_proc.c, pgxml, postgres 9.3.5, line 167 onwards) : -

>      xsltFreeStylesheet(stylesheet);
>      xmlFreeDoc(restree);
>      xmlFreeDoc(doctree);
>      xsltFreeSecurityPrefs(xslt_sec_prefs);
>      xsltFreeTransformContext(xslt_ctxt);  <== crash here

> To this:

>      xsltFreeTransformContext(xslt_ctxt);
>      xsltFreeSecurityPrefs(xslt_sec_prefs);
>      xsltFreeStylesheet(stylesheet);
>      xmlFreeDoc(restree);
>      xmlFreeDoc(doctree);

> No more crash.

... but this seems like a pretty straightforward change: probably the
problem is that the xslt_ctxt has a dangling pointer to the
xslt_sec_prefs, stylesheet, or doctree.

Actually it seems to me the most sensible thing would be to free these
various objects in reverse order of creation, which would mean that it
ought to be

      xmlFreeDoc(restree);
      xsltFreeTransformContext(xslt_ctxt);
      xsltFreeSecurityPrefs(xslt_sec_prefs);
      xsltFreeStylesheet(stylesheet);
      xmlFreeDoc(doctree);

Would you try that on your test case and see if it's OK?

            regards, tom lane

Re: pgxml bug (crash) in xslt_proc.c

From

Mark Simonetti

Date:

11 October 2014, 20:09:31

PS, I forgot to mention that this is on a Windows system.  I have not
yet tried to repeat it on Linux but I can give it a try.

Regards,
Mark.
--

On 11/10/2014 16:52, Mark Simonetti wrote:
> Hi Tom,
> I hadn't really thought of it as a security issue, it came about from
> just trying to use it normally while developing software for one of my
> clients.  At first I found it hard to repeat, but I eventually found a
> query to repeat the problem 100% of the time. Unfortunately the XML I
> used to repeat it is vast and generated from lots of database data so
> it would be hard to submit that as a test case (though I can if it
> would help by capturing the XML data into a file and sending it along
> with the XSLT file).  It seems to be to do with the order in which
> resources are freed:
>
> I changed this (xslt_proc.c, pgxml, postgres 9.3.5, line 167 onwards) : -
>
>     xsltFreeStylesheet(stylesheet);
>     xmlFreeDoc(restree);
>     xmlFreeDoc(doctree);
>     xsltFreeSecurityPrefs(xslt_sec_prefs);
>     xsltFreeTransformContext(xslt_ctxt);  <== crash here
>
> To this:
>
>     xsltFreeTransformContext(xslt_ctxt);
>     xsltFreeSecurityPrefs(xslt_sec_prefs);
>     xsltFreeStylesheet(stylesheet);
>     xmlFreeDoc(restree);
>     xmlFreeDoc(doctree);
>
> No more crash.
>
> The offending part seemed to be freeing the transform "xslt_ctxt"
> context before freeing the doc "doctree".  Indeed if I check the XSLT
> example with the XSLT distribution
> (http://xmlsoft.org/libxslt/downloads.html) they do free the transform
> context first.
>
> I'm guessing that when the transform context is freed, it is trying to
> free something related to the previously already freed document.
>
> I also switched the order in the "catch" segment just above (line 147
> onwards).
>
> Regards,
> Mark.
> --
>
> On 11/10/2014 16:33, Tom Lane wrote:
>> Mark Simonetti <marks@opalsoftware.co.uk> writes:
>>> I think I've found a bug in xslt_proc.c in the pgxml contrib module
>>> that
>>> I've also fixed.  It is a serious bug as it crashes the entire database
>>> backend.  Do I just describe it here, or is there somewhere else to
>>> report that, or is there a way for me to submit the actual bug fix?
>> If you want you can report it to pgsql-security at postgresql.org
>> instead of the regular bugs list.  We'd probably only treat it as
>> a security issue if the bug seems exploitable for more than a mere
>> crash (eg, if it could lead to privilege escalation or arbitrary
>> code execution).  If you're not sure about the possible consequences
>> probably best to let the -security list see it first.
>>
>> Whichever way you report it, please include your proposed fix along
>> with the bug report.
>>
>>             regards, tom lane
>

Re: pgxml bug (crash) in xslt_proc.c

From

Mark Simonetti

Date:

11 October 2014, 20:09:36

Hi Tom,
I hadn't really thought of it as a security issue, it came about from
just trying to use it normally while developing software for one of my
clients.  At first I found it hard to repeat, but I eventually found a
query to repeat the problem 100% of the time. Unfortunately the XML I
used to repeat it is vast and generated from lots of database data so it
would be hard to submit that as a test case (though I can if it would
help by capturing the XML data into a file and sending it along with the
XSLT file).  It seems to be to do with the order in which resources are
freed:

I changed this (xslt_proc.c, pgxml, postgres 9.3.5, line 167 onwards) : -

     xsltFreeStylesheet(stylesheet);
     xmlFreeDoc(restree);
     xmlFreeDoc(doctree);
     xsltFreeSecurityPrefs(xslt_sec_prefs);
     xsltFreeTransformContext(xslt_ctxt);  <== crash here

To this:

     xsltFreeTransformContext(xslt_ctxt);
     xsltFreeSecurityPrefs(xslt_sec_prefs);
     xsltFreeStylesheet(stylesheet);
     xmlFreeDoc(restree);
     xmlFreeDoc(doctree);

No more crash.

The offending part seemed to be freeing the transform "xslt_ctxt"
context before freeing the doc "doctree".  Indeed if I check the XSLT
example with the XSLT distribution
(http://xmlsoft.org/libxslt/downloads.html) they do free the transform
context first.

I'm guessing that when the transform context is freed, it is trying to
free something related to the previously already freed document.

I also switched the order in the "catch" segment just above (line 147
onwards).

Regards,
Mark.
--

On 11/10/2014 16:33, Tom Lane wrote:
> Mark Simonetti <marks@opalsoftware.co.uk> writes:
>> I think I've found a bug in xslt_proc.c in the pgxml contrib module that
>> I've also fixed.  It is a serious bug as it crashes the entire database
>> backend.  Do I just describe it here, or is there somewhere else to
>> report that, or is there a way for me to submit the actual bug fix?
> If you want you can report it to pgsql-security at postgresql.org
> instead of the regular bugs list.  We'd probably only treat it as
> a security issue if the bug seems exploitable for more than a mere
> crash (eg, if it could lead to privilege escalation or arbitrary
> code execution).  If you're not sure about the possible consequences
> probably best to let the -security list see it first.
>
> Whichever way you report it, please include your proposed fix along
> with the bug report.
>
>             regards, tom lane

Re: pgxml bug (crash) in xslt_proc.c

From

Mark Simonetti

Date:

12 October 2014, 18:41:10

On 11/10/2014 17:39, Tom Lane wrote:
> Mark Simonetti <marks@opalsoftware.co.uk> writes:
>> I hadn't really thought of it as a security issue, it came about from
>> just trying to use it normally while developing software for one of my
>> clients.  At first I found it hard to repeat, but I eventually found a
>> query to repeat the problem 100% of the time. Unfortunately the XML I
>> used to repeat it is vast and generated from lots of database data so it
>> would be hard to submit that as a test case (though I can if it would
>> help by capturing the XML data into a file and sending it along with the
>> XSLT file).
> Well, it would be nice to have a test case ...


No problem, I will sort something out though it might be tomorrow now.


>
>> It seems to be to do with the order in which resources are
>> freed:
>> I changed this (xslt_proc.c, pgxml, postgres 9.3.5, line 167 onwards) : -
>>       xsltFreeStylesheet(stylesheet);
>>       xmlFreeDoc(restree);
>>       xmlFreeDoc(doctree);
>>       xsltFreeSecurityPrefs(xslt_sec_prefs);
>>       xsltFreeTransformContext(xslt_ctxt);  <== crash here
>> To this:
>>       xsltFreeTransformContext(xslt_ctxt);
>>       xsltFreeSecurityPrefs(xslt_sec_prefs);
>>       xsltFreeStylesheet(stylesheet);
>>       xmlFreeDoc(restree);
>>       xmlFreeDoc(doctree);
>> No more crash.
> ... but this seems like a pretty straightforward change: probably the
> problem is that the xslt_ctxt has a dangling pointer to the
> xslt_sec_prefs, stylesheet, or doctree.


Yes I thought the same once I found where the problem was it seemed
fairly trivial; finding it wasn't quite so straightforward admittedly!
I'm almost certain it is doctree; xslt_sec_prefs is not touched in the
transformation context free function and stylesheet is never associated
with it (though I suppose it could be indirectly).  I'm just glad I
found it and can use this build of PostgreSQL for now as the people I'm
working for are a rather large client and they want to see it working.
This is the great thing about open source though.


>
> Actually it seems to me the most sensible thing would be to free these
> various objects in reverse order of creation, which would mean that it
> ought to be
>
>        xmlFreeDoc(restree);
>        xsltFreeTransformContext(xslt_ctxt);
>        xsltFreeSecurityPrefs(xslt_sec_prefs);
>        xsltFreeStylesheet(stylesheet);
>        xmlFreeDoc(doctree);
>
> Would you try that on your test case and see if it's OK?


Yep no problem, I will try that tomorrow.


Regards,
Mark.
--