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

Security issue requires project maintainers attention #646

Closed
lirantal opened this issue Mar 25, 2019 · 19 comments
Closed

Security issue requires project maintainers attention #646

lirantal opened this issue Mar 25, 2019 · 19 comments

Comments

@lirantal
Copy link

Hi,

As a member of the Node.js Security WG
I would like to draw your attention to a security report that has been made regarding this package.

I have made attempts to contact the person identified as a maintainer of this package but did not get any answer. What is the best way to reach someone with commit rights over this repo and hopefully npm publishing rights as well, in order to invite them to privately discuss the issue on the HackerOne platform and provide a resolution?

Thanks,
Liran Tal.

References:

@sintaxi
Copy link
Owner

sintaxi commented Mar 25, 2019

Hi Liran, Thanks for reaching out. You can email security vulnerabilities to brock [at] sintaxi.com.

@lirantal
Copy link
Author

@sintaxi I had already sent 2 invites there.
This is now the 3rd time I'm doing. Please check your inbox/spam folder for 2 different invitations (2 issues) and please join them.

@sintaxi
Copy link
Owner

sintaxi commented Mar 25, 2019

@lirantal I've added a SECURITY.md file which includes instructions on how one can report a security vulnerability for the harp library.

https://github.com/sintaxi/harp/blob/master/SECURITY.md

@lirantal
Copy link
Author

Thanks, I appreciate the extra awareness for security concerns 👍
Does it mean you will not join the conversation on HackerOne for the currently outstanding 2 issues that needs attention?

I'm curious why you prefer handling disclosures directly instead of relying on a Node.js Foundation's governing body for this purpose?

@sintaxi
Copy link
Owner

sintaxi commented May 30, 2021

@lirantal Can you please remove the erroneous warning HackerOne has on harp? Do we have to go directly to npm to clear this up?

@lirantal
Copy link
Author

I'm on a weekend time off with the family. Can you share the link to which report is it and the error in it? I'll take a look as soon as I can.

@sintaxi
Copy link
Owner

sintaxi commented May 30, 2021

@lirantal thanks for the quick reply. Feel free to put this off until after the weekend. I don't want to interrupt your time with family.

All versions of Harp are currently flagged as having a "XSS vulnerability" for not escaping output of markdown.

All versions of harp are vulnerable to Cross-Site Scripting. Due to misconfiguration of its rendering engine

The report assumes a third party has access to modifying markdown files which is a fundamental misunderstanding of what harp is. The primary function of harp is to send unescaped markup to the browser and that is the expected behaviour of its users.

harp does not sanitize the HTML output allowing attackers to run arbitrary JavaScript when processing malicious files

For an attacker run arbitrary JavaScript they would have to have access to the file system which would clearly be a compromised system under the security model of a static web server. In Harp, markdown files share the same security model as all other files. Any script that can be added to a markdown file could just the same be served by adding the script to a .js or .html file to be served to the browser. Its an error to consider this a vulnerability because that is Harp's intended purpose and expected behaviour.

Please redact all vulnerability flags for Harp OR demonstrate how Harp is vulnerable beyond having access to the files it serves - which is no different than the vulnerabilities of NGINX, Apache, or any other static web server.

Thanks for your attention on this matter. I hope this can be addressed without further complication.

Have a good time with your family. No response is expected until after the weekend.

thanks,
Brock

# npm audit report

harp  *
Severity: moderate
Cross-Site Scripting - https://npmjs.com/advisories/806
No fix available
node_modules/harp

1 moderate severity vulnerability

Some issues need review, and may require choosing
a different dependency.

@lirantal
Copy link
Author

@sintaxi thanks for explaining. From a personal note, and also as the original triager of the security report, I still consider harp vulnerable. My rational for this is:

  1. Unlike the statement you made in bold, there's nowhere I see in the README that harp is used to send unescpaed and potentially insecure input to the browser.
  2. While you assume that files are already vulnerable if someone would compromise the system - that's a bit naive and missing cases like: A; I'm a developer who mistakenly added HTML/Unescaped markdown syntax that put me in harm's way. B; I'm using this as a blog system and others contribute blog items for me via pull requests that introduce Markdown files. What happens if they push in malicious JavaScript?

Not to promise any action here on any upstream parties who handle the vulnerability but I'd say that if you accept that the behavior of harp is insecure by default then you should put it as such in a cleary security disclaimer on the top and explain to users the exact security risks involved.

A small note about the vulnerability sources - I'll share the information with the Snyk security team, which has the report over at https://snyk.io/vuln/SNYK-JS-HARP-174141. npmjs advisories and others would need to have their own personal handling it. I suggest though that we first reach a conclusion here and then take it upstream.

@sintaxi
Copy link
Owner

sintaxi commented May 31, 2021

@lirantal Your claim of vulnerability is based on false presuppositions about what markdown is as well as a contrived scenario that relies on security breakdowns in places that have nothing to do with harp. You are misleading the public with this claim and you have damaged the harp project as a result. I implore you to reconsider your position before doing more damage.

  1. Unlike the statement you made in bold, there's nowhere I see in the README that harp is used to send unescaped and potentially insecure input to the browser.

Yes, it does. The very first line of the README is "Harp is a static web server that also serves Jade, Markdown, EJS, Less, Stylus, Sass, and CoffeeScript as HTML, CSS, and JavaScript without any configuration" That is what the README has always said. Harp is described as "a static web server with built in preprocessing". No reasonable person expects a static web server to escape the html it sends to the browser because that is not what static web servers do.

  1. While you assume that files are already vulnerable if someone would compromise the system - that's a bit naive and missing cases like: A; I'm a developer who mistakenly added HTML/Unescaped markdown syntax that put me in harm's way. B; I'm using this as a blog system and others contribute blog items for me via pull requests that introduce Markdown files. What happens if they push in malicious JavaScript?

How is serving JavaScript a vulnerability of harp? Blogging with harp is no different than blogging with HTML. Why would there be any expectation that the security model would somehow change for that one and only preprocessor when that is not claimed anywhere in the harp documentation. If anything harp goes out of its way to state that harp sees Markdown no differently than it sees an HTML file. "All preprocessing happens implicitly, so there is nothing to setup. Just name your file with an .md extension, and the Harp web server will serve it as a .html file." and "Both index.md and about.md will be served as an .html file".

Can you as a third party modify the markdown files of a harp project? No. Does harp have any built in way to do this? No. Does harp have any mechanism to expose markdown in a different security model than HTML or JavaScript? No. So why are you imposing this contrived use case without any expectation that security measures would be taken by someone who uses harp that way? The Markdown specification states "Markdown is a lightweight markup language" and that is how it is used in harp.

The so-called vulnerability has not been demonstrated and is therefore invalid. It was reported by someone with a negative reputation on the HackerOne platform who incorrectly assumed third-parties have access to editing the source code of harp projects - which they don't. The track record of the reporter shows they spammed vulnerability reports for several libraries that use markdown.

Please redact this vulnerability report for all versions of harp.

@sintaxi
Copy link
Owner

sintaxi commented May 31, 2021

@lirantal are you aware that your own blog that uses Eleventy treats Markdown the same way as Harp?? So if unescaped markdown output is a security vulnerability why would you use a blogging platform that doesn't escape markdown output?

Screen Shot 2021-05-31 at 1 46 47 AM

@sintaxi sintaxi pinned this issue May 31, 2021
@lirantal
Copy link
Author

lirantal commented Jun 1, 2021

@sintaxi I appreciate your involvement here and want to state again that my comments here are my own personal opinion (as I said already) and don't reflect the Node.js ecosystem security WG or Snyk's. That said, I've shared your request internally with Snyk and I'll update when the folks triage and give me some input.

I entered this conversation as an individual who is active in the security space and am here to help and converse proactively and with constructive feedback. Entirely with good faith and with appreciation to the project and to you as an open source maintainer ❤️

--

To explain my rationale about the vulnerability and how it is nuanced in the detail as something different than say nginx/other static web servers:

  • Harp goes a step further than just serving files, it actually preprocesses them and renders HTML out of a Markdown file. As far as I know, nginx doesn't do this (by default). The vulnerability was reported specifically about this capability, not about serving an HTML file which has a script tag in it. If that was the case, I'd completely agree with you.
  • Unlike nginx and other static web servers, harp can and promotes a programmatic API usage pattern which means developers might adopt it in their projects to build blogs and such, without understanding the potential security pitfalls

If I was a maintainer of this project, I'd put a very clear security disclaimer that explains the security concerns and that harp doesn't do anything on its own to sanitize preprocessed files.

@sintaxi
Copy link
Owner

sintaxi commented Jun 1, 2021

@lirantal thank you helping us move this towards a resolution and taking the time to explain your rationale. Regardless of how this conversation resolves I intend to clarify in documentation how Harp processes Markdown so there is no confusion. (perhaps that is enough for you). At the conclusion of this post I present two options I see as a way to move this forward.

Your rationalle seems to be based on the sum of two claims. Those claims are thus...

Claim 1: Markdown is a "safe by default" subset of HTML. <script> tags are to be escaped to be valid Markdown. Exposing unescaped <script> tags violates the markdown specification.

Claim 2: Harp promotes/exposes an API that would make serving/editing Markdown files that have been edited by a 3rd-party easy or likely.

To me your rationalle is predicated on both the claims being true. If Claim 1 isn't true, then there would be no reason to differentiate Markdown from HTML, JavaScript, Jade, or EJS files and harp is just doing what it says it does. If Claim 2 isn't true then there is no compromised Markdown to worry about.

Going back to the NGINX example. If we put a Rails app behind NGINX to enable third-party editing of the static files that NGINX serves, would that be a vulnerability of NGINX? Should a "Cross-Site Scripting" vulnerability warning be shown to everyone who installs NGINX because you can use Rails to edit the files that NGINX serves? Just so it's understood that is exactly the burden that has been placed on the harp library. The possibility of putting a dubious application behind Harp is being proclaimed as a vulnerability in harp.

You wield a lot of power and it would help us a lot if we can reach an agreement. I propose two suggestions...

  1. You demonstrate both claims are true. 1) Markdown is specified as a safe by default subset of HTML which should escape arbitrary HTML tags, ...and 2) Harp promotes/exposes functionality that makes unreviewed third party editing of Markdown files easy or likely. If this is the case we will agree to display a "disclaimer" as you suggest.

  2. Alternatively, we update the docs to explicitly include the example <script>alert("hello javascript")</script> and clarify how it is used so there is no confusion how harp processes Markdown. In this case I think vulnerability warnings for all versions of harp should be redacted since the behaviour is compliant with the Markdown specification and works as described in the README.

Are you willing to agree to either of these two options?

@lirantal
Copy link
Author

lirantal commented Jun 1, 2021

I'm not sure how you concluded that I'm making any of these claims, but I guess things can easily get lost in just a conversation :-)

The point I am strongly trying to make is that, unlike Nginx being a pure web server that just serves static files, harp goes a step further and actually preprocesses a file's contents to render it to the browser. It perhaps a nuanced detail, but very important in my opinion to not forgo.

I'll try to further iterate that point with more examples to see if I'm able to convey my train of thought better:

  • If React's API by default would allow XSS from JSX you'd consider that a vulnerability, or at least a very insecure default "way of working". Wouldn't you? Because at this point, React is no longer just drawing HTML from you to the browser, but rather preprocessing specific input you give it into the browser and may do so insecurely. So, luckily for us, it does escape potentially dangerous strings.
  • If a static web server would be vulnerable to path traversal, wouldn't you consider it a vulnerability? Sure, its purpose is to serve static files, but if you can traverse up the file system tree, users of this web server may not be aware of that and this could expose sensitive files.
  • Docsify.js was found vulnerable because it allows to load Markdown files and render them to the page as HTML, but it didn't take care of sanitizing files that are originating from remote URLs.
  • If a module bundler like webpack, or a CSS preprocessor like say SASS or others would bundle up the CSS files from you and serve them to the browser, but doing so, it might have a security bug that in certain situations it bundles files incorrectly which creates an XSS. Wouldn't you consider this a security flaw? Sure, it just sends CSS from you to the browser, but it doesn't send it as is, but rather it invokes some logic in the middle which preprocesses the CSS files. Similarly, harp preprocesses markdown files, not sending them directly to the browser as-is.

My last point to make here is - if someone would say "hey Harp is vulnerable because it is serving my HTML which has script tags in it" we would both rule it as invalid because harp just reads the contents of an HTML file, and sends it to the client. If you feed it a security flaw, it sends out a security flaw. The same can be said on an insecure API like Node.js's system command - if a developer does child_process.exec("cd " + userDirectory).

However, in the case of harp, it processes a Markdown file and transforms it to HTML. Harp effectively applies a new logic on an input (the input is file.md), that results in a new output (file.html which is the HTML as a result of the Markdown).

All of the above summarizes my own personal take on why I'd make a case for a web server being vulnerable if a vulnerability such as https://snyk.io/vuln/SNYK-JS-HARP-174141 would've been discussed.

--

Now, to a more practical set of actions. Why as a maintainer of a project, wouldn't you want to provide a security warning/disclaimer to users of your project, so they're aware of potential vulnerabilities? Marked clearly stated how it works by default, which may put users at risk, but it is clearly conveyed in the README:
image

I noticed that you updated the README to similarly show that Markdown data is rendered unsanitized. I think that's a good practical approach that relieves the project from the risks and transfers it to the end user (this is a known risk management security practice).

To be explicit again - I have no weigh in retracting the vulnerability or not, all of this discussion so far is me conveying my own thoughts on the matter and I'm still waiting to hear back from the Snyk security research team on this. I have reasons to believe that your notice in the recent README update would help but let's wait and see.

@sintaxi
Copy link
Owner

sintaxi commented Jun 1, 2021

I think we finally we have a unambiguous way to come to an agreement...

if someone would say "hey Harp is vulnerable because it is serving my HTML which has script tags in it" we would both rule it as invalid because harp just reads the contents of an HTML file, and sends it to the client. If you feed it a security flaw, it sends out a security flaw.

So it boils down to this one thing. If HTML is a subset of Markdown and arbitrary HTML tags such as <script> are valid then you retract your claim that Harp's handling of Markdown is a vulnerability. If Markdown is a safe-by-default subset of HTML and arbitrary HTML tags are invalid, then Harp ought to escape the output or add a disclaimer. Agreed?

@sintaxi
Copy link
Owner

sintaxi commented Jun 1, 2021

@lirantal Docify is a client-side proxy that loads untrusted third-party Markdown files located at any publicly available URL. The vulnerability was demonstrated on the live docsify.js.org website by setting the fragment identifier at docsify.js.org to an untrusted python server. Eg docsify.js.org/#/unknown-third-party.com/do-bad-stuff

docsify.js uses fragment identifiers (parameters after # sign) to load
resources from server-side .md files. it then renders the .md file inside
the HTML page.

For example : https://docsify.js.org/#/quickstart sends an ajax to
https://docsify.js.org/quickstart.md and renders it inside the html page.

due to lack of validation it is possible to provide external URLs after the
/#/ and render arbitrary javascript/HTML inside the page which leads to
DOM-based Cross Site Scripting (XSS).

The report includes steps to reproduce: https://www.exploit-db.com/exploits/48681

@sintaxi
Copy link
Owner

sintaxi commented Jun 2, 2021

Just so that it's understood. The python server was able to send any code it wanted to the docsify.js.org website. The vulnerability was caused by a lack of validation identifying external links. The XSS attack was created with raw html not unescaped markdown.

Python Server...

flask.Response("Html Injection and XSS PoC</p><img src=1
onerror=alert(1)><img src=1 onerror=alert(document.cookie)><p>")

@sintaxi
Copy link
Owner

sintaxi commented Jun 10, 2021

The CommonMark spec for Markdown has a section for HTML blocks.

An HTML block is a group of lines that is treated as raw HTML (and will not be escaped in HTML output).

Example 140 tests script tags specifically...

Mardown...

<script type="text/javascript">
// JavaScript example

document.getElementById("demo").innerHTML = "Hello JavaScript!";
</script>
okay

Output...

<script type="text/javascript">
// JavaScript example

document.getElementById("demo").innerHTML = "Hello JavaScript!";
</script>
<p>okay</p>

The GitHub Advisory Curation Team has concluded Markdown in Harp is compliant with the specification and should not be considered a security vulnerability. They have redacted the XSS vulnerability report for all versions of Harp.

All other known issues have been resolved and all security advisories for Harp are lifted. Harp now installs without any warnings. Yay! Closing this issue now.

Special thanks to @lirantal, @jdcauley, @misterhtmlcss, and the GitHub Advisory Curation Team for helping us address these concerns and move Harp forward.

@sintaxi sintaxi closed this as completed Jun 10, 2021
@lirantal
Copy link
Author

@sintaxi chiming here with an update from the Snyk team, who decided to deem this issue not a vulnerability (updated here: https://snyk.io/vuln/SNYK-JS-HARP-174141). I genuinely appreciate the time, effort, and patience on this, and from a personal side note, thanks for the open conversation with me ❤️ . Even when we were not always seeing eye to eye, you kept the discussion to constructive feedback and positive 🤗

@sintaxi
Copy link
Owner

sintaxi commented Jun 14, 2021

Thank you @lirantal. That is a very gracious message (probably more than what is deserved). Thanks keeping things level and productive. 👍

@sintaxi sintaxi unpinned this issue Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants