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

[GHSA-35jj-wx47-4w8r] WeasyPrint allows the attachment of arbitrary files and URLs to a PDF #4587

Conversation

JLLeitschuh
Copy link

Updates

  • Affected products
  • CVSS
  • CWEs
  • Description
  • References
  • Severity

Comments
I'm doing research for Chainguard on old vulnerabilities that were found/discussed at BlackHat and DEFCON that never had CVE's assigned to them. This one seems like it did, but for a case I hadn't expected. I've attached some additional resources/information I've gathered while re-exploring this vulnerability in more depth.

@github
Copy link
Collaborator

github commented Jul 8, 2024

Hi there @liZe! A community member has suggested an improvement to your security advisory. If approved, this change will affect the global advisory listed at github.com/advisories. It will not affect the version listed in your project repository.

This change will be reviewed by our Security Curation Team. If you have thoughts or feedback, please share them in a comment here! If this PR has already been closed, you can start a new community contribution for this advisory

@github-actions github-actions bot changed the base branch from main to JLLeitschuh/advisory-improvement-4587 July 8, 2024 21:50
@liZe
Copy link

liZe commented Jul 9, 2024

Many security concerns (including the ones added here) are listed in the documentation. Long story short: WeasyPrint is a browser, it has to be configured carefully when used with untrusted HTML / CSS. Letting external users use WeasyPrint on your server is just as letting external users use Chrome on your laptop: they can easily access local files with it, unless you configure it to avoid this feature.

These concerns are unrelated to the original problem described by CVE-2024-28184, which is a real security flaw: users could attach arbitrary files, even when configuring the url_fetcher function to avoid arbitrary downloads. This issue is fixed in 61.2.

@JLLeitschuh
Copy link
Author

Right, I think we're both in agreement here. I'm just adding additional clarification to the advisory, and increasing the severity of the vulnerability given what I understand of the impact

@darakian
Copy link
Contributor

darakian commented Jul 9, 2024

Hey @JLLeitschuh and @liZe

I'm aligned with @liZe here. The additional reference you're adding don't seem to expand on the core bug fixed in Kozea/WeasyPrint@734ee8e for which this GHSA/CVE is about.

The references you're suggesting seem to be general resource references as opposed to anything specific to this bug and so I default to rejecting them as they are unlikely to be relevant to a reader of this advisory in the context of an alert. The severity change also seems overblown to me. Perhaps the confidentiality score should be higher, but if we're changing the CVSS then perhaps attack complexity should also be higher as local file access would be dependent on whatever privileges the process is started with.

@liZe would you be open to adding a note to your docs about not running the weasyprint server as root/admin? I'd be happy to draft a PR for you if so.

liZe added a commit to Kozea/WeasyPrint that referenced this pull request Jul 10, 2024
@liZe
Copy link

liZe commented Jul 10, 2024

@liZe would you be open to adding a note to your docs about not running the weasyprint server as root/admin? I'd be happy to draft a PR for you if so.

I’ve added a paragraph to the documentation.

@darakian
Copy link
Contributor

Right on. As for the PR, how would you like to proceed @liZe?

@liZe
Copy link

liZe commented Jul 10, 2024

As for the PR, how would you like to proceed @liZe?

I think that the extra information is unrelated to the original CVE. I don’t understand why we should add anything.

Attaching external files (or including external images, stylesheets, etc.) is normal behavior for WeasyPrint, just as other browsers. It can lead to security concerns when using untrusted data, and these concerns are described in the documentation, with advice about WeasyPrint’s configuration. The problem is not different from other services available to external users, such as HTTP, FTP or SSH servers, or web frameworks.

@darakian
Copy link
Contributor

Cool. @JLLeitschuh, did you want to hash out the CVSS score or can I go ahead and close this out?

@JLLeitschuh
Copy link
Author

Let me get back to you tomorrow. Sorry

@JLLeitschuh
Copy link
Author

JLLeitschuh commented Jul 11, 2024

Please forgive me if I'm not coming across gently below, I'm in a bit of a rush writing this. Please understand my goal is to approach this with kindness and respect, even if it doesn't always come across.

The references you're suggesting seem to be general resource references as opposed to anything specific to this bug
I think that the extra information is unrelated to the original CVE. I don’t understand why we should add anything.

The extra information is related to the exploitability of this vulnerability and how, due to the url_fetcher not being configured, the security researchers were able to achieve SSRF, leveraging WeasyPrint, to access the cloud metadata endpoint and thus pivot to accessing the cloud infrastructure for Lyft and earn Lyft's maximum bug bounty.

See slide 25 and beyond:
https://media.defcon.org/DEF%20CON%2027/DEF%20CON%2027%20presentations/DEFCON-27-Ben-Sadeghipour-Owning-the-clout-through-SSRF-and-PDF-generators.pdf

All the other links I attaches similarly are referring to abusing WeasyPrint to achieve data exfil via LFI or SSRF.

These are the same impact with the same root cause, WeasyPrint, but just being discussed well before this CVE was assigned.

The severity change also seems overblown to me.

Are you arguing the severity is incorrect, or the CVSS score? I'm basing the score off of what I know of how to enter CVSS, not on the resulting score. The impact, as far as I'm aware, is both LFI, RFI, and SSRF, and if it's in a PDF that's generated and returned to the user, is exposing all that internal data to the user, all of which cause significant confidentiality impact.

Perhaps the confidentiality score should be higher, but if we're changing the CVSS then perhaps attack complexity should also be higher as local file access would be dependent on whatever privileges the process is started with.

According to the way NIST has traditionally scored CVSS (before the slowdown), CVSS should always be scored for worst case, not best case. This is from hearing their team speak during a panel at Vuln Con.

Attaching external files (or including external images, stylesheets, etc.) is normal behavior for WeasyPrint, just as other browsers.

All browsers have the same origin-policy to protect against SSRF, RFI, and LFI. AFAIK WeasyPrint does not ship with a same-origin policy by default.

Many security concerns (including the ones added here) are listed in the documentation.

I do notice LFI mentioned here, but not SSRF. Out of curiosity, why not have a url_fetcher that does nothing by default, and then give end-users the flexibility of specifying one as an opt-in, instead of requiring an opt-out of this security hardening?

I have a few more thoughts, but unfortunately, this is all the time I have rn. I'll try to return to this on Monday. I'm going to be away camping Friday-Sunday.

@darakian
Copy link
Contributor

The extra information is related to the exploitability of this vulnerability and how, due to the url_fetcher not being configured, the security researchers were able to achieve SSRF, leveraging WeasyPrint, to access the cloud metadata endpoint and thus pivot to accessing the cloud infrastructure for Lyft and earn Lyft's maximum bug bounty.

The extra information does not aid anyone trying to fix this bug which is the primary goal of our advisories. We are not in the business of iterating exploitation vectors for advisories.

According to the way NIST has traditionally scored CVSS (before the slowdown), CVSS should always be scored for worst case, not best case. This is from hearing their team speak during a panel at Vuln Con.

Current guidance is reasonable worst-case impact across different deployed environments and it has been common guidence for decades that services should be run with least privilege.
https://www.first.org/cvss/v3.1/specification-document

@liZe
Copy link

liZe commented Jul 11, 2024

The extra information is related to the exploitability of this vulnerability and how, due to the url_fetcher not being configured, the security researchers were able to achieve SSRF, leveraging WeasyPrint, to access the cloud metadata endpoint and thus pivot to accessing the cloud infrastructure for Lyft and earn Lyft's maximum bug bounty.

Again, the fact that you can achieve SSRF when url_fecther is not configured is not related to the original CVE. The original CVE is about attaching files without using url_fecther, even if url_fetcher is configured. These two problems are not related to each other.

All browsers have the same origin-policy to protect against SSRF, RFI, and LFI. AFAIK WeasyPrint does not ship with a same-origin policy by default.

Open file:///etc/passwd in Chrome, it works. Give remote access to a Chrome session on your server, remote users can see your /etc/passwd file. That’s exactly the same with WeasyPrint, the problem is even simpler than SSRF.

Out of curiosity, why not have a url_fetcher that does nothing by default, and then give end-users the flexibility of specifying one as an opt-in, instead of requiring an opt-out of this security hardening?

For the same reason Chrome let you display your local files and websites on your computer by default. If url_fetcher does nothing by default, WeasyPrint can’t reach local files or network resources, you can’t even launch weasyprint https://weasyprint.org/ weasyprint.pdf or weasyprint file.html file.pdf anymore.

@JLLeitschuh
Copy link
Author

JLLeitschuh commented Jul 12, 2024

We are not in the business of iterating exploitation vectors for advisories.

"We" Github, or "we" the CVE system? My understanding was that linking to exploitation vector documentation of CVEs was standard for the CVE system. IIRC it's usually been flagged as an "Exploit" resource. See for example, every resource flagged as "Exploit" on log4shell: https://nvd.nist.gov/vuln/detail/CVE-2021-44228

I've never had a CNA not attach additional relevant vulnerability exploitation POC documentation to a CVE. Perhaps don't include it in the advisory description, but the external links are all relevant.

@darakian
Copy link
Contributor

It seems like we're starting to get cyclical here. As discussed above the references you're suggesting are not relevant and for the benefit of everyone's time I'm going to close this out.

@darakian darakian closed this Jul 12, 2024
@github-actions github-actions bot deleted the JLLeitschuh-GHSA-35jj-wx47-4w8r branch July 12, 2024 21:50
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.

4 participants