Skip to content

OVAL/XCCDF/DS/source: fix memory-corruption bugs on malformed input #2363

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 1 commit into
base: main
Choose a base branch
from
+99 −11
Open

OVAL/XCCDF/DS/source: fix memory-corruption bugs on malformed input #2363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 31 additions & 3 deletions src/DS/sds.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,8 @@
continue;

char* candidate_id = (char*)xmlGetProp(candidate, BAD_CAST "id");
if (candidate_id == NULL)
continue;
if (strcmp(candidate_id, component_id) == 0)
{
component = candidate;
Expand Down Expand Up @@ -489,8 +491,18 @@
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)

Check failure on line 500 in src/DS/sds.c

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor this function to reduce its Cognitive Complexity from 28 to the 25 allowed. 


See more on https://sonarcloud.io/project/issues?id=OpenSCAP_openscap&issues=AZ6M4EHAml3SeofRgYys&open=AZ6M4EHAml3SeofRgYys&pullRequest=2363
{
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)
{
Expand All @@ -512,15 +524,21 @@
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;
}
Expand All @@ -543,6 +561,7 @@
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;
}
Expand All @@ -554,6 +573,7 @@
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;
}
Expand All @@ -567,14 +587,16 @@
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
}
Expand All @@ -583,12 +605,18 @@
}
}

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");
Expand Down
20 changes: 18 additions & 2 deletions src/OVAL/oval_set.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <set> mixing nested <set> 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 <set> 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);
Expand Down
33 changes: 28 additions & 5 deletions src/XCCDF/resolve.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,17 +89,14 @@

// 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);
static void xccdf_resolve_group(struct xccdf_item *child, struct xccdf_item *parent);
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)
{
Expand Down Expand Up @@ -177,7 +174,7 @@
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
Expand Down Expand Up @@ -216,6 +213,32 @@
}
}

// Resolve a list of <warning> 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);

Check warning on line 227 in src/XCCDF/resolve.c

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Make the type of this variable a pointer-to-const. The current type of "parent_text" is "struct oscap_text *". 


See more on https://sonarcloud.io/project/issues?id=OpenSCAP_openscap&issues=AZ6M4EJ8ml3SeofRgYyu&open=AZ6M4EJ8ml3SeofRgYyu&pullRequest=2363
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)

Check failure on line 233 in src/XCCDF/resolve.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=AZ6M4EJ8ml3SeofRgYyt&open=AZ6M4EJ8ml3SeofRgYyt&pullRequest=2363
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)
{
Expand Down
23 changes: 22 additions & 1 deletion src/source/schematron.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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 <catalog> 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);
Expand All @@ -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);
Expand Down Expand Up @@ -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 */
Expand Down Expand Up @@ -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)) {
Expand Down