froala / angular-froala

Angular.js bindings for Froala WYSIWYG HTML Rich Text Editor.
https://froala.com/wysiwyg-editor
MIT License
306 stars 123 forks source link

XSS Vulnerability #172

Open kp-thibaut opened 6 years ago

kp-thibaut commented 6 years ago

The ngModel.$isEmpty function bypass the native froala security cleaning method, by executing the content of value with the JQuery function.

In my case, I just reuse the froala native html.clean method to fix it.

Like this:

ngModel.$isEmpty = function (value) {
    if (!value) {
        return true;
    }

    value = element.froalaEditor('clean.html', value, [], [], false);

    var isEmpty = element.froalaEditor('node.isEmpty', jQuery('<div>' + value + '</div>').get(0));
    return isEmpty;
};

Example of XSS injection concerned: Script URI scheme XSS test<img src="javascript:alert('XSS')">

stefanneculai commented 6 years ago

@kp-thibaut do you think you could make a PR?

kp-thibaut commented 6 years ago

yup, but I will make more tests before.

kp-thibaut commented 6 years ago

@stefanneculai I'm ready to push my branch, but I haven't the right to do this. BTW, I have fixed some lint issues to and all your tests are down due to new JQuery version (3.3.1) by the froala dependencies.

AS the change is not invasive, I push it without testing it via grunt. I've made some tests by my side.

stefanneculai commented 6 years ago

@kp-thibaut please make a PR: https://help.github.com/articles/creating-a-pull-request-from-a-fork/.

kp-thibaut commented 6 years ago

@stefanneculai it's done.

stefanneculai commented 6 years ago

Thanks. We're going to review it shortly and merge it.

kp-thibaut commented 6 years ago

@stefanneculai any news ?

kp-thibaut commented 6 years ago

@stefanneculai, I follow your advancement but I haven't seen any change for the XSS Vulnerabilities.

I'm pretty concerned by this issue, as froala is deploy on all my customers. Security is one of my main priority. And you still have a huge vulnerability for ALL XSS attacks.

The fix I propose and present on my PR is in production for 4 month on every of my customer.

Can you reply to me with any info or update ?

RHinderiks commented 5 years ago

Has this been fixed yet?

kp-thibaut commented 5 years ago

Not even close. They have fixed a XSS issue for the WYSIWYG editor, but here the problem come from the Directive on this line: return element.froalaEditor('node.isEmpty', jQuery('<div>' + value + '</div>').get(0));

Even if not attached to the body, there is a dom element create with the content of value, but this one is never filtered or secure, so there is still a possibility for XSS attacks.

When calling this jQuery('<div>' + value + '</div>') the malicious JS is called. They need to secure the value before using it. You can avoid those XSS attack by using the html.clean method of froala: value = element.froalaEditor('clean.html', value, [], [], false); and then value is secure.