From 25716e691ede8319ea8ed183f9c90a7ef6a6c8b9 Mon Sep 17 00:00:00 2001 From: Edouard Schweisguth Date: Wed, 3 Jun 2026 08:47:03 +0000 Subject: [PATCH] OVAL/XCCDF/DS/source: fix memory-corruption bugs on malformed input - OVAL oval_set.c: a mixing a nested (aggregate) with object_reference/filter children (collective) wrote into the wrong union member, so oval_setobject_free() later freed a model-owned object (use-after-free / double free). Reject children that don't match the established set type. - XCCDF resolve.c: entries were resolved through xccdf_resolve_textlist(), which reads each item as oscap_text -- but a xccdf_warning is smaller, so this read out of bounds (heap-buffer-overflow). Added a dedicated xccdf_resolve_warninglist() operating on warning->text. - DS sds.c: component_id aliased the xlink_href buffer that was freed too early (use-after-free); keep it alive until last use. Also cap catalog component-ref recursion (cyclic catalog -> OOM); this recursion-DoS fix is bundled here because it is interleaved with the UAF fix in the same function. Also a NULL id guard in lookup_component_in_collection. - source/schematron.c: OSCAP_SCHEMATRON_TABLE lacked the {0,NULL,NULL} sentinel its lookup loop relies on -> global-buffer-overflow; added it. Plus NULL guards for missing href/uri, empty , and NULL XPath eval. --- src/DS/sds.c | 34 +++++++++++++++++++++++++++++++--- src/OVAL/oval_set.c | 20 ++++++++++++++++++-- src/XCCDF/resolve.c | 33 ++++++++++++++++++++++++++++----- src/source/schematron.c | 23 ++++++++++++++++++++++- 4 files changed, 99 insertions(+), 11 deletions(-) diff --git a/src/DS/sds.c b/src/DS/sds.c index 15f5079c39..2da39c0f47 100644 --- a/src/DS/sds.c +++ b/src/DS/sds.c @@ -149,6 +149,8 @@ xmlNodePtr lookup_component_in_collection(xmlNodePtr root, const char *component continue; char* candidate_id = (char*)xmlGetProp(candidate, BAD_CAST "id"); + if (candidate_id == NULL) + continue; if (strcmp(candidate_id, component_id) == 0) { component = candidate; @@ -489,8 +491,18 @@ static int ds_sds_dump_component_by_href(struct ds_sds_session *session, char* x return 0; } -int ds_sds_dump_component_ref_as(const xmlNodePtr component_ref, struct ds_sds_session *session, const char* sub_dir, const char* relative_filepath) +// Bound on the catalog component-ref recursion depth. Catalog references nest +// only a level or two in real content; this guard stops a cyclic catalog +// (component A's catalog references B whose catalog references A) from recursing +// until the process runs out of memory. +#define DS_SDS_MAX_COMPONENT_REF_DEPTH 30 + +static int _ds_sds_dump_component_ref_as(const xmlNodePtr component_ref, struct ds_sds_session *session, const char* sub_dir, const char* relative_filepath, int depth) { + if (depth > DS_SDS_MAX_COMPONENT_REF_DEPTH) { + oscap_seterr(OSCAP_EFAMILY_XML, "Catalog component-ref nesting too deep or cyclic; aborting."); + return -1; + } char* cref_id = (char*)xmlGetProp(component_ref, BAD_CAST "id"); if (!cref_id) { @@ -512,15 +524,21 @@ int ds_sds_dump_component_ref_as(const xmlNodePtr component_ref, struct ds_sds_s free(xlink_href_copy); } - xmlFree(xlink_href); xmlFree(cref_id); + // NB: component_id may point into the xlink_href buffer (see + // ds_sds_dump_component_by_href), so xlink_href must stay allocated until + // the last use of component_id below, hence it is freed on every exit path + // instead of here. + if (ret == -2) { // A remote component was not dumped // It should be ok to continue without it + xmlFree(xlink_href); free(target_filename_dirname); return 0; } else if (ret != 0) { + xmlFree(xlink_href); free(target_filename_dirname); return -1; } @@ -543,6 +561,7 @@ int ds_sds_dump_component_ref_as(const xmlNodePtr component_ref, struct ds_sds_s if (!name) { oscap_seterr(OSCAP_EFAMILY_XML, "No 'name' attribute for a component referenced in the catalog of component '%s'.", component_id); + xmlFree(xlink_href); free(target_filename_dirname); return -1; } @@ -554,6 +573,7 @@ int ds_sds_dump_component_ref_as(const xmlNodePtr component_ref, struct ds_sds_s oscap_seterr(OSCAP_EFAMILY_XML, "No or invalid 'uri' attribute for a component referenced in the catalog of component '%s'.", component_id); xmlFree(str_uri); xmlFree(name); + xmlFree(xlink_href); free(target_filename_dirname); return -1; } @@ -567,14 +587,16 @@ int ds_sds_dump_component_ref_as(const xmlNodePtr component_ref, struct ds_sds_s oscap_seterr(OSCAP_EFAMILY_XML, "component-ref with given id '%s' wasn't found in the document! We are looking for it because it's in the catalog of component '%s'.", str_uri + 1 * sizeof(char), component_id); xmlFree(str_uri); xmlFree(name); + xmlFree(xlink_href); free(target_filename_dirname); return -1; } xmlFree(str_uri); - if (ds_sds_dump_component_ref_as(cat_component_ref, session, target_filename_dirname, name) != 0) + if (_ds_sds_dump_component_ref_as(cat_component_ref, session, target_filename_dirname, name, depth + 1) != 0) { xmlFree(name); + xmlFree(xlink_href); free(target_filename_dirname); return -1; // no need to call oscap_seterr here, it's already set } @@ -583,12 +605,18 @@ int ds_sds_dump_component_ref_as(const xmlNodePtr component_ref, struct ds_sds_s } } + xmlFree(xlink_href); free(target_filename_dirname); return 0; } +int ds_sds_dump_component_ref_as(const xmlNodePtr component_ref, struct ds_sds_session *session, const char* sub_dir, const char* relative_filepath) +{ + return _ds_sds_dump_component_ref_as(component_ref, session, sub_dir, relative_filepath, 0); +} + int ds_sds_dump_component_ref(const xmlNodePtr component_ref, struct ds_sds_session *session) { char* cref_id = (char*)xmlGetProp(component_ref, BAD_CAST "id"); diff --git a/src/OVAL/oval_set.c b/src/OVAL/oval_set.c index b373cc6561..53bcfe8bb2 100644 --- a/src/OVAL/oval_set.c +++ b/src/OVAL/oval_set.c @@ -307,12 +307,28 @@ static int _oval_set_parse_tag(xmlTextReaderPtr reader, struct oval_parser_conte if (set->type == OVAL_SET_UNKNOWN) { oval_setobject_set_type(set, OVAL_SET_AGGREGATE); } - return_code = oval_set_parse_tag(reader, context, &oval_set_consume, set); + if (set->type != OVAL_SET_AGGREGATE) { + // A mixing nested with object_reference/filter children + // is invalid: the extension union is already a COLLECTIVE, so adding + // a subset here would corrupt it. Skip the mismatched child. + dW("Ignoring child in a non-aggregate set, line: %d.", + xmlTextReaderGetParserLineNumber(reader)); + return_code = oval_parser_skip_tag(reader, context); + } else { + return_code = oval_set_parse_tag(reader, context, &oval_set_consume, set); + } } else { if (set->type == OVAL_SET_UNKNOWN) { oval_setobject_set_type(set, OVAL_SET_COLLECTIVE); } - if (strcmp(tagname, "object_reference") == 0) { + if (set->type != OVAL_SET_COLLECTIVE) { + // Mismatched child in an aggregate set (see above): writing an + // object/filter into the AGGREGATE subsets union would later cause + // oval_setobject_free() to free a model-owned object (double free). + dW("Ignoring <%s> child in an aggregate set, line: %d.", tagname, + xmlTextReaderGetParserLineNumber(reader)); + return_code = oval_parser_skip_tag(reader, context); + } else if (strcmp(tagname, "object_reference") == 0) { return_code = oscap_parser_text_value(reader, &oval_consume_object_ref, &ctx); } else if (strcmp(tagname, "filter") == 0) { return_code = oval_filter_parse_tag(reader, context, &oval_set_consume_filter, set); diff --git a/src/XCCDF/resolve.c b/src/XCCDF/resolve.c index 3548ec08f9..611967ed75 100644 --- a/src/XCCDF/resolve.c +++ b/src/XCCDF/resolve.c @@ -89,6 +89,7 @@ bool xccdf_benchmark_resolve(struct xccdf_benchmark *benchmark) // prototypes static void xccdf_resolve_textlist(struct oscap_list *child_list, struct oscap_list *parent_list, xccdf_textresolve_func more); +static void xccdf_resolve_warninglist(struct oscap_list *child_list, struct oscap_list *parent_list); static void xccdf_resolve_appendlist(struct oscap_list **child_list, struct oscap_list *parent_list, oscap_cmp_func item_compare, oscap_clone_func cloner, bool prepend); static void xccdf_resolve_value_instance(struct xccdf_value_instance *child, struct xccdf_value_instance *parent); static void xccdf_resolve_profile(struct xccdf_item *child, struct xccdf_item *parent); @@ -96,10 +97,6 @@ static void xccdf_resolve_group(struct xccdf_item *child, struct xccdf_item *par static void xccdf_resolve_rule(struct xccdf_item *child, struct xccdf_item *parent); static void xccdf_resolve_value(struct xccdf_item *child, struct xccdf_item *parent); -static void xccdf_resolve_warning(void *w1, void *w2) { - if (xccdf_warning_get_category(w1) == 0) - xccdf_warning_set_category(w1, xccdf_warning_get_category(w2)); -} static struct xccdf_profile *_xccdf_tailoring_profile_get_real_parent(struct xccdf_tailoring *tailoring, struct xccdf_profile *profile) { @@ -177,7 +174,7 @@ static void xccdf_resolve_item(struct xccdf_item *item, struct xccdf_tailoring * xccdf_resolve_textlist(item->item.description, parent->item.description, NULL); xccdf_resolve_textlist(item->item.question, parent->item.question, NULL); xccdf_resolve_textlist(item->item.rationale, parent->item.rationale, NULL); - xccdf_resolve_textlist(item->item.warnings, parent->item.warnings, xccdf_resolve_warning); + xccdf_resolve_warninglist(item->item.warnings, parent->item.warnings); xccdf_resolve_textlist(item->item.references, parent->item.references, NULL); // resolve platforms @@ -216,6 +213,32 @@ static void xccdf_resolve_textlist(struct oscap_list *child_list, struct oscap_l } } +// Resolve a list of elements. A xccdf_warning is NOT an oscap_text +// (its translatable text is in warning->text), so it must not be run through +// xccdf_resolve_textlist -- doing so reads past the smaller warning struct. +static void xccdf_resolve_warninglist(struct oscap_list *child_list, struct oscap_list *parent_list) +{ + OSCAP_FOR(xccdf_warning, child, oscap_iterator_new(child_list)) { + struct oscap_text *child_text = xccdf_warning_get_text(child); + if (child_text == NULL || oscap_text_get_overrides(child_text)) + continue; + + OSCAP_FOR(xccdf_warning, parent, oscap_iterator_new(parent_list)) { + struct oscap_text *parent_text = xccdf_warning_get_text(parent); + if (parent_text != NULL && + oscap_streq(oscap_text_get_lang(child_text), oscap_text_get_lang(parent_text))) { + char *text = oscap_sprintf("%s%s", oscap_text_get_text(parent_text), oscap_text_get_text(child_text)); + oscap_text_set_text(child_text, text); + free(text); + if (xccdf_warning_get_category(child) == 0) + xccdf_warning_set_category(child, xccdf_warning_get_category(parent)); + break; + } + } + xccdf_warning_iterator_free(parent_iter); + } +} + static void xccdf_resolve_appendlist(struct oscap_list **child_list, struct oscap_list *parent_list, oscap_cmp_func item_compare, oscap_clone_func cloner, bool prepend) { diff --git a/src/source/schematron.c b/src/source/schematron.c index b76761f1ad..f23bd7fa38 100644 --- a/src/source/schematron.c +++ b/src/source/schematron.c @@ -107,7 +107,8 @@ struct oscap_schema_table_entry OSCAP_SCHEMATRON_TABLE[] = { {OSCAP_DOCUMENT_OVAL_DIRECTIVES, "5.11.2", "oval/5.11.2/oval-directives-schematron.xsl"}, {OSCAP_DOCUMENT_OVAL_DIRECTIVES, "5.11.3", "oval/5.11.3/oval-directives-schematron.xsl"}, {OSCAP_DOCUMENT_XCCDF, "1.2", "xccdf/1.2/xccdf_1.2-schematron.xsl"}, - {OSCAP_DOCUMENT_SDS, "1.3", "sds/1.3/source-data-stream-1.3.xsl"} + {OSCAP_DOCUMENT_SDS, "1.3", "sds/1.3/source-data-stream-1.3.xsl"}, + {0, NULL, NULL } // sentinel terminator: the lookup loop below relies on a zeroed doc_type to stop }; static int _validate_sds_components(struct oscap_source *source) @@ -142,6 +143,10 @@ static int _validate_sds_components(struct oscap_source *source) static char *_xcf_resolve_in_catalog(xmlNodePtr resolver_node, const char *uri, xmlXPathContextPtr context) { + if (uri == NULL) { + // a check-content-ref without an @href reaches here with a NULL uri + return NULL; + } if (*uri == '#') { return oscap_strdup(uri); } @@ -176,6 +181,10 @@ static char *_xcf_resolve_in_catalog(xmlNodePtr resolver_node, const char *uri, static xmlNodePtr _xcf_get_component_ref(xmlNodePtr catalog, const char *uri, xmlXPathContextPtr context) { + if (catalog == NULL || catalog->children == NULL) { + // no or an empty one: nothing to resolve against + return NULL; + } char *component_ref_uri = _xcf_resolve_in_catalog(catalog->children->next, uri, context); if (component_ref_uri == NULL) { dD("Can't get component_ref URI using check-content-ref href='%s'", uri); @@ -201,6 +210,10 @@ static xmlNodePtr _xcf_get_component_ref(xmlNodePtr catalog, const char *uri, xm static xmlNodePtr _xcf_get_component(xmlNodePtr component_ref_node, xmlXPathContextPtr context) { char *xlink_href = (char *) xmlGetNsProp(component_ref_node, BAD_CAST "href", BAD_CAST "http://www.w3.org/1999/xlink"); + if (xlink_href == NULL) { + // component-ref without an xlink:href cannot be resolved + return NULL; + } char *xpath = oscap_sprintf("ancestor::ds:data-stream-collection//ds:component[@id='%s']", xlink_href + 1); free(xlink_href); xmlXPathObjectPtr components = xmlXPathNodeEval(component_ref_node, BAD_CAST xpath, context); @@ -420,6 +433,10 @@ static bool _req_src_236_2(xmlXPathContextPtr context) bool res = true; /* The parent rule element in the schematron matches all scap:data-stream elements */ xmlXPathObjectPtr data_streams = xmlXPathEval(BAD_CAST "//scap:data-stream", context); + if (data_streams == NULL || data_streams->nodesetval == NULL) { + xmlXPathFreeObject(data_streams); + return res; + } for (int i = 0; i < data_streams->nodesetval->nodeNr; i++) { xmlNodePtr data_stream_node = data_streams->nodesetval->nodeTab[i]; /* The assert applies only to configuration use cases */ @@ -586,6 +603,10 @@ static bool _req_src_346_1(xmlXPathContextPtr context) bool res = true; /* The parent rule element in the schematron matches all scap:data-stream elements */ xmlXPathObjectPtr data_streams = xmlXPathEval(BAD_CAST "//scap:data-stream", context); + if (data_streams == NULL || data_streams->nodesetval == NULL) { + xmlXPathFreeObject(data_streams); + return res; + } for (int i = 0; i < data_streams->nodesetval->nodeNr; i++) { xmlNodePtr data_stream_node = data_streams->nodesetval->nodeTab[i]; if (!_req_src_346_1_sub1(data_stream_node, context)) {