SVG code incorrectly removed

[NGPixel]

Background & Context

Hey, I'm the maintainer of open source project Wiki.js and we use DOMPurify to sanitize all content when editing a page. We recently added an integration with draw.io which allows the editor to add diagrams to their pages. The generated output is an inline SVG code which gets inserted into the page HTML code. This page HTML code is then feeded to DOMPurify before being both displayed in a Preview window and saved to the DB.

Bug

While most SVG diagrams generated by draw.io are left intact by DOMPurify, adding a line jump in any text element causes DOMPurify to completely wipe the SVG element code (empty output).

Take the following example of a diagram of a square with a text element a + line break + b:

Looking at the SVG code below, you'll notice the generated text is a<br />b. For some reason, this causes DOMPurify to not return anything. As soon as you remove the <br /> tag from the code below, the sanitized output is fine again.

Input

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" width="81px" height="81px" viewBox="-0.5 -0.5 81 81">
  <defs/>
  <g>
    <rect x="0" y="0" width="80" height="80" fill="#ffffff" stroke="#000000" pointer-events="all"/>
    <g transform="translate(-0.5 -0.5)">
      <switch>
        <foreignObject style="overflow: visible; text-align: left;" pointer-events="none" width="100%" height="100%" requiredFeatures="http://www.w3.org/TR/SVG11/feature#Extensibility">
          <div xmlns="http://www.w3.org/1999/xhtml" style="display: flex; align-items: unsafe center; justify-content: unsafe center; width: 78px; height: 1px; padding-top: 40px; margin-left: 1px;">
            <div style="box-sizing: border-box; font-size: 0; text-align: center; ">
              <div style="display: inline-block; font-size: 12px; font-family: Helvetica; color: #000000; line-height: 1.2; pointer-events: all; white-space: normal; word-wrap: normal; ">a<br />b</div>
            </div>
          </div>
        </foreignObject>
        <text x="40" y="44" fill="#000000" font-family="Helvetica" font-size="12px" text-anchor="middle">a...</text>
      </switch>
    </g>
  </g>
  <switch>
    <g requiredFeatures="http://www.w3.org/TR/SVG11/feature#Extensibility"/>
    <a transform="translate(0,-5)" xlink:href="https://desk.draw.io/support/solutions/articles/16000042487" target="_blank">
      <text text-anchor="middle" font-size="10px" x="50%" y="100%">Viewer does not support full SVG 1.1</text>
    </a>
  </switch>
</svg>

Given output

(completely empty output)

Expected output

(same as input)

Thanks!

[MarsWarrior]

Line breaks are removed in SVG's:

Apparently a </br> can be used as a sanitizer bypass in svg's: https://github.com/whatwg/html/issues/5113

I hope it gets fixed soon. Putting a lot of work in creating diagrams and ending up with empty space is not nice 😄

[cure53]

The problem here is that <br> or <p> inside SVG can be abused to create an mXSS problem as @MarsWarrior correctly describes.

Now, in this special situation, we have an SVG that inhabits a foreignObject element that then inhabits a <br>. The sanitizer doesn't yet differentiate between <br> inside SVG and the same with a foreignObject mixed in.

Frankly, I am not sure if by adding this differentiation, we don't create another XSS bug and bypass, so I am extremely hesitant to do any rushed changes here.

[cure53]

For the sake of maintaining our key goal of preventing XSS, I would rather flag this as wontfix. I don't think we can safely check whether any <br> inside SVG is a safe one or will cause mXSS. The risk for new bypasses affecting everyone is too big.

I am quite certain that you can however fix this by using a hook, in which you for example remove the <br> or replace it with a placeholder tag or alike. The uponSanitizeElement should be able to achieve exactly that. I would recommend to use that hook, check for <br> or <p> inside foreignObject, replace them with a placeholder and then change them back.

That should solve your problem and bear no XSS risks for everyone else :)

[stevenxi]

I think the issue is not just the <br/>. Even without the <br/>, the entire <foreignObject> is removed anyway.

So the solution of replacing <br> with a tag from @cure53 won't work here.

Try on https://cure53.de/purify :

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" width="81px" height="81px" viewBox="-0.5 -0.5 81 81">
  <defs/>
  <g>
    <rect x="0" y="0" width="80" height="80" fill="#ffffff" stroke="#000000" pointer-events="all"/>
    <g transform="translate(-0.5 -0.5)">
      <switch>
        <foreignObject style="overflow: visible; text-align: left;" pointer-events="none" width="100%" height="100%" requiredFeatures="http://www.w3.org/TR/SVG11/feature#Extensibility">
          <div  style="display: flex; align-items: unsafe center; justify-content: unsafe center; width: 78px; height: 1px; padding-top: 40px; margin-left: 1px;">
            <div style="box-sizing: border-box; font-size: 0; text-align: center; ">
              <div style="display: inline-block; font-size: 12px; font-family: Helvetica; color: #000000; line-height: 1.2; pointer-events: all; white-space: normal; word-wrap: normal; ">ab</div>
            </div>
          </div>
        </foreignObject>
        <text x="40" y="44" fill="#000000" font-family="Helvetica" font-size="12px" text-anchor="middle">a...</text>
      </switch>
    </g>
  </g>
  <switch>
    <g requiredFeatures="http://www.w3.org/TR/SVG11/feature#Extensibility"/>
    <a transform="translate(0,-5)" xlink:href="https://desk.draw.io/support/solutions/articles/16000042487" target="_blank">
      <text text-anchor="middle" font-size="10px" x="50%" y="100%">Viewer does not support full SVG 1.1</text>
    </a>
  </switch>
</svg>

[cure53]

Yes, it does, however, foreignObject is not permitted by default. This code should do the trick, as terrible and "hacky" as this snippet below might be :)

var dirty = '<svg><foreignObject>12<br />34</foreignObject></svg>';

DOMPurify.addHook('uponSanitizeElement', function(element){
    if(element.querySelectorAll){
        var breaks = element.querySelectorAll('foreignObject br, foreignObject p');
        if(breaks && breaks.length) {
            for(var i = 0; i < breaks.length; i++){
                breaks[i].parentNode.replaceChild(
                    document.createElement('div'), 
                    breaks[i]
                );
            }
        }
    }
});
var clean = DOMPurify.sanitize(dirty, {ADD_TAGS: ['foreignObject']});
console.log(clean)

[cure53]

So, first you need to allow-list foreignObject, do so at your own risk. Then you need to replace certain risky tags with something less risky - and potentially change the output back after sanitization.

We would give no guarantees about this being XSS-safe, but it gets the job done for this specific use-case.

[MarsWarrior]

So, first you need to allow-list foreignObject, do so at your own risk. Then you need to replace certain risky tags with something less risky - and potentially change the output back after sanitization.

We would give no guarantees about this being XSS-safe, but it gets the job done for this specific use-case.

Thanks for the hook tip! However, as @NGPixel stated in his example: only the <br> tag results in NOT displaying the diagrams. In both cases a foreignObjectis defined, so the existence of a that object can't be the problem.

I can't find any reference to a foreignObjectin Wiki.js codebase. So that is weird. This tag doesn't seem to be added to the DOMPurify.sanitize call.

Furthermore:

• I did find the reason that MermaidDiagrams don't have problems with linebreaks. It seems that those diagrams are rendered after sanitizing the html page.
• And - because of the insights I got following this discussion - I found that Wiki.js has a setting that allows unsafe HTML (ie nothing is sanitized). After disabling this flag, my draw.io diagrams are rendered with text containing linebreaks 😄

With that setting I stumbled on some other bugs, but that has nothing to do with this project...

[cure53]

Most welcome, I think we can close this ticket, no? Or do you see anything that needs to be done from our end?

[cure53]

However, as @NGPixel stated in his example: only the
tag results in NOT displaying the diagrams. In both cases a foreignObject is defined, so the existence of a that object can't be the problem.

Not really :) The <br> causes early removal of the whole SVG. The foreignObject would be removed anyway with default settings, but the aforementioned early removal happens before that.

[MarsWarrior]

However, as @NGPixel stated in his example: only the tag results in NOT displaying the diagrams. In both cases a foreignObject is defined, so the existence of a that object can't be the problem.

Not really :) The <br> causes early removal of the whole SVG. The foreignObject would be removed anyway with default settings, but the aforementioned early removal happens before that.

Ah. So the foreignObject tag should be added somewhere in the wiki.js source, as otherwise what I see, without a new line the diagram is rendered, should not be possible!

And regarding your last question:

Most welcome, I think we can close this ticket, no? Or do you see anything that needs to be done from our end?

From my end I understand that what we see is expected behavior, and won't/can't be changed. Furthermore you proposed a solution using hooks. But that is up to the maintainer, @NGPixel .

Although I can't speak for @NGPixel I consider this issue closed. I assume if further assistance is required, @NGPixel will open a new issue 😄

[cure53]

Ah. So the foreignObject tag should be added somewhere in the wiki.js source, as otherwise what I see, without a new line the diagram is rendered, should not be possible!

Exactly. The foreignObject would have been removed anyway as we consider it to be unsafe. But the neutering because of the br inside SVG killed the whole SVG before foreignObject was even looked at.

Although I can't speak for @NGPixel I consider this issue closed.

Cool, thanks all :)

[NGPixel]

Thanks @cure53 for the quick response!

[j-ulrich]

We are currently facing the exact same problem as the OP and I wonder if this process would work and be safe:

1. Check if the foreignObject has exactly one child element and if that child element has xmlns="http://www.w3.org/1999/xhtml"
2. If so, run the content of the foreignElement through DOMPurify and use the sanitized content instead of the original content.
3. If not, remove the foreignObject as a safety measure.

We will try this via a uponSanitizeElement hook but I wonder if this wouldn't be a sensible default behavior for DOMPurify?

[cure53]

That does in fact sound safe :smile: But it won't become the default unless we have extensive test data to really know it's safe.