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

Updating the SSRF category #359

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

TimmyBugcrowd
Copy link
Contributor

Changing the SSRF top category and moving External SSRF to P5.
FROM:
P2 - Broken Access Control (BAC) - Server-Side Request Forgery (SSRF) - Internal High Impact
P3 - Broken Access Control (BAC) - Server-Side Request Forgery (SSRF) - Internal Scan and/or Medium Impact
P4 - Broken Access Control (BAC) - Server-Side Request Forgery (SSRF) - External
P5 - Broken Access Control (BAC) - Server-Side Request Forgery (SSRF) - DNS Query Only

TO:
P2 - Server Security Misconfiguration - Server-Side Request Forgery (SSRF) - Internal High Impact
P3 - Server Security Misconfiguration - Server-Side Request Forgery (SSRF) - Internal Scan and/or Medium Impact
P5 - Server Security Misconfiguration - Server-Side Request Forgery (SSRF) - External(GET request)/DNS Query Only

@vortexau
Copy link

vortexau commented Jun 21, 2023

Tests will all need to function and pass before this can be merged, they seem to be failing on invalid syntax in a JSON file. The original submitter of the PR may also be able to assist as needed.

@amalmurali47
Copy link
Contributor

amalmurali47 commented Jun 21, 2023

External (GET request) / DNS Query Only might be a bit confusing because it seems to conflate two distinct concepts. I think it's best to keep them separate like below:

  • External - GET Request Only
  • External - DNS Query Only

Current:

  • P2 - Broken Access Control (BAC) - Server-Side Request Forgery (SSRF) - Internal High Impact
  • P3 - Broken Access Control (BAC) - Server-Side Request Forgery (SSRF) - Internal Scan and/or Medium Impact
  • P4 - Broken Access Control (BAC) - Server-Side Request Forgery (SSRF) - External
  • P5 - Broken Access Control (BAC) - Server-Side Request Forgery (SSRF) - DNS Query Only

Proposed:

  • P2 - Server Security Misconfiguration - Server-Side Request Forgery (SSRF) - Internal High Impact
  • P3 - Server Security Misconfiguration - Server-Side Request Forgery (SSRF) - Internal Scan and/or Medium Impact
  • P5 - Server Security Misconfiguration - Server-Side Request Forgery (SSRF) - External - GET Request Only
  • P5 - Server Security Misconfiguration - Server-Side Request Forgery (SSRF) - External - DNS Query Only

@amalmurali47
Copy link
Contributor

The tests were failing due to syntax errors in the VRT JSON. I reverted to the last known good version from the master branch. Then, I updated the parent category to Server Security Misconfiguration.

However, the tests continued to fail with the following error:

$ python3 -B lib/validate_vrt.py
...Fs...
======================================================================
FAIL: test_all_vrt_ids_have_all_mappings (tests.test_vrt.TestVrt)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/user/projects/vulnerability-rating-taxonomy/lib/tests/test_vrt.py", line 55, in test_all_vrt_ids_have_all_mappings
    self.all_vrt_ids_have_mapping(mapping['filename'], mapping['name'])
  File "/Users/user/projects/vulnerability-rating-taxonomy/lib/tests/test_vrt.py", line 50, in all_vrt_ids_have_mapping
    self.assertTrue(utils.has_mapping(keyed_mapping, vrt_id_list, key),
AssertionError: False is not true : no cvss_v3 mapping for server_security_misconfiguration.server_side_request_forgery_ssrf.dns_query_only

----------------------------------------------------------------------
Ran 8 tests in 1.136s

FAILED (failures=1, skipped=1)

We might encounter this issue again during reclassification of entries in the future, so I'll document the cause and resolution for future reference.

When SSRF was nested under Broken Access Control, this error didn't occur. The reason is straightforward but not immediately intuitive: the all_vrt_ids_have_mapping() test uses a function called has_mapping(), which recursively verifies the existence of a CVSS V3 mapping for each category, starting from the parent category. While Broken Access Control has a default CVSS mapping, Server Security Misconfiguration does not. The absence of this mapping was the cause of the AssertionError. To resolve this, a specific CVSS mapping was added for dns_query_only under server_side_request_forgery_ssrf in the server_security_misconfiguration category.

Summary of the changes in this PR:

  • Recategorized Server-Side Request Forgery (SSRF) from Broken Access Control to Server Security Misconfiguration.
  • Split the External SSRF variant into external_get_request_only and dns_query_only.
  • Added a specific CVSS v3 mapping for dns_query_only under server_side_request_forgery_ssrf in the server_security_misconfiguration category in the cvss_v3.json file.
  • Updated the corresponding mappings in mappings/cvss_v3/cvss_v3.json, mappings/cwe/cwe.json, and mappings/remediation_advice/remediation_advice.json files.

@amalmurali47
Copy link
Contributor

This should fix #339 and #354. If the changes look good, we can merge this.

@vortexau @trimkadriu Please review when you have a chance. Thanks!

@vortexau
Copy link

Minor updates to be made around the P5s before approval can be provided.

@TimmyBugcrowd
Copy link
Contributor Author

As per discussion, I've made changes and this is how it looks now:
P2 - Server Security Misconfiguration - Server-Side Request Forgery (SSRF) - Internal High Impact
P3 - Server Security Misconfiguration - Server-Side Request Forgery (SSRF) - Internal Scan and/or Medium Impact
P5 - Server Security Misconfiguration - Server-Side Request Forgery (SSRF) - External - Low Impact
P5 - Server Security Misconfiguration - Server-Side Request Forgery (SSRF) - External - DNS Query Only

@TimmyBugcrowd
Copy link
Contributor Author

merged to the intermediate branch.

@amalmurali47
Copy link
Contributor

@TimmyBugcrowd The agreed option was slightly different. External - Low Impact already covers External - DNS Query Only as well, so we can remove P5 - Server Security Misconfiguration - Server-Side Request Forgery (SSRF) - External - DNS Query Only entirely.

It should look like:

  • P2 - Server Security Misconfiguration - Server-Side Request Forgery (SSRF) - Internal High Impact
  • P3 - Server Security Misconfiguration - Server-Side Request Forgery (SSRF) - Internal Scan and/or Medium Impact
  • P5 - Server Security Misconfiguration - Server-Side Request Forgery (SSRF) - External - Low Impact

RRudder added a commit to bugcrowd/templates that referenced this pull request Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants