#789 ✓ staged
Bobby Richter

XSS protection for packaged plugins

Reported by Bobby Richter | October 25th, 2011 @ 05:52 PM | in 1.0 Release (closed)

In trying to get PopcornMaker onto Mozilla servers, some potential security issues were raised in popcorn. I'm not sure if this conversation has already occurred somewhere inside this community before, but security concerns need to be addressed through design choice or patching, and hopefully this ticket will make that happen.

A lot of bugs were filed that take the form of "Popcorn maker is vulnerable to DOM XSS on <PLUGIN_NAME_HERE> item addition" (e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=694384). It's not really obvious how this kind of thing can be exploited until you think of where Popcorn might be deployed for projects that allow the user to specify content.

For example, perhaps users are able to build footnotes using the footnote plugin (obviously) for other users to see on some site that uses popcorn. If a footnote's content is "" there is potential evil to be done when any user sees that footnote. Anyone who's seen last year's defcon videos is aware that you can connect to an IRC server pretty easily with this kind of attack vector (http://www.youtube.com/watch?v=2ctRfWnisSk&amp;t=340) for some fun lulz.

It's pretty easy to see the amount of safety-checking in the snippet below which sets up a footnote instance (note: if you see anything more than "no checking at all", try again):

    _setup: function( options ) {

      var target = document.getElementById( options.target );

      options._container = document.createElement( "div" );
      options._container.style.display = "none";
      options._container.innerHTML  = options.text;

      if ( !target && Popcorn.plugin.debug ) {
        throw new Error( "target container doesn't exist" );
      }
      target && target.appendChild( options._container );
    },

So, to summarize, or for tldr's, there is a serious lack of contextual checks in our plugin code (e.g. text is actually text, links are actually links). I'm not saying that it's a bad thing; we may actively choose to release code that is for "sample purposes only" so that serious developers implement their own flavour of rigor. But, considering how simple most of the fixes are, it may be worth our while to release the entire set of plugins as safe as we can make them.

Heck, I wouldn't even mind something like this:

      options._container.innerHTML  = this.getSafeString( options.text );

Then, even new plugin authors would have our "standard" way of cleaning input if they so please.

p.s. My bias is obvious, since, if PopcornMaker is to live more openly in the wild, I would have to implement these checks for every considerable plugin, or seriously limit access to plugins on a live version of PM.

Comments and changes to this ticket

  • Rick

    Rick October 25th, 2011 @ 07:02 PM

    the easier solution is to add input santizing to PopcornMaker.

  • David Seifried

    David Seifried October 25th, 2011 @ 07:28 PM

    What if a user is using a custom built plugin with popcorn maker that doesn't do this kind of checking? We could do this for our core set of plugins, but I don't think thats covering all the possibilities for problems here. Seems like something will still have to be done on the Popcorn Maker side.

  • Bobby Richter

    Bobby Richter October 25th, 2011 @ 08:05 PM

    @Rick PopcornMaker should certainly have input sanitizing, but what about things that aren't PopcornMaker? Take the "href" param in the image plugin for example. You could make that "javascript:alert(somethingBad);". The manifest for the image plugin lists "href" as a simple text field, but it should probably have some deeper contextual check for sanitation. PopcornMaker still attempts to build editors for simple plugins like image, but it couldn't know that "href" shouldn't have "javascript:" in it unless perhaps the manifest lists it as a "link" instead, or something.

    @dseif Probably. Again, perhaps making the manifests more explicit about the data they're using would help.

    Like I said, we can just leave it up to devs using popcorn to make decisions, which isn't that terrible since they can sanitize ahead of time. But, automating is still a problem. I think reading "manifest: { href: 'url' }" would be beneficial.

    Either that, or... no plugin can be used in PopcornMaker until it has an editor built for it, perhaps.

  • Bobby Richter
  • Rick

    Rick October 25th, 2011 @ 08:27 PM

    FYI, I am not authorized to access that bug.

  • Bobby Richter

    Bobby Richter October 25th, 2011 @ 08:37 PM

    • Assigned user set to “Bobby Richter”
    • State changed from “new” to “peer-review-requested”

    After some deep personal reflection, I'm convinced that simply adding a data type to manifests lets everyone win, and keeps the popcorn core slim.

    This can be used easily in Popcorn Maker now.

    https://github.com/secretrobotron/popcorn-js/tree/url-manifests

  • Rick

    Rick October 25th, 2011 @ 08:48 PM

    • Assigned user cleared.
    • State changed from “peer-review-requested” to “new”

    Also, innerHTML doesn't execute script tags ;)

    http://jsfiddle.net/rwaldron/apwKQ/

    http://jsfiddle.net/rwaldron/WPH2b/

  • Rick

    Rick October 25th, 2011 @ 08:51 PM

    Also, +1 to using modern input types.

  • Rick

    Rick October 25th, 2011 @ 08:55 PM

    What if I want to put javascript:alert("Hi Friendly People"); as the href value? Popcorn shouldn't make decisions like this. It should be up to whoever is using Popcorn, in whatever it is they are implementing, to sanitize. Otherwise we're committing creative censorship.

  • Rick

    Rick October 25th, 2011 @ 08:56 PM

    I'm still curious to know more about the tickets that were reported

  • Bobby Richter

    Bobby Richter October 25th, 2011 @ 09:05 PM

    Agreed. Hence my comment above and its partner in crime, the "url-manifests" branch in my repo.

    Popcorn needn't make the decision, but I think the manifest should be explicit enough to allow the end-user to do contextual sanitation if they so please -- like in PopcornMaker. I can just read the "url" input type and conclude that it should probably not be something whacky.

    Also, there are a bunch of tickets in bugzilla assigned to me that look similar to this:

    Issue:
    It's possible for a user to inject script into their popcorn maker projects; this is not an issue with the current form of the problem but this is likely to change as sharing functionality is introduced.
    
    Steps to reproduce:
    1) start a new project
    2) create new 'image' item
    3) drag it onto the timeline, and set the link to javascript:alert(123)
    4) observe the alert dialog popping up when you click on the image
    
    Recommended remediation:
    validate any user supplied URLs. Check the schemes (would suggest you only allow HTTP/HTTPS) and possibly the host too.
    

    Just trying to clean inputs across all of PopcornMaker in the next couple of days, really.

    In summary, can this move forward now? The change to the manifests is simple enough in popcorn and powerful to iterate on in PopcornMaker.

  • Scott Downe

    Scott Downe October 25th, 2011 @ 09:32 PM

    I am in favour of flagging the necessary risks via manifest, and letting people deploying Popcorn make the choice. After all, it is their server it is on. I think for now, that is the best and easiest fix.

    But, is that really enough? Do we care if people are not aware of the problem, like I was, deploy it, without knowing there was a decision to make? I wonder if there is a way to warn them about the risks, without making the choice. I hate warnings, flags, and modes. There must be a better way to have our cake and eat it too. Something to think about going forward.

    I don't think we have to worry about user created plugins that don't supply the necessary info in the manifest, because popcorn maker can pick and chose its plugins it knows are safe.

    I am going to vote for https://github.com/secretrobotron/popcorn-js/commits/url-manifests for the solution and give it an R+ if we decide on its execution.

  • Bobby Richter

    Bobby Richter October 25th, 2011 @ 09:40 PM

    • Assigned user set to “Bobby Richter”
    • State changed from “new” to “peer-review-requested”

    Thank you sir, for you speedy and eloquent reply.

    I'm not sure how to solve this problem in general; perhaps a link to the wiki on XSS and some examples of why it's bad is the best we can do.

    For now though, the manifest change is perfect. We can add to it later if we come up with a more concrete solution.

    I'm going to place this guy back in PR if no-one objects.

  • Bobby Richter

    Bobby Richter October 25th, 2011 @ 09:40 PM

    • Assigned user changed from “Bobby Richter” to “Scott Downe”
  • Rick

    Rick October 25th, 2011 @ 10:42 PM

    +1 (sorry, I was playing Batman: Arkham City)

  • Rick

    Rick October 25th, 2011 @ 10:47 PM

    Is there some way to grant our Mozilla/Bugzilla accounts permission to view Popcorn related security tickets?

  • Scott Downe

    Scott Downe October 25th, 2011 @ 10:50 PM

    • Assigned user changed from “Scott Downe” to “Rick”
    • State changed from “peer-review-requested” to “super-review-requested”

    Alright, cool.

    I see no reason why this has to sit under me any longer.

    I would feel best though, if Rick could use his experience to give this a SR. I am not really all that familiar with what plugins pose this risk. But, I am confident that the solution is in the manifest.

  • David Humphrey

    David Humphrey October 25th, 2011 @ 11:17 PM

    Rick, you have to get someone with SEC group rights on bugzilla to CC you or have someone in the CC list CC you. Bobby can probably CC you to the bugs he's talking about, since he'll be in the CC list.

    Bobby, can you help Rick get access?

  • David Humphrey

    David Humphrey October 25th, 2011 @ 11:21 PM

    BTW, I totally agree with the "manifest guards types" approach vs. trying to lock down popcorn.js as such. +1

  • cadecairos

    cadecairos October 25th, 2011 @ 11:37 PM

    Yeah this seems like something that Popcorn-Maker should be handling through information in the plug-in manifests. +1 to that.

  • Bobby Richter

    Bobby Richter October 26th, 2011 @ 12:30 AM

    @Rick, I can CC you on these bugs, but they're PopcornMaker focused. I only brought this one here because it seemed more related to popcorn than Popcorn Maker solely. Don't know if you want your inbox being more spammed than it already is.

    I've already started to program against this in my PopcornMaker line. Any problems seen with the patch I posted above? I'd like to motor along.

  • Scott Downe

    Scott Downe October 26th, 2011 @ 01:52 AM

    I picked Rick for SR because of his experience and he was active in the ticket.

    For sake of Motoring along, if Chris or Humph want to SR this tonight, I think that would be just as good. Whoever gets here first.

  • Rick

    Rick October 26th, 2011 @ 09:05 AM

    • State changed from “super-review-requested” to “review-looks-good”

    SR+ - again, sorry - I was on my mobile answerimg from the couch, I thought I had selected review looks good

  • cadecairos

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Popcorn.js is an HTML5 video framework that lets you bring elements of the web into your videos.

Popcorn.js is a project of Web Made Movies, Mozilla's Open Video Lab.

Shared Ticket Bins

Pages