Skip to content

CPE/XCCDF/DS: fix infinite loops in parsers on malformed input #2362

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
+79 −24
Open

CPE/XCCDF/DS: fix infinite loops in parsers on malformed input #2362

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
4 changes: 2 additions & 2 deletions src/CPE/cpedict_ext_priv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ static struct cpe_ext_deprecation *cpe_ext_deprecation_parse(xmlTextReaderPtr re
deprecation->date = (char *) xmlTextReaderGetAttribute(reader, BAD_CAST ATTR_DATE_STR);
if (xmlTextReaderIsEmptyElement(reader) == 0) { // the element contains child nodes
xmlTextReaderNextNode(reader);
while (xmlStrcmp(xmlTextReaderConstLocalName(reader), BAD_CAST TAG_CPE_EXT_DEPRECATION_STR) != 0) {
while (xmlTextReaderConstLocalName(reader) != NULL && xmlStrcmp(xmlTextReaderConstLocalName(reader), BAD_CAST TAG_CPE_EXT_DEPRECATION_STR) != 0) {
if (xmlTextReaderNodeType(reader) != XML_READER_TYPE_ELEMENT) {
xmlTextReaderNextNode(reader);
continue;
Expand Down Expand Up @@ -245,7 +245,7 @@ struct cpe23_item *cpe23_item_parse(xmlTextReaderPtr reader)
item->name = (char *) xmlTextReaderGetAttribute(reader, BAD_CAST ATTR_NAME_STR);
if (xmlTextReaderIsEmptyElement(reader) == 0) { // the element contains child nodes
xmlTextReaderNextNode(reader);
while (xmlStrcmp(xmlTextReaderConstLocalName(reader), BAD_CAST TAG_CPE23_ITEM_STR) != 0) {
while (xmlTextReaderConstLocalName(reader) != NULL && xmlStrcmp(xmlTextReaderConstLocalName(reader), BAD_CAST TAG_CPE23_ITEM_STR) != 0) {
if (xmlTextReaderNodeType(reader) != XML_READER_TYPE_ELEMENT) {
xmlTextReaderNextNode(reader);
continue;
Expand Down
41 changes: 30 additions & 11 deletions src/CPE/cpedict_priv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,7 +617,7 @@
// skip nodes until new element
xmlTextReaderNextElement(reader);

while (xmlStrcmp(xmlTextReaderConstLocalName(reader), TAG_GENERATOR_STR) != 0) {
while (xmlTextReaderConstLocalName(reader) != NULL && xmlStrcmp(xmlTextReaderConstLocalName(reader), TAG_GENERATOR_STR) != 0) {

if ((xmlStrcmp(xmlTextReaderConstLocalName(reader),
TAG_PRODUCT_NAME_STR) == 0) &&
Expand All @@ -644,9 +644,11 @@
"Unknown XML element in CPE dictionary generator, local name is '%s'.",
xmlTextReaderConstLocalName(reader));
}
// element saved. Let's jump on the very next one node (not element, because we need to
// element saved. Let's jump on the very next one node (not element, because we need to
// find XML_READER_TYPE_END_ELEMENT node, see "while" condition and the condition below "while"
xmlTextReaderNextNode(reader);
// Stop at end-of-document so a missing </generator> can't spin this loop.
if (xmlTextReaderNextNode(reader) != 1)
break;

}
}
Expand Down Expand Up @@ -712,10 +714,13 @@
xmlTextReaderNextElementWE(reader, TAG_CPE_ITEM_STR);
// Now it's time to go deaply to cpe-item element and parse it's children
// Do while there is another cpe-item element. Then return.
while (xmlStrcmp(xmlTextReaderConstLocalName(reader), TAG_CPE_ITEM_STR) != 0) {
while (xmlTextReaderConstLocalName(reader) != NULL && xmlStrcmp(xmlTextReaderConstLocalName(reader), TAG_CPE_ITEM_STR) != 0) {

if (xmlTextReaderNodeType(reader) != XML_READER_TYPE_ELEMENT) {
xmlTextReaderNextNode(reader);
// Break on end-of-document/error; xmlTextReaderRead() returns 0
// at EOF without moving, which would otherwise spin this loop.
if (xmlTextReaderNextNode(reader) != 1)

Check failure on line 722 in src/CPE/cpedict_priv.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=AZ6M0kp9XEFIpWlLX1NE&open=AZ6M0kp9XEFIpWlLX1NE&pullRequest=2362
break;
continue;
}

Expand Down Expand Up @@ -836,7 +841,7 @@
notes->lang = (char *) xmlTextReaderXmlLang(reader);
if (xmlTextReaderIsEmptyElement(reader) == 0) { // element contains child nodes
xmlTextReaderNextNode(reader);
while (xmlStrcmp(xmlTextReaderConstLocalName(reader), TAG_NOTES_STR) != 0) {
while (xmlTextReaderConstLocalName(reader) != NULL && xmlStrcmp(xmlTextReaderConstLocalName(reader), TAG_NOTES_STR) != 0) {
if (xmlTextReaderNodeType(reader) != XML_READER_TYPE_ELEMENT) {
xmlTextReaderNextNode(reader);
continue;
Expand Down Expand Up @@ -882,7 +887,7 @@
// jump to next element (which should be product)
xmlTextReaderNextElement(reader);

while (xmlStrcmp(xmlTextReaderConstLocalName(reader), TAG_VENDOR_STR) != 0) {
while (xmlTextReaderConstLocalName(reader) != NULL && xmlStrcmp(xmlTextReaderConstLocalName(reader), TAG_VENDOR_STR) != 0) {

if (xmlTextReaderNodeType(reader) != XML_READER_TYPE_ELEMENT) {
xmlTextReaderNextNode(reader);
Expand Down Expand Up @@ -922,22 +927,36 @@
// initialization
version = cpe_version_new();
version->value = (char *)xmlTextReaderGetAttribute(reader, ATTR_VALUE_STR);
oscap_list_add(product->versions, version);
// child elements may appear out of order / without their parent;
// skip an orphan rather than dereferencing a NULL parent.
if (product != NULL)
oscap_list_add(product->versions, version);
else
cpe_version_free(version);
} else if (xmlStrcmp(xmlTextReaderConstLocalName(reader), TAG_UPDATE_STR) == 0) {
// initialization
update = cpe_update_new();
update->value = (char *)xmlTextReaderGetAttribute(reader, ATTR_VALUE_STR);
oscap_list_add(version->updates, update);
if (version != NULL)
oscap_list_add(version->updates, update);

Check failure on line 941 in src/CPE/cpedict_priv.c

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use of memory after it is freed 


See more on https://sonarcloud.io/project/issues?id=OpenSCAP_openscap&issues=AZ6M0kp9XEFIpWlLX1NF&open=AZ6M0kp9XEFIpWlLX1NF&pullRequest=2362
else
cpe_update_free(update);
} else if (xmlStrcmp(xmlTextReaderConstLocalName(reader), TAG_EDITION_STR) == 0) {
// initialization
edition = cpe_edition_new();
edition->value = (char *)xmlTextReaderGetAttribute(reader, ATTR_VALUE_STR);
oscap_list_add(update->editions, edition);
if (update != NULL)
oscap_list_add(update->editions, edition);

Check failure on line 949 in src/CPE/cpedict_priv.c

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use of memory after it is freed 


See more on https://sonarcloud.io/project/issues?id=OpenSCAP_openscap&issues=AZ6M0kp9XEFIpWlLX1NG&open=AZ6M0kp9XEFIpWlLX1NG&pullRequest=2362
else
cpe_edition_free(edition);
} else if (xmlStrcmp(xmlTextReaderConstLocalName(reader), TAG_LANGUAGE_STR) == 0) {
// initialization
language = cpe_language_new();
language->value = (char *)xmlTextReaderGetAttribute(reader, ATTR_VALUE_STR);
oscap_list_add(edition->languages, language);
if (edition != NULL)
oscap_list_add(edition->languages, language);

Check failure on line 957 in src/CPE/cpedict_priv.c

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use of memory after it is freed 


See more on https://sonarcloud.io/project/issues?id=OpenSCAP_openscap&issues=AZ6M0kp9XEFIpWlLX1NH&open=AZ6M0kp9XEFIpWlLX1NH&pullRequest=2362
else
cpe_language_free(language);
} else {
oscap_seterr(OSCAP_EFAMILY_OSCAP, "Unknown XML element withinin CPE vendor element, local name is '%s'.",
xmlTextReaderConstLocalName(reader));
Expand Down
11 changes: 9 additions & 2 deletions src/CPE/cpelang_priv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,8 +303,10 @@
ret->expr = cpe_testexpr_parse(reader);
} else if (xmlTextReaderNodeType(reader) == XML_READER_TYPE_ELEMENT)
oscap_seterr(OSCAP_EFAMILY_OSCAP, "Unknown XML element in platform");
// get the next node
xmlTextReaderNextNode(reader);
// get the next node; stop at end of document so a <platform> that is
// never closed cannot spin this loop forever.
if (xmlTextReaderNextNode(reader) != 1)
break;
}
return ret;
}
Expand Down Expand Up @@ -388,7 +390,12 @@
// .. and the next node is logical-test element, we need recursive call
if (!xmlStrcmp(xmlTextReaderConstLocalName(reader), TAG_LOGICAL_TEST_STR) &&
xmlTextReaderNodeType(reader) == XML_READER_TYPE_ELEMENT) {
xmlNodePtr before = xmlTextReaderCurrentNode(reader);

Check warning on line 393 in src/CPE/cpelang_priv.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 "before" is "struct _xmlNode *". 


See more on https://sonarcloud.io/project/issues?id=OpenSCAP_openscap&issues=AZ6M0kqSXEFIpWlLX1NI&open=AZ6M0kqSXEFIpWlLX1NI&pullRequest=2362
oscap_list_add(ret->meta.expr, cpe_testexpr_parse(reader));
// A nested logical-test with e.g. an invalid operator returns
// without consuming its node; force progress so this loop can't spin.
if (xmlTextReaderCurrentNode(reader) == before)
xmlTextReaderNextNode(reader);
if (xmlTextReaderDepth(reader) < depth) {
return ret;
} else if (xmlTextReaderDepth(reader) == depth) continue;
Expand Down
34 changes: 26 additions & 8 deletions src/DS/rds_index.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@
while (rds_report_request_index_iterator_has_more(it))
{
struct rds_report_request_index* rr_index = rds_report_request_index_iterator_next(it);
if (strcmp(rds_report_request_index_get_id(rr_index), id) == 0) {
// ids may be missing (NULL) on malformed input; oscap_strcmp is NULL-safe.
if (oscap_strcmp(rds_report_request_index_get_id(rr_index), id) == 0) {
ret = rr_index;
break;
}
Expand All @@ -123,7 +124,7 @@
while (rds_asset_index_iterator_has_more(it))
{
struct rds_asset_index *a_index = rds_asset_index_iterator_next(it);
if (strcmp(rds_asset_index_get_id(a_index), id) == 0) {
if (oscap_strcmp(rds_asset_index_get_id(a_index), id) == 0) {
ret = a_index;
break;
}
Expand All @@ -142,7 +143,7 @@
while (rds_report_index_iterator_has_more(it))
{
struct rds_report_index *r_index = rds_report_index_iterator_next(it);
if (strcmp(rds_report_index_get_id(r_index), id) == 0) {
if (oscap_strcmp(rds_report_index_get_id(r_index), id) == 0) {
ret = r_index;
break;
}
Expand Down Expand Up @@ -200,20 +201,25 @@
xmlChar *inner_ref = relationship_get_inner_ref(relationship_node);

// We now only use arfvocab: but arfrel: is kept here for compatibility
if (oscap_str_startswith((const char *) type_attr, "arfvocab:")
|| oscap_str_startswith((const char *) type_attr, "arfrel:")) {
if (type_attr != NULL
&& (oscap_str_startswith((const char *) type_attr, "arfvocab:")
|| oscap_str_startswith((const char *) type_attr, "arfrel:"))) {
if (oscap_str_endswith((const char*)type_attr, ":isAbout")) {
struct rds_asset_index* asset = rds_index_get_asset(ret, (const char*)inner_ref);
struct rds_report_index* report = rds_index_get_report(ret, (const char*)subject_attr);

rds_asset_index_add_report_ref(asset, report);
// A relationship may reference ids that don't resolve to an asset/
// report in this collection; skip it rather than dereferencing NULL.
if (asset != NULL && report != NULL)

Check failure on line 213 in src/DS/rds_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=AZ6M0knTXEFIpWlLX1NC&open=AZ6M0knTXEFIpWlLX1NC&pullRequest=2362
rds_asset_index_add_report_ref(asset, report);
} else if (oscap_str_endswith((const char*)type_attr, ":createdFor")) {
struct rds_report_request_index *request = rds_index_get_report_request(ret, (const char*)inner_ref);
struct rds_report_index *report = rds_index_get_report(ret, (const char*)subject_attr);

// This is based on the assumption that every report has at most 1 request
// it was "created for".
rds_report_index_set_request(report, request);
if (report != NULL)

Check failure on line 221 in src/DS/rds_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=AZ6M0knTXEFIpWlLX1ND&open=AZ6M0knTXEFIpWlLX1ND&pullRequest=2362
rds_report_index_set_request(report, request);
} else {
dW("Unsupported core:relationship/@type='%s'", (const char *) type_attr);
}
Expand Down Expand Up @@ -268,6 +274,9 @@
{
if (strcmp((const char*)xmlTextReaderConstLocalName(reader), "report-request") != 0) {
// TODO: warning?
// Must advance the reader, otherwise oscap_to_start_element()
// keeps returning this same node and the loop spins forever.
xmlTextReaderRead(reader);
continue;
}

Expand All @@ -289,6 +298,9 @@
{
if (strcmp((const char*)xmlTextReaderConstLocalName(reader), "asset") != 0) {
// TODO: warning?
// Must advance the reader, otherwise oscap_to_start_element()
// keeps returning this same node and the loop spins forever.
xmlTextReaderRead(reader);
continue;
}

Expand All @@ -310,6 +322,9 @@
{
if (strcmp((const char*)xmlTextReaderConstLocalName(reader), "report") != 0) {
// TODO: warning?
// Must advance the reader, otherwise oscap_to_start_element()
// keeps returning this same node and the loop spins forever.
xmlTextReaderRead(reader);
continue;
}

Expand Down Expand Up @@ -355,13 +370,16 @@
{
int ret = 1;

if (s == NULL)
return ret;

struct rds_report_index_iterator *reports_it = rds_index_get_reports(s);
while (rds_report_index_iterator_has_more(reports_it))
{
struct rds_report_index* report_idx = rds_report_index_iterator_next(reports_it);
const char *report_idx_id = rds_report_index_get_id(report_idx);

if (!*report_id || strcmp(report_idx_id, *report_id) == 0) {
if (!*report_id || oscap_strcmp(report_idx_id, *report_id) == 0) {
*report_id = report_idx_id;
ret = 0;
break;
Expand Down
13 changes: 12 additions & 1 deletion src/XCCDF/profile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,9 +223,14 @@

static void xccdf_parse_remarks(xmlTextReaderPtr reader, struct oscap_list* list, int depth)
{
while (oscap_to_start_element(reader, depth))
while (oscap_to_start_element(reader, depth)) {
if (xccdf_element_get(reader) == XCCDFE_REMARK)
oscap_list_add(list, oscap_text_new_parse(XCCDF_TEXT_PLAIN, reader));
else
// A non-<remark> child here would otherwise never be consumed,
// spinning oscap_to_start_element() forever; skip it to progress.
xmlTextReaderRead(reader);
}
}

struct xccdf_item *xccdf_profile_parse(xmlTextReaderPtr reader, struct xccdf_item *bench)
Expand All @@ -241,6 +246,10 @@
int depth = oscap_element_depth(reader) + 1;

while (oscap_to_start_element(reader, depth)) {
// Guard against a child element that no case below consumes (which
// would spin oscap_to_start_element() on the same node forever): if an
// iteration leaves the reader on the same node, force it to advance.
xmlNodePtr _node_before = xmlTextReaderCurrentNode(reader);

Check warning on line 252 in src/XCCDF/profile.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 "_node_before" is "struct _xmlNode *". 


See more on https://sonarcloud.io/project/issues?id=OpenSCAP_openscap&issues=AZ6M0kqoXEFIpWlLX1NJ&open=AZ6M0kqoXEFIpWlLX1NJ&pullRequest=2362
switch (xccdf_element_get(reader)) {
case XCCDFE_SELECT:{
struct xccdf_select *sel = xccdf_select_new();
Expand Down Expand Up @@ -296,6 +305,8 @@
xmlTextReaderConstLocalName(reader));
xmlTextReaderRead(reader);
}
if (xmlTextReaderCurrentNode(reader) == _node_before)
xmlTextReaderRead(reader);
}

return prof;
Expand Down