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.

Sign up for GitHub

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

Jump to bottom

DOMPurify fails on sanitization of Trusted Types sink attributes #660

Closed
tosmolka opened this issue Mar 7, 2022 · 8 comments · Fixed by #699
Closed

DOMPurify fails on sanitization of Trusted Types sink attributes #660

tosmolka opened this issue Mar 7, 2022 · 8 comments · Fixed by #699

Comments

@tosmolka
Copy link
Contributor

tosmolka commented Mar 7, 2022 •
edited

Even if attribute is allowed, DOMPurify performs basic sanitization, such as stringTrim. This can cause Trusted Types Sink violation for attributes that are expecting trusted types (e.g. <script src>).

Background & Context

DOMPurify does not (by design) validate script URLs and we want to perform our own validation using hooks. It turns out DOMPurify calls setAttribute for all allowed attributes and this causes Trusted Types sink violation:

DOMPurify/src/purify.js

Line 1196 in 5c28248

currentNode.setAttribute(name, value);

Bug

Input

<!doctype html>
<html>
    <head>
        <meta http-equiv="Content-Security-Policy" content="trusted-types dompurify; require-trusted-types-for 'script';">
        <script src="https://cdn.jsdelivr.net/npm/dompurify@2.3.6/dist/purify.js"></script>
    </head>
    <body>
        <script type="text/javascript">
          window.addEventListener('load', () => {
            var dirty = `
              <h3>Gallery</h3>
              <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/glightbox/dist/css/glightbox.min.css">
              ${'\x3C'}script src="https://cdn.jsdelivr.net/npm/glightbox/dist/js/glightbox.min.js">${'\x3C'}/script>
              
              <a href="https://biati-digital.github.io/glightbox/demo/img/large/gm2.jpg" class="glightbox" data-title="Test Title">
                <img src="https://biati-digital.github.io/glightbox/demo/img/small/gm2.jpg" alt="image">
              </a>
            `;
            DOMPurify.addHook('uponSanitizeElement', function(node, data) {
              const policy = {
                script: ['https://cdn.jsdelivr.net/npm/glightbox/dist/js/glightbox.min.js'],
                style: ['https://cdn.jsdelivr.net/npm/glightbox/dist/css/glightbox.min.css'],
              };
              if (
                data.tagName === 'script' && !policy.script.includes(node.getAttribute('src')) ||
                data.tagName === 'link' && !policy.style.includes(node.getAttribute('href'))
              ) {
                return node.parentNode.removeChild(node);
              }
            });
            var clean = DOMPurify.sanitize(dirty, {ADD_TAGS: ['link','script'], RETURN_TRUSTED_TYPE: true});
            var iframe = document.createElement('iframe');
            iframe.setAttribute('style', 'width: 800px; height: 600px;');
            iframe.setAttribute('srcdoc', clean);
            iframe.addEventListener('load', () => {
              const lightbox = iframe.contentWindow.GLightbox({});
              lightbox.open();
            });
            document.body.appendChild(iframe);
          });
        </script>
    </body>
</html>

Given output

DOMPurify fails with following exception and GLightbox gallery in iframe is not functional.

purify.js:1196 This document requires 'TrustedScriptURL' assignment.
_sanitizeAttributes @ purify.js:1196
DOMPurify.sanitize @ purify.js:1380

Expected output

DOMPurify sanitizes dirty HTML without throwing an exception and GLightbox gallery in iframe is functional.
@cure53
Copy link
Owner

cure53 commented Mar 7, 2022 •
edited

If I understand the issue correctly, we'd have to pass a policy object using an empty/noop policy, no?

@cure53
Copy link
Owner

cure53 commented Mar 7, 2022

So, my thinking is, we wanna wrap it like this, correct?

if (namespaceURI) {
      currentNode.setAttributeNS(namespaceURI, name, value);
} else {
      /* Fallback to setAttribute() for browser-unrecognized namespaces e.g. "x-schema". */
      value = trustedTypesPolicy
        ? trustedTypesPolicy.createScriptURL(value)
        : value;
      currentNode.setAttribute(name, value);
}

But we also want to only do this for <script src> and nothing else, right?

@cure53
Copy link
Owner

cure53 commented Mar 7, 2022

So, like this, no?

if (lcTag === 'script' && lcName === 'src') {
    value = trustedTypesPolicy
      ? trustedTypesPolicy.createScriptURL(value)
      : value;
}

currentNode.setAttribute(name, value);

@cure53
Copy link
Owner

cure53 commented Mar 9, 2022

cc @tosmolka :)

@tosmolka
Copy link
Contributor Author

@cure53 , I wanted to suggest using TrustedTypePolicyFactory.getAttributeType() but I was having some issues with Chromium implementation.

I filed https://bugs.chromium.org/p/chromium/issues/detail?id=1305293 to get more eyes on the issue I was having. If possible, I'd like to avoid hard-coding the mapping between attributes and expected Trusted Types within the lib such as DOMPurify.

@cure53
Copy link
Owner

cure53 commented Mar 10, 2022

Gotcha, then let's wait until we have more clarity here. And I fully agree.

@cure53
Copy link
Owner

cure53 commented Apr 2, 2022

closing for inactivity, happy to reopen when needed / actionable.

@cure53 cure53 closed this as completed Apr 2, 2022
@tosmolka tosmolka mentioned this issue Jul 18, 2022
Merged
@tosmolka
Copy link
Contributor Author

@cure53 , there was no response from Chromium team and we are starting to get blocked on this issue. I raised PR 699 that proposes fix for attributes without namespaces. This should be enough for our use case.

Can you pls take a look? Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Support sanitization of attributes that require Trusted Types tosmolka/DOMPurify
2 participants
@cure53 @tosmolka