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

[edznux-dd]

Hey :)

I was looking at some of our logs and noticed that openscap was crashing on memory corruption issues, so I decided to dig a bit into it and write some fuzz test (I didn't have the exact source of the crashes).

This is the 1st of 4 fix PR (+ one for the fuzz test), that I tried to split into manageable diff.

(There will be one PR that contains the fuzz-harnesses that I used. They are not comprehensive for all the code path

This current PR mostly ensure that all references are guarded by null checks.

Notes:

• I've read your contribution guideline, and I believe I'm targeting the correct branch, but I might be mistaken?
• I've not created associated individual issues as I believe it creates unnecessary toil when I had to fix the bugs to continue fuzzing anyway (a lot of these bugs where found iteratively, run fuzz -> bug fix -> run fuzz -> bug fix ...)
• I've let all the fuzz test run for 24h once all bug were fixed (not on a huge amount of cores, sure, but still, seems fairly robust now, under the current harnesses.)

For some context on this specific PR:

• the missing XML attributes/namespaces/text make libxml return NULL, which was then passed to strcmp/atoi/atof or dereferenced.
• 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).

[Mab879]

First of all, thank you for this series of PRs.

Please consider adding regression tests.

[Mab879]

Shouldn't this be placed before line 457?

[edznux-dd]

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

[Mab879]

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);

[edznux-dd]

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

[Mab879]

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);

[Mab879]

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);

[sonarqubecloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud