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

Feat: Add CVE support to Snyk datasource #1405

Merged

Conversation

shravankshenoy
Copy link
Contributor

Fixes #1163

Changes Made

Added function datasource_advisory_from_cve to Snyk data source and other helper functions.

Working

An overview of how the function works is given below

  1. generate_payload_from_cve function returns url for a given cve
  2. Html page from the given cve url is fetched using fetch function
  3. parse_cve_advisory_html function extracts all the vulnerabilities and their snyk_ids and package ids from html page
  4. All the affected versions for the affected package id is obtained using extract_html_json_advisories function
  5. Snyk url and html page are got using generate_advisory_payload and fetch functions
  6. All the data is passed into parse_html_advisory function along with advisory_html page created in step 5, and that will return the final VendorData object with affected and fixed versions

generate_purl is a helper function which generates a purl from package url

Results

Tested the app locally by printing the results yielded for CVE-2024-23641

image

This matches the result at https://security.snyk.io/vuln?search=CVE-2024-23641

@shravankshenoy
Copy link
Contributor Author

shravankshenoy commented Jan 29, 2024

Oops looks like one test related to formatting is failing - Black style check failed. Will try to fix that

@ziadhany
Copy link
Collaborator

Oops looks like one test related to formatting is failing - Black style check failed. Will try to fix that

@shravankshenoy you can just run make valid and check the test again with make test

@shravankshenoy
Copy link
Contributor Author

Thanks @ziadhany . All the tests are passing now.

Copy link
Member

@keshav-space keshav-space left a comment

Choose a reason for hiding this comment

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

Thanks, @shravankshenoy. This is a good start.
Please take a look at the suggestions below.

vulntotal/datasources/snyk.py Outdated Show resolved Hide resolved
pkg_name = package_url_split[1].split(":")[1]
namespace = package_url_split[1].split(":")[0]

elif pkg_type in ("golang", "composer"):
Copy link
Member

Choose a reason for hiding this comment

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

PURL creation for golang needs to be properly handled. Take a look at the example below.

For the snyk url https://security.snyk.io/package/golang/github.com%2Fgoauthentik%2Fauthentik%2Fauthentik%2Fproviders%2Foauth2%2Fviews the corresponding purl would be

PackageURL(
    type='golang',
    namespace='github.com/goauthentik/authentik/authentik/providers/oauth2',
    name='views',
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for highlighting this. I have modified the code so that namespace for Go packages is captured correctly.

vulntotal/datasources/snyk.py Outdated Show resolved Hide resolved
Comment on lines 169 to 212
def generate_purl(package_advisory_url):
"""
Generates purl from Package advisory url.

Parameters:
package_advisory_url: URL of the package on Snyk.

Returns:
A PackageURL instance representing the package
"""
package_advisory_url = unquote_plus(
package_advisory_url.replace("https://security.snyk.io/package/", "")
)

package_url_split = package_advisory_url.split("/")
pkg_type = package_url_split[0]

pkg_name = None
namespace = None
version = None
qualifiers = {}

if pkg_type == "maven":
pkg_name = package_url_split[1].split(":")[1]
namespace = package_url_split[1].split(":")[0]

elif pkg_type in ("golang", "composer"):
if package_url_split[1] == "github.com":
pkg_name = package_url_split[-2]
namespace = f"{package_url_split[1]}/{package_url_split[2]}"
version = package_url_split[-1]
else:
pkg_name = package_url_split[-1]
namespace = package_url_split[-2]

elif pkg_type == "linux":
pkg_name = package_url_split[-1]
qualifiers["distro"] = package_url_split[1]

else:
pkg_name = package_url_split[-1]

return PackageURL(
type=pkg_type, name=pkg_name, namespace=namespace, version=version, qualifiers=qualifiers
Copy link
Member

Choose a reason for hiding this comment

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

We must explicitly handle the PURL creation for each ecosystem from the Snyk URL.
For a better understanding on how to create a PURL for a particular ecosystem, you should check https://github.com/package-url/purl-spec/blob/master/PURL-TYPES.rst

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for sharing this. I have tried my best to make sure the purl-spec is followed properly now, but let me know if I missed something.

vulntotal/tests/test_snyk.py Show resolved Hide resolved
@keshav-space keshav-space added the VulnTotal Tool for cross-validating vulnerability label Feb 2, 2024
@shravankshenoy
Copy link
Contributor Author

Thanks a lot for reviewing the code. It not only helped me understand purl-spec much better, but also helped while wrtitng code for some of the other PRs. As a positive side effect of this change, when I ran the code for CVE-2024-23641, it now shows the namespaces, unlike before where namespaces were null.

image

Let me know if any further changes required, would be glad to the the needful :)

Copy link
Member

@keshav-space keshav-space left a comment

Choose a reason for hiding this comment

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

@shravankshenoy see few suggestions below.

elif pkg_type in ("cocoapods", "hex", "nuget", "pip", "rubygems", "unmanaged"):
pkg_name = package_url_split[-1]

return PackageURL(type=pkg_type, name=pkg_name, namespace=namespace)
Copy link
Member

@keshav-space keshav-space Feb 15, 2024

Choose a reason for hiding this comment

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

Before returning a PURL, make sure that we have the bare minimum type and name available, without these PURL creation will fail. In the event that, for some reason, we are not able to get the type and name, log a warning and skip the advisory for this package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Added an if condition which logs an error if type or name is missing. Also datasource_advisory_from_cve function has been modified such that if the generate_purl function returns None, the advisory is skipped

Comment on lines 198 to 203
elif pkg_type == "golang":
if package_url_split[1] == "github.com":
ns_start = package_advisory_url.find("github.com")
ns_end = package_advisory_url.rfind("/")
namespace = package_advisory_url[ns_start:ns_end]
pkg_name = package_url_split[-1]
Copy link
Member

Choose a reason for hiding this comment

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

It is not given that a Go package will always be on GitHub. For instance, Go package https://security.snyk.io/package/golang/blitiri.com.ar%2Fgo%2Fchasquid%2Finternal%2Fsmtp, will not be properly handled by above highlighted code.

Suggested change
elif pkg_type == "golang":
if package_url_split[1] == "github.com":
ns_start = package_advisory_url.find("github.com")
ns_end = package_advisory_url.rfind("/")
namespace = package_advisory_url[ns_start:ns_end]
pkg_name = package_url_split[-1]
elif pkg_type == "golang":
pkg_name = package_url_split[-1]
namespace = "/".join(package_url_split[1:-1])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

vulntotal/tests/test_snyk.py Outdated Show resolved Hide resolved
@shravankshenoy
Copy link
Contributor Author

Thanks for reviewing the code. Made all the requested changes.

While working on these changes, I also realized that I had somehow missed on following the purl spec properly. So I have used the dictionary which we get by calling SnykDataSource.supported_ecosystem(), inverted the keys and values, and use the new dictionary to get the purl-spec compliant package type. Hope that works.

Copy link
Member

@keshav-space keshav-space left a comment

Choose a reason for hiding this comment

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

Thanks++, LGTM!

@keshav-space keshav-space merged commit 30d3357 into aboutcode-org:main Feb 23, 2024
6 of 7 checks passed
TG1999 pushed a commit to TG1999/vulnerablecode that referenced this pull request Jul 19, 2024
* Add function to generate purl from package advisory url

Signed-off-by: Shenoy <[email protected]>

* Add functions to generate cve url and parse the html advisory generated from the cve url

Signed-off-by: Shenoy <[email protected]>

* Add function to SnykDataSource class to get advisories from cve

Signed-off-by: Shenoy <[email protected]>

* Add tests and test data for parse_cve_advisory_html function

Signed-off-by: Shenoy <[email protected]>

* Add test for generate_purl function

Signed-off-by: Shenoy <[email protected]>

* Remove to_dict in snyk test file where not required

Signed-off-by: Shenoy <[email protected]>

* Format code using Black

Signed-off-by: Shenoy <[email protected]>

* Sort imports correctly using isort

Signed-off-by: Shenoy <[email protected]>

* Modify generate_purl function in snyk

Signed-off-by: Shenoy <[email protected]>

* Add additional tests for golang and npm

Signed-off-by: Shenoy <[email protected]>

* Add non-Github Go packages to test, modify generate_purl function, improve error handling

Signed-off-by: Shenoy <[email protected]>

* Modify generate_purl to comply with purl spec

Signed-off-by: Shenoy <[email protected]>

---------

Signed-off-by: Shenoy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
VulnTotal Tool for cross-validating vulnerability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CVE support in SnykDataSource
3 participants