Page MenuHomePhabricator

SVG iframe XSS
Closed, ResolvedPublic

Description

Mario Gomes reported to mozilla an svg xss:

https://bugzilla.mozilla.org/show_bug.cgi?id=966734

This is triggered using an iframe with a srcdoc and xhtml namespace.

We can easily forbid svg files with iframes. I can't tell if it's an oversight that we allow those, or if we made the decision to allow them for some reason. I'll pull down some of the more recent svg uploads and see if embedded iframes are common.


Version: unspecified
Severity: normal

Details

Reference
bz60771

Event Timeline

bzimport raised the priority of this task from to High.
bzimport set Reference to bz60771.
csteipp created this task.Feb 3 2014, 5:43 PM

Adding some multimedia people in case they have input on allowing iframes in svg files. Any of you see a reason we shouldn't forbid them?

Created attachment 14453
PoC from Mario

Attached:

brion added a comment.Feb 3 2014, 6:38 PM

There should be no legit reason to put an HTML iframe inside an SVG graphic, IMO... should be just fine to filter and forbid those...

(In reply to comment #3)

There should be no legit reason to put an HTML iframe inside an SVG graphic,
IMO... should be just fine to filter and forbid those...

I agree.

Created attachment 14470
Whitelist namespaces

This limits the xml namespaces that can be used within svg files. I'm still downloading a new corpus of recent uploads, but so far, only about 5 legitimate images on commons use the http://www.w3.org/1999/xhtml namespace. Everything else that I've found are on the whitelist that I put into this patch.

In the event that iframes ever make it into one of the namespace that we do allow, I forbid them also.

attachment bug60771.patch ignored as obsolete

Created attachment 14489
Whitelist namespaces

Found one more namespace in testing. Tested it against 25k images off commons, and only images that try to use http://www.w3.org/1999/xhtml would be rejected at this point.

attachment bug60771.patch ignored as obsolete

Created attachment 14501
Whitelist namespaces

Tim pointed out that we will want users to know which namespace failed so we can add harmless ones.

attachment bug60771c.patch ignored as obsolete

Created attachment 14576
Whitelist namespaces

Update from Aaron's feedback:

  • Updated comments on splitXmlNamespace
  • Made list of namespaces static

Also made splitXmlNamespace private.

Attached:

aaron added a comment.Feb 12 2014, 7:36 PM

Looks OK to me.

fwiw, reading this i'm glad that the switch from old to new planet software "broke" embedded iframes in RSS feeds that we aggregate from third parties and then display as our feed :)

Deployed to the cluster

21:00 logmsgbot: csteipp finished scap: bug 60771 (duration: 36m 58s)

  • Bug 61278 has been marked as a duplicate of this bug. ***
Gilles moved this task from Untriaged to Done on the Multimedia board.Dec 4 2014, 10:11 AM
Gilles raised the priority of this task from High to Unbreak Now!.
Gilles lowered the priority of this task from Unbreak Now! to High.Dec 4 2014, 11:20 AM