Skip to content

WIP: Include in Firehose models multipe CWEs #33

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.

Sign up for GitHub

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

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

WIP: Include in Firehose models multipe CWEs #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab59bbb
Add suport to multiples cwes
davidCarlos Mar 30, 2017
d3bbebe
Fix tests.
davidCarlos Mar 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/example-2.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@davidmalcolm davidmalcolm Mar 31, 2017

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.

<!-- Example of a report with a trace -->

Expand Down
2 changes: 1 addition & 1 deletion firehose.rng
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@davidmalcolm davidmalcolm Mar 31, 2017

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.

</optional>

Expand Down
55 changes: 45 additions & 10 deletions firehose/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ def from_json(self, jsonobj):
return jsonobj
if self.type == float:
return jsonobj
if self.type == list:
return jsonobj
return self.resolve_type().from_json(jsonobj)

def to_json(obj):
Expand Down Expand Up @@ -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

@davidmalcolm davidmalcolm Mar 31, 2017

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.

Attribute('location', 'Location'),
Attribute('message', 'Message'),
Expand All @@ -251,8 +253,15 @@ def __init__(self,
trace,
severity=None,
customfields=None):
cwes = []
if cwe is not None:
assert isinstance(cwe, int)
if not isinstance(cwe, int):
cwes = cwe
assert isinstance(cwes, list)
for cwe in cwes:
assert isinstance(cwe, int)
else:
assert isinstance(cwe, int)
if testid is not None:
assert isinstance(testid, _string_type)
assert isinstance(location, Location)
Expand All @@ -265,7 +274,7 @@ def __init__(self,
assert isinstance(severity, _string_type)
if customfields is not None:
assert isinstance(customfields, CustomFields)
self.cwe = cwe
self.cwe = cwes
self.testid = testid
self.location = location
self.message = message
Expand All @@ -277,8 +286,11 @@ def __init__(self,
@classmethod
def from_xml(cls, node):
cwe = node.get('cwe')
if cwe is not None:
cwe = int(cwe)
cwe_list = []
if cwe is not None and cwe is not "":
cwes = cwe.split(',')
for cwe in cwes:
cwe_list.append(int(cwe))
testid = node.get('test-id')
location = Location.from_xml(node.find('location'))
message = Message.from_xml(node.find('message'))
Expand All @@ -298,12 +310,18 @@ def from_xml(cls, node):
customfields = CustomFields.from_xml(customfields_node)
else:
customfields = None
return Issue(cwe, testid, location, message, notes, trace, severity, customfields)
return Issue(cwe_list, testid, location, message, notes, trace, severity, customfields)

def to_xml(self):
node = ET.Element('issue')
if self.cwe is not None:
node.set('cwe', str(self.cwe))
if isinstance(self.cwe, list):
cwe_list = ""
for cwe in self.cwe:
cwe_list += ',' + str(cwe)
node.set('cwe', str(cwe_list[1::]))
else:
node.set('cwe', str(self.cwe))
if self.testid is not None:
node.set('test-id', str(self.testid))
node.append(self.message.to_xml())
Expand Down Expand Up @@ -339,7 +357,9 @@ def diagnostic(filename, line, column, kind, msg):
% (self.location.file.givenpath,
self.location.function.name))
if self.cwe:
cwetext = ' [%s]' % self.get_cwe_str()
if isinstance(self.cwe, list):
cwetext = []
cwetext = self.get_cwe_str()
else:
cwetext = ''
diagnostic(filename=self.location.file.givenpath,
Expand Down Expand Up @@ -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

@davidmalcolm davidmalcolm Mar 31, 2017

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.

return 'CWE-%i' % self.cwe
if isinstance(self.cwe, list):
for cwe in self.cwe:
cwe_list_str.append('CWE-%i' % int(cwe))
else:
cwe_list_str.append('CWE-%i' % int(self.cwe))
return cwe_list_str


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

@davidmalcolm davidmalcolm Mar 31, 2017

Choose a reason for hiding this comment

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

Likewise.

if self.cwe is not None:
return 'http://cwe.mitre.org/data/definitions/%i.html' % self.cwe
if isinstance(self.cwe, list):
for cwe in self.cwe:
cwe_list_str.append('http://cwe.mitre.org/data/definitions/%i.html' % cwe)
return cwe_list_str
else:
cwe_list_str.append('http://cwe.mitre.org/data/definitions/%i.html' % self.cwe)
return cwe_list_str


class Failure(Result):
attrs = [Attribute('failureid', _string_type, nullable=True),
Expand Down
2 changes: 1 addition & 1 deletion tests/parsers/test_clanganalyzer_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def test_example_001(self):
self.assertEqual(len(a.results), 2)

w0 = a.results[0]
self.assertEqual(w0.cwe, None)
self.assertEqual(w0.cwe, [])
self.assertEqual(w0.testid, None)
self.assertEqual(w0.message.text,
"Value stored to 'ret' is never read")
Expand Down
2 changes: 1 addition & 1 deletion tests/parsers/test_cppcheck_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def test_example_001(self):
self.assertEqual(len(a.results), 7)
r0 = a.results[0]
self.assertIsInstance(r0, Issue)
self.assertEqual(r0.cwe, None)
self.assertEqual(r0.cwe, [])
self.assertEqual(r0.testid, 'uninitvar')
self.assertEqual(r0.message.text, 'Uninitialized variable: ret')
self.assertEqual(r0.notes, None)
Expand Down
2 changes: 1 addition & 1 deletion tests/parsers/test_gcc_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ def test_values_c(self):
issue = gcc.parse_warning(line, FUNC_NAME)

# Verify the metadata:
self.assertEqual(issue.cwe, None)
self.assertEqual(issue.cwe, [])
self.assertEqual(issue.testid, 'unused-result')
self.assertIsInstance(issue.location, Location)
self.assertIsInstance(issue.location.file, File)
Expand Down
24 changes: 12 additions & 12 deletions tests/test_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ def make_simple_analysis(self):
sut=None,
file_=None,
stats=None),
results=[Issue(cwe=None,
results=[Issue(cwe=[],
testid=None,
location=Location(file=File('foo.c', None),
function=None,
Expand All @@ -60,7 +60,7 @@ def make_complex_analysis(self):
file_=File(givenpath='foo.c',
abspath='/home/david/coding/foo.c'),
stats=Stats(wallclocktime=0.4)),
results=[Issue(cwe=681,
results=[Issue(cwe=[681],
testid='refcount-too-high',
location=Location(file=File(givenpath='foo.c',
abspath='/home/david/coding/foo.c'),
Expand Down Expand Up @@ -127,7 +127,7 @@ def test_creating_simple_analysis(self):
self.assertEqual(a.metadata.sut, None)
self.assertEqual(a.metadata.file_, None)
self.assertEqual(a.metadata.stats, None)
self.assertEqual(w.cwe, None)
self.assertEqual(w.cwe, [])
self.assertEqual(w.testid, None)
self.assertEqual(w.location.file.givenpath, 'foo.c')
self.assertEqual(w.location.file.abspath, None)
Expand All @@ -150,7 +150,7 @@ def test_creating_complex_analysis(self):
self.assertEqual(a.metadata.file_.givenpath, 'foo.c')
self.assertEqual(a.metadata.file_.abspath, '/home/david/coding/foo.c')
self.assertEqual(a.metadata.stats.wallclocktime, 0.4)
self.assertEqual(w.cwe, 681)
self.assertEqual(w.cwe, [681])
self.assertEqual(w.testid, 'refcount-too-high')
self.assertEqual(w.location.file.givenpath, 'foo.c')
self.assertEqual(w.location.file.abspath, '/home/david/coding/foo.c')
Expand Down Expand Up @@ -228,7 +228,7 @@ def test_example_2(self):
self.assertEqual(len(a.results), 1)
w = a.results[0]
self.assertIsInstance(w, Issue)
self.assertEqual(w.cwe, 401)
self.assertEqual(w.cwe, [401,200])
self.assertEqual(w.testid, 'refcount-too-high')
self.assertEqual(w.location.file.givenpath, 'examples/python-src-example.c')
self.assertEqual(w.location.file.abspath, None)
Expand Down Expand Up @@ -491,16 +491,16 @@ def compare_hashes(creator):
def test_cwe(self):
# Verify that the CWE methods are sane:
a, w = self.make_complex_analysis()
self.assertIsInstance(w.cwe, int)
self.assertEqual(w.get_cwe_str(), 'CWE-681')
self.assertIsInstance(w.cwe, list)
self.assertEqual(w.get_cwe_str(), ['CWE-681'])
self.assertEqual(w.get_cwe_url(),
'http://cwe.mitre.org/data/definitions/681.html')
['http://cwe.mitre.org/data/definitions/681.html'])

# Verify that they are sane for a warning without a CWE:
a, w = self.make_simple_analysis()
self.assertEqual(w.cwe, None)
self.assertEqual(w.get_cwe_str(), None)
self.assertEqual(w.get_cwe_url(), None)
self.assertEqual(w.cwe, [])
self.assertEqual(w.get_cwe_str(), [])
self.assertEqual(w.get_cwe_url(), [])

def test_fixup_paths(self):
# Verify that Report.fixup_files() can make paths absolute:
Expand Down Expand Up @@ -535,7 +535,7 @@ def test_gcc_output(self):
w.write_as_gcc_output(output)
self.assertMultiLineEqual(output.getvalue(),
("foo.c: In function 'bar':\n"
"foo.c:10:15: warning: something bad involving pointers [CWE-681]\n"
"foo.c:10:15: warning: something bad involving pointers['CWE-681']\n"
"here is some explanatory text\n"
"foo.c:7:12: note: first we do this\n"
"foo.c:8:10: note: then we do that\n"
Expand Down