Sonarqube Tests finding mitigations after running Reimport-scan

Homopatrol commented 3 years ago

Bug description I’ve come across a bug that causes DefectDojo to erroneously flag mitigations when reuploading a Sonarqube test scan.

Steps to reproduce I imported a sonar-report.txt to DefectDojo

As you can see in the report some errors reported twice however these relate to different components in the project – eg.

"<!DOCTYPE>" Declarations Should Appear Before "<html>" Tags

This initial upload provides the following results:

However, upon reimporting the same report by running the script again DefectDojo seems to find and mitigate 5 results despite the fact I have not changed the report.

Expected behavior I expected the reuploaded test to display the same results with no Mitgated findings.

This error also appears when reuploading Dependency Track and Dependency Check scans so is there an issue with the way DefectDojo is analysing the test results?

Deployment method (select with an X)

[X] k8s/OpenShift with Helm chart
[ ] Kubernetes
[ ] Docker
[ ] setup.bash / legacy-setup.bash

Environment information N/A

Sample scan files (optional) sonar-report.txt

valentijnscholten commented 3 years ago

yeah, I ran into a similar issue last week with Anchore scans. The import functionality was updated 1,5 years or so ago to use a hashcode based on configurable fields. But the reimport uses different logic to match existing findings, so it is causing mismatching and confusing behaviour. I don't think this can be fixed without a code change.

Homopatrol commented 3 years ago

@valentijnscholten Ah okay since this is known to happen with other scans to are you already looking into updating this? Furthermore which part of the code handles the import/repimport functionality so I could look futher into this issue?

valentijnscholten commented 3 years ago

dojo/api_v2/serializers.py and dojo/test/views.py, see also https://github.com/DefectDojo/django-DefectDojo/pull/3629

Homopatrol commented 3 years ago

@valentijnscholten Thanks that's very helpful. - Is this being developed futher to fix the other scanners by adding the correct parsers for each or is there a bigger work required?

ptrovatelli commented 3 years ago

Hello, The re-import matching code is very basic. it didn't evolve when the configurable deduplication was added (my bad) and it wasn't event coherent with legacy deduplication algorithm in the first place

legacy dedupe basically identifies findings by (see comments in settings.dist.py; i say "basically" because there are some more subtle conditions):
- static scanner: ['title', 'cwe', 'line', 'file_path', 'description']
- dynamic scanner: ['title', 'cwe', 'line', 'file_path', 'description', 'endpoints']
re-import identifies findings by : title, severity, numerical_severity (except a few parsers for which exceptions were hardcoded)
configurable deduplication (as detailed in the "readthedoc") :
- as it's name stands can be tuned as wanted through the hash_code which is itself completely configurable.
- Or it can use the "unique_id_from_tool" field in place of the hash_code.
- It's also possible to deduplicate on both the hash_code (functional identifier) and the unique_id_from_tool (technical identifier) in order to have find both technically or functionally similar findings

so I guess at this point if one wants a coherent re-import, they would need to configure the deduplication to match the re-import logic: title, severity, numerical_severity (except for the parsers that have changed this by code: Veracode, Arachni, Anchore). That is not ideal though.

I'll see if I can apply the deduplication configuration to the re-import feature to avoid those discrepancies

ptrovatelli commented 3 years ago

@Homopatrol can you re-post your sonar report please? the one provided has no findings. Thanks

damiencarol commented 3 years ago

Assigning this issue to @ptrovatelli as discussed in #2030

ptrovatelli commented 3 years ago

I've started working on something. still some tests to do and unit tests to update, but it should already fix most of the cases : https://github.com/DefectDojo/django-DefectDojo/pull/3753

Homopatrol commented 3 years ago

@ptrovatelli Yep sorry this one should be populated. - sonar-report.txt

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

valentijnscholten commented 3 years ago

@Homopatrol Can you try again with 2.0.0 or a version that includes #3753? It should be a lot better now.

Homopatrol commented 3 years ago

@valentijnscholten

Thanks, this does work now when uploading a Sonar test in v2.0.3

and re-uploading to v2.0.3 👍

However, I noticed when upgrading to v2.0.3 that a report I had previously uploaded in v1.14.0 was now flagging up mitigations and additional findings after being re-uploaded in v2.0.3.

Is this expected behavior when upgrading DefectDojo? or is there a way of ensuring integrity with reports and findings when upgrading versions?

valentijnscholten commented 3 years ago

Did you perform all the upgrade steps including recalculating all hash codes?

Homopatrol commented 3 years ago

Thank you, I ran ./manage.py dedupe --hash_code_only In my initializer pod and corrected the issue.

Following this I have amended my initializer-job.yaml so the initializer will perform this script every time there is an upgrade

      - name: initializer
        image: "{{ template "initializer.repository" . }}:{{ .Values.tag }}"
        imagePullPolicy: {{ .Values.imagePullPolicy }}
        {{- if .Values.securityContext.enabled }}
        securityContext:
          {{- toYaml .Values.securityContext.djangoSecurityContext | nindent 10 }}
        {{- end }}          
        command: ["/bin/sh", "-c"]
        args:
        {{- if .Release.IsUpgrade }}
          - >
            /entrypoint-initializer.sh &&
            python manage.py dedupe --hash_code_only
        {{ else }}
          - /entrypoint-initializer.sh
        {{- end }}

Is there a reason the current initializer-job.yaml does not perform this manage.py dedupe --hash_code_only script on every upgrade, if not could you implement this feature into the upstream initializer-job.yaml?

valentijnscholten commented 3 years ago

We discussed that but didn't make a decision. On larger installs it can run for a couple of hours, so it's easier to let the admin control it. Unless the admin doesn't read the upgrade instructions ... We could make it async maybe. Either way, the initializer script itself should be changed, not the k8s service command probably. Where is the {{.Release.IsUpgrade}} coming from in your example?

mtcolman commented 3 years ago

@valentijnscholten .Release.IsUpgrade is a built-in object in helm: https://helm.sh/docs/chart_template_guide/builtin_objects/

Release.Name: The release name Release.Namespace: The namespace to be released into (if the manifest doesn’t override) Release.IsUpgrade: This is set to true if the current operation is an upgrade or rollback. Release.IsInstall: This is set to true if the current operation is an install. Release.Revision: The revision number for this release. On install, this is 1, and it is incremented with each upgrade and rollback. Release.Service: The service that is rendering the present template. On Helm, this is always Helm.

ptrovatelli commented 2 years ago

@Homopatrol great! how about creating a PR with that evolution? to prevent unexpected recomputations, I think we should also add a parameter in the initializer's configuration (values.yaml) like "recomputeHashcodesUponUpgrade" defaulted to false so that ppl can activate it or not willingly

@valentijnscholten your thoughts?

thanks!

valentijnscholten commented 2 years ago

Good idea. Would be nice to have something similar in the docker-compose setup. But I don't think it has an IsUpgrade boolean built in, so it might require custom code in the initializer.

ptrovatelli commented 2 years ago

the problem with changing the initalizer script only is that it won't know whether we're doing an upgrade or not will it? or maybe automatically recompute if we have detected a model change. that would be better that being based on Release.IsUpgrade because it will trigger less often (won't trigger for upgrades that don't include a model change).

how about opening an issue first to discuss solutions ?

DefectDojo / django-DefectDojo

Sonarqube Tests finding mitigations after running Reimport-scan #3675