Fix CVSS 4.0 score when a lower macrovector ties the current score - #21
Open
bjhijmans wants to merge 1 commit into
Open
Fix CVSS 4.0 score when a lower macrovector ties the current score#21bjhijmans wants to merge 1 commit into
bjhijmans wants to merge 1 commit into
Conversation
calculateMeanDistance() decided whether an equivalence class counted toward the mean-distance divisor with a truthy check on its available distance (`if ($availableDistance->eqN)`). Because Cvss4Distance defaults every field to 0.0, an EQ whose lower macrovector exists but scores identically to the current macrovector — a legitimate available distance of exactly 0.0 — was indistinguishable from an EQ with no lower macrovector at all, and was wrongly excluded from the divisor. Dropping such an EQ shrinks `existingLower`, inflates the mean distance, and biases the final score downward. For example, CVSS:4.0/AV:N/AC:L/AT:P/PR:H/UI:P/VC:L/VI:H/VA:L/SC:H/SI:H/SA:H/E:P/MAV:L/MAC:H/MAT:P/MPR:N/MUI:P scored 5.5 instead of 5.6 (EQ3 and EQ4 each have a valid lower macrovector at the same 5.7 score). Gate each EQ on whether its lower macrovector is defined (`!is_null($lowerValues[n])`) — mirroring the FIRST reference implementation — by threading the already-computed $lowerVectorValues into calculateMeanDistance(). The normalized-severity contributions still use $availableDistance and remain 0.0 in the tie case. Add the affected vector to the CvssTest data provider as a regression test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix CVSS 4.0 score bias when a lower macrovector ties the current score
Closes #20
Summary
CVSS 4.0 vectors can be scored ~0.1 too low whenever an equivalence class (EQ) has a defined lower macrovector that happens to score the same as the current macrovector. The bug is in
Cvss40Calculator::calculateMeanDistance().Reproduction
(https://www.first.org/cvss/calculator/4.0#CVSS:4.0/AV:N/AC:L/AT:P/PR:H/UI:P/VC:L/VI:H/VA:L/SC:H/SI:H/SA:H/E:P/MAV:L/MAC:H/MAT:P/MPR:N/MUI:P)
Root cause
The final score is
initialValue − meanDistance, where the mean distance sums per-EQ contributions and divides by the number of EQs that have a defined lower macrovector.calculateMeanDistance()selected which EQs count with a truthy check on the float available distance:Cvss4Distancetypes every field as a plain, non-nullablefloatdefaulting to0.0, andcalculateAvailableDistance()only overwrites a field when the lower macrovector exists. So two different states collapse to the same falsy0.0:0.0(should be included).Excluding the second case shrinks the divisor, inflates the mean distance, and biases the score downward.
For the vector above (macrovector
111110, initial5.7):211110121110111111111210111120Fix
Gate each EQ on whether its lower macrovector is defined (
!is_null($lowerValues[n])) instead of the truthiness of the distance, matching the FIRST reference. The$lowerVectorValuesarray — whose elements are?float(lookupMicroVector()returnsnullfor an undefined macrovector) — is passed intocalculateMeanDistance(). Normalized-severity contributions still use$availableDistanceand correctly remain0.0in the tie case, so only the divisor changes.Added a regression test for the affected string.