search

facebook / docusaurus

Easy to maintain open source documentation websites.
https://docusaurus.io
MIT License
56.86k stars 8.56k forks source link

Docusaurus depends on trim@0.0.1 with CVE-2020-7753 vulnerability #7275

Closed octogonz closed 2 years ago

octogonz commented 2 years ago

Have you read the Contributing Guidelines on issues?

Prerequisites

Description

@docusaurus/mdx-loader@2.0.0-beta.18 depends on @mdx-js/mdx@1.6.22 which depends on remark-parse@8.0.3 which is breaking our CI builds with this security scanner failure:

[INFO] _________________________________________________________________________________________________________________________________________________ 
[INFO] |Security Alerts                                                                                                                                | 
[INFO] |_______________________________________________________________________________________________________________________________________________| 
[INFO] |Alert title                             |Affected component                      |Severity                      |Due date                      | 
[INFO] |________________________________________|________________________________________|______________________________|______________________________| 
[INFO] |CVE-2020-7753                           |trim 0.0.1                              |High                          |                              | 
[INFO] |________________________________________|________________________________________|______________________________|______________________________| 
[INFO]  
##[warning]Component Governance detected 1 security related alerts at or above 'High' severity. Microsoft’s Open Source policy requires that all high and critical security vulnerabilities found by this task be addressed by upgrading vulnerable components. Vulnerabilities in indirect dependencies should be addressed by upgrading the root dependency.
[INFO] To change the severity threshold or build result, either dismiss the alerts in Component Governance or update the settings of this build task. 
[INFO] Please contact OpenSourceEngSupport@microsoft.com with questions. 
[INFO] Took 1.3847636 seconds to query alerts.

Reproducible demo

N/A

Steps to reproduce

Here's an easy way to repro the issue:

  1. npm init
  2. npm install @docusaurus/mdx-loader
  3. npm audit

                       === npm audit security report ===

                                 Manual Review
             Some vulnerabilities require your attention to resolve

          Visit https://go.npm.me/audit-guide for additional guidance

  High            Regular Expression Denial of Service in trim

  Package         trim

  Patched in      >=0.0.3

  Dependency of   @docusaurus/mdx-loader

  Path            @docusaurus/mdx-loader > @mdx-js/mdx > remark-parse > trim

  More info       https://github.com/advisories/GHSA-w5p7-h5w8-2hfq

Expected behavior

Docusaurus should not depend on packages with high severity security vulnerabilities.

Actual behavior

Security vulnerability

Your environment

Self-service

Josh-Cena commented 2 years ago

Please see https://github.com/facebook/docusaurus/issues/6394

Josh-Cena commented 2 years ago

If your CI requires zero audit warning, your CI is broken. DO NOT rely on security audit for front end build pipelines.

octogonz commented 2 years ago

DO NOT rely on security audit for front end build pipelines.

@slorber Could we get a second opinion on this advice?

However in a large code base, the way an NPM package is used is difficult to keep track of. Code evolves constantly. The analysis itself can be tricky. For example Docusaurus's hybrid client/server rendering makes it non-obvious whether a given NPM package will be executed by the browser runtime or Node.js tooling.

Josh-Cena commented 2 years ago

Apparently, the same issue has been responded to by multiple maintainers:

My response represents the response you will get from every single maintainer, and generally most maintainers in the front-end ecosystem. Docusaurus builds a static site; there's no way a user could be exploited by any security vulnerability whatsoever, unless they actively XSS themselves.

Josh-Cena commented 2 years ago

The data flow of Docusaurus is always one-way: developer -> build pipeline -> deployment env -> user. The user never sends data back to the server. When we say "Docusaurus has a server-side", this server-side is not the same as Next.js: it's build-time only, and once deployed, it's only a bunch of static assets.

The only viable attack vector that we are aware of is if you are outsourcing translations and injecting them via dangerouslySetInnerHtml, which may cause an XSS attack from a malicious translation contributor if there's no proofreading process—due to this consideration, all of our HTML text labels are untranslatable.

The lesson is that security audits are broken—it's not my word, it's the consensus from every OSS maintainer. Whenever you open a security issue somewhere, don't just say "there's a known CVE"—prove to us how this CVE can be actually exploited as an attack vector. Even better, please report this through the security portal for responsible disclosure.

octogonz commented 2 years ago

The user never sends data back to the server.

Take a look at our Docusaurus site:

https://rushstack.io/community/events/

Users do login-with-GitHub and then interact with a Node.js service via web pages that are rendered by the Docusaurus engine. You're really making a lot of assumptions about how a particular Docusaurus site might work.

Josh-Cena commented 2 years ago

That's your own code. Unless it's wired through some official Docusaurus API, we can't help with that. Since you are reporting it to upstream, and you have traced it down to exactly remark-parse, I'll assume you know whether any security threat comes in that chain😅

But anyways, I have spent enough time demonstrating to you why (a) it doesn't matter and (b) how to responsibly report security bugs in the front-end ecosystem. It's now your responsibility to abide to that.

octogonz commented 2 years ago

we can't help with that

Upgrading your @mdx-js/mdx dependency would fix this issue.

Since you are reporting it to upstream, and you have traced it down to exactly remark-parse, I'll assume you know whether any security threat comes in that chain

Yes, but I'm not the person who is a concern. It is the not-knowledgeable engineer who later adds another project to the same monorepo.

For example, let's say this new project is an Express service that happens to invoke trim() on a potentially malicious input, resulting in a real denial-of-service. Our security scanner should flag that, but oops! -- CVE-2020-7753 is exempted because at the time it seemed irrelevant for the Docusaurus project. (If I understand correctly, you're actually arguing to go further and disable every security check for the entire repo; I can't imagine any corporate compliance group tolerating that approach!)

See, security vulnerabilities by nature are tricky to analyze. This is why it's simpler just to work to get them eliminated from the dependency tree, rather than getting into armchair debates about each vulnerability that comes along.

it's not my word, it's the consensus from every OSS maintainer

As a maintainer of OSS myself, I would certainly give a lower priority to fixing an irrelevant-seeming vulnerability. But I would not dismiss it or argue that we shouldn't bother fixing the vulnerability at all, particularly for a high profile project like Docusaurus.

Josh-Cena commented 2 years ago

Upgrading your @mdx-js/mdx dependency would fix this issue.

That's the whole point of every issue I've been pointing to—right? We bump dependencies every week, there's no reason why we won't upgrade it if we can. However, upgrading to MDX v2 has been postponed to Docusaurus v3 because it would be a huge breaking change that will influence every single user, and it also uses ESM which we are not compatible with yet. We have #4029 and #6520 to track them.

If this audit warning really bothers you, consider persuading the remark maintainers to release a patch version for the particular version of remark-parse used by MDX v1. (Alert: I'm pretty sure they won't like the idea)

If I understand correctly, you're actually arguing to go further and disable every security check for the entire repo

I'm saying you should ignore it for Docusaurus because it's a front-end build tool, repeat "build tool". It doesn't offer any runtime features. Your comparison to Express is inadequate and there's a world of difference in between, which is why I said the "single direction of data flow" matters. Express is neither front-end nor "build pipeline". I don't see why my carefully-crafted statement that "DO NOT rely on security audit for front end build pipelines" can be stretched so much.

security vulnerabilities by nature are tricky to analyze

Totally agree. That's why I'm spending so much time demonstrating to you it doesn't matter, instead of instantly dismissing it as "it doesn't matter; now go".

particularly for a high profile project like Docusaurus.

Well, there are much higher-profile projects that take literally the same stance: https://github.com/facebook/create-react-app/issues/11174

Again, we don't work on your team, we can't control your security policy. All we can ask you is, when you open a bug report, please please provide a reproduction. Maintainers have limited time (I have relatively much more time than @slorber but I'm volunteering), so it's important for you to demonstrate yourself how a particular bug is Docusaurus' issue, and for CVEs, how it causes tangible harm. It takes you and your team probably less than 5 minutes to run npm audit and write that report, but had we not been aware of this issue before, it would take us much more time than that to actually verify it's a security threat. The burden is on you, if you want to see it fixed, to demonstrate end-to-end why it's a problem and how we can go about fixing it. Security bugs are no exception to the reproduction → cause track-down → solution proposal workflow.

slorber commented 2 years ago

See, security vulnerabilities by nature are tricky to analyze.

Not all of them IMHO.

We are a build tool that runs in your CI to produce a static site output, based on inputs you commit yourself to Git.

To be affected by such Denial-of-Service you'd have to want to hack yourself and commit malicious content on Git.

And it would only DOS your CI (potentially blocking or slowing down the deployment), not really affecting real site visitors.

I don't think it's good to disable/ignore all security warnings, but I guess in our context you can just ignore all those Regular Expression Denial of Service errors by default.

I believe those shouldn't have Severity: Hight in the first place 🤷‍♂️ and in our particular context it's probably not even Medium.

See also https://overreacted.io/npm-audit-broken-by-design/

However in a large code base, the way an NPM package is used is difficult to keep track of. Code evolves constantly. The analysis itself can be tricky. For example Docusaurus's hybrid client/server rendering makes it non-obvious whether a given NPM package will be executed by the browser runtime or Node.js tooling.

We keep the browser runtime very small in core and theme-classic so there's only a limited number of dependencies being used there.

If there's a DOS vulnerability in a browser package that could affect real users, we'll look into it as a priority.

Now if you don't know if it's server or browser code, you can ask (like in this issue) and we'll tell you.

We don't have the bandwidth to fix all those harmless vulnerabilities ourselves, but I understand if your company remains afraid of such vulnerabilities.

It remains possible for you: