You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.
Currently, the notifications package takes the raw output of marked and writes it to the notification's innerHTML. If notification content can be user-controlled in some way, this makes it easy to write security exploits using tags like <iframe></iframe> (despite Atom's cross-origin protections, it's possible to load arbitrary files on disk, for example).
While one could argue that it's the responsibility of the notifications caller to sanitize the HTML before sending it through, it's often not obvious that 1) notifications take Markdown and 2) notifications accept HTML in Markdown.
Description
Currently, the notifications package takes the raw output of
marked
and writes it to the notification'sinnerHTML
. If notification content can be user-controlled in some way, this makes it easy to write security exploits using tags like<iframe></iframe>
(despite Atom's cross-origin protections, it's possible to load arbitrary files on disk, for example).Steps to Reproduce
Example:
See https://statuscode.ch/2017/11/from-markdown-to-rce-in-atom/ for a real example of how to turn this into an RCE if an unguarded HTML file can be discovered on disk.
While one could argue that it's the responsibility of the notifications caller to sanitize the HTML before sending it through, it's often not obvious that 1) notifications take Markdown and 2) notifications accept HTML in Markdown.
Having HTML in notifications is definitely still useful - but would it be an acceptable compromise to run the output of
marked
through https://github.com/cure53/DOMPurify before displaying it? (This is what settings-view does as well: https://github.com/atom/settings-view/blob/e962a54f488cfd5b3b0be5ab99be70fa8a2260e8/lib/package-readme-view.js#L43)I can submit a pull request if this seems reasonable.
The text was updated successfully, but these errors were encountered: