RedHatProductSecurity / cvss

CVSS2/3/4 library with interactive calculator for Python 2 and Python 3
GNU Lesser General Public License v3.0
79 stars 28 forks source link

Result errors due to float variables #59

Closed Xsze closed 4 hours ago

Xsze commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

Xsze commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 4 hours ago

Resolved by https://github.com/RedHatProductSecurity/cvss/pull/61 .