Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for CVSSv4 #45

Merged
merged 4 commits into from
Dec 18, 2023
Merged

Conversation

jobiewinserapck
Copy link
Contributor

-Added class CVSS4
-Based the CVSS4 class on the js implementation found here: https://github.com/RedHatProductSecurity/cvss-v4-calculator -Added new tests for CVSS4
-Added new constants and exceptions for CVSS4
-Added json schema for CVSS4

@skontar
Copy link
Collaborator

skontar commented Dec 8, 2023

This looks really good. Thank you!

@skontar
Copy link
Collaborator

skontar commented Dec 8, 2023

@mprpic can you please check CI? It seems to fail for some reason.

@skontar
Copy link
Collaborator

skontar commented Dec 8, 2023

@jobiewinserapck I presume you used the js implementation to generate test vectors?

@jobiewinserapck
Copy link
Contributor Author

jobiewinserapck commented Dec 8, 2023

Yes, I generated the vectors using the js implementation. I could Maybe get some screenshots tomorrow of how I did so. If thats helpful.

The only test failures I encountered after writing this was due to python3's rounding not being "away from zero", causing scores such as 7.25 to round towards 7.2 rather than 7.3 as expected (hence the round_away_from_zero func)

@skontar
Copy link
Collaborator

skontar commented Dec 8, 2023

No need for screenshots, just curious. Rounding was always "fun" previously. I am glad you resolved it easily.

@skontar
Copy link
Collaborator

skontar commented Dec 8, 2023

@jobiewinserapck can you rebase with master? I think that should re-run the CI with latest setup.

@jobiewinserapck
Copy link
Contributor Author

Sure thing I'll do that ASAP tomorrow morning

-Added class CVSS4
-Based the CVSS4 class on the js implementation found here:
https://github.com/RedHatProductSecurity/cvss-v4-calculator
-Added new tests for CVSS4
-Added new constants and exceptions for CVSS4
-Added json schema for CVSS4
cvss/cvss4.py Outdated Show resolved Hide resolved
cvss/cvss4.py Outdated Show resolved Hide resolved
cvss/cvss4.py Outdated Show resolved Hide resolved
cvss/cvss4.py Outdated Show resolved Hide resolved
cvss/cvss4.py Show resolved Hide resolved
@skontar
Copy link
Collaborator

skontar commented Dec 11, 2023

@mprpic can you please review?

@jobiewinserapck
Copy link
Contributor Author

I'ved added those improvements, thanks for that @skontar!

@marco-silva0000
Copy link

just commenting for support, gogogo

-Fixed various typos: CVSS3 -> CVSS4
-Added malformed CVSS4MalformedError check for wrong cvss prefix
-Fixed prefix in clean_vector: "CVSS:4/" -> "CVSS:4.0/"
@jobiewinserapck
Copy link
Contributor Author

Just commenting to say I've reformatted cvss/cvss4.py, hopefully the pipeline will pass on next run

Copy link
Collaborator

@mprpic mprpic left a comment

Choose a reason for hiding this comment

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

Can you also please add import statements into the cvss/__init__.py file?

from .cvss4 import CVSS4
from .exceptions import CVSS4Error

cvss/cvss4.py Show resolved Hide resolved
cvss/cvss4.py Outdated Show resolved Hide resolved
-Added additional CVSS4 imports to cvss/__init__.py
-Added additional metric checks that raise CVSS4MalformedError
-Cleaned up some left over comments
-Added additional tests for malformed CVSS4 strings
@jobiewinserapck
Copy link
Contributor Author

I've pushed up a commit addressing the issues raised by @mprpic.

I also added some additional tests for malformed cvss strings

@skontar
Copy link
Collaborator

skontar commented Dec 17, 2023

@mprpic please re-review.

Copy link
Collaborator

@mprpic mprpic left a comment

Choose a reason for hiding this comment

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

LGTM!

@skontar skontar merged commit b72d144 into RedHatProductSecurity:master Dec 18, 2023
10 checks passed
@skontar
Copy link
Collaborator

skontar commented Feb 5, 2024

@jobiewinserapck there were recent changes to rounding in official Javascript implementation. Would you be so kind and double-check everything is still shiny and both implementations match? I think that the Python one will be more correct as it uses Decimal.

@AdrianVollmer
Copy link

I don't think round_away_from_zero is correct:

$ python << EOF
from decimal import ROUND_HALF_UP
from decimal import Decimal as D
def round_away_from_zero(x, dp=1):
    return float(D(x).quantize(D("0." + "0" * dp), rounding=ROUND_HALF_UP))

for i in range(6):
    print(round_away_from_zero(i + 0.05))
EOF
0.1
1.1
2.0
3.0
4.0
5.0

It's inconsistent.

I would have said we should just do round(x*10)/10 like the JavaScript implementation, but Math.round(0.5) is 1 in JS and round(0.5) is 0 in Python3 and 1 in Python2. Python3 follows IEEE 754.

The specification document is not clear on which rounding convention should be used:

This score is rounded to one decimal place.

@skontar
Copy link
Collaborator

skontar commented Feb 6, 2024

Correct .quantize call should be able to resolve that difference. I do not recall what we did for v3.1 but we were able to get exactly the same results between Python and Javascript.

@skontar
Copy link
Collaborator

skontar commented Feb 6, 2024

@AdrianVollmer if the input is Decimal then round_away_from_zero works correctly I think.

>>> float((D(2) + D(5) / D(100)).quantize(D("0.0"), rounding=ROUND_HALF_UP))
2.1

@AdrianVollmer
Copy link

Okay, but this function is only used once here and is passed a float:

self.base_score = round_away_from_zero(value, 1)

@AdrianVollmer
Copy link

I think this function will suit this particular use case:

def round_away_from_zero(value):
    # Only works correctly for positive values
    return int(value * 10 + 0.5) / 10.0

Since we only need to round values between 0.0 and 10.0, it should be fine.

$ python << EOF
for i in range(10):
 print(int((i+0.05)*10 + 0.5)/10)
EOF
0.1
1.1
2.1
3.1
4.1
5.1
6.1
7.1
8.1
9.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants