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

Result errors due to float variables #59

Closed
Xsze opened this issue Jul 15, 2024 · 8 comments
Closed

Result errors due to float variables #59

Xsze opened this issue Jul 15, 2024 · 8 comments

Comments

@Xsze
Copy link

Xsze commented Jul 15, 2024

Hello,

I'm doing a port of the cvss v4 calculator on powershell and during my tests i've encountered a problem with how you handle your variables.

You let javascipt and python define the type of the variables but they tend to use a floating point type, and using float type can cause non precise output.
Exemple with : CVSS:4.0/AV:N/AC:H/AT:N/PR:N/UI:P/VC:H/VI:L/VA:H/SC:H/SI:N/SA:N/E:U/CR:H/IR:M/AR:M/MAV:P/MAC:L/MAT:P/MPR:H/MUI:A/MVC:N/MVI:L/MVA:L/MSC:N/MSI:H/MSA:S/S:N/AU:Y/R:A/V:C/RE:H/U:Amber

image
value is 1.4 and score eq4 is 0.5
image
result due to float is 0.8999999999999999

image
Maxseverity eq4 = 6 and step is 0.1 but the result is 0.6000000000000001
image
all of these give the final result before rounding a score of 0.9500000000000001 who will be rounded up cause of this 0.0000000000000001 added due tot float type where normally 0.95 shoud have been rounded down. ( if folowing the logic of the first's online calculator who use value.tofixed(1) to produce the final rounding )

You should not let your scripts use a floating point type variable

As I've seen all the calculators have this problem (First's online calculator, the python one and the redhatproductsecurity's online one)

@skontar
Copy link
Collaborator

skontar commented Jul 24, 2024

Hello. Thanks for reporting!

It is a known issue and it is unfortunate that it exists as we had to specifically resolve it in CVSS v3.1 and introduced this regression in CVSS v4. It was the original plan to avoid floating point operations altogether but some people on the wider team did not understand the risk well enough when some additional math was introduced to the algorithm.

We are planning to fix it, likely in a similar fashion as the v3.1 does.

However, are you sure that Python one is wrong? I thought that the algorithm there uses Decimal instead of float?

@skontar
Copy link
Collaborator

skontar commented Jul 24, 2024

OK, for the record, v4 implementation did not always use Decimal here in the Python implementation.

@Xsze
Copy link
Author

Xsze commented Jul 24, 2024

Pretty sure, in fact the screenshots I posted in this ticket are from the python script.
I discovered this issue from the python script and then i checked the other sources.

@skontar
Copy link
Collaborator

skontar commented Jul 24, 2024

Yeah, I should have been more diligent in the review 🤦. It should be mostly easy to solve in Python, a bit harder to match it with Javascript. I will try to escalate and see if we can find a bit of resources to fix this once and for all.

@skontar
Copy link
Collaborator

skontar commented Aug 1, 2024

Can you explain why 0.95 should be rounded down? I see that this is the result which value.tofixed(1) to produces, but that is not expected by me.

In Python it is caused by 0.95 actually being 0.9499... in float representation

>>> print(f"{0.95:0.150f}")
0.949999999999999955591079014993738383054733276367187500000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
>>> round(0.95, 1)
0.9

After the planned fix we expect 0.95 to be rounded up in all implementation.

@Xsze
Copy link
Author

Xsze commented Aug 1, 2024

This is what the First's online calculator use to do the final rounding and to me it's not right either. But as there is no rule to do the rounding and the First is the reference with the cvss, that's where the clients are looking when they want to manualy verify a score result, that's why i use this as an exemple.

But i openend another issue (58) to cover the final rounding not correctly defined and you said you had plan to do something like in cvss v3.1 ( allways rounding up i assume).

@skontar
Copy link
Collaborator

skontar commented Aug 2, 2024

OK, cool. We will try to find a way to be consistent across both implementations and update the reference document to have rounding requirement matching the fix.

@skontar
Copy link
Collaborator

skontar commented Sep 7, 2024

Resolved by #61 .

@skontar skontar closed this as completed Sep 7, 2024
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

No branches or pull requests

2 participants