1557651 - Apply security checks to synced addons when pulling down from the server, not just when pushing up

Status: RESOLVED INVALID

(Firefox :: Sync, defect)

Description

[Ryan Kelly [:rfkelly]]

As noted in Bug 1538008 Comment 2, the addons sync engine has a number of basic security-related checks that it does to determine which addons to sync, in particular this check whether the source URI is trusted:

https://dxr.mozilla.org/mozilla-central/rev/b8d6494caa5707d7f52c64d5d2f66d0254e69e88/services/sync/modules/engines/addons.js#619

As far as I can tell though, we only perform this check when deciding whether to push up a locally-installed addon to the server, via this call to isAddonSyncable here:

https://dxr.mozilla.org/mozilla-central/rev/b8d6494caa5707d7f52c64d5d2f66d0254e69e88/services/sync/modules/engines/addons.js#711

This is of absolutely no value in cases like Bug 1538008 where sync is used as a sandbox escape - malicious data in the sync server could cause the connected browser to sync down an addon from any untrusted source.

I feel like we should be able to apply this check in applyIncoming as well:

https://dxr.mozilla.org/mozilla-central/rev/b8d6494caa5707d7f52c64d5d2f66d0254e69e88/services/sync/modules/engines/addons.js#271

But I don't know enough about the mechanics of the addons sync engine to say for sure. Mark, does this assessment seem right to you?

It would also be worth thinking about whether there are other checks we could perform on the incoming addon record, but I don't have any off the top of my head.

Comment 1

[Tom Ritter [:tjr]]

I stared at this code for a while.

In applyIncoming we don't know where the extension is going to be coming from; so we can't apply the check there. The source of the extension comes from the response to the server, which looks like this: https://searchfox.org/mozilla-central/source/services/sync/tests/unit/addon1-search.json

We parse that structure here:
https://searchfox.org/mozilla-central/rev/0da35261b6789eec65476dbdd4913df6e235af6d/toolkit/mozapps/extensions/internal/AddonRepository.jsm#605

The most logical place (to me) to add a check where we have the url of the xpi we're downloading is https://searchfox.org/mozilla-central/rev/0da35261b6789eec65476dbdd4913df6e235af6d/services/sync/modules/addonutils.js#309

However, as far as I can tell, we look up the addons to sync (obtaining that json search result) from here: https://searchfox.org/mozilla-central/rev/0da35261b6789eec65476dbdd4913df6e235af6d/toolkit/mozapps/extensions/internal/AddonRepository.jsm#502

Which only searches in extensions.getAddons.get.url - so the attacker needs to either overwrite that value, or only sync an extension that's published on AMO. So while we can still add the check for defense in depth, it seems we may be intentionally or inadvertently accomplishing the same thing already...

I could be wrong though. I traced/tried to understand this code based on the test_addons_engine.js test

Comment 2

[Ryan Kelly [:rfkelly]]

it seems we may be intentionally or inadvertently accomplishing the same thing already...

Ah, interesting! It may be worth us ensuring that we have tests to cover this so it doesn't get regressed, if we don't already.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

(In reply to Tom Ritter [:tjr] from comment #1)

I stared at this code for a while.

Thanks - addon sycning is quite impenetrable code. It's very old, and some things could probably be modernized - eg:

In applyIncoming we don't know where the extension is going to be coming from; so we can't apply the check there. The source of the extension comes from the response to the server, which looks like this: https://searchfox.org/mozilla-central/source/services/sync/tests/unit/addon1-search.json

Yeah - we hit amo directly and I'd be surprised if the addons support in the browser doesn't have a way to get this info in a better way.

Which only searches in extensions.getAddons.get.url - so the attacker needs to either overwrite that value, or only sync an extension that's published on AMO. So while we can still add the check for defense in depth, it seems we may be intentionally or inadvertently accomplishing the same thing already...

Right - we always go through AMO (assuming those "global" prefs aren't set) because we need to ensure we grab the correct platform, for example. AddonRespository.jsm is an abstraction around AMO and the "addons repository" concept which already exists in the addons manager (sob)

In short:

• The URL for the addon isn't in the incoming payload - we ask AMO for it based on the ID.
• We have an additional check that the URL handed back from AMO is https, but otherwise trust it.
• The only preferences which support changing what we consider "AMO" are the same preferences defined by the addons manager itself, and bug 1538015 will tighten up whether any of those prefs can be changed by sync.

So I don't think there's anything to do here.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:markh, could you have a look please?

For more information, please visit auto_nag documentation.