8000 add support for calculating CVSS score from the CVSS vector by ziadhany · Pull Request #747 · aboutcode-org/vulnerablecode · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

add support for calculating CVSS score from the CVSS vector #747

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 16 commits into from
Nov 18, 2022
Merged

add support for calculating CVSS score from the CVSS vector #747

merged 16 commits into from
Nov 18, 2022

Conversation

ziadhany
Copy link
Collaborator

Reference: #713

Signed-off-by: Ziad ziadhany2016@gmail.com

@ziadhany ziadhany force-pushed the calculate_cvss_vector branch 2 times, most recently from 4d3ae93 to 2890329 Compare May 21, 2022 12:15
pombredanne
pombredanne requested changes May 25, 2022
View reviewed changes
Copy link
Member
@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good... Can you add some tests?

@ziadhany ziadhany mentioned this pull request Aug 30, 2022
Closed
pombredanne
pombredanne requested changes Sep 9, 2022
View reviewed changes
Copy link
Member
@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍
@ziadhany do you mind to rebase on the latest code and also regen the migration on the latest code?
Also there is a need for a data migrations to mig 8000 rate the current CVSS vector to this new approach

@ziadhany ziadhany force-pushed the calculate_cvss_vector branch 2 times, most recently from f0f7f67 to 171c831 Compare September 12, 2022 14:55
@pombredanne
Copy link
Member

re:

Also there is a need for a data migrations to migrate the current CVSS vector to this new approach

CVSSV31,
CVSSV31_VECTOR,

say we have two rows with these data:

  • scoring_system: CVSSV31_VECTOR value:"CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:L/I:L/A:H"
  • scoring_system: CVSSV31 value: "8.6"

After the migration(s) I would like to see only one record:

  • scoring_system: CVSSV31 value: "8.6" scoring_elements: "CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:L/I:L/A:H"

This is best done in three steps:

  1. model schema update adding scoring_elements
  2. data migration that searches and adds or computes the score for CVSS severities and add these to one of the two records
    2.1 one way:
    starting from this record - scoring_system: CVSSV31_VECTOR value:"CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:L/I:L/A:H"
    update the record this - update scoring_system: CVSSV31, copy value to value to scoring_elements, compute value
    2.2 other way: group pairs of records and merge the two in one of the two records
  3. data migration to remove the extra record

pombredanne
pombredanne requested changes Sep 13, 2022
View reviewed changes
@zia 8000 dhany ziadhany mentioned this pull request Sep 22, 2022
Open
9 tasks
@ziadhany ziadhany force-pushed the calculate_cvss_vector branch from ffc6c72 to 9563a61 Compare September 24, 2022 03:04
@ziadhany ziadhany force-pushed the calculate_cvss_vector branch 2 times, most recently from e0a8e81 to df1ea13 Compare October 3, 2022 20:02
@TG1999 TG1999 added this to the v31.0 milestone Oct 11, 2022
@TG1999 TG1999 marked this pull request as draft October 18, 2022 16:26
@ziadhany
Rebase Migrations
f779900 
Signed-off-by: ziadhany <ziadhany2016@gmail.com>

Change the max length of scoring_elements

Signed-off-by: ziadhany <ziadhany2016@gmail.com>

Fix typo in swap function and fix migration test

Signed-off-by: ziadhany <ziadhany2016@gmail.com>

Add a migration test

Signed-off-by: ziadhany <ziadhany2016@gmail.com>

Rename migration variable model

Signed-off-by: ziadhany <ziadhany2016@gmail.com>

Fix test

Signed-off-by: ziadhany <ziadhany2016@gmail.com>

Remove duplicate rows for CVSS

Signed-off-by: ziadhany <ziadhany2016@gmail.com>
@ziadhany ziadhany force-pushed the calculate_cvss_vector branch from b85064c to f779900 Compare November 1, 2022 12:27
pombredanne
pombredanne requested changes Nov 1, 2022
View reviewed changes
Copy link
Member
@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update! please see some comments inline.

@pombredanne
Rename migrations before merging
2c1b24e 
To avoid merge conflicts

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne
Format code and improve expectations
bee1609 
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne
Merge latest main branch
c14f06b 
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne
Update migratiosn with latest main branch
ca2ae13 
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne
Correct field description
afa6011 
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne
Rename migration
582c895 
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne
Refactor compute to CVSS score
0d4fa0b 
Use a subclass of the related scoring systems

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne
Update migration approach with bulk operations
af6cfcc 
- Conflate migration 32 and 33 in a single one
- Use subclasses for CVSS ScoringSystem with their score computation
- Update tests

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne
Remove merged CVSS vector scores
90b5c64 
* Track the CVSS vector as scoring elements and not a separate score
* Also add and use scoring elements everywhere we use
  VulnerabilitySeverity
* Update api, importers, improvers and tests

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne
Skip the migrations/ dir in black
176ef26 
Otherwise we had some test_migrations files skipped and not formatted

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne
Fix upstream, live tests
db8cd9a 
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne
Remove renamed file reference
c3f8d5d 
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne
Use scoring elements everywhere
13cbf84 
Also fix tests to use a regen env variable consistently

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne
Do not format test data
2aa7cbe 
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne
Copy link
Member

@ziadhany I pushed these updates:

  1. merge the latest main branch including handling overlapping migration numbers
  2. update the the migration(s) to use a bulk update/bulk delete approach
  3. propagate the use of scoring_elements everywhere including importers, tests, API, etc.
  4. added some cosmetic improvements to regen tests fixtures easily

The new migration(s) handle all possible duplicates cases this way:

  • keep a mapping of severities keyed by the "unique together" values of the model
  • keep a set of severity ids to delete
  • for each CVSS-related severity:
    • if this is a vector:
      • update this to move value to scoring elements and compute score in value and use plain cvss scoring system
    • if it does not exist in the mapping
      • add it to this mapping
    • else
      • if needed merge/add vector to the existing severity
      • mark this severity for deletion
  • bulk delete the set of ids
  • bulk update severities mapping

I need to test run it on the whole DB to validate this is correct

@pombredanne
Copy link
Member
< 8000 tr class="d-block">

@ziadhany This is essentially the same approach as yours, but folded in a single migration with bulk updates

@pombredanne
Delete dupes before updating severities
550ac20 
This is to avoid any integrity violations in migration

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne pombredanne marked this pull request as ready for review November 14, 2022 13:30
@pombredanne pombredanne requested a review from TG1999 November 14, 2022 13:32
@ziadhany
Copy link
Collaborator Author

LGTM , I tested it with my local database

@TG1999
Copy link
Contributor
TG1999 commented Nov 15, 2022

@ziadhany please add CHANGEOLG for this PR
6D40

TG1999
TG1999 approved these changes Nov 15, 2022
View reviewed changes
Copy link
Contributor
@TG1999 TG1999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@pombredanne pombredanne merged commit e38eb1b into aboutcode-org:main Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pombredanne pombredanne pombredanne approved these changes

@TG1999 TG1999 TG1999 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v31.0
Development

Successfully merging this pull request may close these issues.
3 participants
@ziadhany @pombredanne @TG1999
0