BUG: XSS vulnerability in iframe attribute src

davidgabrichidze commented 7 months ago

GrapesJS version

[X] I confirm to use the latest version of GrapesJS

What browser are you using?

Edge v122

Reproducible demo link

https://jsfiddle.net/bwreyq29/1/

Describe the bug

How to reproduce the bug? open this link https://jsfiddle.net/bwreyq29/1/ and javascript code attached to src attribute will be executed automatically.

What is the expected behaviour? This code shouldn't run

What is the current behaviour? XSS vulnerability exists

Code of Conduct

[X] I agree to follow this project's Code of Conduct

bernesto commented 7 months ago

This is unavoidable when using fromElement to load from an active DOM element. The element of the page loads and executes synchronously. GrapesJS would never have a chance to process and disarm the XSS html.

This would need to be addressed by preventing the malicious code from ever loading. This should be handled at the server on save. But, if on the client processing is needed, by wrapping the code in script tags, and then sanitizing the code prior to loading it in to the editor like this:

https://jsfiddle.net/bernesto/5gcxa0jm/1/

I do see that when loading via the components property at initialization or dynamically post from a remote source, the same XSS vulnerability exists, and this may be something we want to prevent.

Personally, I believe that the server should be processing all saves and sanitizing. A malicious actor could always emulate a client request and save an malicious XSS to your server regardless of the editor used.

@artf what do you think?

In your opinion, should an XSS sanitizer be built into the editor? And if so, that begs the question; how do you feel about fromElement anyways? Should it be reimplemented as something similar to my example, where content is provided using script tags instead? E.g. maybe providing a source (script template) and destination (editor) element IDs instead?

Not only would this allow us to sanitize for XSS vulnerabilities, but it would also eliminate the browser processing and painting those initial DOM elements twice (once on load, second to the new frame). This may actually speed up the editor load time as a happy benefit.

artf commented 7 months ago

Yeah indeed with fromElement option this is inevitable, also a reason why I marked it as deprecated besides seeing people using it the wrong way. That option should only be used for debugging/prototyping and we need to update the docs 🥲

I wouldn't push an entire sanitizer here (might be a pre-parser option though) but at least I can add a new option to prevent "unsafe" attribute values.

bernesto commented 7 months ago

I think the pre-parser option is a really good idea. It sticks to the 'plug-in' per feature concept.

How about updating fromElement to accept a string element ID or boolean. If bool == true, works as it does now, parsing the container HTML. If a string ID, it uses the contents of the element and the container becomes the editor as it does now. Then we could update the docs in favor of a HTML script template for development. This prevents any braking changes, and promotes proper usage.

const editor = grapesjs.init({
  container: '#gjs',
  fromElement: '#mySource',
  height: '100%',
  storageManager: { type: 0 },
  plugins: ['gjs-blocks-basic']
});

davidgabrichidze commented 7 months ago

Thanks for the insightful discussion. I want to clarify that while I used fromElement in my example, the core issue remains the same with my use of editor.loadProjectData and editor.getProjectData.

Here's how I currently handle HTML compilation:

export class HtmlTemplateEditorComponent
  implements OnInit, ControlValueAccessor
{
....
  private compileHtml(html: string, styles: string, script: string) {
    const regexForIndex = /(<\/body>)/g;
    const everyLastBodyTagIndex = html.search(regexForIndex);
    const result =
      `<head><style>${styles}</style></head>` +
      html.slice(0, everyLastBodyTagIndex) +
      `<script>${script}</script>` +
      html.slice(everyLastBodyTagIndex);

    return result;
  }

  private buildContentModel(): HtmlTemplateEditorContentModel {
    const html = this.editor.getHtml();
    const styles = this.editor.getCss() as string;
    const script = this.editor.getJs();

    return {
      content: this.compileHtml(html, styles, script),
      contentObject: { ...this.editor.getProjectData(), assets: null },
    };
  }
....
}

My primary focus is on best practices for preventing XSS within this context. Will the upcoming pre-parser feature in GrapesJS adequately address these concerns?

Looking forward to your suggestions.

bernesto commented 7 months ago

This will depend on how @artf decides he'd like that architected. I can see how it would make the sense to be inserted in the pipeline where projectData and components sources coalesce prior to being written to the frame DOM and/or when reading HTML/CSS out.

That said, you mentioned 'best practices'. BP for security would be parsing and filtering the data at the server prior to storage, where it is beyond the manipulation of nefarious parties. e.g. A user may be well meaning, but a browser plugin may not. For instance it may take advantage of client RTEs with contenteditables to inject hidden code to mine bitcoin in a web worker for instance... (not a suggestion by any means lol)

Client side code filtering should be exclusively used for user experience. i.e. Bubbling up warnings from user interactions prior to round tripping to the server to create a responsive and intuitive experience. I speculate that is part of why Artur is more inclined to not include this filtering in the client, favoring pre/post processor of some sort so you can float your own if you need. I personally really like his modular approach and flexibility, as this allows choice in implementation.

artf commented 7 months ago

Totally agree with @bernesto indeed no matter how hard we try to make it safe, it will never be enough and I don't want to give the impression that the library is "so safe" to justify a missing server-side validation. The current options (eg. allowUnsafeAttr, allowUnsafeAttrValue) avoid only the basic common stuff.

davidgabrichidze commented 7 months ago

@artf @bernesto thanks for you support and super fast response.

@artf what is ETA of the next Release?

artf commented 7 months ago

@davidgabrichidze I'll try to push the next release in a few days.

GrapesJS / grapesjs