Closed Thomas-Alex closed 5 years ago
Hello AT, just a possible minor nit directly inline:
AT writes:
[...]
- Updated parser to handle CVSS:3.1 version identifier in vector strings [...]
| --- a/cvss/parser.py | +++ b/cvss/parser.py | @@ -18,14 +18,14 @@ def parse_cvss_from_text(text): | A list of CVSS objects. | """ | # Looks for substrings which resemble CVSS2 or CVSS3 vectors. | - # CVSS3 vector starts with 'CVSS:3.0/' prefix - matched by non-capturing group. | + # CVSS3 vector starts with 'CVSS:3.0/' or 'CVSS:3.1/' prefix - matched by non-capturing group. | # Minimum vector length is 26. | - matches = re.compile(r'(?:CVSS:3.0/)?[A-Za-z:/]{26,}').findall(text) | + matches = re.compile(r'(?:CVSS:3.[0|1]/)?[A-Za-z:/]{26,}').findall(text)
This will also patch against `CVSS:3.|', is it intended or should the RE be adjusted to:
r'(?:CVSS:3.[01]/)?[A-Za-z:/]{26,}'
Hello AT, just a possible minor nit directly inline: AT writes: [...] * Updated parser to handle CVSS:3.1 version identifier in vector strings [...] | --- a/cvss/parser.py | +++ b/cvss/parser.py | @@ -18,14 +18,14 @@ def parse_cvss_from_text(text): | A list of CVSS objects. | """ | # Looks for substrings which resemble CVSS2 or CVSS3 vectors. | - # CVSS3 vector starts with 'CVSS:3.0/' prefix - matched by non-capturing group. | + # CVSS3 vector starts with 'CVSS:3.0/' or 'CVSS:3.1/' prefix - matched by non-capturing group. | # Minimum vector length is 26. | - matches = re.compile(r'(?:CVSS:3.0/)?[A-Za-z:/]{26,}').findall(text) | + matches = re.compile(r'(?:CVSS:3.[0|1]/)?[A-Za-z:/]{26,}').findall(text) This will also patch against `CVSS:3.|', is it intended or should the RE be adjusted to: r'(?:CVSS:3.[01]/)?[A-Za-z:/]{26,}'
You're right. Updated per your suggestion. Thanks!
Thanks for your contribution!
Unfortunately, I cannot use it as is for the following reasons:
I am planning to implement CVSS 3.1 changes next week as well as update thousands of tests to match the reference implementation.
Thanks for your contribution!
Unfortunately, I cannot use it as is for the following reasons:
* The implementation needs to be backwards compatible with CVSS 3.0 and as far as I see modified_isc computation is different. * I was involved with the design of the recommended rounding formula and as far as I know it should match perfectly the previous implementation using Decimal. Its intention was to get the same results in languages which do not have exact arithmetic.
I am planning to implement CVSS 3.1 changes next week as well as update thousands of tests to match the reference implementation.
Thanks for the feedback.
Regarding modified_isc, I assumed one would prefer to backport the new ModifiedImpact sub-formula so a CVSS:3.0/ prefixed vector string would not suffer the lowering of the environmental score when a higher metric was chosen for MC (i.e., H produces a lower score than L).
I suppose there is a bit of conflict between what one would expect from seeing a CVSS:3.0 prefixed vector:
(1) "I expect this to be scored using CVSS 3.0 scoring guidance and producing environmental output from an error prone calculation in some cases"
or
(2) "I expect this to be scored using CVSS 3.0 scoring guidance w/o any error prone calculations."
Preserving CVSS 3.0 calculations, you could expect to say "well, just update your 3.0 vector stings to 3.1" but then you'd also have to reanalyze the vulnerability using 3.1 guidance, which probably isn't feasible.
Regardless, this is a singular edge case, so it's probably not a big deal - just thought I'd mention the logic behind the code change.
As for the rounding formula, you're right, that's my mistake. The existing formula seems to work fine.
I look forward to your update next week!
Hi @Thomas-Alex and @iamleot , would you mind looking at https://github.com/skontar/cvss/pull/23 ? Thanks!
Thanks for your contribution! Ideas and analysis performed in this pull request were used to implement #23 .
Allows CVSS:3.1 prefix in CVSS3 and parser
Updates rounding formula
Updates ModifiedImpact sub-formula for Environmental Metric Group per 3.1 specifications.