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

WIP: Include in Firehose models multipe CWEs #33

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

davidCarlos
Copy link
Contributor

This PR allows include in Firehose's xml multiple CWEs related to a same warning. Now the cwe field can have more than one CWE, separated by comma. The Issue model expects a list of CWEs, but to keep compatibility with the existent parsers, pass a integer value still is possible.

@davidCarlos
Copy link
Contributor Author

Exemple with multiple CWEs.

firehose2

Copy link
Member

@davidmalcolm davidmalcolm left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this. As noted in the comments below, I'd prefer a somewhat different approach (sorry).

@@ -28,7 +28,7 @@
</metadata>

<results>
<issue cwe="401" test-id="refcount-too-high">
<issue cwe="401,200" test-id="refcount-too-high">

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for posting this.

Could you please add a fresh example e.g. taken from the flawfinder example you posted a screenshot of?

I'm not keen on having comma-separated values in the XML; I think it would be cleaner to introduce elements for these IDs, so that client code doesn't have to parse the attributes. Instead we should add a new zero-or-more child element to the XML schema.

We might want to support other categorization schemes - the readme.rst talks about:

potentially with other IDs and URLs, e.g. the ID "SIG30-C" with
URL https://www.securecoding.cert.org/confluence/display/seccode/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers
so maybe we want to generalize things accordingly.

@@ -121,7 +121,7 @@
http://cwe.mitre.org/data/definitions/131.html
-->
<attribute name="cwe">
<data type="integer"/>
<data type="string"/>
</attribute>
Copy link
Member

Choose a reason for hiding this comment

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

As noted above, this probably should be something like:

<zeroOrMore>
  <choice>
      <element name="cwe">
         <attribute name="cwe">
            <data type="integer"/>
         </attribute>
      </element>
      <element name="something-else"/>
  </choice>
<zeroOrMore>

or somesuch.

@@ -233,7 +235,7 @@ def from_json(cls, jsonobj):
raise TypeError('unknown type: %r' % jsonobj['type'])

class Issue(Result):
attrs = [Attribute('cwe', int, nullable=True),
attrs = [Attribute('cwe', list, nullable=True),
Attribute('testid', _string_type, nullable=True),
Copy link
Member

Choose a reason for hiding this comment

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

"cwe" should probably be replaced by "externalids" or somesuch, for external IDs. (Not sure of the name). It would be a list of instances of some base class, with CWE being a subclass.

@@ -379,12 +399,27 @@ def accept(self, visitor):
self.trace.accept(visitor)

def get_cwe_str(self):
cwe_list_str = []
if self.cwe is not None:
Copy link
Member

Choose a reason for hiding this comment

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

This would become a method of a new CWE class.


def get_cwe_url(self):
cwe_list_str = []
Copy link
Member

Choose a reason for hiding this comment

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

Likewise.

@davidmalcolm
Copy link
Member

Note that also I've recently added documentation, so any big change like this would need changes to the docs.

@davidCarlos
Copy link
Contributor Author

Thanks for the review. I will keep working on this PR based on your comments. In the case of PR #31, could we use only one CWE in the final xml for a while (This issue is bigger than i thought)? When we finish this PR we include this new feature in the parsers.

@davidmalcolm
Copy link
Member

Thanks. Clearly this needs more thought, let's come up with a design for how to handle this (maybe mention the issue on the mailing list?).

@davidCarlos davidCarlos changed the title Include in Firehose models multipe CWEs WIP: Include in Firehose models multipe CWEs Apr 5, 2017
@davidCarlos
Copy link
Contributor Author

Hey @davidmalcolm, i will work again on this PR. I will try to add a new model to support multiples cwes. Probably we will have to fix all the parsers that collects cwes.

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