Обсуждение: pgxml bug (crash) in xslt_proc.c
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. --
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
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
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 >
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
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. --