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

🐛 Fix XSS Vulnerability in SVG Upload Handling #19646

Closed
wants to merge 3 commits into from

Conversation

TeneBrae93
Copy link

This PR addresses a vulnerability in the handling of SVG file uploads (especially in staff profile pictures) within Ghost CMS. By integrating DOMPurify, we ensure that SVG files are sanitized to remove potential XSS attacks, enhancing the security of the platform without affecting the core functionalities.

Changes Made:

  • Integrated DOMPurify to sanitize SVG content during the upload process in update.js.
  • Updated package.json to include dompurify as a dependency, ensuring the sanitization process is robust and effective.

Impact:

  • Enhances the security of Ghost CMS by preventing XSS attacks through SVG uploads.
  • Maintains the user experience and functionalities related to SVG uploads, ensuring only malicious content is targeted and removed.

Testing:

  • Manual testing was conducted with various SVG files to ensure legitimate uploads remain unaffected while potential XSS payloads are sanitized.

This fix contributes to the ongoing effort to maintain and improve the security posture of Ghost CMS, ensuring it remains a safe and reliable platform for users and developers alike.

Additional Notes:

  • This PR is submitted in response to the ability to execute JavaScript in SVG files when uploaded as profile pictures. Further discussions and contributions to this fix are welcome. This was initially submitted to security@ghost.org but was declined since staff are trusted users. This PR will fix the core issue and prevent XSS in SVG uploads.

Thank you for considering this contribution to enhance Ghost CMS's security.

This commit introduces DOMPurify as a dependency to sanitize SVG files during upload. SVG files can potentially contain malicious code leading to Cross-Site Scripting (XSS) vulnerabilities. By sanitizing SVG content with DOMPurify, we mitigate the risk of XSS, enhancing the security of the Ghost CMS platform.

- Added `dompurify` and `jsdom` to `package.json` dependencies for server-side DOM emulation and sanitization.
- Updated file validation logic to include SVG sanitization in `upload.js`.
- Ensured that all SVG files are checked and sanitized before being saved or processed further.
- Added relevant documentation in `README.md` and `CONTRIBUTING.md` to guide developers on the necessity and usage of DOMPurify for handling SVG uploads.

This security measure is crucial for protecting both the Ghost CMS platform and its users from potential XSS attacks through SVG file uploads. While DOMPurify aims to sanitize SVG files thoroughly, it's configured to strike a balance between security and usability, ensuring valid SVG files remain functional after sanitization.
This commit addresses a vulnerability by integrating DOMPurify to sanitize SVG file uploads, preventing potential XSS attacks within Ghost CMS. The changes made in the `update.js` file ensure that all SVG files undergo a sanitization process before being processed or stored.

Key Changes:
- Integrated DOMPurify within `update.js` to sanitize SVG files against XSS vulnerabilities.
- Added `jsdom` to emulate a DOM environment for DOMPurify to operate in a server-side context.
- Updated the SVG file validation workflow to include a sanitization step using DOMPurify, ensuring SVG files are clean before acceptance.
- Ensured backward compatibility and non-disruption of existing functionalities by selectively applying sanitization only to SVG files.
Fixed syntax causing errors.
@Yaniv-git
Copy link

If the sanitized SVG content is then served via an SVG file (meaning Content-Type: image/svg+xml) the sanitization should have only the SVG namespace (taken example from here):

const clean = DOMPurify.sanitize(dirty, {USE_PROFILES: {svg: true}});

Because if not, we introduce namespace confusion and could be prone to mXSS. for example, in a normal parsing (as done in DOMPurify):
image
vs how the browser will parse it when the Content-Type: image/svg+xml
image

@9larsons
Copy link
Contributor

@TeneBrae93 Thanks for the comprehensive report and proposed fix! I've merged a PR here. dompurify had a few issues, namely that it always modifies the base file/content and doesn't provide a flag to tell you if it actually did any cleaning, meaning we couldn't write a replicable test using it.

@9larsons 9larsons closed this May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants