Closed Bug 1472197 Opened 2 years ago Closed Last year

Security Review: Return-to-AMO

Categories

(Firefox :: Security: Review Requests, enhancement)

64 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 64

People

(Reporter: cr, Assigned: pauljt)

References

(Blocks 1 open bug, )

Details

(Whiteboard: review)

Risk Assessment for Return-to-AMO

https://trello.com/c/fiSvzyRH
Target Milestone: Future → Firefox 64
Version: Trunk → 64 Branch
This is bug 1475354. It's unclear to me what the status here is. It sounds like the flow will end up with about:welcome. Not clear if that means changes to about welcome or not. Leaving open to figure that out. But we might just want to test this. (ill file a dependent bug)
Assignee: nobody → wleung
This security review has missed the boat, and it needs testing now so Im taking this back and turning it back into a testing bug.
Assignee: wleung → ptheriault
Group: mozilla-employee-confidential

We discussed security review in several other bugs (1511173, 1511104), and I think we are basically complete as far as security review of this feature goes. Our notes are captured in the document here:
https://docs.google.com/document/d/1u_TcLFQ3e7Gn4UhotyGLpz2J9NnmScD11NaRm_3gaDE/edit#

The main risk focused on with this review as "could this flow be used by an attacker to coerce a user into installing a malicious add-on"? While it's possible (as its possible for AMO addons to be malicious), but I think its unlikely to be a useful attack vector for an attacker (as compared to just a regular phishing site encouraging a user to install a malicious executable). The factors that lead me to this assessment are:

  • this flow can only be used to prompt for installation of an addon, there is still user involvement in the install
  • Its only possible for publicly listed add-ons (no unlisted add-ons)
  • It applies only to users who do NOT already have Firefox

It's noted that as the design currently stands it would be possible for a 3rd party to take a link which generates the a stub-installer for a specific add-on and send that anywhere. It might be possible to mitigate this by www.mozilla.org checking the referrer of a link [1]. But this seems an unlikely attack vector due to the considerations listed above.

Just also to note for posterity, at the time of writing there is still ongoing discussion in 1511104 as to the need to sign parameters coming from AMO, so that www.mozilla.org could verify the link was generated by AMO. I think this is good defense in depth to limit the attack surface on www.mozilla.org download page, but as pointed out in 1511104#c55, this link could always be copied and used elsewhere.

[1] a recommendation has been provided in https://bugzilla.mozilla.org/show_bug.cgi?id=1511104#c58

Status: NEW → RESOLVED
Closed: Last year
Depends on: 1511104, 1511173
Resolution: --- → FIXED
Summary: Risk Assessment: Return-to-AMO → Security Review: Return-to-AMO
Whiteboard: rra → review

PLOT TWIST (repeating from 1511104):

OK, so we're going to whitelist extensions on the API side of things.

So links will remain populated with all the params (so attribution will not be disrupted, just adding the utm_content param), but the API will only respond when the request is from an extension on the whitelist.

No changes required in FF or Bedrock (this bug), change to AMO side off trains.

This will constrain that risk to a list that we are already maintaining. This should mitigate as much risk as possible.

Will this bug require any manual validation from the QA team? if yes please provide some info on how to correctly validate, thanks!

Flags: needinfo?(ptheriault)

(In reply to Vlad Jiman from comment #5)

Will this bug require any manual validation from the QA team? if yes please provide some info on how to correctly validate, thanks!

No, I don't think so. There is nothing to verify in Firefox, the change here is on the AMO side, and I would expect changes to the AMO API would have appropriate tests to validate their functionality.

Flags: needinfo?(ptheriault)
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.