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

Potential Incorrect Rounding For Final Score #48

Closed
alex88510 opened this issue Jan 5, 2024 · 7 comments
Closed

Potential Incorrect Rounding For Final Score #48

alex88510 opened this issue Jan 5, 2024 · 7 comments

Comments

@alex88510
Copy link

alex88510 commented Jan 5, 2024

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.

return value.toFixed(1)

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
Copy link
Collaborator

skontar commented Jan 5, 2024

@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
Copy link
Contributor

pandatix commented Jan 5, 2024

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

@skontar
Copy link
Collaborator

skontar commented Jan 5, 2024

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

@bjedwards
Copy link

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
Copy link
Collaborator

skontar commented Jan 19, 2024

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
Copy link
Collaborator

skontar commented Jan 28, 2024

@bjedwards can you check #49 ?

skontar added a commit that referenced this issue Feb 5, 2024
@skontar
Copy link
Collaborator

skontar commented Feb 5, 2024

Resolved by #49

@skontar skontar closed this as completed Feb 5, 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

4 participants