Replace postMessage iframe origin check with x-frame-options and frame-ancestors CSP declaration

mozilla / fxa-content-server

DEPRECATED - Migrated to https://github.com/mozilla/fxa

Mozilla Public License 2.0

163 stars 120 forks source link

Replace postMessage iframe origin check with x-frame-options and frame-ancestors CSP declaration #2606

Closed rfk closed 8 years ago

rfk commented 9 years ago

In discussion with @shane-tomlinson he described a number of scenarios by which a malicious site could embed FxA in an iframe for phising or similar purposes. Unfortunately we can't blanketly deny iframing because we need it for some reliers, but we should try to be as restrictive about it as possible.

Consider this a meta-bug for "allow less iframability"; @shane-tomlinson can you please fill in concrete suggestions for things we should tighten up when you get a chance?

vladikoff commented 8 years ago

from mtg: @rfk what should we do regarding this bug?

rfk commented 8 years ago

I'll take this for triage into aha or similar

rfk commented 8 years ago

@shane-tomlinson has been working on a plan for this, I'm re-delegating this issue to him for revamping, closing, feature-carding or whatever seems appropriate.

shane-tomlinson commented 8 years ago

Here's my plan:

Ditch support for OAuth reliers, only allow the firstrun & sync flows hosted on www.mozilla.org
Have firstrun/sync flows open the iframe with a new query parameter, ?origin=current_origin
On the server side of the content server, check if service=sync & origin is specified. If so, check the passed in origin against a list of allowed origins. If the origin is not a member of the list, some appropriate error is displayed. Stop.
Set an X-FRAME-OPTIONS: ALLOW-FROM header with the origin.
Set an iframe-ancestor CSP directive with the origin.
Let the browser deal with problems from there.

shane-tomlinson commented 8 years ago

Addendum:

Rip out all of the extra content-server client code to do the postMessage dance.

vladikoff commented 8 years ago

Have firstrun/sync flows open the iframe with a new query parameter, ?origin=current_origin

I'm a bit out of the loop on this, why is this needed, can we just use browser apis to check the origin?

shane-tomlinson commented 8 years ago

why is this needed

Because the postMessage dance has timing problems.

use browser apis to check the origin?

Which browser APIs? If they are supported on our supported platforms, I'd like to consider it.

rfk commented 8 years ago

IIUC, both x-frame-options and iframe-ancestor require you to know the parent iframe's origin up-front, hence the little dance with ?origin=current_origin.

I'm not aware of a js api for directly checking the parent frame's origin.

rfk commented 8 years ago

Also, I like this plan, especially this part:

Let the browser deal with problems from there.

\o/

vladikoff commented 8 years ago

@shane-tomlinson revisiting this bug, is firstrun the only iframe source now? Should we just check for that and decline everything else?

shane-tomlinson commented 8 years ago

@shane-tomlinson revisiting this bug, is firstrun the only iframe source now? Should we just check for that and decline everything else?

Yup - that's what we currently do. This bug has morphed into something slightly different - replace the postMessage dance w/ x-frame-options ALLOW-FROM and iframe-ancestor CSP declarations. I should update the title...tomorrow.

davismtl commented 8 years ago

I just thought I would include this bug since I think it is related to this issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1262892

They posted a comment on it today and sent me a message to see if and when we could prioritize this. Let's discuss this issue in our next planning.