From da50724a2b046e56c036f7c856502467e4623d82 Mon Sep 17 00:00:00 2001 From: Edouard Schweisguth Date: Wed, 3 Jun 2026 08:46:38 +0000 Subject: [PATCH 1/2] XCCDF/OVAL/CPE/DS: fix NULL-pointer dereferences on malformed input Missing XML attributes/namespaces/text make libxml return NULL, which was then passed to strcmp/atoi/atof or dereferenced. Guard these and make the shared string helpers NULL-tolerant. - common/util.h: oscap_str_startswith()/oscap_str_endswith() return false on a NULL string (matching oscap_strcmp/oscap_streq). - OVAL: guard the optional `version` attribute before atoi() in oval_state, oval_definition, oval_object, oval_variable, oval_resultDefinition; guard the `reported` attribute (oval_directives); guard a NULL frame from a duplicate variable id (oval_varModel). - XCCDF: NULL-safe atof() for empty lower/upper-bound and score elements (value.c, result.c); reject a NULL (benchmark.c); skip a with no @idref before strlen() (item.c); reject a NULL profile in tailoring (tailoring.c). - DS: NULL-safe id comparisons in sds_index / rds; reject a component with no id as a hash key (ds_rds_session.c). --- src/DS/ds_rds_session.c | 5 +++++ src/DS/rds.c | 6 +++--- src/DS/sds_index.c | 14 ++++++++++---- src/OVAL/oval_definition.c | 4 +++- src/OVAL/oval_directives.c | 2 +- src/OVAL/oval_object.c | 4 +++- src/OVAL/oval_state.c | 4 +++- src/OVAL/oval_varModel.c | 8 ++++++-- src/OVAL/oval_variable.c | 4 +++- src/OVAL/results/oval_resultDefinition.c | 2 +- src/XCCDF/benchmark.c | 4 ++++ src/XCCDF/item.c | 5 +++++ src/XCCDF/result.c | 3 ++- src/XCCDF/tailoring.c | 4 ++++ src/XCCDF/value.c | 6 ++++-- src/common/util.h | 8 ++++++-- 16 files changed, 63 insertions(+), 20 deletions(-) diff --git a/src/DS/ds_rds_session.c b/src/DS/ds_rds_session.c index e62a5b6c2b..62434dcfce 100644 --- a/src/DS/ds_rds_session.c +++ b/src/DS/ds_rds_session.c @@ -106,6 +106,11 @@ int ds_rds_session_set_target_dir(struct ds_rds_session *session, const char *ta int ds_rds_session_register_component_source(struct ds_rds_session *session, const char *content_id, struct oscap_source *component) { + if (content_id == NULL) { + // A report/asset with no id cannot be used as a hash-table key. + oscap_seterr(OSCAP_EFAMILY_OSCAP, "Cannot register a Result DataStream component without an id."); + return -1; + } if (!oscap_htable_add(session->component_sources, content_id, component)) { oscap_seterr(OSCAP_EFAMILY_OSCAP, "Content '%s' has already been register with Result DataStream session: %s", content_id, oscap_source_readable_origin(session->source)); diff --git a/src/DS/rds.c b/src/DS/rds.c index e4c827b8fe..2dc0118b12 100644 --- a/src/DS/rds.c +++ b/src/DS/rds.c @@ -245,7 +245,7 @@ static xmlNodePtr ds_rds_add_ai_from_xccdf_results(xmlDocPtr doc, xmlNodePtr ass char* id_candidate = oscap_sprintf("asset%i", suffix); xmlChar* id = xmlGetProp(child_asset, BAD_CAST "id"); - if (strcmp(id_candidate, (const char*)id) == 0) + if (oscap_strcmp(id_candidate, (const char*)id) == 0) { suffix++; } @@ -392,8 +392,8 @@ static int ds_rds_report_inject_ai_target_id_ref(xmlDocPtr doc, xmlNodePtr test_ xmlChar* system_attr = xmlGetProp(duplicate_candidate, BAD_CAST "system"); xmlChar* name_attr = xmlGetProp(duplicate_candidate, BAD_CAST "name"); - if (strcmp((const char*)system_attr, ai_ns_uri) == 0 && - strcmp((const char*)name_attr, asset_id) == 0) { + if (oscap_strcmp((const char*)system_attr, ai_ns_uri) == 0 && + oscap_strcmp((const char*)name_attr, asset_id) == 0) { xmlFree(system_attr); xmlFree(name_attr); diff --git a/src/DS/sds_index.c b/src/DS/sds_index.c index ac40210e43..93a1e0fae1 100644 --- a/src/DS/sds_index.c +++ b/src/DS/sds_index.c @@ -246,7 +246,7 @@ struct ds_stream_index* ds_sds_index_get_stream(struct ds_sds_index* sds, const while (ds_stream_index_iterator_has_more(streams)) { struct ds_stream_index* stream = ds_stream_index_iterator_next(streams); - if (strcmp(ds_stream_index_get_id(stream), stream_id) == 0) + if (oscap_strcmp(ds_stream_index_get_id(stream), stream_id) == 0) { ret = stream; break; @@ -419,20 +419,23 @@ int ds_sds_index_select_checklist(struct ds_sds_index* s, int ret = 1; + if (s == NULL) + return ret; + struct ds_stream_index_iterator* streams_it = ds_sds_index_get_streams(s); while (ds_stream_index_iterator_has_more(streams_it)) { struct ds_stream_index* stream_idx = ds_stream_index_iterator_next(streams_it); const char* stream_id = ds_stream_index_get_id(stream_idx); - if (!*datastream_id || strcmp(stream_id, *datastream_id) == 0) + if (!*datastream_id || oscap_strcmp(stream_id, *datastream_id) == 0) { struct oscap_string_iterator* checklists_it = ds_stream_index_get_checklists(stream_idx); while (oscap_string_iterator_has_more(checklists_it)) { const char* checklist_id = oscap_string_iterator_next(checklists_it); - if (!*component_id || strcmp(checklist_id, *component_id) == 0) + if (!*component_id || oscap_strcmp(checklist_id, *component_id) == 0) { *component_id = checklist_id; *datastream_id = ds_stream_index_get_id(stream_idx); @@ -459,13 +462,16 @@ int ds_sds_index_select_checklist_by_benchmark_id(struct ds_sds_index* s, int ret = 1; + if (s == NULL) + return ret; + struct ds_stream_index_iterator* streams_it = ds_sds_index_get_streams(s); while (ds_stream_index_iterator_has_more(streams_it)) { struct ds_stream_index *stream_idx = ds_stream_index_iterator_next(streams_it); const char *stream_id = ds_stream_index_get_id(stream_idx); - if (!*datastream_id || strcmp(stream_id, *datastream_id) == 0) + if (!*datastream_id || oscap_strcmp(stream_id, *datastream_id) == 0) { const char *candidate_component_ref_id = (const char*)oscap_htable_get(stream_idx->component_id_to_component_ref_id, mapped_component_id); if (candidate_component_ref_id) { diff --git a/src/OVAL/oval_definition.c b/src/OVAL/oval_definition.c index c6169447c4..d04b6afeb3 100644 --- a/src/OVAL/oval_definition.c +++ b/src/OVAL/oval_definition.c @@ -438,7 +438,9 @@ int oval_definition_parse_tag(xmlTextReaderPtr reader, struct oval_parser_contex id = NULL; char *version = (char *)xmlTextReaderGetAttribute(reader, BAD_CAST "version"); - oval_definition_set_version(definition, atoi(version)); + if (version != NULL) { + oval_definition_set_version(definition, atoi(version)); + } free(version); version = NULL; diff --git a/src/OVAL/oval_directives.c b/src/OVAL/oval_directives.c index 55f110be9d..07b4a823f9 100644 --- a/src/OVAL/oval_directives.c +++ b/src/OVAL/oval_directives.c @@ -364,7 +364,7 @@ int oval_result_directives_parse_tag(xmlTextReaderPtr reader, struct oval_parser if ( (int) type != OVAL_ENUMERATION_INVALID) { /*reported */ xmlChar *boolstr = xmlTextReaderGetAttribute(reader, BAD_CAST "reported"); - bool reported = (strcmp((const char *)boolstr, "1") == 0) || (strcmp((const char *)boolstr, "true") == 0); + bool reported = oscap_streq((const char *)boolstr, "1") || oscap_streq((const char *)boolstr, "true"); free(boolstr); oval_result_directives_set_reported(directives, type, reported); diff --git a/src/OVAL/oval_object.c b/src/OVAL/oval_object.c index 31f076f3a0..f3f732099e 100644 --- a/src/OVAL/oval_object.c +++ b/src/OVAL/oval_object.c @@ -366,7 +366,9 @@ int oval_object_parse_tag(xmlTextReaderPtr reader, struct oval_parser_context *c oval_object_set_deprecated(object, deprecated); version = (char *)xmlTextReaderGetAttribute(reader, BAD_CAST "version"); - oval_object_set_version(object, atoi(version)); + if (version != NULL) { + oval_object_set_version(object, atoi(version)); + } ret = oval_parser_parse_tag(reader, context, &_oval_object_parse_tag, object); diff --git a/src/OVAL/oval_state.c b/src/OVAL/oval_state.c index 5b86708bfa..a3aadcfe80 100644 --- a/src/OVAL/oval_state.c +++ b/src/OVAL/oval_state.c @@ -315,7 +315,9 @@ int oval_state_parse_tag(xmlTextReaderPtr reader, struct oval_parser_context *co int deprecated = oval_parser_boolean_attribute(reader, "deprecated", 0); oval_state_set_deprecated(state, deprecated); char *version = (char *)xmlTextReaderGetAttribute(reader, BAD_CAST "version"); - oval_state_set_version(state, atoi(version)); + if (version != NULL) { + oval_state_set_version(state, atoi(version)); + } free(version); oval_operator_t operator = oval_operator_parse(reader, "operator", OVAL_OPERATOR_AND); oval_state_set_operator(state, operator); diff --git a/src/OVAL/oval_varModel.c b/src/OVAL/oval_varModel.c index e9c025bedd..bc166e273b 100644 --- a/src/OVAL/oval_varModel.c +++ b/src/OVAL/oval_varModel.c @@ -194,8 +194,12 @@ static int _oval_variable_model_parse_variable_values return_code = xmlTextReaderRead(reader); char *value = (char *)xmlTextReaderValue(reader); - ov = oval_value_new(frame->datatype, value); - oval_collection_add(frame->values, ov); + // frame is NULL when this variable's id duplicated an earlier one + // (see _oval_variable_model_parse_variable); ignore its values. + if (frame != NULL) { + ov = oval_value_new(frame->datatype, value); + oval_collection_add(frame->values, ov); + } free(value); } else { dW("Unprocessed tag: <%s:%s>.", namespace, tagname); diff --git a/src/OVAL/oval_variable.c b/src/OVAL/oval_variable.c index 7338c9f177..aa68a02a41 100644 --- a/src/OVAL/oval_variable.c +++ b/src/OVAL/oval_variable.c @@ -1129,7 +1129,9 @@ int oval_variable_parse_tag(xmlTextReaderPtr reader, struct oval_parser_context oval_variable_set_deprecated(variable, deprecated); version = (char *)xmlTextReaderGetAttribute(reader, BAD_CAST "version"); - oval_variable_set_version(variable, atoi(version)); + if (version != NULL) { + oval_variable_set_version(variable, atoi(version)); + } oval_datatype_t datatype = oval_datatype_parse(reader, "datatype", OVAL_DATATYPE_UNKNOWN); oval_variable_set_datatype(variable, datatype); diff --git a/src/OVAL/results/oval_resultDefinition.c b/src/OVAL/results/oval_resultDefinition.c index a5db1a5acb..078d05cde5 100644 --- a/src/OVAL/results/oval_resultDefinition.c +++ b/src/OVAL/results/oval_resultDefinition.c @@ -257,7 +257,7 @@ int oval_result_definition_parse_tag(xmlTextReaderPtr reader, struct oval_parser struct oval_result_definition *definition; xmlChar *definition_id = xmlTextReaderGetAttribute(reader, BAD_CAST "definition_id"); xmlChar *definition_version = xmlTextReaderGetAttribute(reader, BAD_CAST "version"); - int resvsn = atoi((char *)definition_version); + int resvsn = (definition_version != NULL) ? atoi((char *)definition_version) : 0; oval_result_t result = oval_result_parse(reader, "result", OVAL_ENUMERATION_INVALID); diff --git a/src/XCCDF/benchmark.c b/src/XCCDF/benchmark.c index 9775e4a48a..1da32e8e33 100644 --- a/src/XCCDF/benchmark.c +++ b/src/XCCDF/benchmark.c @@ -451,6 +451,10 @@ XCCDF_ITEM_ADDER_REG(benchmark, profile, profiles) bool xccdf_benchmark_add_result(struct xccdf_benchmark *benchmark, struct xccdf_result *item) { + if (item == NULL) { + // xccdf_result_new_parse() returns NULL on a malformed TestResult. + return false; + } const char *id = xccdf_result_get_id(item); if (id != NULL) { // Resolve possible conflicts of the IDs in the list of TestResults. diff --git a/src/XCCDF/item.c b/src/XCCDF/item.c index 2bd045c59e..c7f80a941d 100644 --- a/src/XCCDF/item.c +++ b/src/XCCDF/item.c @@ -760,6 +760,11 @@ bool xccdf_item_process_attributes(struct xccdf_item *item, xmlTextReaderPtr rea void xccdf_item_add_applicable_platform(struct xccdf_item *item, xmlTextReaderPtr reader) { char *platform_idref = xccdf_attribute_copy(reader, XCCDFA_IDREF); + if (platform_idref == NULL) { + // A with no @idref has nothing to reference; ignore it + // rather than passing NULL to the regex/strlen below. + return; + } /* Official Windows 7 CPE according to National Vulnerability Database * CPE Dictionary as of 2018-08-29 is 'cpe:/o:microsoft:windows_7'. diff --git a/src/XCCDF/result.c b/src/XCCDF/result.c index fab6bc512c..7bcae72008 100644 --- a/src/XCCDF/result.c +++ b/src/XCCDF/result.c @@ -1453,7 +1453,8 @@ static struct xccdf_score *xccdf_score_new_parse(xmlTextReaderPtr reader) if (xccdf_attribute_has(reader, XCCDFA_MAXIMUM)) score->maximum = xccdf_attribute_get_float(reader, XCCDFA_MAXIMUM); else score->maximum = XCCDF_SCORE_MAX_DAFAULT; - score->score = atof(oscap_element_string_get(reader)); + const char *score_str = oscap_element_string_get(reader); + score->score = score_str ? atof(score_str) : 0.0; return score; } diff --git a/src/XCCDF/tailoring.c b/src/XCCDF/tailoring.c index 11a267cb40..29370ed43b 100644 --- a/src/XCCDF/tailoring.c +++ b/src/XCCDF/tailoring.c @@ -81,6 +81,10 @@ void xccdf_tailoring_free(struct xccdf_tailoring *tailoring) bool xccdf_tailoring_add_profile(struct xccdf_tailoring *tailoring, struct xccdf_profile *profile) { + if (profile == NULL) { + // xccdf_profile_parse() returns NULL on a malformed . + return false; + } xccdf_profile_set_tailoring(profile, true); return oscap_list_add(tailoring->profiles, XITEM(profile)); } diff --git a/src/XCCDF/value.c b/src/XCCDF/value.c index 932a09dc1a..25bf2c57f1 100644 --- a/src/XCCDF/value.c +++ b/src/XCCDF/value.c @@ -144,13 +144,15 @@ struct xccdf_item *xccdf_value_parse(xmlTextReaderPtr reader, struct xccdf_item case XCCDFE_LOWER_BOUND: if (type == XCCDF_TYPE_NUMBER) { val = _xccdf_value_find_or_create_instance(XVALUE(value), selector, type); - val->lower_bound = atof(oscap_element_string_get(reader)); + const char *lb = oscap_element_string_get(reader); + val->lower_bound = lb ? atof(lb) : 0.0; } break; case XCCDFE_UPPER_BOUND: if (type == XCCDF_TYPE_NUMBER) { val = _xccdf_value_find_or_create_instance(XVALUE(value), selector, type); - val->upper_bound = atof(oscap_element_string_get(reader)); + const char *ub = oscap_element_string_get(reader); + val->upper_bound = ub ? atof(ub) : 0.0; } break; case XCCDFE_CHOICES: diff --git a/src/common/util.h b/src/common/util.h index 3d83fc9e43..dab8c62c68 100644 --- a/src/common/util.h +++ b/src/common/util.h @@ -343,13 +343,17 @@ static inline bool oscap_streq(const char *s1, const char *s2) { return oscap_strcmp(s1, s2) == 0; } -/// Check whether str starts with "prefix" +/// Check whether str starts with "prefix" (a NULL str never matches) static inline bool oscap_str_startswith(const char *str, const char *prefix) { + if (str == NULL || prefix == NULL) + return false; return strncmp(str, prefix, strlen(prefix)) == 0; } -/// Check whether str ends with "suffix" +/// Check whether str ends with "suffix" (a NULL str never matches) static inline bool oscap_str_endswith(const char *str, const char *suffix) { + if (str == NULL || suffix == NULL) + return false; const size_t str_len = strlen(str); const size_t suffix_len = strlen(suffix); if (suffix_len > str_len) From 906022e8472c17b3d23fc3eddbc103741773f26c Mon Sep 17 00:00:00 2001 From: Edouard Schweisguth Date: Thu, 4 Jun 2026 08:21:45 +0000 Subject: [PATCH 2/2] address PR comments --- src/DS/sds_index.c | 6 +++--- src/XCCDF/result.c | 7 ++++++- src/XCCDF/value.c | 18 ++++++++++++++---- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/DS/sds_index.c b/src/DS/sds_index.c index 93a1e0fae1..989504d55a 100644 --- a/src/DS/sds_index.c +++ b/src/DS/sds_index.c @@ -454,6 +454,9 @@ int ds_sds_index_select_checklist(struct ds_sds_index* s, int ds_sds_index_select_checklist_by_benchmark_id(struct ds_sds_index* s, const char *benchmark_id, const char **datastream_id, const char **component_ref_id) { + if (s == NULL) + return 1; + const char *mapped_component_id = (const char*)oscap_htable_get(s->benchmark_id_to_component_id, benchmark_id); if (!mapped_component_id) { oscap_seterr(OSCAP_EFAMILY_XML, "Can't map benchmark ID '%s' to any component ID.", benchmark_id); @@ -462,9 +465,6 @@ int ds_sds_index_select_checklist_by_benchmark_id(struct ds_sds_index* s, int ret = 1; - if (s == NULL) - return ret; - struct ds_stream_index_iterator* streams_it = ds_sds_index_get_streams(s); while (ds_stream_index_iterator_has_more(streams_it)) { diff --git a/src/XCCDF/result.c b/src/XCCDF/result.c index 7bcae72008..da97513192 100644 --- a/src/XCCDF/result.c +++ b/src/XCCDF/result.c @@ -1454,7 +1454,12 @@ static struct xccdf_score *xccdf_score_new_parse(xmlTextReaderPtr reader) score->maximum = xccdf_attribute_get_float(reader, XCCDFA_MAXIMUM); else score->maximum = XCCDF_SCORE_MAX_DAFAULT; const char *score_str = oscap_element_string_get(reader); - score->score = score_str ? atof(score_str) : 0.0; + if (score_str == NULL) { + dW("Empty element is invalid, rejecting."); + xccdf_score_free(score); + return NULL; + } + score->score = atof(score_str); return score; } diff --git a/src/XCCDF/value.c b/src/XCCDF/value.c index 25bf2c57f1..c687d5c7a1 100644 --- a/src/XCCDF/value.c +++ b/src/XCCDF/value.c @@ -143,16 +143,26 @@ struct xccdf_item *xccdf_value_parse(xmlTextReaderPtr reader, struct xccdf_item break; case XCCDFE_LOWER_BOUND: if (type == XCCDF_TYPE_NUMBER) { - val = _xccdf_value_find_or_create_instance(XVALUE(value), selector, type); const char *lb = oscap_element_string_get(reader); - val->lower_bound = lb ? atof(lb) : 0.0; + if (lb == NULL) { + dW("Empty element is invalid, rejecting ."); + xccdf_value_free(value); + return NULL; + } + val = _xccdf_value_find_or_create_instance(XVALUE(value), selector, type); + val->lower_bound = atof(lb); } break; case XCCDFE_UPPER_BOUND: if (type == XCCDF_TYPE_NUMBER) { - val = _xccdf_value_find_or_create_instance(XVALUE(value), selector, type); const char *ub = oscap_element_string_get(reader); - val->upper_bound = ub ? atof(ub) : 0.0; + if (ub == NULL) { + dW("Empty element is invalid, rejecting ."); + xccdf_value_free(value); + return NULL; + } + val = _xccdf_value_find_or_create_instance(XVALUE(value), selector, type); + val->upper_bound = atof(ub); } break; case XCCDFE_CHOICES: