From a34e02bfaf6246b482ec04a3f7c200a1ae6b7df0 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 14 Aug 2012 18:25:55 -0400 Subject: [PATCH] Prevent access to external files/URLs via XML entity references. xml_parse() would attempt to fetch external files or URLs as needed to resolve DTD and entity references in an XML value, thus allowing unprivileged database users to attempt to fetch data with the privileges of the database server. While the external data wouldn't get returned directly to the user, portions of it could be exposed in error messages if the data didn't parse as valid XML; and in any case the mere ability to check existence of a file might be useful to an attacker. The ideal solution to this would still allow fetching of references that are listed in the host system's XML catalogs, so that documents can be validated according to installed DTDs. However, doing that with the available libxml2 APIs appears complex and error-prone, so we're not going to risk it in a security patch that necessarily hasn't gotten wide review. So this patch merely shuts off all access, causing any external fetch to silently expand to an empty string. A future patch may improve this. In HEAD and 9.2, also suppress warnings about undefined entities, which would otherwise occur as a result of not loading referenced DTDs. Previous branches don't show such warnings anyway, due to different error handling arrangements. Credit to Noah Misch for first reporting the problem, and for much work towards a solution, though this simplistic approach was not his preference. Also thanks to Daniel Veillard for consultation. Security: CVE-2012-3489 --- src/backend/utils/adt/xml.c | 28 ++++++++++++++++++++++++++++ src/test/regress/expected/xml.out | 20 ++++++++++++++++++++ src/test/regress/expected/xml_1.out | 14 ++++++++++++++ src/test/regress/sql/xml.sql | 6 ++++++ 4 files changed, 68 insertions(+) diff --git a/src/backend/utils/adt/xml.c b/src/backend/utils/adt/xml.c index a8a4c376dd4..81aac773aa1 100644 --- a/src/backend/utils/adt/xml.c +++ b/src/backend/utils/adt/xml.c @@ -48,6 +48,7 @@ #ifdef USE_LIBXML #include #include +#include #include #include #include @@ -86,6 +87,8 @@ int xmloption; static StringInfo xml_err_buf = NULL; +static xmlParserInputPtr xmlPgEntityLoader(const char *URL, const char *ID, + xmlParserCtxtPtr ctxt); static void xml_errorHandler(void *ctxt, const char *msg,...); static void xml_ereport_by_code(int level, int sqlcode, const char *msg, int errcode); @@ -886,6 +889,9 @@ pg_xml_init(void) /* Now that xml_err_buf exists, safe to call xml_errorHandler */ xmlSetGenericErrorFunc(NULL, xml_errorHandler); + /* set up our entity loader, too */ + xmlSetExternalEntityLoader(xmlPgEntityLoader); + #ifdef USE_LIBXMLCONTEXT /* Set up memory allocation our way, too */ xml_memory_init(); @@ -910,6 +916,9 @@ pg_xml_init(void) * about, anyway. */ xmlSetGenericErrorFunc(NULL, xml_errorHandler); + + /* set up our entity loader, too */ + xmlSetExternalEntityLoader(xmlPgEntityLoader); } } @@ -1322,6 +1331,25 @@ xml_pstrdup(const char *string) #endif /* USE_LIBXMLCONTEXT */ +/* + * xmlPgEntityLoader --- entity loader callback function + * + * Silently prevent any external entity URL from being loaded. We don't want + * to throw an error, so instead make the entity appear to expand to an empty + * string. + * + * We would prefer to allow loading entities that exist in the system's + * global XML catalog; but the available libxml2 APIs make that a complex + * and fragile task. For now, just shut down all external access. + */ +static xmlParserInputPtr +xmlPgEntityLoader(const char *URL, const char *ID, + xmlParserCtxtPtr ctxt) +{ + return xmlNewStringInputStream(ctxt, (const xmlChar *) ""); +} + + /* * xml_ereport --- report an XML-related error * diff --git a/src/test/regress/expected/xml.out b/src/test/regress/expected/xml.out index ecca5896a70..7db5f917bed 100644 --- a/src/test/regress/expected/xml.out +++ b/src/test/regress/expected/xml.out @@ -502,3 +502,23 @@ SELECT xpath('//b', 'one two three etc'); {two,etc} (1 row) +-- External entity references should not leak filesystem information. +SELECT XMLPARSE(DOCUMENT ']>&c;'); + xmlparse +----------------------------------------------------------------- + ]>&c; +(1 row) + +SELECT XMLPARSE(DOCUMENT ']>&c;'); + xmlparse +----------------------------------------------------------------------- + ]>&c; +(1 row) + +-- This might or might not load the requested DTD, but it mustn't throw error. +SELECT XMLPARSE(DOCUMENT ' '); + xmlparse +------------------------------------------------------------------------------------------------------------------------------------------------------ +   +(1 row) + diff --git a/src/test/regress/expected/xml_1.out b/src/test/regress/expected/xml_1.out index d542b0689a9..d654056b08b 100644 --- a/src/test/regress/expected/xml_1.out +++ b/src/test/regress/expected/xml_1.out @@ -456,3 +456,17 @@ LINE 1: SELECT xpath('//b', 'one two three etc'... ^ DETAIL: This functionality requires the server to be built with libxml support. HINT: You need to rebuild PostgreSQL using --with-libxml. +-- External entity references should not leak filesystem information. +SELECT XMLPARSE(DOCUMENT ']>&c;'); +ERROR: unsupported XML feature +DETAIL: This functionality requires the server to be built with libxml support. +HINT: You need to rebuild PostgreSQL using --with-libxml. +SELECT XMLPARSE(DOCUMENT ']>&c;'); +ERROR: unsupported XML feature +DETAIL: This functionality requires the server to be built with libxml support. +HINT: You need to rebuild PostgreSQL using --with-libxml. +-- This might or might not load the requested DTD, but it mustn't throw error. +SELECT XMLPARSE(DOCUMENT ' '); +ERROR: unsupported XML feature +DETAIL: This functionality requires the server to be built with libxml support. +HINT: You need to rebuild PostgreSQL using --with-libxml. diff --git a/src/test/regress/sql/xml.sql b/src/test/regress/sql/xml.sql index 086eedd2700..1e5026b04c8 100644 --- a/src/test/regress/sql/xml.sql +++ b/src/test/regress/sql/xml.sql @@ -163,3 +163,9 @@ SELECT xpath('', ''); SELECT xpath('//text()', 'number one'); SELECT xpath('//loc:piece/@id', 'number one', ARRAY[ARRAY['loc', 'http://127.0.0.1']]); SELECT xpath('//b', 'one two three etc'); + +-- External entity references should not leak filesystem information. +SELECT XMLPARSE(DOCUMENT ']>&c;'); +SELECT XMLPARSE(DOCUMENT ']>&c;'); +-- This might or might not load the requested DTD, but it mustn't throw error. +SELECT XMLPARSE(DOCUMENT ' ');