Frederik Braun [:freddy] Reporter
	Description • 9 years ago

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://foo.gaiamobile.org origin.


(Rating as sec-low, since IAC currently requires a certified app)

	Michiel de Jong [:michielbdejong]
	Comment 1 • 9 years ago

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?

	Frederik Braun [:freddy] Reporter
	Comment 2 • 9 years ago

Yes, thank you for the clarification.
The main concern is that some app other than the Sync app receives the key material.

	Frederik Braun [:freddy] Reporter
	Comment 3 • 9 years ago

Quoting ferjm's hints from IRC:
> there are different ways depending on what we want to do https://bugzilla.mozilla.org/show_bug.cgi?id=1173666 https://bugzilla.mozilla.org/show_bug.cgi?id=1169344 https://mxr.mozilla.org/mozilla-central/source/dom/webidl/InterAppConnection.webidl
> https://wiki.mozilla.org/WebAPI/Inter_App_Communication_Alt_proposal
> … 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.

	Michiel de Jong [:michielbdejong]
	Comment 4 • 9 years ago

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?

	Frederik Braun [:freddy] Reporter
	Comment 5 • 9 years ago

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 :)

	Fernando Jiménez Moreno [:ferjm] Assignee
	Comment 6 • 9 years ago

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- https://bugzilla.mozilla.org/show_bug.cgi?id=1225450#c4
[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 https://github.com/daleharvey/fxospwdmgr/blob/master/manifest.json#L18 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 https://github.com/daleharvey/fxospwdmgr/blob/master/manifest.json#L14 ?
[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...

	Michiel de Jong [:michielbdejong]
	Comment 7 • 9 years ago

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).

	Frederik Braun [:freddy] Reporter
	Comment 8 • 9 years ago

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 :))

	Fernando Jiménez Moreno [:ferjm] Assignee
	Comment 10 • 9 years ago

https://github.com/mozilla-b2g/gaia/commit/35943e523844dea9259a391b6032dfa56a668c1a