Skip to content

XCCDF/OVAL/CPE/DS: fix NULL-pointer dereferences on malformed input #2361

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
edznux-dd wants to merge 2 commits into
base: main
Choose a base branch
from
Open

XCCDF/OVAL/CPE/DS: fix NULL-pointer dereferences on malformed input #2361

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da50724
XCCDF/OVAL/CPE/DS: fix NULL-pointer dereferences on malformed input
edznux-dd Jun 3, 2026
906022e
address PR comments
edznux-dd Jun 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/DS/ds_rds_session.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
6 changes: 3 additions & 3 deletions src/DS/rds.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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++;
}
Expand Down Expand Up @@ -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);
Expand Down
14 changes: 10 additions & 4 deletions src/DS/sds_index.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@
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;
Expand Down Expand Up @@ -419,20 +419,23 @@

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)

Check failure on line 438 in src/DS/sds_index.c

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this code to not nest more than 3 if|for|do|while|switch statements. 


See more on https://sonarcloud.io/project/issues?id=OpenSCAP_openscap&issues=AZ6Mye1t2Ak3xlgmpnXA&open=AZ6Mye1t2Ak3xlgmpnXA&pullRequest=2361
{
*component_id = checklist_id;
*datastream_id = ds_stream_index_get_id(stream_idx);
Expand All @@ -459,13 +462,16 @@

int ret = 1;

if (s == NULL)
Copy link
Copy Markdown
Member

@Mab879 Mab879 Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be placed before line 457?

Copy link
Copy Markdown
Author

@edznux-dd edznux-dd Jun 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, yep, thanks 😅 (fixed in 906022e)

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) {
Expand Down
4 changes: 3 additions & 1 deletion src/OVAL/oval_definition.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
2 changes: 1 addition & 1 deletion src/OVAL/oval_directives.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
4 changes: 3 additions & 1 deletion src/OVAL/oval_object.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
4 changes: 3 additions & 1 deletion src/OVAL/oval_state.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
8 changes: 6 additions & 2 deletions src/OVAL/oval_varModel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 3 additions & 1 deletion src/OVAL/oval_variable.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/OVAL/results/oval_resultDefinition.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
4 changes: 4 additions & 0 deletions src/XCCDF/benchmark.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
5 changes: 5 additions & 0 deletions src/XCCDF/item.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <platform> 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'.
Expand Down
3 changes: 2 additions & 1 deletion src/XCCDF/result.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Copy Markdown
Member

@Mab879 Mab879 Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like might be better here.

if (score_str == NULL) {
    dW("Empty <score> element is invalid, rejecting.");
    xccdf_score_free(score);
    return NULL;
}
score->score = atof(score_str);

return score;
}

Expand Down
4 changes: 4 additions & 0 deletions src/XCCDF/tailoring.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <Profile>.
return false;
}
xccdf_profile_set_tailoring(profile, true);
return oscap_list_add(tailoring->profiles, XITEM(profile));
}
Expand Down
6 changes: 4 additions & 2 deletions src/XCCDF/value.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Copy Markdown
Member

@Mab879 Mab879 Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defaulting 0.0 might not be right thing here. Something like below might be better.

        if (lb == NULL) {
            dW("Empty <lower-bound> element is invalid, rejecting <Value>.");
            xccdf_value_free(value);
            return NULL;
        }
        val = _xccdf_value_find_or_create_instance(XVALUE(value), selector, type);
        val->lower_bound = atof(lb);

Copy link
Copy Markdown
Author

@edznux-dd edznux-dd Jun 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I wasn't sure if it was preferable to default or to fail. sounds good to me :)
Applied your suggestion in 906022e as well

}
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;
Copy link
Copy Markdown
Member

@Mab879 Mab879 Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as lower bound. Something like

   const char *ub = oscap_element_string_get(reader);
        if (ub == NULL) {
            dW("Empty <upper-bound> element is invalid, rejecting <Value>.");
            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:
Expand Down
8 changes: 6 additions & 2 deletions src/common/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading