1209326 - Use correct manifest properties when verifying addon update

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect, P2)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

When an addon is updated we check that the newly downloaded update has been properly signed.

But in addition to that we should also check that the addon is an update to the addon which the user has. This in order to prevent an attacker from hacking the marketplace website and updating all users of an addon to an old version of some previously signed addon which has known security issues.

To do this we should check that the "version" property is greater than the currently installed version, and that the "id" property is the same as the currently installed version.

The format for the "version" property is defined here:
https://developer.chrome.com/extensions/manifest/version
Important to note is that this is not a simple number, but rather something on the form of x.y.z.n

The format for the "id" property is probably just an arbitrary string. It is up to the marketplace to make sure that this string is unique.

Comment 1

[Māris Fogels [:mars] (please needinfo)]

[Blocking Requested - why for this release]: potential issue for 2.5, :fabrice or :pauljt to evaluate

Comment 2

[Paul Theriault [:pauljt] (no longer reading bugmail)]

So this is a secondary control (the primary being the security of the marketplace) but it sounds important to me. Checking that a user actually _has_ the add-on version for which an update is being applied sounds like a pretty basic check to me. 

But actually do we enforce this in the marketplace yet? Or do developers just have to know that they have to increase the version string?

I'm a bit nervous about _not blocking_ on this, since I don't really know the marketplace developers controls very well. But not sure that this is really a reason to make this block. I'll leave this to fabrice to decide - if its a simple low risk change, I think we should do this, if only to reduce the attack vectors from marketplace.

Comment 3

[David Durst [:ddurst]]

(In reply to Paul Theriault [:pauljt] from comment #2)
> But actually do we enforce this in the marketplace yet? Or do developers
> just have to know that they have to increase the version string?

That is the only exposure this has in Marketplace right now. The id (a UUID) is generated when the add-on is submitted. The version is supplied in the manifest and has to conform to those same rules (meaning, those in https://developer.chrome.com/extensions/manifest/version -- basically x.y.z.n), and new versions submitted by a developer in Marketplace must increase from the previous version (1.0.0.1 -> 1.0.0.2 or 1.1, et al). Both are added to the mini-manifest.

Comment 4

[Candice Serran (:cserran)]

There was a note that we need evaluation on this. NI for Michael (ENG) and Naoki (ENG)...can you either of you way in on this please?

Comment 5

[Michael Henretty [:mikehenrty][:mhenretty]]

Pretty late in the game for this guys. This bug was filed almost a month ago, and now less than one week before deadline we are deciding to block on it. In the postmortem let's figure out why the communication broke down.


Dale, you are working on the silent add-on updates. Can you weigh in on the effort here, and can you perhaps roll this into the work you are doing there?

Comment 6

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I would say that it's needed as it's a security risk outlined in comment 0.
I am unsure the amount of work that it would take to develop this; to better the product (and the web), I think we should block on this.

We'll have to run some negative test cases as well as positive to ensure that the code is correct.
Transferring NI to Mike to see how he feels about the situation since he's the one testing.

Comment 7

[[:fabrice] Fabrice Desré]

This needs some gecko work, but this is not hard. As usual, most of the work will be writing the update test. There should not be manual testing needed.

Comment 8

[Mike Lien[:mlien]]

From my perspective, I'll echo Michael's comment it's late at this time to block. But I think it's pretty important to check and verify the correct properties. Since we cannot do anything on user/device side for manual testing, I'd like to forward the NI to Krupa for more input.

Comment 9

[Dale Harvey (:daleharvey)]

This isnt particularly related to https://bugzilla.mozilla.org/show_bug.cgi?id=1216301 and touches a different bit of code / is tested differently, can take a look after but wont be part of the same patch

Comment 10

[krupa raj[:krupa]]

We have verified that changing add-on properties like name, description (in the manifest.json) across consecutive versions get reflected in marketplace listing pages. However, this has not been verified on client-side by my team since that is out-of-scope for us.

Comment 11

[Mahendra Potharaju [:mahe]]

Dale,

Any chances of looking at this in the week after Orlando? If not, I'd prefer to defer this to 2.6.

Comment 12

[BMO Automation]

Firefox OS is not being worked on