Skip to content
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.

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

feat: use location.message if available as the file name; use shortDescription if available as the category #10

Merged
merged 2 commits into from Oct 21, 2022

Conversation

candrews
Copy link
Contributor

@candrews candrews commented Oct 7, 2022

Having "Primary Location" display the same value for every row (such as /package.json or /pom.xml) is not very user friendly.

SARIF allows a "message" to be provided for a location and that can have more useful information. See:
https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317675

The result is that instead of showing many rows with "Primary Location" of "/pom.xml", now each row has a different value such as "Package: org.postgresql:postgresql@42.3.6" and "package: org.yaml:snakeyaml@1.30"

Signed-off-by: Craig Andrews candrews@integralblue.com

@candrews
Copy link
Contributor Author

candrews commented Oct 7, 2022

Currently, here's what's displayed by this plugin:
image

Compare that to what's displayed by Snyk's Fortify SSC plugin which implements a similar approach this PR:
image

@rsenden
Copy link
Collaborator

rsenden commented Oct 8, 2022

Thanks for your contribution; as stated in the other PR, I'll first need to check our company policies around 3rd-party PR's.

I do see one potential issue with this PR; usually SSC would use the primary location to display the actual location in the source code snippet when you expand the issue, for example displaying the actual line in pom.xml on which this issue was identified. If we change the primary location to something that cannot be resolved to a source file, then SSC won't be able to show the source code snippet.

So, maybe we should make your suggested behavior optional, for example based on a system property (as SSC doesn't provide any functionality for parser plugins to have configuration options). Maybe we should even provide make this configurable on a tool-by-tool basis, based on the SARIF tool property, i.e. have the system property list the tools for which this functionality should be enabled.

Thoughts?

@candrews
Copy link
Contributor Author

candrews commented Oct 8, 2022

I do see one potential issue with this PR; usually SSC would use the primary location to display the actual location in the source code snippet when you expand the issue, for example displaying the actual line in pom.xml on which this issue was identified.

I've only seen that behavior in Fortify SSC when view Fortify SCA / FPR findings. I didn't even know that was possible for anything else. Do you have any tips on things I can check to see why this viewing functionality isn't working for this plugin or for the Snyk's Fortify SSC plugin?

If we change the primary location to something that cannot be resolved to a source file, then SSC won't be able to show the source code snippet.

Is there something we can with StaticVulnerabilityBuilder so that Fortify SSC shows the "human friendly" location (ex, "org.postgres:postgres@42.3.6") but also knows the physical file location for use in the display of the source code snippet?

I really want to make the UI look like what Snyk's plugin produces as shown in the issue description. Having a whole bunch of rows all with the location set to "pom.xml" is just not workable for users.

So, maybe we should make your suggested behavior optional, for example based on a system property (as SSC doesn't provide any functionality for parser plugins to have configuration options).

I don't like the idea of using system properties for configuration since it would require modifications to the deployment of Fortify SSC (k8s, script, or bespoke), and any change to them requires Fortify SSC to be restarted (meaning downtime has to be scheduled, testing has to be done, etc) - it would be a very hard sell to the operations group.

Maybe we should even provide make this configurable on a tool-by-tool basis, based on the SARIF tool property, i.e. have the system property list the tools for which this functionality should be enabled.

I think altering behavior based on the SARIF file's tool property is going to lead to frustration and confusion. As a user, I would see vastly different behavior for SARIF produced by different tools - since SARIF is a standard, I'd expect all SARIF to behave the same. I think ideal behavior is based on the presence of standard SARIF fields (or at least quasi-standardized common properties).

@rsenden
Copy link
Collaborator

rsenden commented Oct 8, 2022

Never mind, I mixed up SSC views for standard Fortify vulnerabilities versus parser-provided vulnerabilities; the latter usually don't show source code snippets so probably the primary location doesn't need to match actual source file location after all. Once I get approval for merging 3rd-party PRs, I'll merge this one as well. Sorry for any confusion caused.

Having "Primary Location" display the same value for every row (such as `/package.json` or `/pom.xml`) is not very user friendly.

SARIF allows a "message" to be provided for a location and that can have
more useful information. See:
https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317675

The result is that instead of showing many rows with "Primary Location"
of "/pom.xml", now each row has a different value such as "Package:
org.postgresql:postgresql@42.3.6" and "package: org.yaml:snakeyaml@1.30"

Signed-off-by: Craig Andrews <candrews@integralblue.com>
From the SARIF specification regarding name:
> A reportingDescriptor object MAY contain a property named name whose value is a localizable string (§3.5.1) containing an identifier that is understandable to an end user. If the name of a rule contains implementation details that change over time, a tool author might alter a rule's name (while leaving the stable id property (§3.49.3) unchanged).
> NOTE: A rule name is suitable in contexts where a readable identifier is preferable and where the lack of stability is not a concern.
> EXAMPLE: "name": "SpecifyMarshalingForPInvokeStringArguments"

See: https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317843

The name isn't very human readable. And the same name is often used repeatedly, making Fortify SSC's table view have many rows with the same values, reducing usability.

Here's the definition of shortDescription:
> A reportingDescriptor object MAY contain a property named shortDescription whose value is a localizable multiformatMessageString object (§3.12, §3.12.2) that provides a concise description of the reporting item. The shortDescription property SHOULD be a single sentence that is understandable when visible space is limited to a single line of text.
>
> EXAMPLE:
> "Specify marshaling for P/Invoke string arguments."

See: https://docs.oasis-open.org/sarif/sarif/v2.1.0/os/sarif-v2.1.0-os.html#_Toc34317845

By using the shortDescription, if it is available, the Fortify SSC table view contains much more readable information that is less uselessly repetitive.

Signed-off-by: Craig Andrews <candrews@integralblue.com>
@candrews
Copy link
Contributor Author

I updated this PR to add a commit which will use shortDescription if available. I added to this PR for this change, rather than make a new PR, because this change builds off of a change to message resolution that I introduced here originally.

Please let me know if you need to separate out these changes or if is any other way I can facilitate getting these improvements merged.

Thanks again!

@candrews candrews changed the title feat: use location.message if available as the file name feat: use location.message if available as the file name; use shortDescription if available as the category Oct 11, 2022
@rsenden rsenden merged commit 5af746a into fortify:main Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants