From 91c9c56dcca01bfe3f9dae74fb75dcf792ebe58b Mon Sep 17 00:00:00 2001 From: Nick Wellnhofer Date: Wed, 31 Aug 2022 13:35:23 +0200 Subject: [PATCH] Make generate-id() deterministic Rework the generate-id() function to return deterministic values. We use a simple incrementing counter and store ids in the 'psvi' member of nodes which was freed up by previous commits. The presence of an id is indicated by a new "source node" flag. This fixes long-standing problems with reproducible builds, see https://bugzilla.gnome.org/show_bug.cgi?id=751621 This also hardens security, as the old implementation leaked the difference between a heap and a global pointer, see https://bugs.chromium.org/p/chromium/issues/detail?id=1356211 The old implementation could also generate the same id for dynamically created nodes which happened to reuse the same memory. Ids for namespace nodes were completely broken. They now use the id of the parent element together with the hex-encoded namespace prefix. CVE: CVE-2023-40403 Upstream-Status: Backport [https://gitlab.gnome.org/GNOME/libxslt/-/commit/82f6cbf8ca61b1f9e00dc04aa3b15d563e7bbc6d] Signed-off-by: Hitendra Prajapati --- libxslt/functions.c | 107 +++++++++++++++++++++++++----- libxslt/xsltInternals.h | 1 + libxslt/xsltutils.h | 1 + tests/REC/test-12.4-1.out | 11 +++ tests/REC/test-12.4-1.xml | 6 ++ tests/REC/test-12.4-1.xsl | 38 +++++++++++ tests/exslt/common/dynamic-id.out | 13 ++++ tests/exslt/common/dynamic-id.xml | 1 + tests/exslt/common/dynamic-id.xsl | 29 ++++++++ 9 files changed, 191 insertions(+), 16 deletions(-) create mode 100644 tests/REC/test-12.4-1.out create mode 100644 tests/REC/test-12.4-1.xml create mode 100644 tests/REC/test-12.4-1.xsl create mode 100644 tests/exslt/common/dynamic-id.out create mode 100644 tests/exslt/common/dynamic-id.xml create mode 100644 tests/exslt/common/dynamic-id.xsl diff --git a/libxslt/functions.c b/libxslt/functions.c index 7887dda..da25c24 100644 --- a/libxslt/functions.c +++ b/libxslt/functions.c @@ -693,11 +693,16 @@ xsltFormatNumberFunction(xmlXPathParserContextPtr ctxt, int nargs) */ void xsltGenerateIdFunction(xmlXPathParserContextPtr ctxt, int nargs){ - static char base_address; + xsltTransformContextPtr tctxt; xmlNodePtr cur = NULL; xmlXPathObjectPtr obj = NULL; - long val; - xmlChar str[30]; + char *str; + const xmlChar *nsPrefix = NULL; + void **psviPtr; + unsigned long id; + size_t size, nsPrefixSize; + + tctxt = xsltXPathGetTransformContext(ctxt); if (nargs == 0) { cur = ctxt->context->node; @@ -707,16 +712,15 @@ xsltGenerateIdFunction(xmlXPathParserContextPtr ctxt, int nargs){ if ((ctxt->value == NULL) || (ctxt->value->type != XPATH_NODESET)) { ctxt->error = XPATH_INVALID_TYPE; - xsltTransformError(xsltXPathGetTransformContext(ctxt), NULL, NULL, + xsltTransformError(tctxt, NULL, NULL, "generate-id() : invalid arg expecting a node-set\n"); - return; + goto out; } obj = valuePop(ctxt); nodelist = obj->nodesetval; if ((nodelist == NULL) || (nodelist->nodeNr <= 0)) { - xmlXPathFreeObject(obj); valuePush(ctxt, xmlXPathNewCString("")); - return; + goto out; } cur = nodelist->nodeTab[0]; for (i = 1;i < nodelist->nodeNr;i++) { @@ -725,22 +729,93 @@ xsltGenerateIdFunction(xmlXPathParserContextPtr ctxt, int nargs){ cur = nodelist->nodeTab[i]; } } else { - xsltTransformError(xsltXPathGetTransformContext(ctxt), NULL, NULL, + xsltTransformError(tctxt, NULL, NULL, "generate-id() : invalid number of args %d\n", nargs); ctxt->error = XPATH_INVALID_ARITY; - return; + goto out; + } + + size = 30; /* for "id%lu" */ + + if (cur->type == XML_NAMESPACE_DECL) { + xmlNsPtr ns = (xmlNsPtr) cur; + + nsPrefix = ns->prefix; + if (nsPrefix == NULL) + nsPrefix = BAD_CAST ""; + nsPrefixSize = xmlStrlen(nsPrefix); + /* For "ns" and hex-encoded string */ + size += nsPrefixSize * 2 + 2; + + /* Parent is stored in 'next'. */ + cur = (xmlNodePtr) ns->next; + } + + psviPtr = xsltGetPSVIPtr(cur); + if (psviPtr == NULL) { + xsltTransformError(tctxt, NULL, NULL, + "generate-id(): invalid node type %d\n", cur->type); + ctxt->error = XPATH_INVALID_TYPE; + goto out; } - if (obj) - xmlXPathFreeObject(obj); + if (xsltGetSourceNodeFlags(cur) & XSLT_SOURCE_NODE_HAS_ID) { + id = (unsigned long) *psviPtr; + } else { + if (cur->type == XML_TEXT_NODE && cur->line == USHRT_MAX) { + /* Text nodes store big line numbers in psvi. */ + cur->line = 0; + } else if (*psviPtr != NULL) { + xsltTransformError(tctxt, NULL, NULL, + "generate-id(): psvi already set\n"); + ctxt->error = XPATH_MEMORY_ERROR; + goto out; + } + + if (tctxt->currentId == ULONG_MAX) { + xsltTransformError(tctxt, NULL, NULL, + "generate-id(): id overflow\n"); + ctxt->error = XPATH_MEMORY_ERROR; + goto out; + } + + id = ++tctxt->currentId; + *psviPtr = (void *) id; + xsltSetSourceNodeFlags(tctxt, cur, XSLT_SOURCE_NODE_HAS_ID); + } - val = (long)((char *)cur - (char *)&base_address); - if (val >= 0) { - snprintf((char *)str, sizeof(str), "idp%ld", val); + str = xmlMalloc(size); + if (str == NULL) { + xsltTransformError(tctxt, NULL, NULL, + "generate-id(): out of memory\n"); + ctxt->error = XPATH_MEMORY_ERROR; + goto out; + } + if (nsPrefix == NULL) { + snprintf(str, size, "id%lu", id); } else { - snprintf((char *)str, sizeof(str), "idm%ld", -val); + size_t i, j; + + snprintf(str, size, "id%luns", id); + + /* + * Only ASCII alphanumerics are allowed, so we hex-encode the prefix. + */ + j = strlen(str); + for (i = 0; i < nsPrefixSize; i++) { + int v; + + v = nsPrefix[i] >> 4; + str[j++] = v < 10 ? '0' + v : 'A' + (v - 10); + v = nsPrefix[i] & 15; + str[j++] = v < 10 ? '0' + v : 'A' + (v - 10); + } + str[j] = '\0'; } - valuePush(ctxt, xmlXPathNewString(str)); + valuePush(ctxt, xmlXPathWrapString(BAD_CAST str)); + +out: + xmlXPathFreeObject(obj); } /** diff --git a/libxslt/xsltInternals.h b/libxslt/xsltInternals.h index 74a2b64..2fd1f68 100644 --- a/libxslt/xsltInternals.h +++ b/libxslt/xsltInternals.h @@ -1787,6 +1787,7 @@ struct _xsltTransformContext { unsigned long opLimit; unsigned long opCount; int sourceDocDirty; + unsigned long currentId; /* For generate-id() */ }; /** diff --git a/libxslt/xsltutils.h b/libxslt/xsltutils.h index dcfd139..6c14ecf 100644 --- a/libxslt/xsltutils.h +++ b/libxslt/xsltutils.h @@ -250,6 +250,7 @@ XSLTPUBFUN xmlXPathCompExprPtr XSLTCALL #ifdef IN_LIBXSLT #define XSLT_SOURCE_NODE_MASK 15 #define XSLT_SOURCE_NODE_HAS_KEY 1 +#define XSLT_SOURCE_NODE_HAS_ID 2 int xsltGetSourceNodeFlags(xmlNodePtr node); int diff --git a/tests/REC/test-12.4-1.out b/tests/REC/test-12.4-1.out new file mode 100644 index 0000000..237a9f2 --- /dev/null +++ b/tests/REC/test-12.4-1.out @@ -0,0 +1,11 @@ + + + id1 + id2 + id3 + id2ns + id2nsC3A4C3B6C3BC + id4 + id5 + id6 + diff --git a/tests/REC/test-12.4-1.xml b/tests/REC/test-12.4-1.xml new file mode 100644 index 0000000..84484f6 --- /dev/null +++ b/tests/REC/test-12.4-1.xml @@ -0,0 +1,6 @@ + + + text + + + diff --git a/tests/REC/test-12.4-1.xsl b/tests/REC/test-12.4-1.xsl new file mode 100644 index 0000000..5cf5dd3 --- /dev/null +++ b/tests/REC/test-12.4-1.xsl @@ -0,0 +1,38 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/exslt/common/dynamic-id.out b/tests/exslt/common/dynamic-id.out new file mode 100644 index 0000000..1b7b7ba --- /dev/null +++ b/tests/exslt/common/dynamic-id.out @@ -0,0 +1,13 @@ + + + id1 + id2 + id3 + id4 + id5 + id6 + id7 + id8 + id9 + id10 + diff --git a/tests/exslt/common/dynamic-id.xml b/tests/exslt/common/dynamic-id.xml new file mode 100644 index 0000000..69d62f2 --- /dev/null +++ b/tests/exslt/common/dynamic-id.xml @@ -0,0 +1 @@ + diff --git a/tests/exslt/common/dynamic-id.xsl b/tests/exslt/common/dynamic-id.xsl new file mode 100644 index 0000000..8478f6a --- /dev/null +++ b/tests/exslt/common/dynamic-id.xsl @@ -0,0 +1,29 @@ + + + + + + + + + + + + + + + + + + + + + + + + + +