DefectDojo persistence provider error handling

🐞 Bug report

Describe the bug

Running a large-scale Amass scan, we noticed that the DefectDojo persistence provider was failing. With closer inspection, we found that DefectDojo crashes on importing some endpoints which it regards as invalid. These domain names generally include underscores.

The persistence provider then restarts and tries again (failing), and again, etc.. Each time it restarts, it creates a new duplicate DefectDojo test object.

On another note, since amass is officially supported by SCB, but not by the DefectDojo persistence provider (uses generic), is this issue something that SCB actively supports?

Steps To Reproduce

Run the DefectDojo persistence provider and import list of findings with an invalid domain name.

Expected behavior

The persistence provider should not crash.
The persistence provider should be as atomic as possible (preventing duplicate imports).
Discussion: how should DefectDojo and the persistence provider handle an improperly formatted generic findings?

System (please complete the following information):

secureCodeBox Version: 3.0.1
DefectDojo version 2.1.0

Screenshots / Logs

[30/Aug/2021 20:17:01] ERROR [django.request:224] Internal Server Error: /api/v2/reimport-scan/
Traceback (most recent call last):
  File "/app/./dojo/importers/reimporter/reimporter.py", line 162, in process_parsed_findings
    endpoint.clean()
  File "/app/./dojo/models.py", line 1218, in clean
    raise ValidationError(errors)
django.core.exceptions.ValidationError: ['Host "_invalid._host.com" has invalid format']
[...]

2021-08-30 20:14:21 INFO  VersionedEngagementsStrategy:191 - Using ProductType Id: 1
Exception in thread "main" org.springframework.web.client.HttpServerErrorException$InternalServerError: 500 Internal Server Error: [
<!doctype html>
<html lang="en">
<head>
  <title>Server Error (500)</title>
</head>
<body>
  <h1>Server Error (500)</h1><p></p>
</body>
</html>
]
	at org.springframework.web.client.HttpServerErrorException.create(HttpServerErrorException.java:100)
	at org.springframework.web.client.DefaultResponseErrorHandler.handleError(DefaultResponseErrorHandler.java:188)
	at org.springframework.web.client.DefaultResponseErrorHandler.handleError(DefaultResponseErrorHandler.java:125)
	at org.springframework.web.client.ResponseErrorHandler.handleError(ResponseErrorHandler.java:63)
	at org.springframework.web.client.RestTemplate.handleResponse(RestTemplate.java:819)
	at org.springframework.web.client.RestTemplate.doExecute(RestTemplate.java:777)
	at org.springframework.web.client.RestTemplate.execute(RestTemplate.java:711)
	at org.springframework.web.client.RestTemplate.exchange(RestTemplate.java:602)
	at io.securecodebox.persistence.defectdojo.service.ImportScanService.createFindings(ImportScanService.java:100)
	at io.securecodebox.persistence.defectdojo.service.ImportScanService.reimportScan(ImportScanService.java:117)
	at io.securecodebox.persistence.strategies.VersionedEngagementsStrategy.run(VersionedEngagementsStrategy.java:90)
	at io.securecodebox.persistence.DefectDojoPersistenceProvider.main(DefectDojoPersistenceProvider.java:42)

@EndPositive

@EndPositive thanks for the bug report.

We wanted to change our Findings Schema such that the location field is required and has to be a valid URL anyway. As DefectDojo uses the hyperlink parse method. I think that should fix the issue and make sure we do not run into it again. It might take some days as it will force us to adjust some scanners that do currently not produce valid URLs.

On another note, since amass is officially supported by SCB, but not by the DefectDojo persistence provider (uses generic), is this issue something that SCB actively supports?

I am not sure I understand correctly. We definetly want all our scanners to work with defectDojo even if the scanner is not supported there. If the error is on defectDojo side we can not guarantee that it can/will be fixed. Anyway we will definetly try :)

Discussion: how should DefectDojo and the persistence provider handle an improperly formatted generic findings?
My idea would be to skip the hook after the first try such that other hooks etc. can still run. Do you have any thoughts on this? @EndPositive

@EndPositive

We wanted to change our Findings Schema such that the location field is required and has to be a valid URL anyway. As DefectDojo uses the hyperlink parse method. I think that should fix the issue and make sure we do not run into it again. It might take some days as it will force us to adjust some scanners that do currently not produce valid URLs.

Awesome, that would indeed solve this specific issue. Maybe you could consider having the Parser-SDK remove improperly formatted findings (until all scanners have been updated), maybe under a feature flag.

I am not sure I understand correctly. We definetly want all our scanners to work with defectDojo even if the scanner is not supported there. If the error is on defectDojo side we can not guarantee that it can/will be fixed. Anyway we will definetly try :)

That was the direction of my question, yeah. I can imagine there are more scenarios where an issue should be solved in DefectDojo, but like this one, it may also be solved in SCB. It would be helpful if we can simply only create an issue here instead of users needing to figure out in which project the issue would belong.

My idea would be to skip the hook after the first try such that other hooks etc. can still run. Do you have any thoughts on this? @EndPositive

We are currently running the persistence provider as a ReadOnly hook, allowing other hooks to continue already. I'm not sure if a failing ReadAndWriteHook should be allowed to pass, since it severely changes the finding format which may be used in the ReadOnly hook Cascading Scans.

I'd prefer a more sophisticated solution to this problem than just accepting the failure. Maybe it's possible to create a new finding in DefectDojo reporting that the import has failed? Maybe you could even use DefectDojo's notification framework (I'm not sure if the API is exposed)? Would it be possible to remove tests for which an import has failed?

Awesome, that would indeed solve this specific issue. Maybe you could consider having the Parser-SDK remove improperly formatted findings (until all scanners have been updated), maybe under a feature flag.

We are worried such a feature might lead to silent errors. Also it is probably feasible and sensible to first change all the parsers to be in line with the findings schema again before we put them on the main branch.

It would be helpful if we can simply only create an issue here instead of users needing to figure out in which project the issue would belong.

This is fine with me if it appears to be related to the secureCodeBox.

I'd prefer a more sophisticated solution to this problem than just accepting the failure. Maybe it's possible to create a new finding in DefectDojo reporting that the import has failed? Maybe you could even use DefectDojo's notification framework (I'm not sure if the API is exposed)? Would it be possible to remove tests for which an import has failed?

I think the notification API is exposed so we could potentially do this. Also I think the tests could be removed.

@EndPositive

@EndPositive seems that _invalid._host.com is a valid URL host and defectDojo will adjust their Regex (which they use in addition to the earlier mentioned hyperlink parse method) so these hosts will not produce errors in the future DefectDojo/django-DefectDojo#5049. We will work on validating our locations to be valid URLs and the error handling anyway.

Did you make the _invalid._host.com up to protect your data or was that host actually produced by amass?

@JohannesZahn

@JohannesZahn see my comment in DefectDojo's issue thread.

FYI DefectDojo/django-DefectDojo#5049 is merged.

We want to improve the printed error output in the DD persistence provider. But we schedule this for later sprint and therefore move it to backlog.

Whats left to do is to enhance the error logs of our defectdojo hook to print a helpful error message when requests to DefectDojo fail. This would help users to debug why the connection isn't working e.g. because their user doesn't have enough rights in DefectDojo.

Since we did lot ofrefactoringsince, I've digged into the old versions. Problem is in the "defectdojo-client-lib" (in io.securecodebox.persistence.defectdojo.service.ImportScanService#createFindings):

protected ImportScanResponse createFindings(ScanFile scanFile, String endpoint, long lead, String currentDate, ScanType scanType, long testType, MultiValueMap<String, Object> options) {
    ...    
   return restTemplate.exchange(defectDojoUrl + "/api/v2/" + endpoint + "/", HttpMethod.POST, payload, ImportScanResponse.class).getBody();
    } catch (HttpClientErrorException e) {
      throw new DefectDojoPersistenceException("Failed to attach findings to engagement.");
    }
}

Problem is that the causing Exception is completely ignored.

Will be fixed by secureCodeBox/defectdojo-client-java#127

Navigation Menu

Repository navigation

DefectDojo persistence provider error handling #618

🐞 Bug report

Describe the bug

Steps To Reproduce

Expected behavior

System (please complete the following information):

Screenshots / Logs

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

DefectDojo persistence provider error handling #618

Description

🐞 Bug report

Describe the bug

Steps To Reproduce

Expected behavior

System (please complete the following information):

Screenshots / Logs

Activity

JohannesZahn commented on Aug 31, 2021

EndPositive commented on Aug 31, 2021

JohannesZahn commented on Sep 3, 2021

JohannesZahn commented on Sep 7, 2021

EndPositive commented on Sep 7, 2021

damiencarol commented on Sep 8, 2021

Weltraumschaf commented on Feb 16, 2022

J12934 commented on Aug 30, 2023

Weltraumschaf commented on Jul 12, 2024

Weltraumschaf commented on Jul 12, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions