Closed Bug 1225450 Opened 8 years ago Closed 8 years ago

Check origin for Sync Inter-App-Communication (IAC)


(Firefox OS Graveyard :: Sync, defect, P3)

Gonk (Firefox OS)


(Not tracked)

2.6 S2 - 12/4


(Reporter: freddy, Assigned: ferjm)



(Keywords: csectype-spoof, sec-low, sec-want)


(1 file)

Let's assert that messages come from the correct origin.
This needs to happen in System, Settings and Sync apps.

This bug may require some investigation as to whether |event.origin| exists, then we can just check for the well-known app:// origin.

(Rating as sec-low, since IAC currently requires a certified app)
Blocks: 1212834
IIUC we want to restrict that they *go to* the correct origin, right? The sync requests come from the system app and go to the sync app, containing sensitive data (kB, assertion).

The important thing to restrict is other certified apps announcing 'gaia::sync::request' in their manifest and eavesdropping on these events as they come from the systems app. The other way around is less of a risk, iiuc?
Flags: needinfo?(fbraun)
Yes, thank you for the clarification.
The main concern is that some app other than the Sync app receives the key material.
Flags: needinfo?(fbraun)
Blocks: fxos-sync
Quoting ferjm's hints from IRC:
> there are different ways depending on what we want to do
> … from the system app, we are supposed to be able to specify a set of rules when calling .connect(). But I don't remember if that's actually implemented. From the sync and settings apps, we can specify these rules on the manifest or what bug 1173666  introduced.
Since the passwords addon will be syncing its own data, and the sync app will be syncing the history data, we should check if the origin is one of:
* the sync app
* the passwords addon

This means they could hijack each other's IAC requests, but that's not such a big issue, as long as we exclude all other origins, right?
Is the password add-on too receiving the crypto material for syncing?
I was assuming this all happens in the sync app.

Maybe we could use and extend the existing diagrams I did for the security review in bug 1212834 :)
It seems that we won't be able to do this for extensions:

[22:05:00]  <ferjm>	fabrice hey, do you know if we allow setting a custom origin on addons?
[22:06:09]  <@fabrice>	ferjm: likely, but you should not use that, since addons will get their own special moz-extension: url anyway
[22:09:54]  <ferjm>	fabrice yeah, but the moz-extension: urls will be different per installation, just like app:// ones, right? I need a fixed origin to check that requests are coming from the password manager to avoid leaking the sync crypto keys
[22:10:29]  <@fabrice>	ferjm: yes they will be different. What are you trying to do exactly?
[22:12:47]  <ferjm>	fabrice I am trying two things: 1- allow the registration of new synchronizers (right now we have only the sync app syncing history and bookmarks but we want the passwords manager to sync passwords without needing to expose them through a datastore). 2-
[22:17:19]  <@fabrice>	ferjm: I don't know what the overall design is, but it seems that handing the keys to the extensions is backward. Why not have the extension just send unencrypted data to the sync engine? That's how it worked with the "old" toolkit/ sync engines
[22:23:54]  <ferjm>	fabrice, that would be way too many IPC calls :\ and we would still have the issue of trusting the extension to receive the passwords data coming from sync.
[22:24:59]  <ferjm>	fabrice why shouldn't we use a custom origin on an extension? how are moz-extension:// urls different to app:// urls on that?
[22:26:02]  <@fabrice>	ferjm: they change at each restart of the extension
[22:27:34]  <ferjm>	fabrice ugh, ok, I see.
[22:29:04]  <@fabrice>	ferjm: so... I'm not sure how we can make extensions trusted for that, but I would rather give them only passwords instead of keys
[22:34:21]  <ferjm>	fabrice I guess we could have one key per collection to minimize the risk... but that would involve asking other clients to do the same. In any case, I think we are being a little bit paranoids here given that the keys are exposed via IAC, which is and will likely ever be certified. I'll comment on the bug. Thanks!
[22:34:47]  <@fabrice>	gandalf: not really unfortunately...
[22:35:27]  <@fabrice>	ferjm: the password manager injects itself in the system app right?
[22:38:31]  <ferjm>	fabrice the content script is injected in all apps and it has a background script, which I guess runs in the system app process
[22:38:55]  <ferjm>	fabrice maybe we can use the addon id ?
[22:39:03]  <@fabrice>	ferjm: for now yes, but there is work ongoing to move background scripts in their own process
[22:40:24]  <@fabrice>	ferjm: how would you enforce that?
[22:45:57]  <ferjm>	fabrice I guess we would need the IAC API to expose the ID for extensions the same way that it exposes the origin for apps within connection requests
[22:46:47]  <@fabrice>	why do you need IAC at all? That's strange to mix WebExtensions and IAC
[22:48:36]  <ferjm>	fabrice that's currently the way we wake the sync app up to start the synchronization process of history and bookmarks. Maybe we need another mechanism for extensions...
Flags: needinfo?(fbraun)
Let's just check for the Sync app's origin for now, so that we can enable read-only history sync in master?

When we add passwords sync we can e.g. change it to 'the sync app and any addon', or see what others options we have (also seeing fabrice's last remark).
Yeah, maybe we need a different communication channel. Only makes sense to switch if it provides authenticity though.
Michiel: And yes, we can just check for the sync origin for now and revisit later, when we enable password sync.

(Next time, please include a question with the needinfo request, so I get the context in the same email :))
Flags: needinfo?(fbraun)
Assignee: nobody → ferjmoreno
Priority: -- → P3
Target Milestone: --- → 2.6 S2 - 12/4
Attachment #8694267 - Flags: review?(mbdejong)
Attachment #8694267 - Flags: review?(mbdejong) → review+
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.