Do you think there are any changes that are needed for CSD.1?
From: email@example.com <firstname.lastname@example.org> On Behalf Of Larry Golding (Comcast)
Sent: Friday, June 8, 2018 9:02 AM
To: 'O'Neil, Yekaterina Tsipenyuk' <email@example.com>; 'Michael Fanning' <Michael.Fanning@microsoft.com>
Subject: RE: [sarif] Change bars for Issue #158 (result.correlationId)
I filed Issue #190: “Distinguish ‘tool-specific’ from ‘generic’ properties” for CSD.2:
Some properties, like
result.ruleId, can only be filled by the analysis tool (let's call them "tool-specific properties"). Others, like
result.fingerprints, can be filled by other components of the SARIF ecosystem, such as the result managements system (let's call them "generic properties").
Today, the spec says that the analysis tool "SHOULD NOT" fill
result.fingerprints, which is too strong a statement. We need to explain the concept of tool-specific vs. generic properties, and encourage tool vendors to fill only the tool-specific properties -- without making a normative statement. Perhaps we can explain this in a non-normative Appendix.
The whole idea is to make it as easy as possible for tools to emit valid SARIF, by minimizing the amount of work they have to do.
Michael, a couple of comments:
- You wrote “maybe we’re too draconian with SHALL NOT”. The spec says “SHOULD NOT,” not “SHALL NOT”. But Yekaterina argues that even that is confusing. I will file an issue to revisit the issue of distinguishing “tool-specific” from “generic” properties in CSD.2. It might be a non-normative Appendix.
- I agree with Yekaterina, I don’t think a hierarchical rule works here. She wrote “[the] majority of our dataflow rules, for example, are helper rules that don’t result in any finding on their own – they just help the analyzer do its job … only the sink rule corresponds to a ‘specific criterion of correctness verified by a static analysis tool’”. I don’t think it makes sense to qualify the id of the “sink rule” with the ids of the helper rules that helped the analyzer find the violation.”
Instead, I suggest that Fortify use the property bag to store the helper rule ids:
"ruleId": "xxxx", # The "sink" rule that identifies the “violation of correctness”
"fortify/helperRuleIds": [ "yyyy", "zzzz" ]
I am confused about #2. Do you mean use result.ruleId property as a hierarchical property? Can you give an example?
If you think you have a finished fingerprint that is extremely resilient, agree you should populate it. Perhaps we can clarify the spec text to note that the SHOULD NOT designation here is to discourage per-tool fingerprinting (in favor of standards/SARIF-driven fingerprinting, which uses a broad range of SARIF data in concert with partial fingerprints). I agree with Larry’s point that, in the main, SARIF producers NEED NOT populate it. But maybe we’re too draconian with SHALL NOT. If you think you can compute a finished fingerprint, why not? On the other hand, what we really want tools producers to do is to think about their specific partial fingerprints that derive from a tool’s particular understanding of things (and which excludes things like file locations which are ‘understood’ already in the format because we have placeholders for them).
Your item #2 sounds like a hierarchical tag to me, i.e., you would tag results with the various contributors to the finding, organized under a common root so that you can treat them as a set.
- Ok, understood, but it does feel like a re-phrase of some sort would be helpful.
- Yeah, we indeed don’t mean the same thing by the term “rule”. Not all of our rules result in a specific finding on their own. In fact, majority of our dataflow rules, for example, are helper rules that don’t result in any finding on their own – they just help the analyzer do its job. So, the dataflow finding is generated by a set of rules, and only the sink rule corresponds to a “specific criterion of correctness verified by a static analysis tool”.
Michael – please read this carefully to see if I’ve correctly conveyed your philosophy for analysis tool design.
1. First of all, don’t worry, you can indeed populate result.fingerprints. “SHOULD NOT” means “don’t do it unless you have a good reason,” and I guess your tool has a good reason 😊
The reason we said “SHOULD NOT” is related to a tool design philosophy that Michael has advocated. I’m not sure how explicitly we have discussed it. We want to make it as easy as possible for vendors to build SARIF-compliant tools. So we distinguish between properties that the analysis tool is “uniquely qualified” (my words) to fill, and properties that post-processors and the result management system (RMS) can fill, because they are not tool-specific. An example of a “non-tool-specific” property is run.baselineInstanceGuid. We wouldn’t want every tool to have to discover the baseline run, or to provide a command line argument to specify it.
Michael and I considered result.fingerprints to fall into the “non-tool-specific” category as well. The RMS can compute the fingerprint in the same way for all tools. A tool that has special fingerprinting requirements can use result.partialFingerprints to convey that information to the RMS.
The question as I see it is how to tell a tool vendor “Don’t worry! Someone else can fill out this field, so you don’t need to bother.” Perhaps saying that the tool SHOULD NOT fill the field isn’t the best way to convey that.
2. Multiple rule ids: I’d like to know more about this. It’s possible that we don’t mean the same thing by the word “rule”. SARIF’s Terminology section defines it as a “specific criterion for correctness verified by a static analysis tool.”
So from SARIF’s point of view, if a programming artifact violates more than one “criterion for correctness,” that means they are two distinct results. One criterion for correctness might be “Don’t use weak hash algorithms.” Another might be “Don’t use tainted data without sanitizing it”. SARIF wouldn’t put both of those in the same result, even if the same simulated execution trace (code flow) revealed both problems.
Could you give me an example of a single result (or “finding,” or “observation”) made by Fortify that violates more than one criterion for correctness?
Sorry for not bringing these up earlier, but I have a couple of comments:
- Regarding result.fingerprints property: the spec says that “A direct SARIF producer SHOULD NOT populate this property.” In that case, what should we do with our instance ids which are actually generated by the analyzer? I thought that this property is what we would use for them. On the other hand, the spec also says: “EXAMPLE: In this example, the producer has calculated a fingerprint using version 2 of a fingerprinting method it refers to as "contextRegionHash"”, implying that the producer does calculate the fingerprint.
- Regarding result.ruleId property: majority of Fortify results are produced with the help of more than one rule, so this really should be an array.
Normally I incorporate amendments adopted by the TC into the provisional draft without asking for further review. In the case of Issue #158 (Introduce result.correlationId and clarify purpose of result.fingerprints array), the changes were substantive enough that I wanted to show them to you explicitly. I’ve attached a change-barred version of the provisional draft that shows the changes I made based on the TC’s feedback.
I am going to merge these changes, along with the other changes we adopted today. But if you disagree with the way I incorporated the feedback on #158, now’s your chance to tell me.