🎉 Advance reimport to update fix_available field #12633

[manuel-sommer]

• I advanced fix_available to be updated within each reimport: Add "has fix" filter for CVE finding #12633 (comment)
• I added field fix_version to populate a dedicated fixed version in the header of a finding. This field can also be used to be updated equal to fix_available within the reimport. An example screenshot can be found here:

- I added anchore grype to populate these fields

#12633 (comment)

[manuel-sommer]

@valentijnscholten is this the right direction for updating fix_available in reimport?

[valentijnscholten]

Yes

[manuel-sommer]

Shall I target this against bugfix, or do we want a longer testphase and keep it against dev?

[dryrunsecurity]

🔴 Risk threshold exceeded.

This pull request makes multiple edits to sensitive files (models, templates, importers, and a DB migration) and raises two security concerns: a potential denial-of-service where anchore_grype may create a fix_version string exceeding the Finding.fix_version 100-char DB limit causing DataError on import, and a business-logic flaw where the re-importer can update fix_available/fix_version on existing findings without granular permission checks or explicit audit logging, allowing authorized users to misrepresent remediation status.

🔴 Configured Codepaths Edit in dojo/models.py

Vulnerability	Configured Codepaths Edit
Description	Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.

🔴 Configured Codepaths Edit in dojo/templates/dojo/view_finding.html

Vulnerability	Configured Codepaths Edit
Description	Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.

🔴 Configured Codepaths Edit in dojo/importers/default_reimporter.py

Vulnerability	Configured Codepaths Edit
Description	Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.

🔴 Configured Codepaths Edit in dojo/models.py

Vulnerability	Configured Codepaths Edit
Description	Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.

🔴 Configured Codepaths Edit in dojo/importers/default_reimporter.py

Vulnerability	Configured Codepaths Edit
Description	Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.

🔴 Configured Codepaths Edit in dojo/importers/default_reimporter.py

Vulnerability	Configured Codepaths Edit
Description	Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.

🔴 Configured Codepaths Edit in dojo/importers/default_reimporter.py

Vulnerability	Configured Codepaths Edit
Description	Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.

🔴 Configured Codepaths Edit in dojo/templates/dojo/view_finding.html

Vulnerability	Configured Codepaths Edit
Description	Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.

🔴 Configured Codepaths Edit in dojo/db_migrations/0243_finding_fix_version.py

Vulnerability	Configured Codepaths Edit
Description	Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.

🔴 Configured Codepaths Edit in dojo/models.py

Vulnerability	Configured Codepaths Edit
Description	Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.

🔴 Configured Codepaths Edit in dojo/importers/default_reimporter.py

Vulnerability	Configured Codepaths Edit
Description	Sensitive edits detected for this file. Sensitive file paths and allowed authors can be configured in .dryrunsecurity.yaml.

Denial of Service via DataError in dojo/tools/anchore_grype/parser.py

Vulnerability

Denial of Service via DataError

Description

The fix_version field in the Finding model has a max_length of 100 characters. The anchore_grype parser concatenates multiple fix versions into a comma-separated string for this field. If the combined length of these versions exceeds 100 characters, attempting to save the Finding object will result in a DataError at the database level. This error will cause the entire scan import process to fail, leading to a denial of service for the import functionality.

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

Lines 133 to 136 in b47805e

	fix_version = ", ".join(vuln_fix_versions)
	for fix_version in vuln_fix_versions:
	finding_mitigation += f"\n- {fix_version}"

Business Logic Flaw - Insufficient Controls on Re-imported Fields in dojo/importers/default_reimporter.py

Vulnerability

Business Logic Flaw - Insufficient Controls on Re-imported Fields

Description

The re-import process allows an authorized user to update the fix_available and fix_version fields of existing findings directly from an uploaded scan file. The code in process_matched_mitigated_finding and process_matched_active_finding applies these changes if the values differ, without additional granular permission checks specifically for these fields or explicit audit logging of these modifications. This allows an authorized user to potentially misrepresent the remediation status of vulnerabilities.

django-DefectDojo/dojo/importers/default_reimporter.py

Lines 482 to 484 in b47805e

	if existing_finding.fix_available != unsaved_finding.fix_available:
	existing_finding.fix_available = unsaved_finding.fix_available
	existing_finding.fix_version = unsaved_finding.fix_version

Business Logic Flaw in dojo/importers/default_reimporter.py

Vulnerability

Business Logic Flaw

Description

The re-import process allows an authorized user to update the fix_available and fix_version fields of existing findings based on the content of an uploaded scan file. While the re-import process itself requires specific permissions (e.g., 'dojo.add_finding' or 'dojo.change_finding'), there are no additional, granular permission checks specifically for modifying the fix_available or fix_version fields. Furthermore, the current implementation does not appear to generate explicit audit log entries for changes to these specific fields during a re-import. This means a user with re-import permissions could potentially manipulate these fields to misrepresent the remediation status of vulnerabilities, impacting reporting and compliance, without a clear audit trail for these specific changes.

django-DefectDojo/dojo/importers/default_reimporter.py

Lines 595 to 597 in b47805e

	if existing_finding.fix_available != unsaved_finding.fix_available:
	existing_finding.fix_available = unsaved_finding.fix_available
	existing_finding.fix_version = unsaved_finding.fix_version

We've notified @mtesauro.

---

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

[valentijnscholten]

The main reason fields are not updated during reimport is that there's a risk it overwrites fields that were changed by the user. At least that's what I think the reason is. And this could be an issue here are the user may have set the fix_available field to None or False explicity. On the other hand we want to make that if a fix has become available we update the finding. What are your thoughts on this @Maffooch @mtesauro

[manuel-sommer]

I mean that is what is requested also in the linked issue explicitly.

[valentijnscholten]

We've discussed this and since this is sort of a "status" field or "meta" field it's OK to let reimport update it. Two remarks:

• I think we would just always need to update it, so also the case where it goes from True to False (or to None)
• I think it would have to be updated for any existing finding, so all branches of process_matched_finding.

[Maffooch]

We've discussed this and since this is sort of a "status" field or "meta" field it's OK to let reimport update it. Two remarks:

• I think we would just always need to update it, so also the case where it goes from True to False (or to None)
• I think it would have to be updated for any existing finding, so all branches of process_matched_finding.

I agree with this approach

[manuel-sommer]

Please review @valentijnscholten

[manuel-sommer]

Another point @valentijnscholten :
I can also remove the mitigation field from the deduplication settings in general and update it as well. Also take a look here:
#13053
#13055
With these two PRs, all mitigation fields in use would be removed from HASHCODE_FIELDS_PER_SCANNER in settings.dist.py
This enables us to update the mitigation field also as soon as a fix_available changes. These two fields often correlate.

[valentijnscholten]

Maybe start with just the fix_available field first as we are all in agreement that is good change to make.

[manuel-sommer]

Could we release this for the next release on Monday @mtesauro ?

[manuel-sommer]

They're running now but are you sure you've committed all the migrations needed?

Yeah, I made an error while rebasing and had two migrations point to the same db migration. Since that rebase, these unit test errors persist.

[manuel-sommer]

Can you please write some unit tests for the change of state? Ideally every change made in the reimporter class should have coverage

done

[valentijnscholten]

They're running now but are you sure you've committed all the migrations needed?

Yeah, I made an error while rebasing and had two migrations point to the same db migration. Since that rebase, these unit test errors persist.

I synced the migration with the text change made earlier, now they are green.

[manuel-sommer]

Ah, ok. Thank you. :-)

[manuel-sommer]

A question here @Maffooch and @valentijnscholten : Shall I rename "fix_version" to "fix_description"?
Then, also static findings can be covered in a better way.

[valentijnscholten]

I'm afraid I'm not following, could you give some more context?

[manuel-sommer]

Dynamic Finding:
fix_version = 2.3.4

Static Finding:
fix_version = Do this to remediate the static finding.

Thus, fix_version is not really a version fix in static finding, or we rather point on the mitigation field in this regard or leave it open.

[valentijnscholten]

The fix_version should contain 1 or more versions in which the issue is fixed. If it is a code change or configuration change or something else which is not about updating to a newer version we should use the mitigation field (or sometimes it's part of the description)

[manuel-sommer]

The fix_version should contain 1 or more versions in which the issue is fixed. If it is a code change or configuration change or something else which is not about updating to a newer version we should use the mitigation field (or sometimes it's part of the description)

Then, @valentijnscholten I would suggest to remove it as only a small number of vuln scanners post multiple fixed versions. Also, we can put that information to mitigation

[valentijnscholten]

What do you want the remove? I think the fix_version field is useful to have as can be a column on reports/exports/dashboards.

[manuel-sommer]

Ok, sounds legid

[manuel-sommer]

fyi @kiblik