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

[STAL-2643] Export taint analysis violations to SARIF #517

Merged
merged 5 commits into from
Oct 2, 2024

Conversation

jasonforal
Copy link
Collaborator

@jasonforal jasonforal commented Sep 27, 2024

Notes

What problem are you trying to solve?

Violations from our Java taint analysis implementation are represented by a flow of taint from a source variable to a sink method (and thus it has multiple "code regions" associated with it). This is distinctly different than a "standard" static analysis violation, which only has one code region.

So while we can detect and flag taint flow violations within rules, we can't currently export the flows via SARIF.

What is your solution?

codeFlows in SARIF

This PR modifies our SARIF output to include a "codeFlows" field in an analysis result.

The codeFlows property is intended for use by analysis tools that provide execution path details that illustrate a possible problem in the code.

You can see example JSON output here.

JavaScript Violation API

As part of this, the Violation class constructor has been changed to allow for more permissive passing in of objects/parameters to be interpreted as code regions in violations.

Before, a violation had to be created like

const v = new Violation(node.start.line, node.start.col, node.end.line, node.end.col, "message here");

Now, the constructor accepts a string message and is variadic for the rest of the properties. All of the following now work (and are equivalent in terms of the resulting region):

// "old" style
const v = new Violation("message here", node.start.line, node.start.col, node.end.line, node.end.col);
// Passing in a TreeSitterNode
const v = new Violation("message here", node);
// Passing in positions
const v = new Violation("message here", node.start, { line: node.end + 1, col: node.col });

Additionally, a TaintFlow can be passed in:

const flows = ddsa.getTaintSources(node);
const flow = flows[0];
const v = new Violation("message here", flow);

Note that this doesn't affect the old stella buildError API.

Small details:

  • A taint flow's fingerprint is a combination of all of the code regions it has in the flow. This uses the same methodology as our existing fingerprint.
  • A taint flow is ignored if any of its regions would be ignored (regardless of which specific line has the "base" violation).

Alternatives considered

What the reviewer should know

@jasonforal jasonforal requested a review from a team as a code owner September 27, 2024 22:07
@jasonforal jasonforal requested review from dastrong and juli1 and removed request for dastrong September 27, 2024 22:07
Copy link
Collaborator

@juli1 juli1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewing the code, it seems to replace the existing mechanism to handle position to annotate and integrate this with data flow. we should have a position that is used for annotation. and if there is data flow, we add this information on top. we should not merge these two concepts.

Copy link
Collaborator

@juli1 juli1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we at least document the need for PhantomData?

@jasonforal
Copy link
Collaborator Author

Sure, I'll add intra-doc links

Base automatically changed from jf/STAL-2713 to main October 2, 2024 20:54
@jasonforal jasonforal merged commit 139add5 into main Oct 2, 2024
70 checks passed
@jasonforal jasonforal deleted the jf/STAL-2643-2 branch October 2, 2024 21:31
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.

2 participants