Repository: airbnb/knowledge-repo
Issue category: Security Vulnerability

Issue:
XSS vulnerability
Auto-reviewers: @NiharikaRay @matthewwardrop @earthmancash @danfrankj 

Hello, guys!

There is a cross-site scripting (XSS) vulnerability in the Knowledge Repo 0.7.4 (other versions may be affected as well) which allows remote attackers to inject arbitrary JavaScript via post comments functionality.

Steps to reproduce:
Just open any post like this https://site.com/post/posts/new_report.kp
and add the following code as a comment/PoC:
<script>alert("Client side attacks is possible here!");</script>

Impact: An unauthenticated evil user can leverage the vulnerability to conduct various types of client side attacks, for example, conducting a browser mining, redirecting users to malicious sites or hijacking the user’s browser using malware.

As an irrefutable evidence please take a look at the attached screen-shot.

Mitigation: Details on how to prevent XSS can be found here: https://www.owasp.org/index.php/XSS_(Cross_Site_Scripting)_Prevention_Cheat_Sheet

PoC:
![xss](https://user-images.githubusercontent.com/6551435/41200588-f832ab3c-6caf-11e8-9f4f-117d153a5473.png)
XSS vulnerabilities in comments marked as safe
Since all comments are marked as safe in `markdown-rendered.html`, someone could write Javascript as a comment and it gets interpreted as raw JS, opening up an XSS vulnerability. It seems like this is done so comments can be written in markdown, possibly for LaTeX as well (though removing `|safe` still seems to render LaTeX correctly). Do you consider the XSS vulnerability a problem? Would you accept a PR to fix it? If so, I was thinking that script tags could be stripped at time of submission, prior to posting to the database. You could also potentially strip all HTML period. It's not clear whether raw HTML should be necessary for any of the supported comment types. 

Thank you!

Auto-reviewers: @NiharikaRay @matthewwardrop @earthmancash ancash @danfrankj



Hints:
Thanks Ekzorcist. I'll make sure this gets fixed before the next release :).
XSS is also present on 0.7.6
Is this one fixed @fahrishb?
Also raised in #254. We should prioritize fixing this.
Hi @redyaffle,

I suspect that this is just one in a myriad of security vulnerabilities that exist throughout the knowledge repo codebase. In the past, this was not considered to be a particularly serious problem, since we internally used the knowledge repo in a trusted computing environment, but as it grows and matures, this sort of thing will be a big deal. I haven't yet personally reviewed much of the code pertaining to comments, so I cannot say exactly why we mark comment content as safe, but my guess is that as you say we render the content server-side before displaying it. It seems, therefore, reasonable to think about adding some functionality to strip unwanted tags/etc from the comment pre-rendering, and if you want to submit a patch that does this, it will be more than welcome!

Many things for getting involved!

Patch:
diff --git a/knowledge_repo/app/routes/comment.py b/knowledge_repo/app/routes/comment.py
index 988d82840..5fe02bce6 100644
--- a/knowledge_repo/app/routes/comment.py
+++ b/knowledge_repo/app/routes/comment.py
@@ -6,7 +6,7 @@
   - /delete_comment
 """
 import logging
-from flask import request, Blueprint, g
+from flask import request, Blueprint, g, escape
 
 from .. import permissions
 from ..proxies import db_session, current_user
@@ -43,8 +43,7 @@ def post_comment():
                              .first())
     else:
         comment = Comment(post_id=post.id)
-
-    comment.text = data['text']
+    comment.text = escape(data['text'])
     comment.user_id = current_user.id
     db_session.add(comment)
     db_session.commit()


Test patch: