Skip to content

Conversation

tamasmak
Copy link
Contributor

@tamasmak tamasmak commented Dec 6, 2023

AppSec Kit handles and shows the found vulnerabilities per dependency. It can happen that two dependencies are affected by the same vulnerability:
Screenshot 2023-12-07 at 10 42 36

In cases like this if the security team wants to provide separate analysis for the two vulnerabilities then the current logic to get the analysis is not correct. Currently, we get an analysis based only on the vulnerability id and after we check if the affected dependency's parent's group, name and version match the ones provided in the analysis assessment.

We need to extend the logic with checking the group, name and version when we get the analysis to be able to choose the right analysis for the given vulnerability and dependency combination.

Dependencies and their parent:
Screenshot 2023-12-07 at 10 50 33
Same in the generated SBOM:
Screenshot 2023-12-07 at 10 44 59

@tamasmak tamasmak self-assigned this Dec 6, 2023
@tamasmak tamasmak requested a review from heruan December 6, 2023 17:35
Copy link
Member

@heruan heruan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we have an analysis of CVE-2023-6378 for logback-classic, but the application is affected by the same vulnerability and only depends on logback-core? Would the analysis be shown after this change?

@tamasmak
Copy link
Contributor Author

tamasmak commented Dec 8, 2023

What if we have an analysis of CVE-2023-6378 for logback-classic, but the application is affected by the same vulnerability and only depends on logback-core? Would the analysis be shown after this change?

No, because I realized that the current vulnerability analysis’ structure is using Map<String, CLASS> almost everywhere, so I think the current vulnerability analysis structure is not suitable to handle cases like this. The structure should be improved like this:
In VulnerabilityAnalysis we should store the VulnerabilityDetails in List
private Map<String, List<VulnerabilityDetails>> vulnerabilities = new HashMap<>();
instead of
private Map<String, VulnerabilityDetails> vulnerabilities = new HashMap<>();
then we could have a structure like this

{
  "vulnerabilities": {
    "CVE-2023-6378": [
      {
        "dependency": {
          "name": "ch.qos.logback:logback-classic",
          "affectedVersions": ["[,1.4.12)"]
        },
        "assessments": {
          "com.vaadin:vaadin-spring-boot-starter": {
            "affectedVersions": {
              "[24.3.0,]": {
                "status": "TRUE_POSITIVE",
                "comment": "Analysis in case of logback-classic"
              }
            }
          }
        }
      },
      {
        "dependency": {
          "name": "ch.qos.logback:logback-core",
          "affectedVersions": ["[,1.4.12)"]
        },
        "assessments": {
          "com.vaadin:vaadin-spring-boot-starter": {
            "affectedVersions": {
              "[24.3.0,]": {
                "status": "TRUE_POSITIVE",
                "comment": "Analysis in case of logback-core"
              }
            }
          }
        }
      }
    ]
  }
}

and provide analysis for both cases.
Note that this is a coincidence that the two affected dependencies have the same parent, it can also happen that two dependencies are affected by the same vulnerability but have a different parent.
But the main goal of this PR, to also check the group, name and version is still needed, just we need to change how we store the analysis.

@heruan
Copy link
Member

heruan commented Dec 8, 2023

We can change the analysis structure if we keep it compatible with older kit versions, e.g. by introducing a new property instead of changing the existing one. If we do this, do we still need this PR?

@heruan heruan added the question Further information is requested label Feb 27, 2024
@heruan heruan marked this pull request as draft February 27, 2024 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

question Further information is requested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants