RedHatProductSecurity / cvss-v4-calculator

CVSS v4.0 calculator
https://redhatproductsecurity.github.io/cvss-v4-calculator/
BSD 2-Clause "Simplified" License
32 stars 22 forks source link

Potential Incorrect Rounding For Final Score #48

Closed alex88510 closed 7 months ago

alex88510 commented 8 months ago

Due to the issue where 0.1 + 0.2 !== 0.3 (but 0.30000000000000004) in JavaScript, it might cause unexpected result when the value is rounded to 1 decimal point with toFixed() at line 527 in app.js.

https://github.com/RedHatProductSecurity/cvss-v4-calculator/blob/8791cb235d03009faa08f571d176c5d24a52f908/app.js#L527

For example, with the following vector, the score before rounding with toFixed(1) is 0.35. The expected final score should be 0.4 but get 0.3 instead.

CVSS:4.0/AV:N/AC:L/AT:P/PR:L/UI:A/VC:L/VI:L/VA:N/SC:N/SI:N/SA:N/E:U

Just additional info, with this vector CVSS:4.0/AV:N/AC:L/AT:P/PR:N/UI:N/VC:H/VI:H/VA:N/SC:H/SI:H/SA:H/E:P, the score before rounding is 8.95. The expected final score should be 9.0 (Critical) but get 8.9 (High) instead, will have a difference in severity rating.

skontar commented 8 months ago

@ViperGeek This is exactly what I wanted to avoid from the beginning. We should have never used floating point arithmetic again. https://github.com/RedHatProductSecurity/cvss uses exact arithmetic but we should check if it behaves there as expected.

pandatix commented 8 months ago

Was able to reproduce on the calculator and the JS implementation, but not on the Go one, so pretty much language-specific.

skontar commented 8 months ago

The problem is specific to use of IEEE 754 floating point arithmetic. It may differ between 32-bit and 64-bit floats.

bjedwards commented 7 months ago

I am not sure the solution is "don't use floating both arithmetic", but probably just to decide on the rounding mechanism we should use.

toFixed is a bit odd, and we should probably use Math.round(value*10)/10. This uses "round half up" heuristic which slightly biases values higher, the default in python is "round half even" which caused this bug BJEdwards/cvss4py#3.

skontar commented 7 months ago

The solution was always "use fixed point arithmetic and as a last step divide by 10". The original algorithm had only lookup and no math. When we needed to extend it with additional arithmetic steps there was not enough attention. Now I guess it is too late to do it properly.

Decision on rounding would probably be sufficient as it was in v3.1. It makes me sad that I know we could completely avoid rounding issues from the beginning.

skontar commented 7 months ago

@bjedwards can you check https://github.com/RedHatProductSecurity/cvss-v4-calculator/pull/49 ?

skontar commented 7 months ago

Resolved by #49