From: Tom Lane <tgl@sss.pgh.pa.us>
To: Erik Wienhold <ewie@ewie.name>
cc: fuboat@outlook.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18617: PostgreSQL Server Subprocess Crashes by the XPATH
 Function Expression with Crafted Arguments
In-reply-to: <160394.1726271824@sss.pgh.pa.us>
References: <18617-1cee4d2ed1f4e7ae@postgresql.org>
 <08a0ba98-2dec-4773-8bfd-9eaced528372@ewie.name>
 <104996.1726245143@sss.pgh.pa.us> <107366.1726246563@sss.pgh.pa.us>
 <160394.1726271824@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Fri, 13 Sep 2024 19:57:04 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <356323.1726333651.0@sss.pgh.pa.us>
Date: Sat, 14 Sep 2024 13:07:54 -0400
Message-ID: <356363.1726333674@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/356363.1726333674%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <356323.1726333651.1@sss.pgh.pa.us>

I wrote:
> Based on Nick Wellnhofer's response there, I've experimented with the
> attached WIP patch, and it does seem to prevent the problem as long as
> you have a non-ancient libxml2.  This is only WIP because there are
> other xmlXPathCompile calls we'd have to fix.

Here's a fleshed-out patch.  I first thought that the XmlTableRoutine
code might need significant surgery, but it turns out not to be a
problem as long as we're willing to assume that XmlTableSetDocument
is called before XmlTableSetRowFilter or XmlTableSetColumnFilter.
Since the sole caller does it like that, this doesn't seem too
onerous.  I put in Asserts to check that, though.

> Nick also suggested that we not bother with a separate xmlXPathCompile
> call if we're just going to throw away the compiled expression after
> one use.  Perhaps that's good cleanup, not sure.  I don't know if
> anyone has serious ambitions of re-using the compiled XPath
> expressions.

I looked at this but realized that it'd cause a user-visible change
in error messages.  For instance, in xpath_internal we have

        xpathcomp = xmlXPathCtxtCompile(xpathctx, xpath_expr);
        if (xpathcomp == NULL || xmlerrcxt->err_occurred)
            xml_ereport(xmlerrcxt, ERROR, ERRCODE_INTERNAL_ERROR,
                        "invalid XPath expression");

        xpathobj = xmlXPathCompiledEval(xpathcomp, xpathctx);
        if (xpathobj == NULL || xmlerrcxt->err_occurred)
            xml_ereport(xmlerrcxt, ERROR, ERRCODE_INTERNAL_ERROR,
                        "could not create XPath object");

So syntax errors in the XPath expression draw "invalid XPath
expression", but if we merged these into one they'd draw
"could not create XPath object", or at least the same message
as execution-time errors.  That seems strictly worse for users,
and even if it were OK it's not the sort of thing I'd like to
change in minor releases.  So I think we're best off just doing
the minimum change s/xmlXPathCompile/xmlXPathCtxtCompile/.

I would have liked to include a regression test case showing
that the stack overflow is prevented, but I think we can't
since it would fail with pre-2.9.11 libxml2.

BTW, surely ERRCODE_INTERNAL_ERROR is the wrong errcode to use
here?  But that's a matter for a different patch.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="prevent-libxml2-stack-overrun.patch";
	charset="us-ascii"
Content-ID: <356323.1726333651.2@sss.pgh.pa.us>
Content-Description: prevent-libxml2-stack-overrun.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/contrib/xml2/xpath.c b/contrib/xml2/xpath.c
index f94b622d92..0fdf735faf 100644
--- a/contrib/xml2/xpath.c
+++ b/contrib/xml2/xpath.c
@@ -386,7 +386,7 @@ pgxml_xpath(text *document, xmlChar *xpath, xpath_work=
space *workspace)
 			workspace->ctxt->node =3D xmlDocGetRootElement(workspace->doctree);
 =

 			/* compile the path */
-			comppath =3D xmlXPathCompile(xpath);
+			comppath =3D xmlXPathCtxtCompile(workspace->ctxt, xpath);
 			if (comppath =3D=3D NULL)
 				xml_ereport(xmlerrcxt, ERROR, ERRCODE_EXTERNAL_ROUTINE_EXCEPTION,
 							"XPath Syntax Error");
@@ -649,7 +649,7 @@ xpath_table(PG_FUNCTION_ARGS)
 						ctxt->node =3D xmlDocGetRootElement(doctree);
 =

 						/* compile the path */
-						comppath =3D xmlXPathCompile(xpaths[j]);
+						comppath =3D xmlXPathCtxtCompile(ctxt, xpaths[j]);
 						if (comppath =3D=3D NULL)
 							xml_ereport(xmlerrcxt, ERROR,
 										ERRCODE_EXTERNAL_ROUTINE_EXCEPTION,
diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c
index 1a07876cd5..41b1a5c6b0 100644
--- a/src/backend/utils/adt/xml.c
+++ b/src/backend/utils/adt/xml.c
@@ -4448,7 +4448,13 @@ xpath_internal(text *xpath_expr_text, xmltype *data=
, ArrayType *namespaces,
 			}
 		}
 =

-		xpathcomp =3D xmlXPathCompile(xpath_expr);
+		/*
+		 * Note: here and elsewhere, be careful to use xmlXPathCtxtCompile not
+		 * xmlXPathCompile.  In libxml2 2.13.3 and older, the latter function
+		 * fails to defend itself against recursion-to-stack-overflow.  See
+		 * https://gitlab.gnome.org/GNOME/libxml2/-/issues/799
+		 */
+		xpathcomp =3D xmlXPathCtxtCompile(xpathctx, xpath_expr);
 		if (xpathcomp =3D=3D NULL || xmlerrcxt->err_occurred)
 			xml_ereport(xmlerrcxt, ERROR, ERRCODE_INTERNAL_ERROR,
 						"invalid XPath expression");
@@ -4819,7 +4825,10 @@ XmlTableSetRowFilter(TableFuncScanState *state, con=
st char *path)
 =

 	xstr =3D pg_xmlCharStrndup(path, strlen(path));
 =

-	xtCxt->xpathcomp =3D xmlXPathCompile(xstr);
+	/* We require XmlTableSetDocument to have been done already */
+	Assert(xtCxt->xpathcxt !=3D NULL);
+
+	xtCxt->xpathcomp =3D xmlXPathCtxtCompile(xtCxt->xpathcxt, xstr);
 	if (xtCxt->xpathcomp =3D=3D NULL || xtCxt->xmlerrcxt->err_occurred)
 		xml_ereport(xtCxt->xmlerrcxt, ERROR, ERRCODE_SYNTAX_ERROR,
 					"invalid XPath expression");
@@ -4850,7 +4859,10 @@ XmlTableSetColumnFilter(TableFuncScanState *state, =
const char *path, int colnum)
 =

 	xstr =3D pg_xmlCharStrndup(path, strlen(path));
 =

-	xtCxt->xpathscomp[colnum] =3D xmlXPathCompile(xstr);
+	/* We require XmlTableSetDocument to have been done already */
+	Assert(xtCxt->xpathcxt !=3D NULL);
+
+	xtCxt->xpathscomp[colnum] =3D xmlXPathCtxtCompile(xtCxt->xpathcxt, xstr)=
;
 	if (xtCxt->xpathscomp[colnum] =3D=3D NULL || xtCxt->xmlerrcxt->err_occur=
red)
 		xml_ereport(xtCxt->xmlerrcxt, ERROR, ERRCODE_DATA_EXCEPTION,
 					"invalid XPath expression");

------- =_aaaaaaaaaa0--