Merge pull request #4116 from jeremylong/issue-4112

Report CRITICAL (CVSSv3) or HIGH (CVSSv2) when highest-ranking CVSS-scored and unscored vulnerabilities are both present

core/src/main/java/org/owasp/dependencycheck/dependency/Vulnerability.java

@@ -29,6 +29,7 @@
 import org.apache.commons.lang3.builder.EqualsBuilder;
 import org.apache.commons.lang3.builder.HashCodeBuilder;
 import org.jetbrains.annotations.NotNull;
+import org.owasp.dependencycheck.utils.SeverityUtil;
 
 /**
  * Contains the information about a vulnerability.
@@ -416,19 +417,74 @@ public String toString() {
     }
 
     /**
-     * Compares two vulnerabilities.
+     * Compares two vulnerabilities.<br>
+     * Natural order of vulnerabilities is defined as decreasing in severity and alphabetically by name for equal severity.
+     * This way the most severe issues are listed first in a sorted list.
+     * <br>
+     * This uses a {@link #bestEffortSeverityLevelForSorting() best-effort ordering} for severity as the variety of sources do
+     * not guarantee a consistent availability of standardized severity scores. The bestEffort severity level estimation will use
+     * CVSSv3 baseScore for comparison when available on both sides. If any of the vulnerabilities does not have a CVSSv3 score
+     * the sort order may be off, but it will be consistent.
+     * <br>
+     * The ranking (high to low) of severity can be informally represented as
+     * {@code &lt;CVSSv3 critical> >> &lt;Unscored recognized critical> >>
+     *     &lt;Unscored unrecognized (assumed Critical)> >> &lt;Score-based comparison for high-or-lower scoring severities with
+     *     recognized unscored severities taking the lower bound of the comparable CVSSv3 range>  }
      *
      * @param o a vulnerability to be compared
      * @return a negative integer, zero, or a positive integer as this object is
-     * less than, equal to, or greater than the specified vulnerability
+     * less than , equal to, or greater than the specified vulnerability
+     * @see #bestEffortSeverityLevelForSorting()
      */
     @Override
     public int compareTo(@NotNull Vulnerability o) {
         return new CompareToBuilder()
+                .append(o.bestEffortSeverityLevelForSorting(), this.bestEffortSeverityLevelForSorting())
                 .append(this.name, o.name)
                 .toComparison();
     }
 
+    /**
+     * Compute a best-effort score for the severity of a vulnerability for the purpose of sorting.
+     *<br>
+     * Note that CVSSv2 and CVSSv3 scores are essentially uncomparable. For the purpose of sorting we nevertheless
+     * treat them comparable, with an exception for the 9.0-10.0 range. For that entire range CVSSv3 is scoring more severe than
+     * CVSSv2, so that the 'CRITICAL' severity is retained to be reported as the highest severity after sorting on
+     * descending severity.
+     *<br>
+     * For vulnerabilities not scored with a CVSS score we estimate a score from the severity text. For textual severities
+     * assumed or sementically confirmed to be of a critical nature we assign a value in between the highest CVSSv2 HIGH and
+     * the lowest CVSSv3 CRITICAL severity level.
+     *
+     * @see SeverityUtil#estimatedSortAdjustedCVSSv3(String)
+     * @see SeverityUtil#sortAdjustedCVSSv3BaseScore(float)
+     * @return A float value that allows for best-effort sorting on vulnerability severity
+     */
+    private float bestEffortSeverityLevelForSorting() {
+        if (this.cvssV3 != null) {
+            return SeverityUtil.sortAdjustedCVSSv3BaseScore(this.cvssV3.getBaseScore());
+        }
+        if (this.cvssV2 != null) {
+            return this.cvssV2.getScore();
+        }
+        return SeverityUtil.estimatedSortAdjustedCVSSv3(this.unscoredSeverity);
+    }
+
+    /**
+     * The report text to use for highest severity when this issue is ranked highest.
+     *
+     * @return The string to display in the report, clarifying for unrecognized unscored severities that critical is assumed.
+     */
+    public String getHighestSeverityText() {
+        if (this.cvssV3 != null) {
+            return this.cvssV3.getBaseSeverity();
+        }
+        if (this.cvssV2 != null) {
+            return this.cvssV2.getSeverity();
+        }
+        return SeverityUtil.unscoredToSeveritytext(this.unscoredSeverity);
+    }
+
     /**
      * Sets the CPE that caused this vulnerability to be flagged.
      *

core/src/main/java/org/owasp/dependencycheck/utils/SeverityUtil.java

@@ -17,13 +17,21 @@
  */
 package org.owasp.dependencycheck.utils;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 /**
  * Utility to estimate severity level scores.
  *
  * @author Jeremy Long
  */
 public final class SeverityUtil {
 
+    /**
+     * The logger.
+     */
+    private static final Logger LOGGER = LoggerFactory.getLogger(SeverityUtil.class);
+
     /**
      * Private constructor for a utility class.
      */
@@ -57,4 +65,138 @@ public static float estimateCvssV2(String severity) {
                 return 3.9f;
         }
     }
+
+    /**
+     * Converts a textual severity to the text that should be used to signal
+     * it in a report.
+     * @param severity The textual unscored severity
+     * @return The severity when properly recognized, otherwise the severity extended with a remark that it was not recognized and
+     *  assumed to represent a critical severity.
+     */
+    public static String unscoredToSeveritytext(final String severity) {
+        switch (Severity.forUnscored(severity)) {
+            case CRITICAL:
+            case HIGH:
+            case MEDIUM:
+            case LOW:
+            case INFO:
+                return severity;
+            case ASSUMED_CRITICAL:
+            default:
+                final String sevText;
+                if ("0.0".equals(severity)) {
+                    sevText = "Unknown";
+                } else {
+                    sevText = severity;
+                }
+                return sevText + " (not recognized; assumed to be critical)";
+        }
+    }
+
+    /**
+     * Creates an estimated sort-adjusted CVSSv3 score for an unscored textual severity.
+     * For recognized severities below critical it returns a value at the lower bound of the CVSSv3 baseScore for that severity.
+     * For recognized critical severities it returns a score in-between the upper bound of the HIGH CVSSv2 score and the lowest
+     * sort-adjusted CVSSv3 critical score, so that unscored critical vulnerabilties are ordered in between CRITICAL scored CVSSv3
+     * rated vulnerabilities and HIGH-scored CVSSv2 rated vulnerabilities.
+     * For unrecognized severities it returns a score in-between the top HIGH CVSSv2 score and the estimatedSortAdjustedCVSSv3
+     * score for an unscored severity recognized as critical, so that recognized critical will win over unrecognized severities
+     * while unrecognized severities are assumed to be of a critical nature.
+     *
+     * @param severity The textual severity, may be null
+     * @return A float that can be used to numerically sort vulnerabilities in approximated severity (highest float represents
+     * highest severity).
+     * @see #sortAdjustedCVSSv3BaseScore(float)
+     */
+    public static float estimatedSortAdjustedCVSSv3(final String severity) {
+        switch (Severity.forUnscored(severity)) {
+            case CRITICAL:
+                return 10.2f;
+            case HIGH:
+                return 7.0f;
+            case MEDIUM:
+                return 4.0f;
+            case LOW:
+                return 0.1f;
+            case INFO:
+                return 0.0f;
+            case ASSUMED_CRITICAL:
+            default:
+                SeverityUtil.LOGGER.debug("Unrecognized unscored textual severity: {}, assuming critical score as worst-case "
+                                           + "estimate for sorting",
+                                           severity);
+                return 10.1f;
+        }
+    }
+
+    /**
+     * Compute an adjusted CVSSv3 baseScore that ensures that CRITICAL CVSSv3 scores will win over HIGH CVSSv2 and CRITICAL
+     * unscored severities to allow for a best-effort sorting that enables the report to list a reliable 'highest severity'
+     * in the report.
+     *
+     * @param cvssV3BaseScore The cvssV3 baseScore severity of a vulnerability
+     * @return The cvssV3 baseScore, adjusted if necessary in order to guarantee that CVSSv3 CRITICAL scores will rate higher than
+     * CVSSv2 HIGH, unscored critical severities and unscored unrecognized severities (which are assumed for sorting to be of a
+     * critical nature)
+     * @see #estimatedSortAdjustedCVSSv3(String)
+     */
+    public static float sortAdjustedCVSSv3BaseScore(final float cvssV3BaseScore) {
+        if (cvssV3BaseScore >= 9.0f) {
+            return cvssV3BaseScore + 1.3f;
+        }
+        return cvssV3BaseScore;
+    }
+
+    /**
+     * An enum to translate unscored severity texts to a severity level of a defined set of severities.
+     * Allows for re-use of the text-to-severity mapping in multiple helper methods.
+     */
+    private enum Severity {
+        /**
+         * A severity level for textual values that should be regarded as accompanying a critical severity vulnerability
+         */
+        CRITICAL,
+        /**
+         * A severity level for textual values that are not recognized and therefor assumed to be accompanying a critical severity
+         * vulnerability
+         */
+        ASSUMED_CRITICAL,
+        /**
+         * A severity level for textual values that should be regarded as accompanying a high severity vulnerability
+         */
+        HIGH,
+        /**
+         * A severity level for textual values that should be regarded as accompanying a medium severity vulnerability
+         */
+        MEDIUM,
+        /**
+         * A severity level for textual values that should be regarded as accompanying a low severity vulnerability
+         */
+        LOW,
+        /**
+         * A severity level for textual values that should be regarded as accompanying a vulnerability of informational nature
+         */
+        INFO;
+
+        public static Severity forUnscored(String value) {
+            switch (value == null ? "none" : value.toLowerCase()) {
+            case "critical":
+                return CRITICAL;
+            case "high":
+                return HIGH;
+            case "moderate":
+            case "medium":
+                return MEDIUM;
+            case "info":
+            case "informational":
+                return INFO;
+            case "low":
+            case "unknown":
+            case "none":
+                return LOW;
+            default:
+                return ASSUMED_CRITICAL;
+            }
+        }
+    }
 }

core/src/main/resources/templates/htmlReport.vsl

@@ -760,26 +760,16 @@ Getting Help: <a href="https://github.com/jeremylong/DependencyCheck/issues" tar
                         #end</td>
                         #set($cveImpact=-1)
                         #set($cveSeverity="&nbsp;")
-                        #foreach($vuln in $dependency.getVulnerabilities())
+                        #if($dependency.getVulnerabilities().size()>0)
+                            #set($severestVuln=$dependency.getVulnerabilities(true).iterator().next())
                             ## yes - we are mixing v2 and v3...  no consistency in data so doing the best we can
-                            #if ($vuln.cvssV3)
-                                #if ($cveImpact<$vuln.cvssV3.baseScore)
-                                    #set($cveImpact=$vuln.cvssV3.baseScore)
-                                    #set($cveSeverity=$enc.html($vuln.cvssV3.baseSeverity))
-                                #end
-                            #elseif ($vuln.cvssV2)
-                                #if ($cveImpact<$vuln.cvssV2.score)
-                                    #set($cveImpact=$vuln.cvssV2.score)
-                                    #set($cveSeverity=$enc.html($vuln.cvssV2.severity))
-                                #end
-                            #end
-                            #if ($vuln.unscoredSeverity)
-                                #if($vuln.unscoredSeverity.equals("0.0"))
-                                    #set($cveSeverity="Unknown")
-                                #else
-                                    #set($cveSeverity=$enc.html($vuln.unscoredSeverity))
-                                #end
+                            ## with a set sorted approximately on descending severity
+                            #if ($severestVuln.cvssV3)
+                                #set($cveImpact=$severestVuln.cvssV3.baseScore)
+                            #elseif ($severestVuln.cvssV2)
+                                #set($cveImpact=$severestVuln.cvssV2.score)
                             #end
+                            #set($cveSeverity=$enc.html($severestVuln.highestSeverityText))
                         #end
                         #set($sortValue=$cveImpact*10)
                         <td data-sort-value="$sortValue">$cveSeverity</td>

core/src/test/java/org/owasp/dependencycheck/analyzer/RubyBundleAuditAnalyzerIT.java

@@ -157,9 +157,16 @@ public void testAddCriticalityToVulnerability() throws AnalysisException, Databa
                     "ruby/vulnerable/gems/sinatra/Gemfile.lock"));
             analyzer.analyze(result, engine);
             Dependency dependency = engine.getDependencies()[0];
-            Vulnerability vulnerability = dependency.getVulnerabilities(true).iterator().next();
-            assertEquals(5.0f, vulnerability.getCvssV2().getScore(), 0.0);
-
+            boolean found =false;
+            for (Vulnerability vulnerability : dependency.getVulnerabilities()) {
+                if ("CVE-2015-3225".equals(vulnerability.getName())) {
+                    found = true;
+                    // validate that the score is from NVD rather than translated from the Bundle Audit severity text
+                    assertEquals(5.0f, vulnerability.getCvssV2().getScore(), 0.0);
+                    break;
+                }
+            }
+            assertTrue("CVE-2015-3225 was not found among the vulnerabilities",found);
         } catch (InitializationException | DatabaseException | AnalysisException | UpdateException e) {
             LOGGER.warn("Exception setting up RubyBundleAuditAnalyzer. Make sure Ruby gem bundle-audit is installed. You may also need to set property \"analyzer.bundle.audit.path\".");
             Assume.assumeNoException("Exception setting up RubyBundleAuditAnalyzer; bundle audit may not be installed, or property \"analyzer.bundle.audit.path\" may not be set.", e);