Reimport: Special statuses should be respected from reports

[Maffooch]

When performing a reimport, the status of a finding from the report that is either false positive, out of scope, or risk accepted is being discarded. This is due to a flaw in the way findings are calculated for the close_old_findings function.

Let's say we have a finding in the database that is active. In the next reimport, the same finding is being reported form the tool as false positive. Reimport will do the following:

1. Store the list of original findings
2. Parse the findings
3. Find a match between this new false positive finding, and the existing active finding
4. Set the active finding to false positive, and save the finding to the database
5. Determine the findings to be closed by subtracting the findings from the report from the original list of findings from step 1.
   • This is just the original active finding (though the record is stale since it was updated in step 4.)
6. Close the active finding by setting the mitigated status
7. Find out that the false positive status was discarded

This situation is caused from attempting to close a stale record, that may or may not already be closed, and therefore overwrite any other statuses. The solution here is simple: fetch the latest status on the finding before deciding if it needs to be closed.

[sc-10908]

[dryrunsecurity]

🟡 Please give this pull request extra attention during review.

This pull request contains a potential mass assignment vulnerability in the Checkmarx One parser and highlights some SQL injection risks, with most findings being low-confidence observations about parsing logic and state consistency that do not introduce direct security threats.

⚠️ Potential Mass Assignment Vulnerability in dojo/tools/checkmarx_one/parser.py

Vulnerability	Potential Mass Assignment Vulnerability
Description	The code uses mass assignment by using the **instance_details syntax to pass multiple attributes to the Finding model constructor, which could potentially include unintended or malicious attributes if instance_details is not properly validated or filtered

django-DefectDojo/dojo/tools/checkmarx_one/parser.py

Lines 175 to 189 in 796ee74

	date = self._parse_date(instance.get("firstFoundDate"))
	else:
	date = self._parse_date(instance.get("foundDate"))
	instance_details = self.determine_state(instance)
	instance_details.update(base_finding_details)
	# Create the finding object
	finding = Finding(
	severity=instance.get("severity").title(),
	date=date,
	file_path=instance.get("destinationFileName"),
	line=instance.get("destinationLine"),
	**instance_details,
	)
	# Add some details to the description
	if node_snippet := get_node_snippet(instance.get("nodes", [])):

💭 Unconfirmed Findings (4)

Vulnerability	SQL Injection Vulnerabilities
Description	Critical severity (CWE-89) vulnerabilities found in WebGoat Java source code files. Involves untrusted user inputs directly concatenated into SQL queries without proper input validation or parameterization, potentially allowing arbitrary SQL syntax injection.

Vulnerability	Potential State Consistency Risks
Description	Identified in default_reimporter.py, these risks involve possible race conditions or stale data issues during finding reimport. Mitigation was added by refreshing specific database fields before processing.

Vulnerability	Parsing State Determination Considerations
Description	Found in checkmarx_one/parser.py, potential risks include external documentation reference changes, state determination based on string comparisons, and lack of input validation on state values.

Vulnerability	No Direct Security Vulnerabilities
Description	Most changes appear to be improvements in test coverage and parsing logic. No introduction of hardcoded credentials or sensitive information was detected.

---

All finding details can be found in the DryRun Security Dashboard.

[valentijnscholten]

When a report has findings that are mitigated since the previous import, a similar problem will occur. I don't think this PR solves that yet. A workaround for that was done in #12204.

I think the root cause is not that old data is being passed into close_old_findings. The root cause is that findings that are mitigated (including false positive, risk accepted or out of scope) are passed into close_old_findings.

The close_old_findings method is meant to close findings that are no longer in the report. Or at least that was how it was designed in the past. To achieve that, reimport keeps track of:

• new findings -> new_items
• reactivated findings -> reactivated_items
• untouched findings -> unchanged_items
• false_p/risk_accepted/out of scope findings -> not tracked <-- this is the root cause
• mitigated findings -> not tracked <-- similar root cause

The result is that too many findings are passed into close_old_findings:

django-DefectDojo/dojo/importers/default_reimporter.py

Line 238 in 3fd00f0

self.to_mitigate = (set(self.original_items) - set(self.reactivated_items) - set(self.unchanged_items))

Although the refresh solution may work for now, it might not work in the future if something changes in the reimport. The result of the refresh might also depend on the transaction isolation being used.

The refresh implementation also results in incorrect counts for the updated_count field and for the values passed into notification handler:

django-DefectDojo/dojo/importers/default_reimporter.py

Lines 128 to 138 in 3fd00f0

	updated_count = (
	len(closed_findings) + len(reactivated_findings) + len(new_findings)
	)
	self.notify_scan_added(
	self.test,
	updated_count,
	new_findings=new_findings,
	findings_reactivated=reactivated_findings,
	findings_mitigated=closed_findings,
	findings_untouched=untouched_findings,
	)

I think tracking mitigated findings and "special status findings" should be added so we can stick to the original design that close_old_findings only processes findings that are no longer in the report.

i.e.

self.to_mitigate = (set(self.original_items) - set(self.reactivated_items) - set(self.unchanged_items)) - set(self.mitigated_items)

Alternative the mitigated findings and "special status" findings can be tracked separately from each other.

The proposed tracking solution would additionally expose another error in the reimporter where findings are marked as false positive/risk accepted/out of scope without setting the mitigated and is_mitigated_fields:

django-DefectDojo/dojo/importers/default_reimporter.py

Lines 606 to 617 in 3fd00f0

	elif unsaved_finding.risk_accepted or unsaved_finding.false_p or unsaved_finding.out_of_scope:
	logger.debug("Reimported mitigated item matches a finding that is currently open, closing.")
	logger.debug(
	f"Closing: {existing_finding.id}: {existing_finding.title} "
	f"({existing_finding.component_name} - {existing_finding.component_version})",
	)
	existing_finding.risk_accepted = unsaved_finding.risk_accepted
	existing_finding.false_p = unsaved_finding.false_p
	existing_finding.out_of_scope = unsaved_finding.out_of_scope
	existing_finding.active = False
	if self.verified is not None:
	existing_finding.verified = self.verified

If the user chooses to uncheck the close old findings input parameter, the findings that are false positive/out of scope/risk accepted end up as active==False but also is_mitigated==False.

For findings that have no special status, but are mitigated, this is done better:

django-DefectDojo/dojo/importers/default_reimporter.py

Line 590 in 3fd00f0

if not (existing_finding.mitigated and existing_finding.is_mitigated):

[valentijnscholten]

I created a small draft PR that shows what it would look like. I didn't test or add any unit tests yet.

[valentijnscholten]

Turns out that by tracking the mitigated findings, we see that in some unit tests a findings get mitigated and reactivated within the same import. I guess for now it's OK to stick to the refresh solution as that solves the reported problem and doesn't seem to make things worse.

[mtesauro]

Approved