ZAP import scan results failing with 'NoneType' object has no attribute 'group'

professorgrumps commented 7 years ago

It appears that if a ZAP xml report is generated with a uri that is not fully qualified, an exception is thrown upon importing it to DefectDojo.

More generally it appears this regex shown in the screenshot showing the stack trace may be too restrictive? Just guessing.

I am using the latest docker image of defect-dojo (appsecpipeline/django-defectdojo)

untitled

I have attached 2 scrubbed ZAP report files showing the problem:

'doesnotwork.xml': this is the raw XML file, scrubbed for privacy. Importing this fails with the attached screenshot
'works.xml': I have manually added 'fake.com' to the uri field in the XML file, and this is imported successfully

the only difference between the two are line 12:

in 'doesnotwork': <uri>http://scrubbed/auth/signOut</uri>

in 'works': <uri>http://scrubbed.fake.com/auth/signOut</uri>

To reproduce the problem:

attempt to import the 'doesnotwork.xml' file as ZAP scan file, get the attached screenshot.

And heres the copy-pasta of the stack, scrubbed a bit.

Request Method: POST
Request URL: ..../engagement/8/import_scan_results

Django Version: 1.8.10
Python Version: 2.7.10
Installed Applications:
('django.contrib.auth',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.sites',
 'django.contrib.messages',
 'django.contrib.staticfiles',
 'polymorphic',
 'overextends',
 'django.contrib.admin',
 'django.contrib.humanize',
 'gunicorn',
 'tastypie',
 'djangobower',
 'auditlog',
 'dojo',
 'tastypie_swagger',
 'watson',
 'tagging',
 'custom_field',
 'imagekit')
Installed Middleware:
('django.middleware.common.CommonMiddleware',
 'django.contrib.sessions.middleware.SessionMiddleware',
 'django.middleware.csrf.CsrfViewMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware',
 'django.contrib.messages.middleware.MessageMiddleware',
 'django.middleware.clickjacking.XFrameOptionsMiddleware',
 'dojo.middleware.LoginR
[evidence.zip](https://github.com/OWASP/django-DefectDojo/files/688140/evidence.zip)
equiredMiddleware')

Traceback:
File "/django-DefectDojo/venv/local/lib/python2.7/site-packages/django/core/handlers/base.py" in get_response
  132.                     response = wrapped_callback(request, *callback_args, **callback_kwargs)
File "/django-DefectDojo/venv/local/lib/python2.7/site-packages/django/contrib/auth/decorators.py" in _wrapped_view
  22.                 return view_func(request, *args, **kwargs)
File "/django-DefectDojo/dojo/engagement/views.py" in import_scan_results
  270.                 parser = import_parser_factory(file, t)
File "/django-DefectDojo/dojo/tools/factory.py" in import_parser_factory
  33.         parser = ZapXmlParser(file, test)
File "/django-DefectDojo/dojo/tools/zap/parser.py" in __init__
  33.             self.items = self.get_items(tree, test)
File "/django-DefectDojo/dojo/tools/zap/parser.py" in get_items
  60.             site = Site(node)
File "/django-DefectDojo/dojo/tools/zap/parser.py" in __init__
  137.             self.items.append(Item(alert))
File "/django-DefectDojo/dojo/tools/zap/parser.py" in __init__
  201.             protocol = mregex.group(1)

Exception Type: AttributeError at /engagement/8/import_scan_results
Exception Value: 'NoneType' object has no attribute 'group'

Here are the xml files in-line

works.xml

<?xml version="1.0"?><OWASPZAPReport version="2.5.0" generated="Thu, 5 Jan 2017 16:43:07">
<site name="http://scrubbed" host="scrubbed" port="80" ssl="false"><alerts><alertitem>
  <pluginid>10010</pluginid>
  <alert>Cookie No HttpOnly Flag</alert>
  <name>Cookie No HttpOnly Flag</name>
  <riskcode>1</riskcode>
  <confidence>2</confidence>
  <riskdesc>Low (Medium)</riskdesc>
  <desc>&lt;p&gt;A cookie has been set without the HttpOnly flag, which means that the cookie can be accessed by JavaScript. If a malicious script can be run on this page then the cookie will be accessible and can be transmitted to another site. If this is a session cookie then session hijacking may be possible.&lt;/p&gt;</desc>
  <instances>
  <instance>
  <uri>http://scrubbed.fake.com/auth/signOut</uri>
  <param>scrubbed</param>
  <evidence>Set-Cookie: scrubbed</evidence>
  </instance>
  </instances>
  <count>1</count>
  <solution>&lt;p&gt;Ensure that the HttpOnly flag is set for all cookies.&lt;/p&gt;</solution>
  <reference>&lt;p&gt;http://www.owasp.org/index.php/HttpOnly&lt;/p&gt;</reference>
  <cweid>16</cweid>
  <wascid>13</wascid>
</alertitem>
</alerts></site>
</OWASPZAPReport>

doesntwork.xml

<?xml version="1.0"?><OWASPZAPReport version="2.5.0" generated="Thu, 5 Jan 2017 16:43:07">
<site name="http://scrubbed" host="scrubbed" port="80" ssl="false"><alerts><alertitem>
  <pluginid>10010</pluginid>
  <alert>Cookie No HttpOnly Flag</alert>
  <name>Cookie No HttpOnly Flag</name>
  <riskcode>1</riskcode>
  <confidence>2</confidence>
  <riskdesc>Low (Medium)</riskdesc>
  <desc>&lt;p&gt;A cookie has been set without the HttpOnly flag, which means that the cookie can be accessed by JavaScript. If a malicious script can be run on this page then the cookie will be accessible and can be transmitted to another site. If this is a session cookie then session hijacking may be possible.&lt;/p&gt;</desc>
  <instances>
  <instance>
  <uri>http://scrubbed/auth/signOut</uri>
  <param>scrubbed</param>
  <evidence>Set-Cookie: scrubbed</evidence>
  </instance>
  </instances>
  <count>1</count>
  <solution>&lt;p&gt;Ensure that the HttpOnly flag is set for all cookies.&lt;/p&gt;</solution>
  <reference>&lt;p&gt;http://www.owasp.org/index.php/HttpOnly&lt;/p&gt;</reference>
  <cweid>16</cweid>
  <wascid>13</wascid>
</alertitem>
</alerts></site>
</OWASPZAPReport>

Thanks in advance!

devGregA commented 7 years ago

Hi @professorgrumps, thank you for the detailed submission. @grendel513 @aaronweaver this is an interesting one. We could set endpoint to null if the url isn't correctly qualified. Not sure it would be a good idea to store an unqualified url with the qualified endpoints? any thoughts or input? otherwise I'll just not create an endpoint reference for this situation.

grendel513 commented 7 years ago

Agree with your assessment, thanks for looking into it.

Jay

On Thu, Jan 5, 2017 at 2:35 PM, Greg Anderson notifications@github.com wrote:

Hi @professorgrumps https://github.com/professorgrumps, thank you for the detailed submission. @grendel513 https://github.com/grendel513 @aaronweaver https://github.com/aaronweaver this is an interesting one. We could set endpoint to null if the url isn't correctly qualified. Not sure it would be a good idea to store an unqualified url with the qualified endpoints? any thoughts or input? otherwise I'll just not create an endpoint reference for this situation.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/OWASP/django-DefectDojo/issues/183#issuecomment-270750150, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOQ3ruDyvsTzYHwT53CpECkflGVopGyks5rPVQNgaJpZM4LcB2U .

professorgrumps commented 7 years ago

Thanks for the response @devGregA @grendel513 - just a clarification on our use case: we plan to use ZAP reports that are run against internal test hosts whose hostname may infact not be FQ'ed. For example, we may have a shared test instance of an application that internally is resolved by 'http://mytestapp/'

Therefore, we will expect our ZAP reports to have unqualified URLs / URIs identified by a scan.
Hence, would it not make sense to have 'mytestapp' show up as a legit endpoint when imported by defect-dojo?

Hope this makes sense, and excuse my ignorance in the above - just started playing with defect-dojo and I'm excited about the problem it could potentially solve for us!

devGregA commented 7 years ago

Hi @professorgrumps haven't forgotten about this just still trying to figure out how to make the use case work.

professorgrumps commented 7 years ago

Sorry for the delay / thanks for the update @devGregA , looking forward to suggestions / resolutions!

grendel513 commented 7 years ago

@devGregA - We could have a setting that allows unqualified/invalid URLs. I don't think we should, by default accept invalid URLs.

devGregA commented 7 years ago

@grendel513 - agreed! Would require a completely separate parser for endpoints would it not?

devGregA commented 7 years ago

@grendel513 is the URL displayed on a finding if it doesn't match the endpoint? I don't believe so? I wonder if we did a if endpoint == None: display URL. If that would resolve this issue. I do not think we should store non-FQDN endpoints due to plans with scan integration 2.0.

devGregA commented 7 years ago

Hi everyone, we have discussed this at length and will not be able to support non-FQDN endpoints because we would break our database schema. Endpoints are stored as FQDN's broken into their proper parts / labels. This construction is used in a variety of different views that would have to be altered to accommodate a non-FQDN model. I Wish I had better news :(. I will push an update that handles this case and has an appropriate error message. I'm pretty sure I know @grendel513 's feelings regarding this @aaronweaver @mtesauro please let me know if you feel strongly otherwise.

devGregA commented 7 years ago

@professorgrumps also just wanted to highlight that I greatly appreciate the detailed files / steps you provided to reproduce the issue. I'm sorry I couldn't provide a better outcome.

aaronweaver commented 7 years ago

@devGregA Would it be worth adding an option to tag on a local domain suffix? For example in the global settings or on the import level have an option to tack on a suffix such as corp.local.

Example: If validation for endpoint fails tack on corp.local. qaserver01 --> qaserver01.corp.local

professorgrumps commented 7 years ago

@devGregA @aaronweaver @grendel513 thanks a bunch for the effort, and sorry for the delay in my response.

For our use case and completeness... we have both a suitable workaround as well as a bit of "might have been holding it wrong" - after updating ZAP, then running our acceptance tests thru it and importing the raw export into DDJ as described above... turns our ZAP is now reporting the FQDN in the export. Its also possible our tests were update to explicitly use FQDNs. Either way, its working for us now!

I also see the reason for not fully supporting non-FQDN from DDJ's point of view - certainly makes sense.

Thanks again!

DefectDojo / django-DefectDojo

ZAP import scan results failing with 'NoneType' object has no attribute 'group' #183