HDF->SARIF converter output doesn't map status to kind/level/rank correctly

candrews commented 1 year ago

The SARIF generated by this tool from HDF does not have HDF's status mapped to SARIF's kind and level correctly.

HDF's status can have the following values and these are the kind/level values I believe they should map to:

passed should map to pass and level either unset or none
failed should map to fail and level either warning, error, or note which is already implemented by SarifLevelFromHdfImpact
error should map to review and level either unset or none
skipped should map to notApplicable and level either unset or none

Also, if kind is set to fail, then rank can be set. If kind is any value other than fail, then rank must not be set.

To reproduce:

Take this sample HDF: openscap-report.hdf.json.gz
Use this project to convert it to SARIF: npx @microsoft/sarif-multitool@4.2.2 convert -t Hdf -o "openscap-report.sarif" "openscap-report.hdf.json" Here's the resulting sarif (I pretty formatted it for readability): openscap-report.sarif.gz
Open openscap-report.sarif

Expected: A variety of kind and level values should be present. The SARIF is basically useless at the moment as every result shows as a finding, whether it passed or not. For some example:

xccdf_org.ssgproject.content_rule_accounts_no_uid_except_zero has an HDF status of passed but in the SARIF has a level of error (level should be none or absent, and kind should be set to passed)
xccdf_org.ssgproject.content_rule_no_netrc_files has an HDF status of skipped but in the SARIF has a level of error (level should be none or absent, and kind should be set to notApplicable)
rank is present only when kind is fail or absent.

Actual:

kind is never present
level is always either note, error, or warning; the none value are never used.

Amndeep7 commented 1 year ago

Hi - I'm a member of the MITRE SAF team, which is the team that manages the Heimdall Data Format (HDF). Thanks @candrews for spotting this regression in behavior.

@aaronlippold @ejaronne - wanted to bring this to your awareness as well, and would appreciate your input on corrections to this mapping.

I think what Craig has proposed above is good but for two areas:

skipped should be mapped to review not notApplicable since tests can be skipped for a variety of reasons such as needing manual review due to the automation failing or not being possible.
HDF's impact field should be looked at as well to determine kind/level values. Specifically, if the impact is set to 0 (within some epsilon value, usually .1), then we mark that as having a none severity, i.e. it is a notApplicable test.

candrews commented 1 year ago

skipped should be mapped to review not notApplicable since tests can be skipped for a variety of reasons such as needing manual review due to the automation failing or not being possible.

The HDF in this issue's description, openscap-report.hdf.json.gz, was generated by running oscap --rule xccdf_org.ssgproject.content_rule_accounts_no_uid_except_zero - all others rules were skipped. And they are all marked as skipped as expected in the HDF. If this converter does as suggested and translates hdf skipped to SARIF review, then someone will be expected to review all of those results that were intentionally skipped. That drastically reduces the utility of these tools. I believe that hdf skipped should be translated to SARIF notApplicable.

aaronlippold commented 1 year ago

The question is more about what action was taken - If no test was run then it is a Not Reviewed - if the test was marked as Not Applicable - therefore outside the bounds of the validation that is a different condition. Aka NA means that it was an active choice to not need to test the requirement.

We would have to look at the SARIF specification to determine which state is being communicated.

candrews commented 1 year ago

We would have to look at the SARIF specification to determine which state is being communicated.

The SARIF specification regarding level seems very clear:

·         "pass": The rule specified by ruleId (§3.27.5), ruleIndex (§3.27.6), and/or rule (§3.27.7) was evaluated, and no problem was found.
·         "open": The specified rule was evaluated, and the tool concluded that there was insufficient information to decide whether a problem exists.
NOTE 1: This value is used by proof-based tools. Sometimes such a tool can prove that there is no violation (kind = "pass"), sometimes it can prove that there is a violation (kind = "fail"), and sometimes it does not detect a violation but is unable to prove that there is none (kind = "open"). In such a tool, a kind value of "open" might be an indication that the user should add additional assertions to enabe the tool to determine if there is a violation.
·         "informational": The specified rule was evaluated and produced a purely informational result that does not indicate the presence of a problem. (See the example below.)
·         "notApplicable": The rule specified by ruleId was not evaluated, because it does not apply to the analysis target.
·         "review": The result requires review by a human user to decide if it represents a problem.
NOTE 2: This value is used by tools that are unable to check for certain conditions, but that wish to bring to the user’s attention the possibility that there might be a problem. For example, an accessibility checker might produce a result with the message "Do not use color alone to highlight important information," with kind = "review". A user might address this issue by visually inspecting the UI.
·         "fail": The result represents a problem whose severity is specified by the level property (§3.27.10).

The problem is that the HDF specification (for lack thereof) is not clear regarding what the values of status mean - what does a status of skipped mean in HDF? I'm assuming, based on the behavior of https://github.com/mitre/saf when it converted XCCDF to HDF, that HDF's status of skipped has the same meaning as the SARIF level of notApplicable.

aaronlippold commented 1 year ago

skipped in HDF is the same as review from the above definition and notApplicable is the same in both. We don't really have an informational status in HDF at the moment but I would map that to NA at the moment given its statement of because it does not apply to the analysis target.

aaronlippold commented 1 year ago

In OHDF:

skipped - no tests were run - aka we have no results data so human review is required.
not applicable - the test results do not apply to the analysis target regardless of pass, fail or skipped results
pass - means the test met the expectation
failed - means the test failed the expectation
error - means the test threw a error, stack-trace and or did not execute as expected.

aaronlippold commented 1 year ago

These should be defined in the schema files

candrews commented 1 year ago

In OHDF:

skipped - no tests were run - aka we have no results data so human review is required.

not applicable - the test results do not apply to the analysis target regardless of pass, fail or skipped results

pass - means the test met the expectation

failed - means the test failed the expectation

error - means the test threw a error, stack-trace and or did not execute as expected.

This project only knows about these HDF status values: error failed passed skipped https://github.com/microsoft/sarif-sdk/blob/5b729309f4a5c006e331ebe9db3ed96f35ae5d40/src/Sarif.Converters/HdfModel/ControlResultStatus.cs#L6

The mitre/heimdall2 library similarly only knows about that same list of statuses, too: https://github.com/mitre/heimdall2/blob/351f4cf6db8fcb265810da5f3d21607064aaebd0/libs/inspecjs/src/generated_parsers/v_1_0/exec-json.ts#L299

As far as I can tell, nothing is aware of or uses the "OHDF" not_applicable status.

aaronlippold commented 1 year ago

Not applicable is calculated via the impact as part of the consolidated results.

When the impact of the result is set to zero the status is set to not applicable

Amndeep7 commented 1 year ago

they might need to leverage inspecjs and the contextualizedexecution files in order to extract out all the necessary information. see the reverse converters in hdf-converters: https://github.com/mitre/heimdall2/blob/master/libs/hdf-converters/src/converters-from-hdf/asff/reverse-asff-mapper.ts#L150

candrews commented 1 year ago

Not applicable is calculated via the impact as part of the consolidated results.

When the impact of the result is set to zero the status is set to not applicable

That's a simple and clear bit of logic - I can implement that :)

they might need to leverage inspecjs and the contextualizedexecution files

That is not clear to me... what benefit would that provide and what exactly is suggested? Is the aforementioned logic insufficient?

Amndeep7 commented 1 year ago

what benefit

the logic is already implemented (https://github.com/mitre/heimdall2/blob/master/libs/inspecjs/src/compat_impl/compat_inspec_1_0.ts#L234) and is available for you to just access (https://github.com/mitre/heimdall2/blob/master/libs/inspecjs/src/compat_impl/compat_inspec_1_0.ts#L212)

candrews commented 1 year ago

@aaronlippold @Amndeep7 heimdall doesn't currently map XCCDF rule result of notselected to HDF impact of 0. Here's a PR that fixes that: https://github.com/mitre/heimdall2/pull/4758

And here's a PR to this project that maps HDF impact of 0 to SARIF kind of NotApplicable: https://github.com/microsoft/sarif-sdk/pull/2701

candrews commented 1 year ago

This issue has been resolved as of version 4.2.1.

microsoft / sarif-sdk

HDF->SARIF converter output doesn't map status to kind/level/rank correctly #2695