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

XSS in p:textEditor #3214

Closed
cnsgithub opened this issue Jan 25, 2018 · 29 comments

Comments

Projects
None yet
7 participants
@cnsgithub
Copy link
Contributor

commented Jan 25, 2018

1) Environment

  • PrimeFaces version: primefaces-6.2.RC2-SNAPSHOT
  • Application server + version: Wildfly 11
  • Affected browsers: all

2) Expected behavior

There is a limited set of HTML tags quilljs is able to generate via toolbar. However, it is possible to manipulate the DOM on the client side and submit a tampered version to the server which in turn should sanitize the HTML before persisting or sendig it back to the client.

3) Actual behavior

It is possible to inject arbitrary HTML/JS code in DOM. The code could then be persisted to the database and reflected to clients trying to edit using p:textEditor or getting the text rendered through any component that uses escape="false".

4) Steps to reproduce

You can test the reflective XSS on your showcase site.

  1. Go to https://www.primefaces.org/showcase/ui/input/textEditor.xhtml
  2. Open developer tools (F12) and enter in the console: $('.ql-container').next('input:hidden').val("<script>alert('oops');</script>");
  3. Press the submit button. An alert will be shown after returning from server side.

5) Sample XHTML

n/a

6) Sample bean

n/a

7) Mitigation

This time it is a bit more difficult to fix the issue, because we cannot simply escape all HTML/JS. However, what we can do is to sanitize user HTML input - depending on the quilljs features turned on and thus are available via the toolbar. I.e., if images are not allowed, we should sanitize in a way, that all image tags are dropped and so on. And once again, OWASP is your best friend: https://github.com/OWASP/java-html-sanitizer Btw, I am not a member of OWASP... ;-) I have used it successfully as a workaround for this PrimeFaces issue.

@melloware

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2018

Isn't that exactly why escape="false" is offered? You are turning off the escaping because you know you want to render HTML? I use it all the time in outputText that I want to format.

@cnsgithub

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2018

I have to disagree.

Would you also say it's okay if you had defined a p:spinner component with min="0" and max="10", but JSF accepted -1, i.e. a value that is out of the bounds?

You can compare this to p:textEditor: the developer decides to use it in the assumption the component only allows basic HTML like styling, fonts and images. But unfortunately there is the bad guy that submits arbitrary HTML and the server (PrimeFaces) accepts it without validating.

That's definetely not okay.

@martin654

This comment has been minimized.

Copy link

commented Jan 26, 2018

Hi!
I will share my experience with escape="false". From my point of view it is a security risk using it without thinking about consequences. In our app we used it only for output, but it was always sanitized using org.jsoup.Jsoup.clean method. The example @cnsgithub provided definitely is a security risk from my point of view.
Cheers!

@melloware

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2018

I see what you mean. I thought you were saying to auto-cleanse the WYSIWYG entry for the customer. There may be some use cases where the end user wants to allow JavaScript typed in there. Maybe they are using this to dynamically edit some web pages that get stored in the DB and then rendered upon command. So as long as you leave the ability to turn the sanitize completely OFF if the user chooses to.

What makes this component different than say a Spinner validation is that you don't know all the use cases for how someone will use it. For example WordPress lets you enter HTML and Javascript because that is stored in the database and rendered to the end user that way. Wordpress understands the whole point of giving you an editor is that you are editing web pages Javascript and all.

@cnsgithub

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2018

I am not sure if I got it correctly. Where can javascript be entered in quilljs, p:textEditor resp.? Just can't find an official way to do so.

@melloware

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2018

@cnsgithub Maybe you are misunderstanding what I am saying? Right now do the following:

  1. Go to https://www.primefaces.org/showcase/ui/input/textEditor.xhtml
  2. Right in the WYSIWYG editor on the screen type <script>alert('oops');</script>

You will see the Javascript alert and I agree that is bad.

What I am saying is what if I am using the TextEditor component in my webapp to allow admins to edit some text that I want to store in a database that happens to be in the language of Javascript? So I am using the editor to truly edit Javascript snippets that I store in the database. I just want to make sure that whatever solution is presented allows this scenario to still be viable and that by cleansing any JS or HTML code you don't actually affect the text that was entered by the user.

@jxmai

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2018

Hmm, I wasn't aware of this issue. XSS is doable because quill itself does not sanitize the HTML output.
A comment which can contain an execution JS on other machines is a bad news ...

Anyway, The expected behavior should be the same as the original editor:

https://www.primefaces.org/showcase/ui/input/editor.xhtml

image


Someone discussed this problem on quill site. Let's see if those info can help to give a clue.

https://github.com/quilljs/quill/issues/510

quilljs/quill#514

@cnsgithub

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2018

Ok, indeed I wasn't aware that it is even simpler and can be done just by copy&paste...

If you really think it could be a valid use case, then we should definetely provide an opt-in attribute for this.

And yes, the old p:editor does not have this copy&paste 'feature'.

@cnsgithub

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2018

Pull request: #3221

@prakhartkg

This comment has been minimized.

Copy link

commented Apr 12, 2018

Hi,

Can anyone give me any solution on this I am using primeface texteditor and while putting
"><img src=x onerror=confirm('hello')>
its executing because I am using h:output text with escap=false

@cnsgithub

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2018

@prakhartkg Use e.g. https://github.com/OWASP/java-html-sanitizer by your own as long as my pull request isn't merged.

tandraschko added a commit that referenced this issue Oct 5, 2018

Merge pull request #3221 from cnsgithub/fixes-3214-texteditor
[OWASP] fixes #3214 - XSS in p:textEditor

tandraschko added a commit that referenced this issue Oct 5, 2018

tandraschko added a commit that referenced this issue Oct 5, 2018

Merge pull request #4121 from primefaces/revert-3221-fixes-3214-texte…
…ditor

Revert "[OWASP] fixes #3214 - XSS in p:textEditor"

@tandraschko tandraschko reopened this Oct 5, 2018

@tandraschko

This comment has been minimized.

Copy link
Member

commented Oct 5, 2018

We can't use the PR as it depends on Guava, which is to big to be shaded.
Hoping for another HTML Sanitizer PR ;)

@cnsgithub

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2018

That's a pity. 😢 But thanks for giving it a try.

I think other HTML sanitizers are rare...

Let's fork OWASP/java-html-sanitizer and do it without guava just to fit PrimeTek's no-third-party-deps policy. 😉 I just checked the occurrences of guava imports: https://github.com/OWASP/java-html-sanitizer/search?q=in%3Afile+%22import+com.google%22&unscoped_q=in%3Afile+%22import+com.google%22 That's too many, sorry. And it would of course prevent us from version upgrades.

Just an idea, couldn't we just postpone this security feature to 6.3 or 7.0 and add a compile-time dependency to OWASP/java-html-sanitizer only, mentioning it in the user guide as already done for other deps?

image

@tandraschko

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

the problem is that guava is really a fat boy and adding it as a dependeny could also cause conflicts when other libraries use guava, too.
Same as with ASM, therefore everyone is shading it.

@cnsgithub

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2018

Normally this is where jigsaw or wildfly modules or things like that come into play?

@tandraschko

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

yup

@cnsgithub

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2018

Let's fork OWASP/java-html-sanitizer, shade guava there and release a all-in-one version? Or ask them to do so?

@cnsgithub

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2018

@tandraschko

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

is it the right project?
seems like no owasp project uses guava, just java-html-sanitizer

cnsgithub added a commit to cnsgithub/primefaces that referenced this issue Oct 23, 2018

@cnsgithub

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2018

Just in case someone decides to revive PR #3221, please don't forget to integrate rel="noopener noreferrer" as denoted in:

@mertsincan

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

Thanks a lot, @cnsgithub ;)
We decided to add this fix. Github files seem confused. So I've added it manually. Could you please check it?
Many thanks.

@melloware

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2018

So does this mean PF brings in the Sanitizer and Guava no matter what now?

@tandraschko

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

yes
if someone uses textEditor, he must add the sanitizer + guava dependency
we will not shade it until the sanitizer removes guava

@melloware

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2018

OK then doesn't the dependency need to be marked with optional or provided and with newer 20181114.1 version?

        <dependency>
            <groupId>com.googlecode.owasp-java-html-sanitizer</groupId>
            <artifactId>owasp-java-html-sanitizer</artifactId>
            <version>20181114.1</version>
            <optional>true</optional>
        </dependency> 
@melloware

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2018

OK just so we are all on the same page. I just tried to run the PF Extensions showcase and got this error parsing faces-config.xml.

Caused by: com.sun.faces.config.ConfigurationException:
Source Document: jar:file:/C:/Users/elefkof/.m2/repository/org/primefaces/primefaces/6.3-SNAPSHOT/primefaces-6.3-SNAPSHOT.jar!/META-INF/faces-config.xml
Cause: Class 'org.primefaces.component.texteditor.TextEditorRenderer' is missing a runtime dependency: java.lang.NoClassDefFoundError: org/owasp/html/HtmlPolicyBuilder

So to me it appears this dependency is NOT optional and is now a required compile time dependency. I am OK with that but then we need to remove the "provided" marker and let everyone know that HTML sanitzier and Guava are now coming with PrimeFaces as transitive dependencies.

I notice it was added to the PF Showcase to make it work.

        <!-- HTML sanitizer for the textEditor -->
        <dependency>
            <groupId>com.googlecode.owasp-java-html-sanitizer</groupId>
            <artifactId>owasp-java-html-sanitizer</artifactId>
            <version>20181114.1</version>
        </dependency>

So basically any PF app cannot even start up without this dependency?

@tandraschko

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

i will fix this to make it optional

still bit unhappy with everyting
adding 2,5mb guava for a 200kb lib is bullshit... -> https://twitter.com/olivergierke/status/1065195542916067328

@cnsgithub

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2018

@mertsincan I'm happy to read that! 🍺

@tandraschko

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

@melloware could you retest? (If possible please also test the warning log, no time left for me today)

@melloware

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2018

@tandraschko Looks great I was able to start the PE Showcase without the HTML sanitizer dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.