Allow SVG uploads

[ryelle]

The newly redesigned sites have many accent images that would be best handled as SVG (for example, the showcase header https://github.com/WordPress/wporg-showcase-2022/issues/26). SVG will scale well across device sizes and not get blurry.

The current approach - uploading the SVG to the CDN and using it as an external URL - works for now, but if a content editor or designer wants to change the image, they won't be able to upload the new image. So the admin & designer roles should be able to upload SVGs.

@StevenDufresne added a quick filter to showcase, but removed it after some feedback from @dd32 — https://github.com/WordPress/wporg-showcase-2022/pull/37#pullrequestreview-1176647512

In all honesty, this doesn't belong in the theme, and I'm not sure the context of adding it.

There's also something to say that this should be using unfiltered_upload, but that isn't available to anyone on WordPress.org.

I think I'd rather enable a plugin such as https://wordpress.org/plugins/safe-svg/ (or one of the others) on the site instead if SVG upload is required.

So I've tried to capture more context here, and added it to this repo to decide whether it should be custom code in mu-plugins, or adding a new plugin to w.org. I've tagged it "low priority" for now since the CDN workaround works, but ideally we'll solve this sometime during this round of redesigns (to prepare for future design updates).

[iandunn]

Safe SVG is by far the most secure plugin that I'm aware of; the others I've seen didn't use any sanitization at all. I haven't checked in a few years, though.

I'd still strongly recommend against running it on w.org, though. It's fundamentally limited because it can't use a real DOM to parse the SVG, so there's a myriad of bypasses. That's why Cure53 (SVG security experts) gave up on a PHP approach and wrote DomPurify instead. That can't be used in Core, but we could potentially use it for w.org if we wanted to setup a Node server or something.

A simpler approach would be to ensure that they're only ever loaded from the CDN domain, so that cross-domain protections exist. Even then it's still kind of fragile, since it's relying on code to bandaid a fundamental vulnerability.

SVGs are just a train-wreck from a security perspective. Would webp be an option instead? I know it's not vector-based, but it might still address the underlying performance concerns. If not, IMO it'd be best to just use pngs and high-quality jpegs despite the performance issues. Or accept imperfect images so that they're fast. IMO most users would prefer that tradeoff.

There's lots of background in https://core.trac.wordpress.org/ticket/24251

Related: https://github.com/WordPress/performance/issues/427, https://github.com/WordPress/gutenberg/issues/16484

[adamsilverstein]

That's why Cure53 (SVG security experts) gave up on a PHP approach and wrote DomPurify instead.

@iandunn Fascinating! Can you share any links with more details about the original attempt from Cure53 at PHP sanitization?

For the performance lab plugin we have been looking at the same underlying sanitization library (https://github.com/darylldoyle/svg-sanitizer) which interestingly states that "The work is largely borrowed from DOMPurify." on the readme.

It's fundamentally limited because it can't use a real DOM to parse the SVG, so there's a myriad of bypasses

That sounds bad. Essentially your point is that no many how many tests are added there can always be some unexpected input so this will never be completely safe?

wrote DomPurify instead. That can't be used in Core

Why not? Could we run this library in the browsers as users upload their SVGs in the editor or the media library and sanitize them "on the fly" before uploading them to the server? Or is there a license issue?

[iandunn]

more details about the original attempt from Cure53 at PHP sanitization?

Section 4 of Crouching Tiger – Hidden Payload and https://security.stackexchange.com/a/30390/8467 have some details on the PHP approach and the challenges of it.

[svg-sanitizer] is largely borrowed from DOMPurify

Yeah, there's some history in #24251-core. I'm guessing he ported the code and unit tests to PHP as much as possible, and took some inspiration from the design principles etc.

there can always be some unexpected input so this will never be completely safe?

That's my understanding, yeah. There are some examples in https://security.stackexchange.com/a/30390/8467 and https://core.trac.wordpress.org/ticket/24251#comment:34

There may be some more examples Crouching Tiger... and in The Image that Called Me.

Could we run this library in the browsers as users upload their SVGs

I think the problem there is that we can't trust what the client sends.

I'm guessing that it would usually be performant to run it when images are displayed, but then users with older browsers would be vulnerable.

[dd32]

Safe SVG is by far the most secure plugin that I'm aware of; the others I've seen didn't use any sanitization at all. I haven't checked in a few years, though.

I'd still strongly recommend against running it on w.org, though.

I think I agree, It's likely the most secure plugin for this purpose, but may not be 100% secure.

Would your hesitations be resolved if the plugin was only activated on sites with trusted people and/or only for those who would be able to commit to a repo?

If it's being used for trusted users on certain sites, as an extra layer of protection against them uploading something accidentally malicious, and not just allowing anyone to upload a SVG just because they happen to be a make.w.org/* post contributor, I think using such a plugin is probably a reasonable way forward.

[iandunn]

Yeah, I think this would be acceptable:

• only ever enqueued from the CDN domain - this is the most important thing IMO
• sanitized by Safe SVG
• user is an editor or admin (it sounds like this is on Safe SVG's roadmap)
• all of the above is enforced by code, not db options or convention

Alternatively, a Core-first solution would be to hire Cure53 to do an audit, and get the library merged to Core. I'm not optimistic that it'd pass, but I'd be happy to be wrong.

[dd32]

only ever enqueued from the CDN domain - this is the most important thing IMO

I've submitted a systems request to either create a dedicated uploads CDN, or to find out if s.w.org is the best location for that. Noting that we'd need to block direct non-cdn file access too.

https://make.wordpress.org/systems/2022/12/05/dedicated-uploads-cdn/

[iandunn]

Ah, yeah, redirecting (or just blocking) access to wordpress.org/.../*.svg at the network layer would be great 👍🏻