Re: [OE-core] [hardknott][PATCH 1/3] expat: fix CVE-2022-25313

["Mittal, Anuj" <anuj.mittal@intel.com>]

I had tried building these three fixes but it looks like they are
causing a ptest to fail in libxml-parser-perl for qemuarm64:

https://autobuilder.yocto.io/pub/non-release/20220414-11/testresults/qemuarm64-ptest/libxml-parser-perl.log

Thanks,

Anuj

On Thu, 2022-03-31 at 13:51 +0800, kai wrote:
> From: Kai Kang <kai.kang@windriver.com>
> 
> Backport patch to fix CVE-2022-25313.
> 
> CVE: CVE-2022-25313
> 
> Signed-off-by: Kai Kang <kai.kang@windriver.com>
> ---
>  .../expat/expat/CVE-2022-25313.patch          | 233
> ++++++++++++++++++
>  meta/recipes-core/expat/expat_2.2.10.bb       |   1 +
>  2 files changed, 234 insertions(+)
>  create mode 100644 meta/recipes-core/expat/expat/CVE-2022-
> 25313.patch
> 
> diff --git a/meta/recipes-core/expat/expat/CVE-2022-25313.patch
> b/meta/recipes-core/expat/expat/CVE-2022-25313.patch
> new file mode 100644
> index 0000000000..569324303e
> --- /dev/null
> +++ b/meta/recipes-core/expat/expat/CVE-2022-25313.patch
> @@ -0,0 +1,233 @@
> +CVE: CVE-2022-25313
> +Upstream-Status: Backport
> [https://github.com/libexpat/libexpat/commit/9b4ce651]
> +
> +Ref:
> +* https://github.com/libexpat/libexpat/pull/558
> +
> +Signed-off-by: Kai Kang <kai.kang@windriver.com>
> +
> +From 9b4ce651b26557f16103c3a366c91934ecd439ab Mon Sep 17 00:00:00
> 2001
> +From: Samanta Navarro <ferivoz@riseup.net>
> +Date: Tue, 15 Feb 2022 11:54:29 +0000
> +Subject: [PATCH] Prevent stack exhaustion in build_model
> +
> +It is possible to trigger stack exhaustion in build_model function
> if
> +depth of nested children in DTD element is large enough. This
> happens
> +because build_node is a recursively called function within
> build_model.
> +
> +The code has been adjusted to run iteratively. It uses the already
> +allocated heap space as temporary stack (growing from top to
> bottom).
> +
> +Output is identical to recursive version. No new fields in data
> +structures were added, i.e. it keeps full API and ABI compatibility.
> +Instead the numchildren variable is used to temporarily keep the
> +index of items (uint vs int).
> +
> +Documentation and readability improvements kindly added by
> Sebastian.
> +
> +Proof of Concept:
> +
> +1. Compile poc binary which parses XML file line by line
> +
> +```
> +cat > poc.c << EOF
> + #include <err.h>
> + #include <expat.h>
> + #include <stdio.h>
> +
> + XML_Parser parser;
> +
> + static void XMLCALL
> + dummy_element_decl_handler(void *userData, const XML_Char *name,
> +                            XML_Content *model) {
> +   XML_FreeContentModel(parser, model);
> + }
> +
> + int main(int argc, char *argv[]) {
> +   FILE *fp;
> +   char *p = NULL;
> +   size_t s = 0;
> +   ssize_t l;
> +   if (argc != 2)
> +     errx(1, "usage: poc poc.xml");
> +   if ((parser = XML_ParserCreate(NULL)) == NULL)
> +     errx(1, "XML_ParserCreate");
> +   XML_SetElementDeclHandler(parser, dummy_element_decl_handler);
> +   if ((fp = fopen(argv[1], "r")) == NULL)
> +     err(1, "fopen");
> +   while ((l = getline(&p, &s, fp)) > 0)
> +     if (XML_Parse(parser, p, (int)l, XML_FALSE) != XML_STATUS_OK)
> +       errx(1, "XML_Parse");
> +   XML_ParserFree(parser);
> +   free(p);
> +   fclose(fp);
> +   return 0;
> + }
> +EOF
> +cc -std=c11 -D_POSIX_C_SOURCE=200809L -lexpat -o poc poc.c
> +```
> +
> +2. Create XML file with a lot of nested groups in DTD element
> +
> +```
> +cat > poc.xml.zst.b64 << EOF
> +KLUv/aQkACAAPAEA+DwhRE9DVFlQRSB1d3UgWwo8IUVMRU1FTlQgdXd1CigBAHv/58AJ
> AgAQKAIA
> +ECgCABAoAgAQKAIAECgCABAoAgAQKHwAAChvd28KKQIA2/8gV24XBAIAECkCABApAgAQ
> KQIAECkC
> +ABApAgAQKQIAEClVAAAgPl0+CgEA4A4I2VwwnQ==
> +EOF
> +base64 -d poc.xml.zst.b64 | zstd -d > poc.xml
> +```
> +
> +3. Run Proof of Concept
> +
> +```
> +./poc poc.xml
> +```
> +
> +Co-authored-by: Sebastian Pipping <sebastian@pipping.org>
> +---
> + expat/lib/xmlparse.c | 116 +++++++++++++++++++++++++++++-----------
> ---
> + 1 file changed, 79 insertions(+), 37 deletions(-)
> +
> +diff --git a/expat/lib/xmlparse.c b/expat/lib/xmlparse.c
> +index 4b43e613..594cf12c 100644
> +--- a/lib/xmlparse.c
> ++++ b/lib/xmlparse.c
> +@@ -7317,44 +7317,15 @@ nextScaffoldPart(XML_Parser parser) {
> +   return next;
> + }
> + 
> +-static void
> +-build_node(XML_Parser parser, int src_node, XML_Content *dest,
> +-           XML_Content **contpos, XML_Char **strpos) {
> +-  DTD *const dtd = parser->m_dtd; /* save one level of indirection
> */
> +-  dest->type = dtd->scaffold[src_node].type;
> +-  dest->quant = dtd->scaffold[src_node].quant;
> +-  if (dest->type == XML_CTYPE_NAME) {
> +-    const XML_Char *src;
> +-    dest->name = *strpos;
> +-    src = dtd->scaffold[src_node].name;
> +-    for (;;) {
> +-      *(*strpos)++ = *src;
> +-      if (! *src)
> +-        break;
> +-      src++;
> +-    }
> +-    dest->numchildren = 0;
> +-    dest->children = NULL;
> +-  } else {
> +-    unsigned int i;
> +-    int cn;
> +-    dest->numchildren = dtd->scaffold[src_node].childcnt;
> +-    dest->children = *contpos;
> +-    *contpos += dest->numchildren;
> +-    for (i = 0, cn = dtd->scaffold[src_node].firstchild; i < dest-
> >numchildren;
> +-         i++, cn = dtd->scaffold[cn].nextsib) {
> +-      build_node(parser, cn, &(dest->children[i]), contpos,
> strpos);
> +-    }
> +-    dest->name = NULL;
> +-  }
> +-}
> +-
> + static XML_Content *
> + build_model(XML_Parser parser) {
> ++  /* Function build_model transforms the existing parser->m_dtd-
> >scaffold
> ++   * array of CONTENT_SCAFFOLD tree nodes into a new array of
> ++   * XML_Content tree nodes followed by a gapless list of zero-
> terminated
> ++   * strings. */
> +   DTD *const dtd = parser->m_dtd; /* save one level of indirection
> */
> +   XML_Content *ret;
> +-  XML_Content *cpos;
> +-  XML_Char *str;
> ++  XML_Char *str; /* the current string writing location */
> + 
> +   /* Detect and prevent integer overflow.
> +    * The preprocessor guard addresses the "always false" warning
> +@@ -7380,10 +7351,81 @@ build_model(XML_Parser parser) {
> +   if (! ret)
> +     return NULL;
> + 
> +-  str = (XML_Char *)(&ret[dtd->scaffCount]);
> +-  cpos = &ret[1];
> ++  /* What follows is an iterative implementation (of what was
> previously done
> ++   * recursively in a dedicated function called "build_node".  The
> old recursive
> ++   * build_node could be forced into stack exhaustion from input as
> small as a
> ++   * few megabyte, and so that was a security issue.  Hence, a
> function call
> ++   * stack is avoided now by resolving recursion.)
> ++   *
> ++   * 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.
> ++   */
> ++  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 */
> ++  str = (XML_Char *)&ret[dtd->scaffCount];
> ++
> ++  /* Push source tree root node index onto the stack */
> ++  (--stackTop)->numchildren = 0;
> ++
> ++  for (; dest < destLimit; dest++) {
> ++    /* Pop source tree node index off the stack */
> ++    const int src_node = (int)(stackTop++)->numchildren;
> ++
> ++    /* Convert item */
> ++    dest->type = dtd->scaffold[src_node].type;
> ++    dest->quant = dtd->scaffold[src_node].quant;
> ++    if (dest->type == XML_CTYPE_NAME) {
> ++      const XML_Char *src;
> ++      dest->name = str;
> ++      src = dtd->scaffold[src_node].name;
> ++      for (;;) {
> ++        *str++ = *src;
> ++        if (! *src)
> ++          break;
> ++        src++;
> ++      }
> ++      dest->numchildren = 0;
> ++      dest->children = NULL;
> ++    } else {
> ++      unsigned int i;
> ++      int cn;
> ++      dest->name = NULL;
> ++      dest->numchildren = dtd->scaffold[src_node].childcnt;
> ++      dest->children = &dest[1];
> ++
> ++      /* 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;
> ++      for (i = 0, cn = dtd->scaffold[src_node].firstchild;
> ++           i < dest->numchildren; i++, cn = dtd-
> >scaffold[cn].nextsib) {
> ++        (stackTop + i)->numchildren = (unsigned int)cn;
> ++      }
> ++    }
> ++  }
> + 
> +-  build_node(parser, 0, ret, &cpos, &str);
> +   return ret;
> + }
> + 
> +-- 
> +2.33.0
> +
> diff --git a/meta/recipes-core/expat/expat_2.2.10.bb b/meta/recipes-
> core/expat/expat_2.2.10.bb
> index f99fa7edb6..7454718dca 100644
> --- a/meta/recipes-core/expat/expat_2.2.10.bb
> +++ b/meta/recipes-core/expat/expat_2.2.10.bb
> @@ -20,6 +20,7 @@ SRC_URI =
> "https://github.com/libexpat/libexpat/releases/download/R_${VERSION_T
> A
>             file://CVE-2022-25235.patch \
>             file://CVE-2022-25236-1.patch \
>             file://CVE-2022-25236-2.patch \
> +           file://CVE-2022-25313.patch \
>             "
>  
>  UPSTREAM_CHECK_URI =
> "https://github.com/libexpat/libexpat/releases/"
> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#163801):
> https://lists.openembedded.org/g/openembedded-core/message/163801
> Mute This Topic: https://lists.openembedded.org/mt/90149640/3616702
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe:
> https://lists.openembedded.org/g/openembedded-core/unsub [
> anuj.mittal@intel.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>