naturalcrit / homebrewery

Create authentic looking D&D homebrews using only markdown
https://homebrewery.naturalcrit.com
MIT License
1.09k stars 326 forks source link

Script injection vulnerability #546

Open jimcullenaus opened 7 years ago

jimcullenaus commented 7 years ago

Problem: inserting arbitrary JavaScript into documents is possible, and it will execute. It also seems to cause the entire rest of the page to not render. The person with edit permission to the document cannot then remove the JS, because the edit field is also blanked out.

Solution: disable <script> tags. Do not allow execution of any JavaScript entered by the users.

For an example, try creating a new blank document, entering some dummy text, and then pasting this at the bottom:

<script> document.title = "The title"; </script> Or, see this document (and its source, which is available).

Note that the title in your web browser changes.

dekelpilli commented 7 years ago

Was just about to report this. In addition to what is specified here, it seems to mess with the user page of whoever owns the page. The following is a screenshot of my user page after I added a script to one of my brews -- https://i.imgur.com/0cdimPi.jpg -- these are all images I've used in my brews. Below the normal page, it also has some plaintext version of some of my brews (\ns, \ts, and all) -- https://i.imgur.com/4q61IaH.png

Also, the screenshots might look yellow, that part is on my end, not homebrewery's.

Also, @jimcullenaus - you can still view the source code section for that brew, then copy paste that to retrieve any work you might have thought you lost. To fix the problem i described, just delete the brew it was in and then you can make a new one with the copy pasted markdown (minus the script).

goopscoop commented 6 years ago

I solved this in a different project with the sanitize-html node module. Super easy to setup. Want me to take care of this for you?

nivthefox commented 6 years ago

Of course, the problem is there's also legitimate reasons to add Javascript, such as zoomable images, or the like. This is not as easy to fix as simply disabling script tags, but I'll look into it.

jimcullenaus commented 6 years ago

I cannot see any legitimate reason to use JavaScript in a homebrewery document. Homebrewery is for building up pages that look high-quality like the D&D published books. There's no room for dynamic content in that. All it does is increase the attack surface.

SANSd20 commented 6 years ago

I concur with @jimcullenaus. While the homebrewery documents do have the ability to link, the over all use case is primarily to create static documents.

If you are needing Javascript for your use case, you might want to fork this project (if that is possible) and come up with your own solution.

AlexeySachkov commented 5 years ago

Should be fixed by #811

calculuschild commented 4 years ago

Closing because this appears to be solved.

5e-Cleric commented 6 months ago

Reopening because a closing script tag still breaks the website, see #3434

dbolack-ab commented 5 months ago

Is this resolved by the DOMPurify addition?

5e-Cleric commented 5 months ago

Is this resolved by the DOMPurify addition?

Testing on a deployment, simply adding </script> somewhere in the text...

image

Nope, still an issue

Getting a single error in the console: Uncaught SyntaxError: Invalid or unexpected token

ericscheid commented 5 months ago

DOMpurify etc are not going to fix this.

This is what is happening..

  1. the brew source is combined with the style source, metadata, and other bits into a javascript object
  2. this object then is put through JSON.stringify()
  3. and the resulting string is then included the http-response payload as a part of the html
  4. it appears like this: <script>start_app({"version":"3.12.0","url":"/edit/5M-3mB5QPvE2","brew":{"text":"brew text </script>","pageCount":"1","renderer":"V3", [etc] })</script>
  5. this is then delivered to the browser as a http-response
  6. the browser then parses the response into html elements
  7. and finally it renders the html, and runs the javascript start_app(...)

The subtle trickiness here though is that in step 6, when the browser is parsing html, it sees <script> and says "ah, this is a script element .. which runs until a closing </script> tag appears". The problem is that the parser sees the string </script> prematurely, and says "ah, end of the <script> element. Any text it finds after that point is outside of the script element. Then, in step 7, when the browser goes to run the javascript, it sees <script>[the script]</script>, where [the script] is all this and only this: start_app({"version":"3.12.0","url":"/edit/5M-3mB5QPvE2","brew":{"text":"brew text. That is not valid javascript, and thus we get the exception of Uncaught SyntaxError: Invalid or unexpected token. The rest of the intended script has been cut off by the parser.

So, in template.js we need to defang the string we pass in to start_app.

When writing js directly into html, this is usually and easily done like this const string = "</"+"script>";

Change line 26 to <script>start_app(${JSON.stringify(props).replace(/<\/script>/g,'</"+"script>'})</script>