File expat-CVE-2022-25313-fix-regression.patch of Package expat

Overview Repositories Revisions Requests Users Attributes Meta

File expat-CVE-2022-25313-fix-regression.patch of Package expat

From b12f34fe32821a69dc12ff9a021daca0856de238 Mon Sep 17 00:00:00 2001
From: Samanta Navarro <ferivoz@riseup.net>
Date: Sat, 19 Feb 2022 23:59:25 +0000
Subject: [PATCH 1/3] Fix build_model regression.

The iterative approach in build_model failed to fill children arrays
correctly. A preorder traversal is not required and turned out to be the
culprit. Use an easier algorithm:

Add nodes from scaffold tree starting at index 0 (root) to the target
array whenever children are encountered. This ensures that children
are adjacent to each other. This complies with the recursive version.

Store only the scaffold index in numchildren field to prevent a direct
processing of these children, which would require a recursive solution.
This allows the algorithm to iterate through the target array from start
to end without jumping back and forth, converting on the fly.

Co-authored-by: Sebastian Pipping <sebastian@pipping.org>
---
 expat/lib/xmlparse.c | 79 ++++++++++++++++++++++++++------------------
 1 file changed, 47 insertions(+), 32 deletions(-)

Index: expat-2.4.4/lib/xmlparse.c
===================================================================
--- expat-2.4.4.orig/lib/xmlparse.c
+++ expat-2.4.4/lib/xmlparse.c
@@ -7359,39 +7359,58 @@ build_model(XML_Parser parser) {
    *
    * The iterative approach works as follows:
    *
-   * - We use space in the target array for building a temporary stack structure
-   *   while that space is still unused.
-   *   The stack grows from the array's end downwards and the "actual data"
-   *   grows from the start upwards, sequentially.
-   *   (Because stack grows downwards, pushing onto the stack is a decrement
-   *   while popping off the stack is an increment.)
-   *
-   * - A stack element appears as a regular XML_Content node on the outside,
-   *   but only uses a single field -- numchildren -- to store the source
-   *   tree node array index.  These are the breadcrumbs leading the way back
-   *   during pre-order (node first) depth-first traversal.
-   *
-   * - The reason we know the stack will never grow into (or overlap with)
-   *   the area with data of value at the start of the array is because
-   *   the overall number of elements to process matches the size of the array,
-   *   and the sum of fully processed nodes and yet-to-be processed nodes
-   *   on the stack, cannot be more than the total number of nodes.
-   *   It is possible for the top of the stack and the about-to-write node
-   *   to meet, but that is safe because we get the source index out
-   *   before doing any writes on that node.
+   * - We have two writing pointers, both walking up the result array; one does
+   *   the work, the other creates "jobs" for its colleague to do, and leads
+   *   the way:
+   *
+   *   - The faster one, pointer jobDest, always leads and writes "what job
+   *     to do" by the other, once they reach that place in the
+   *     array: leader "jobDest" stores the source node array index (relative
+   *     to array dtd->scaffold) in field "numchildren".
+   *
+   *   - The slower one, pointer dest, looks at the value stored in the
+   *     "numchildren" field (which actually holds a source node array index
+   *     at that time) and puts the real data from dtd->scaffold in.
+   *
+   * - Before the loop starts, jobDest writes source array index 0
+   *   (where the root node is located) so that dest will have something to do
+   *   when it starts operation.
+   *
+   * - Whenever nodes with children are encountered, jobDest appends
+   *   them as new jobs, in order.  As a result, tree node siblings are
+   *   adjacent in the resulting array, for example:
+   *
+   *     [0] root, has two children
+   *       [1] first child of 0, has three children
+   *         [3] first child of 1, does not have children
+   *         [4] second child of 1, does not have children
+   *         [5] third child of 1, does not have children
+   *       [2] second child of 0, does not have children
+   *
+   *   Or (the same data) presented in flat array view:
+   *
+   *     [0] root, has two children
+   *
+   *     [1] first child of 0, has three children
+   *     [2] second child of 0, does not have children
+   *
+   *     [3] first child of 1, does not have children
+   *     [4] second child of 1, does not have children
+   *     [5] third child of 1, does not have children
+   *
+   * - The algorithm repeats until all target array indices have been processed.
    */
   XML_Content *dest = ret; /* tree node writing location, moves upwards */
   XML_Content *const destLimit = &ret[dtd->scaffCount];
-  XML_Content *const stackBottom = &ret[dtd->scaffCount];
-  XML_Content *stackTop = stackBottom; /* i.e. stack is initially empty */
+  XML_Content *jobDest = ret; /* next free writing location in target array */
   str = (XML_Char *)&ret[dtd->scaffCount];
 
-  /* Push source tree root node index onto the stack */
-  (--stackTop)->numchildren = 0;
+  /* Add the starting job, the root node (index 0) of the source tree  */
+  (jobDest++)->numchildren = 0;
 
   for (; dest < destLimit; dest++) {
-    /* Pop source tree node index off the stack */
-    const int src_node = (int)(stackTop++)->numchildren;
+    /* Retrieve source tree array index from job storage */
+    const int src_node = (int)dest->numchildren;
 
     /* Convert item */
     dest->type = dtd->scaffold[src_node].type;
@@ -7413,16 +7432,12 @@ build_model(XML_Parser parser) {
       int cn;
       dest->name = NULL;
       dest->numchildren = dtd->scaffold[src_node].childcnt;
-      dest->children = &dest[1];
+      dest->children = jobDest;
 
-      /* Push children to the stack
-       * in a way where the first child ends up at the top of the
-       * (downwards growing) stack, in order to be processed first. */
-      stackTop -= dest->numchildren;
+      /* Append scaffold indices of children to array */
       for (i = 0, cn = dtd->scaffold[src_node].firstchild;
-           i < dest->numchildren; i++, cn = dtd->scaffold[cn].nextsib) {
-        (stackTop + i)->numchildren = (unsigned int)cn;
-      }
+           i < dest->numchildren; i++, cn = dtd->scaffold[cn].nextsib)
+        (jobDest++)->numchildren = (unsigned int)cn;
     }
   }
 
Index: expat-2.4.4/tests/runtests.c
===================================================================
--- expat-2.4.4.orig/tests/runtests.c
+++ expat-2.4.4/tests/runtests.c
@@ -2664,6 +2664,82 @@ START_TEST(test_dtd_elements) {
 }
 END_TEST
 
+static void XMLCALL
+element_decl_check_model(void *userData, const XML_Char *name,
+                         XML_Content *model) {
+  UNUSED_P(userData);
+  uint32_t errorFlags = 0;
+
+  /* Expected model array structure is this:
+   * [0] (type 6, quant 0)
+   *   [1] (type 5, quant 0)
+   *     [3] (type 4, quant 0, name "bar")
+   *     [4] (type 4, quant 0, name "foo")
+   *     [5] (type 4, quant 3, name "xyz")
+   *   [2] (type 4, quant 2, name "zebra")
+   */
+  errorFlags |= ((xcstrcmp(name, XCS("junk")) == 0) ? 0 : (1u << 0));
+  errorFlags |= ((model != NULL) ? 0 : (1u << 1));
+
+  errorFlags |= ((model[0].type == XML_CTYPE_SEQ) ? 0 : (1u << 2));
+  errorFlags |= ((model[0].quant == XML_CQUANT_NONE) ? 0 : (1u << 3));
+  errorFlags |= ((model[0].numchildren == 2) ? 0 : (1u << 4));
+  errorFlags |= ((model[0].children == &model[1]) ? 0 : (1u << 5));
+  errorFlags |= ((model[0].name == NULL) ? 0 : (1u << 6));
+
+  errorFlags |= ((model[1].type == XML_CTYPE_CHOICE) ? 0 : (1u << 7));
+  errorFlags |= ((model[1].quant == XML_CQUANT_NONE) ? 0 : (1u << 8));
+  errorFlags |= ((model[1].numchildren == 3) ? 0 : (1u << 9));
+  errorFlags |= ((model[1].children == &model[3]) ? 0 : (1u << 10));
+  errorFlags |= ((model[1].name == NULL) ? 0 : (1u << 11));
+
+  errorFlags |= ((model[2].type == XML_CTYPE_NAME) ? 0 : (1u << 12));
+  errorFlags |= ((model[2].quant == XML_CQUANT_REP) ? 0 : (1u << 13));
+  errorFlags |= ((model[2].numchildren == 0) ? 0 : (1u << 14));
+  errorFlags |= ((model[2].children == NULL) ? 0 : (1u << 15));
+  errorFlags |= ((xcstrcmp(model[2].name, XCS("zebra")) == 0) ? 0 : (1u << 16));
+
+  errorFlags |= ((model[3].type == XML_CTYPE_NAME) ? 0 : (1u << 17));
+  errorFlags |= ((model[3].quant == XML_CQUANT_NONE) ? 0 : (1u << 18));
+  errorFlags |= ((model[3].numchildren == 0) ? 0 : (1u << 19));
+  errorFlags |= ((model[3].children == NULL) ? 0 : (1u << 20));
+  errorFlags |= ((xcstrcmp(model[3].name, XCS("bar")) == 0) ? 0 : (1u << 21));
+
+  errorFlags |= ((model[4].type == XML_CTYPE_NAME) ? 0 : (1u << 22));
+  errorFlags |= ((model[4].quant == XML_CQUANT_NONE) ? 0 : (1u << 23));
+  errorFlags |= ((model[4].numchildren == 0) ? 0 : (1u << 24));
+  errorFlags |= ((model[4].children == NULL) ? 0 : (1u << 25));
+  errorFlags |= ((xcstrcmp(model[4].name, XCS("foo")) == 0) ? 0 : (1u << 26));
+
+  errorFlags |= ((model[5].type == XML_CTYPE_NAME) ? 0 : (1u << 27));
+  errorFlags |= ((model[5].quant == XML_CQUANT_PLUS) ? 0 : (1u << 28));
+  errorFlags |= ((model[5].numchildren == 0) ? 0 : (1u << 29));
+  errorFlags |= ((model[5].children == NULL) ? 0 : (1u << 30));
+  errorFlags |= ((xcstrcmp(model[5].name, XCS("xyz")) == 0) ? 0 : (1u << 31));
+
+  XML_SetUserData(g_parser, (void *)(uintptr_t)errorFlags);
+  XML_FreeContentModel(g_parser, model);
+}
+
+START_TEST(test_dtd_elements_nesting) {
+  // Payload inspired by a test in Perl's XML::Parser
+  const char *text = "<!DOCTYPE foo [\n"
+                     "<!ELEMENT junk ((bar|foo|xyz+), zebra*)>\n"
+                     "]>\n"
+                     "<foo/>";
+
+  XML_SetUserData(g_parser, (void *)(uintptr_t)-1);
+
+  XML_SetElementDeclHandler(g_parser, element_decl_check_model);
+  if (XML_Parse(g_parser, text, (int)strlen(text), XML_TRUE)
+      == XML_STATUS_ERROR)
+    xml_failure(g_parser);
+
+  if ((uint32_t)(uintptr_t)XML_GetUserData(g_parser) != 0)
+    fail("Element declaration model regression detected");
+}
+END_TEST
+
 /* Test foreign DTD handling */
 START_TEST(test_set_foreign_dtd) {
   const char *text1 = "<?xml version='1.0' encoding='us-ascii'?>\n";
@@ -11727,6 +11803,7 @@ make_suite(void) {
   tcase_add_test(tc_basic, test_memory_allocation);
   tcase_add_test(tc_basic, test_default_current);
   tcase_add_test(tc_basic, test_dtd_elements);
+  tcase_add_test(tc_basic, test_dtd_elements_nesting);
   tcase_add_test__ifdef_xml_dtd(tc_basic, test_set_foreign_dtd);
   tcase_add_test__ifdef_xml_dtd(tc_basic, test_foreign_dtd_not_standalone);
   tcase_add_test__ifdef_xml_dtd(tc_basic, test_invalid_foreign_dtd);

Places

File expat-CVE-2022-25313-fix-regression.patch of Package expat

Places