Message ID | 20220331055139.1975-1-kai.kang@windriver.com |
---|---|
State | New |
Headers | show |
Series | [hardknott,1/3] expat: fix CVE-2022-25313 | expand |
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] > -=-=-=-=-=-=-=-=-=-=-=- >
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/58AJAgAQKAIA +ECgCABAoAgAQKAIAECgCABAoAgAQKHwAAChvd28KKQIA2/8gV24XBAIAECkCABApAgAQKQIAECkC +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_TA 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/"