Skip to content

Adds Samba Advisory #1802

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

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

Conversation

Rishi-source
Copy link

Fixes #102

keshav-space

This comment was marked as duplicate.

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 @Rishi-source, see feedback and suggestions below. parse_advisory_row is way too convoluted and is trying to do too many things at once. Affected version parsing is not working correctly. Also, browsing the samba package throws a Server Error (500). Make sure to run the import locally and cross validate the quality of the imported vulnerability against upstream advisory.

Comment on lines +60 to +61
if not hasattr(self, "advisory_data"):
return 0
Copy link
Member

Choose a reason for hiding this comment

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

This will not be required, if fetch step fails for some reason entire pipeline would be terminated.

Comment on lines +137 to +141
cve_ids = []
for link in cells[4].find_all("a"):
cve_id = link.get_text().strip()
if re.match(r"CVE-\d{4}-\d{4,}", cve_id):
cve_ids.append(cve_id)
Copy link
Member

Choose a reason for hiding this comment

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

CVE parsing is not clean it leads to 404 Client Error: Not Found for url: https://www.samba.org/samba/security/CVE-2019-19344..html

Comment on lines +163 to +168
if version_match:
affected_packages.append(
AffectedPackage(
package=base_purl, fixed_version=f"{version_match.group(1)}-fixed"
)
)
Copy link
Member

Choose a reason for hiding this comment

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

I honestly do not understand why would you put affected_version in fixed version.

And why are you mutating the original version by appending -fixed to version.

)
else:
affected_packages.append(
AffectedPackage(package=base_purl, fixed_version="unknown")
Copy link
Member

Choose a reason for hiding this comment

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

Why invent imaginary unknown fixed version?

detailed_summary = issue_desc

for cve_id in cve_ids:
details = self.get_advisory_details(cve_id)
Copy link
Member

Choose a reason for hiding this comment

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

This will not work for generated id SAMBA-VULN-

references=references,
affected_packages=affected_packages,
date_published=date_published,
url="https://www.samba.org/samba/history/security.html",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the url be the individual announcement like https://www.samba.org/samba/security/CVE-2023-3961.html?

Comment on lines +247 to +252
if version_match:
affected_packages.append(
AffectedPackage(
package=base_purl, fixed_version=f"{version_match.group(1)}-fixed"
)
)
Copy link
Member

Choose a reason for hiding this comment

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

Again why would you put affected_version in fixed version field.

@Rishi-source
Copy link
Author

Hi @keshav-space ,
I’ll make the necessary changes in the code as per your suggestions. Apologies for not performing thorough testing before raising the PR

@@ -78,6 +79,20 @@
ubuntu_usn.UbuntuUSNImporter,
fireeye.FireyeImporter,
oss_fuzz.OSSFuzzImporter,
ruby.RubyImporter,
Copy link
Member

Choose a reason for hiding this comment

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

These lines should not be there when using pipelines

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

Successfully merging this pull request may close these issues.

Collect samba
4 participants