542020 - Prevent random embedding of personas pages, nuisance hover previews

Status: RESOLVED FIXED

(Websites Graveyard :: getpersonas.com, defect)

Description

[Johnathan Nightingale [:johnath]]

+++ This bug was initially created as a clone of Bug #541308 +++

In bug 541308 we discuss the fact that getpersonas pages can be iframed by arbitrary sites on the net, allowing them to produce nuisance on-hover previews and even installs on click.  While these installs are easily undoable, it's still annoying, and worse, to some users it feels like a genuine security bug.

We think this can be fixed web-side, instead of client side, by putting in a basic "am I framed?" check. The problem with just doing:

if (self != top) {
  // Disable previews/installs
}

is that it would kill our own ability to use pages in an iframe on sites like http://www.mozilla.com/firefox/3.6/firstrun/

Shaver proposed that we handle this with postMessage ( https://developer.mozilla.org/en/DOM/window.postMessage ). So basically, the logic would be:

// on getPersonas pages that support preview/install

if (self != top) {
  // We're in a frame, disable previews/installs
}

function receiveMessage(event) {
  if (event.origin is in mozilla.com or otherwise trusted 
      && event.data says "reactivate" or whatever) {
    // Turn previews/installs back on
  }
}
window.addEventListener("postmessage", receiveMessage, false)


And then on sites like the firstrun page which use the iframing in this way, we just include a line onLoad that does

iframeElement.contentWindow.postMessage("reactivate", "*");


Arbitrary pages out there in the world could copy all of this, but they wouldn't pass the event.origin test in the getpersonas page, so their previews and installs would be disabled -- no more nuisance.

Does that all make sense?  This bug should update getpersonas AND existing firstrun content (so that fixing this doesn't temporarily break firstrun).  I'll file a clone for the AMO galleries.

Comment 1

[Ryan Doherty (:rdoherty)]

Makes sense, will be worked on this week.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Will any changes be required on www.mozilla.com?

Comment 3

[Ryan Doherty (:rdoherty)]

(In reply to comment #2)
> Will any changes be required on www.mozilla.com?

Yes:

iframeElement.contentWindow.postMessage("reactivate", "*");


will need to be added to all our pages that use an iframe to embed personas.

Comment 4

[Mike Shaver (:shaver emeritus)]

That can be added harmlessly before the personas site is changed, though (and should be triggered by the iframe signalling onload, right?).

Comment 5

[Ryan Doherty (:rdoherty)]

Attachment: Mozilla.com patch for 3.6 firstrun

Example patch for mozilla.com. Will need some help moving this into other pages and locales. Would like a couple people to review.

Comment 6

[Ryan Doherty (:rdoherty)]

Attachment: getpersonas.com js patch

Patch for getpersonas.com that disables previews/installs if embedded in iframe and enables only if postmessage is from mozilla.com & data == 'activatePersonas'

Comment 7

[Reed Loden [:reed]]

Comment on attachment 423748 [details] [diff] [review]
getpersonas.com js patch

>+function receiveMessage(event) {
>+  if (event.origin == 'http://www.mozilla.com' &&
>+      event.data == 'activatePersonas') {
>+      PERSONAS_ENABLED = true;
>+    }

Only allowing http://www.mozilla.com is going to break all of our staging sites. Also, we still have to support locale-based subdomains for a while... I'm wondering how many users are still hitting the page via one of the subdomains.

Comment 8

[Ryan Doherty (:rdoherty)]

(In reply to comment #7)
> Only allowing http://www.mozilla.com is going to break all of our staging
> sites. Also, we still have to support locale-based subdomains for a while...
> I'm wondering how many users are still hitting the page via one of the
> subdomains.

Ah, yes, I forgot about those. I can add staging, do we point any of our builds to locale-specific subdomains?

Comment 9

[Reed Loden [:reed]]

Comment on attachment 423746 [details] [diff] [review]
Mozilla.com patch for 3.6 firstrun

So, in general, I think we should try to keep JS-only files as .js so they get correctly cached and gzip'd. Could we do something with minify, too, for all the js things? I'm also curious how onload will interact with the JS loading so late, but maybe that's not an issue.

Comment 10

[Ryan Doherty (:rdoherty)]

(In reply to comment #9)
> (From update of attachment 423746 [details] [diff] [review])
> So, in general, I think we should try to keep JS-only files as .js so they get
> correctly cached and gzip'd. Could we do something with minify, too, for all
> the js things? I'm also curious how onload will interact with the JS loading so
> late, but maybe that's not an issue.

I had intended in adding it to a js file, but the firstrun page doesn't have any external files that this could be added to (all are js libraries and stats code). I don't want to add an extra JS file b/c that's one more file to include. 

We could use the minify tools we have to reduce http requests, but I don't want to change more code than necessary. Probably a style/opinion thing, if others really think we should do it I can.

I think the onload event firing will be ok as long as the external JS file is included above the iframe.

Comment 11

[Mike Shaver (:shaver emeritus)]

Can we just make it accept *.mozilla.com and *.mozilla.org, which I think we can trust to not abuse for nuisance?

Comment 12

[chris hofmann]

anyplace on those domains with user generated content would still be open to hosting abuse.  here in bugzilla, amo comments, and probably other places.

Comment 13

[Ryan Doherty (:rdoherty)]

moving to 2.5, 2.4 is going out tonight with fixes for more important security issues.

Comment 14

[Ryan Doherty (:rdoherty)]

Attachment: getpersonas.com js patch v2

Updated js for getpersonas.com. I accidentally committed the previous one, so this is a diff against trunk.

Comment 15

[Ryan Doherty (:rdoherty)]

Attachment: Mozilla.com patch for 3.6 firstrun v2

Comment 16

[Reed Loden [:reed]]

Comment on attachment 426119 [details] [diff] [review]
getpersonas.com js patch v2

>+var ALLOWED_ORIGINS = ['http://www.mozilla.com', 'http://www-trunk.stage.mozilla.com'];

Should allow http or https for all hosts. Also need to support *.www.mozilla.com, as I'm just not sure we still don't have users using the old locale-based subdomains. Plus, add www.authstage.mozilla.com. I'm not too worried about locale-based subdomains on the staging servers. How about dev environments? Should we permit localhost or something?

Comment 17

[Reed Loden [:reed]]

Comment on attachment 426126 [details] [diff] [review]
Mozilla.com patch for 3.6 firstrun v2

>@@ -47,7 +49,6 @@ $page_title = 'Welcome to Firefox';
>     </p>
> 
> </div>
>-
> <?php
> @include_once "{$config['file_root']}/{$lang}/includes/footers/3.6/portal-pages.inc.php";

Why are you removing this line? Please keep it.

Other than that nit, this is fine. r=reed

Comment 18

[Ryan Doherty (:rdoherty)]

Attachment: getpersonas.com js patch v3

Should have used a regex from the start. Tested with www.mozilla.com, http://www-trunk.stage.mozilla.com, https, locale subdomains, etc.

Comment 19

[Reed Loden [:reed]]

Comment on attachment 427846 [details] [diff] [review]
getpersonas.com js patch v3

See comment #12 as to why we don't want *.mozilla.com.

Comment 20

[Ryan Doherty (:rdoherty)]

Attachment: getpersonas.com js patch v4

This should about do it:

* www
* www-trunk.stage
* www.authstage
* *.www
* http & httpS

Comment 21

[Reed Loden [:reed]]

Comment on attachment 428284 [details] [diff] [review]
getpersonas.com js patch v4

r=reed

You could collapse this regex some, but what's there is fine as-is for now. If we need to add more sites later, we can.

Thanks for the work on this.

Comment 22

[chris hofmann]

this will probably get yanked from amo soon but someone figure out how to inject personas content on amo at:

(https://addons.mozilla.org/en-US/firefox/addon/155045/

code looked like this:


</select>
        </div>
      </fieldset>
      <span id="search-data"
        data-version="any">

      </span>
      <input type="hidden" name="advanced" id="id_advanced" />
    </div>   </form>
</div>        </div>
                                <ol class="breadcrumbs">
            <li><a href="/en-US/firefox/">Add-ons for Firefox</a></li>
                <li><a href="/en-US/firefox/personas/">Personas</a></li>

                <li>Adobe Flash Platform</li>
      </ol>


<hgroup>
  <h2 class="addon">
    <img src="https://www.getpersonas.com/static/5/0/197350/preview_small.jpg" class="icon"/>
    <span>
      Adobe Flash Platform
    </span>

  </h2>
  <h4 class="author">by makedonskii</h4>
</hgroup>

<div id="persona" class="primary" role="main" data-id="155045">
    <div class="featured">
    <div class="featured-inner object-lead">

      <div id="addon-summary-wrapper">
        <div id="persona-summary" class="">

          <div class="persona persona-large">
  <div class="persona-inner">
    <div class="persona-preview">
            <div 
      title="Adobe Flash Platform" style="background-image:url('https://www.getpersonas.com/static/5/0/197350/preview_large.jpg')"
      data-browsertheme="{&#34;iconURL&#34;:&#34;https://www.getpersonas.com/static/5/0/197350/preview_small.jpg&#34;,&#34;description&#34;:&#34;Adobe Flash Persona&#34;,&#34;accentcolor&#34;:&#34;#b52f2f&#34;,&#34;header&#34;:&#34;http://www.getpersonas.com/static/5/0/197350/flashpersonaforfirefox.jpg&#34;,&#34;footerURL&#34;:&#34;http://www.getpersonas.com/static/5/0/197350/flashpersonafooterforfirefox.jpg&#34;,&#34;textcolor&#34;:&#34;#ffffff&#34;,&#34;id&#34;:&#34;197350&#34;,&#34;category&#34;:null,&#34;headerURL&#34;:&#34;http://www.getpersonas.com/static/5/0/197350/flashpersonaforfirefox.jpg&#34;,&#34;name&#34;:&#34;Adobe Flash Platform&#34;,&#34;author&#34;:&#34;makedonskii&#34;,&#34;footer&#34;:&#34;http://www.getpersonas.com/static/5/0/197350/flashpersonafooterforfirefox.jpg&#34;,&#34;updateURL&#34;:&#34;https://www.getpersonas.com/update_check/197350&#34;,&#34;previewURL&#34;:&#34;https://www.getpersonas.com/static/5/0/197350/preview_large.jpg&#34;}">
              </div>

      
          </div>
  </div>
</div>

Comment 23

[Reed Loden [:reed]]

(In reply to comment #22)
> this will probably get yanked from amo soon but someone figure out how to
> inject personas content on amo at:
> 
> (https://addons.mozilla.org/en-US/firefox/addon/155045/

I'm not sure what's wrong here... AMO has an entire personas section (https://addons.mozilla.org/en-US/firefox/personas/), and since addons.mozilla.org is already whitelisted similar to getpersonas.com, the personas will show up. What problem are you seeing?

Comment 24

[chris hofmann]

Attachment: screenshot of the addon page

I guess I just wasn't expecting to see a persona preview when I'm looking at something called "Adobe Flash Platform"  as did the person reporting this to the addon abuse channel.

here was their comment.

"... I had some problem with flash contents in my FF 4, so I searched "flash" in addons website. Then I found this addon, with the very confusing name "Adobe Flash Platform", and carelessly clicked on "add to firefox".

I think the author intentionally use this name to mislead careless users to install it, for some reason, may be a prank. "

Maybe a rename of the addon to call it something like "Persona for Adobe Flash" or some variation that associates it with personas rather than the platform or the plugin would be helpful here.

Comment 25

[Justin Scott [:fligtar]]

A few months ago we allowed Personas to show up in the default search criteria, and we've seen a number of problems where people have called their Personas StumbleUpon, Firesheep, Adobe Flash, etc.

Users then install them thinking they're the extension and write bad reviews/abuse reports saying it doesn't actually work.

We were talking about this yesterday and decided we should stop showing them in the default search results. This reminds me to file the bug.

Comment 26

[Daniel Veditz [:dveditz]]

Maybe personas pages should look visually distinct from add-on pages in AMO (ditto search -- really had to look closely today to make sure I was installing the IMDB searchbar thing and not an IMDB site-related add-on).

Anyway, it's not this bug because AMO is allowed to host personas.

Did the reviewed patch from February ever land? Maybe this bug is fixed.

Comment 27

[Wil Clouser [:clouserw]]

Doesn't look like it.  Brandon, can you double check that attachment 428284 [details] [diff] [review] doesn't break anything and then land it?  Thanks.

Comment 28

[Brandon Savage [:brandon]]

This seems to work. Landed in r88676.

Comment 29

[Vishal Kamdar [:vish_moz]]

Could one of you please verify this fix on the Personas staging server? http://personas.stage.mozilla.com