search

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

Updated CVSSv4.0 based on specifications #27

Closed giorgioditizio closed 1 year ago

giorgioditizio commented 1 year ago

Pull request CVSSv4.0 updated accordingly to specifications

ViperGeek commented 1 year ago

Would appreciate this being reviewed/approved as soon as possible. I'd like to merge this into https://github.com/FIRSTdotorg/cvss-v4-calculator first thing Monday morning.

skontar commented 1 year ago

@ViperGeek I am firmly against rushing this change. In current state it would be very hard to understand and maintain. In current state it would not be a good reference implementation.

Hello @giorgioditizio ! If this code works as expected, then it is a great proof-of-concept. However, from my point of view it is hard to understand and very hard to maintain as is.

I see some options:

  1. Use this as a proof-of-concept branch, which we can test against. Then write a proper solution based on properly written computation requirements (do we have them available already)?
  2. Gradually improve this merge request, so it can be understood by multiple contributors and maintained in the future.
  3. Merge it as is, but I would like you as a contributor to also commit to a lot of changes to the code to make it easier to understand and maintain. I cannot commit to maintain this code as is in the future.
ViperGeek commented 1 year ago

Hi @skontar. I agree that the merge should be limited to the updated lookup table and supporting math to implement the SIG-approved "Mode (3) adjusted aka XSS fix" interpolation method. I personally cannot comment on the best, least floating point method of implementing this mode, but we do need to find a way to move forward with the interpolation method reviewed and approved.

I cannot comment on the coding style inconsistencies.

bjedwards commented 1 year ago

Hi @skontar I understand there have been issues in the past getting consistent results with floating point implementations in the past. I think one solution is to say "We assume that you are running this on IEEE 754 double precision floating point standard", and we make explicit how rounding works (Round Up to nearest Multiple, ie Math.ceil(x*10)/10. That should cover most languages/compilers/architectures.

Finally, because you haven't been attending the meeting I assume you are unaware that we planned to release the calculator (at least in a prototype form) at the FIRST conference this week. This is something that has been discussed and planned for. Moreover, while there are some last minute changes, many of your concerns has been available and actively worked on by myself Fabio, and Giorgio since mid-March. We are aware this is far from perfect, but we were working towards an implementation that satisfied the SIG requirements, allowed us to test multiple options, and rapidly prototype. As I am sure you have experienced, stripping away the code that is used for testing often results in something less than perfect.

I understand your are uncomfortable with current state as a reference implementation, and I would agree. I would stress that we should merge this branch and present it as a prototype for public comment, and we will work with you to create a more acceptable reference implementation.

skontar commented 1 year ago

@bjedwards there are unfortunately far more problems with floating-point arithmetic. It can be reduced by the approach you proposed, but not fully eliminated. For example even different order of calculations, which may still fit the requirements, may cause issues. However, it can be fully eliminated by doing all computations in fixed point arithmetic (basically by multiplying everything by 10) and then shifting decimal point as the last operation.

Thank you for the explanation of the intended approach. It seems that I should have attended some of the meetings to have a better understanding of intended steps. I am sorry for misunderstanding. I believe what you propose is a good plan. Lets see if it will work for at least some of us to quickly meet to discuss how exactly to proceed.

skontar commented 1 year ago

Hi @giorgioditizio . We had a discussion with other SIG members to clarify the intention of this pull request and followup work. Please disregard most of the review comments, I have just left the ones where I believe we should make change.

After we merged license changes, there is now a conflict. If you could resolve the conflict I can merge it. Thanks!

giorgioditizio commented 1 year ago

Hi @skontar . I resolved the conflict and fixed the code accordingly to the remaining review comments. Let me know if it is ok. Thanks