Closed Bug 1386984 Opened 8 years ago Closed 8 years ago

Move reviewer tools under a different domain

Categories

(Cloud Services :: Operations: AMO, task)

Product:
Cloud Services
Component:
Operations: AMO
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: muffinresearch, Assigned: wezhou)

References

Details

Currently content scripts are restricted on AMO. This means that with the switch to 57 we're essentially cutting off the reviewers abilities to use extensions to help with the review process. To fix that can we look at what it would take to move the reviewer tools to a different subdomain e.g. reviewers.a.m.o Before we move forward we need to know the following: * Be sure that a subdomain of a.m.o would work to remove the content-script restriction. * Know the paths involved to redirect traffic. * Run this past security to make sure that doing this isn't introducing more problems than it solves. * For example we'd need to confirm that mozAddonManager is not exposed to the subdomain. There's probably more things to consider feel free to add them as they come up.
I'll look into confirming the subdomain part, thanks for filing the bug!
Flags: needinfo?(philipp)
http://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/toolkit/components/extensions/WebExtensionPolicy.cpp#392 is the check that denies protected domains from running content scripts. This leads to http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManagerWebAPI.cpp#39 which suggests that only specific hosts are blocked, and e.g. reviewers.addons.mozilla.org would work. As for the paths involved, I suppose engineering could make a better comment on this, but from my perspective this is: * /firefox/files/ (for filebrowser modifications) * /editors/ (for queue, review pages, etc)
Flags: needinfo?(philipp)
Assignee: nobody → wezhou
+1 for that I heavily depend on my scripts for reviewing which I run using GreaseMonkey. At the moment, there isn't a problem but once GM is ported to Extension, I suppose the scripts will no longer run.
One additional thing I'd like to look into as part of this is the possibility of reducing the session lengths specifically for this host - it can be something we look at later though.
I've checked with engineering and the paths mentioned are sufficient. ni'ing psiinon for the security part of comment 0
Flags: needinfo?(sbennetts)
How would these scripts be managed? Via AMO or ?? Who would have perms to upload them, and would they be reviewed at all?
If you were asking me ...... These are user scripts that I wrote to facilitate my own reviewing. Other reviewers may have similar for their own. There are also addons, written by the reviewers to facilitate reviewing (same as those written to facilitate MDN editing). These addons often rely on tabs.executeScript() Firefox has disabled tabs.executeScript() on a number of domains (e.g. addons.mozilla.org, discovery.addons.mozilla.org, testpilot.firefox.com). This hard-coded restriction interferes with tabs.executeScript() and anything that relies on it such as copying to Clipboard (since there isn't any Clipboard API at the moment).
Simon, if we separate out the reviewer tools, in theory any developer could upload an add-on to AMO that would execute scripts on that page. Those add-ons may or may not be reviewed by a human. There is a risk that if an add-on reviewer (who is the only group with access to the reviewer tools) could install a signed add-on that runs scripts on the reviewer tools, doing things like approving all sorts of add-ons. This would require the reviewer to install an add-on that happens to contain such code though. That said, this is not the risk that led to content scripts being blocked on addons.mozilla.org. See bug 1295324 for details on why that happened. tl;dr; it is because scripts could install any further add-ons using the mozAddonManager API.
An alternative approach could be that we allow add-ons signed with the internal Mozilla signing certificate to use content scripts on AMO and then have those add-ons signed using that cert.
I'm nervous about malicious add-ons being able to affect the reviewer process, even if it means they have to be installed by reviewers. I like Andreas' proposal, although it will make things a bit more difficult for reviewers to upload and update their tools. Do we have any idea how many add-ons are in use that would need to be signed, and how often they get updated?
Flags: needinfo?(sbennetts)
The review process is already behind login security. Only reviewers or people who have especially been given access can interact with it. Therefore, the security should not be an issue and if there is, it should be tightened from the login side. Once above is dealt with, being able to use scripts and addons in order to facilitate reviews takes a greater precedence. Having considered the aforementioned, IMHO, moving the review process to a separate subdomain seems to be the more feasible option e.g. moving it from addons.mozilla.org to addons-review.mozilla.org Having special addons signed especially, besides adding more complexity, would mean only those approved can be used which prevents reviewers from using their own scripts or addons. Personally speaking, I don't use any addons for reviewing however I use my own scripts (which would be affected once GreaseMonkey becomes a WebExtension).
I'm not sure I agree with this. The fact that the review process is protected by authentication definitely limits the damage that can be done. Addons can no longer be installed on the bahalf of non reviewers if they visit AMO. But (as I understand it) reviewers are still at risk if they install malicious addons. These add-ons will be able to manipulate the review process on behalf of the reviewers, and no extra authentication mechanisms will prevent that. Either reviewers have to be _very_ careful which addons they install or we need something like the signed addons option that Andreas suggested. I'm still in favour of the latter :)
(In reply to Simon Bennetts [:psiinon] from comment #12) > I'm not sure I agree with this. > The fact that the review process is protected by authentication definitely > limits the damage that can be done. > Addons can no longer be installed on the bahalf of non reviewers if they > visit AMO. > But (as I understand it) reviewers are still at risk if they install > malicious addons. > These add-ons will be able to manipulate the review process on behalf of the > reviewers, and no extra authentication mechanisms will prevent that. That is exactly the risk.
I totally understand the concerns. To be honest, reviewers should be trusted to be able to know which addon they should not install. We trust the reviewers to pass/fail the same addons. ;) On the other hand, how is it any different than the status quo which has been the case for many years? In fact, new WebExtension addons are a lot more limited than the old legacy addons. Please note that the AMO hard-coded restriction in Firefox on WebExtensions can be bypassed by using a different browser and/or an older Firefox browser etc Anyway ... In that case, that would ONLY cover addons (I think there are 2-3 addons that have been made for the reviewers!!) and not scripts. What will be the situation with personal/individual script?
I've had a chat with :ulfr and it sounds like the signing option could be expensive in terms of time and effort to set up and maintain. Do we give any opsec guidance to reviewers? For example using a separate profile for reviewing add-ons, one with only the add-ons they _really_ need and trust in? I've had a quick look at https://wiki.mozilla.org/Add-ons/Reviewers/Guide and now spotted anything relevant. Another option could be for Firefox to detect (and maybe disable by default?) add-ons when on the new AMO reviewer domain. Users could then be asked to explicitly enable only the add-ons they want to use. I'm guessing this could require Firefox changes, so I dont know how practical this would be. :erosman if reviewers use another browser then we dont have a problem - these browsers wont access the mozAddonManager API :) But we definitely dont want to be in the situation where we inadvertantly encourage reviewers to use older FX versions. Re personal/individual scripts, I'm afraid I dont know - maybe someone else can address this concern?
(In reply to Simon Bennetts [:psiinon] from comment #15) > I've had a chat with :ulfr and it sounds like the signing option could be > expensive in terms of time and effort to set up and maintain. It's already set up and used: https://wiki.mozilla.org/Add-ons/InternalSigning > Do we give any opsec guidance to reviewers? For example using a separate > profile for reviewing add-ons, one with only the add-ons they _really_ need > and trust in? > I've had a quick look at https://wiki.mozilla.org/Add-ons/Reviewers/Guide > and now spotted anything relevant. Not that I know of. > > Another option could be for Firefox to detect (and maybe disable by > default?) add-ons when on the new AMO reviewer domain. Users could then be > asked to explicitly enable only the add-ons they want to use. I'm guessing > this could require Firefox changes, so I dont know how practical this would > be. Hm yea I'm not sure Firefox developers would agree building an maintaining something with such a small target audience.
Out of curiosity ... > these browsers wont access the mozAddonManager API Is there a way to control that API and prevent abuse/misuse?! From my own situation, everything that I do with my scripts (and Styles), deal with the displayed DOM (searching, tidying, highlighting, counting etc etc) I don't use any addons on AMO (except GreaseMonkey and Stylish to run scripts & styles). I don't interact with JS on those pages with my scripts at all. Is that, i.e. limited access to the mozAddonManager API, something worth considering?
We've just has a vidyo call about this. We agreed to separate the 2 issues: * Allowing reviewer tools to work with 57 * Preventing malicious add-ons from manipulating the reviewer process The new domain will help solve the problem with reviewer tools, so that should go ahead. Note that its a non trivial change, but Stuart is going to lead on that. Re add-ons manipulating the review process, we're going to start with OpSec advice and guidance. I'll lead on that. Firefox changes are still an option, but we'll get some feedback from reviewers to see how significant a problem this really is.
Thanks for the summary! I was also just writing one, so let me expand on one point: We will have the WebExtensions team consider creating a separate permission for access to reviewer tools, so that add-on reviewers installing a random add-on are aware that it could access the review tools subdomain. This will not block the prior steps though in favor of allowing reviewers to continue using their tools. Nevertheless, doing so will tighten security and without it becoming a major inconvenience. erosman, for your GM scripts if we move forward with the permission, you may have to create a custom unlisted GM that also has the mentioned permission and be extra careful of what GM scripts you run. Part of the opsec guidance will likely be to use a separate profile for reviewing, this way it will be harder for other add-ons to hijack the review process. The better option may be to migrate your GM scripts to a WebExtension once we've taken care of the first steps, but you are welcome to wait until we've made a decision on the permission.
I am overcautious by nature anyway. I only have 1 GM script not written by me (and 100s written by me) so I hardly ever install GM script from elsewhere (prefer to write my own) and I always check the code before installing anything (addon or script). ;) Over 80+% of the addons that I use are also written by me. I only have a handful of other addons and I have checked their code before installing. TBH, I have no idea if GM will be ready for FF57. They don't seem to have progressed much. As suggested by Philipp, I may also convert the script into an Addon for my use. I only use ONE script on AMO and one small Style. General Comment ... The restriction on AMO also affect pages other than the review pages. A fact that has caused a lot of negative feedback from users of addons, and addon developers, because addons don't work on AMO. Are those pages also exposed to mozAddonManager API? How do they deal (have been dealing) with non-WebExtension Legacy addons so far? If the only concern is "mozAddonManager API", can access to "mozAddonManager API" be restricted by hard-coding it into Firefox?
I spoke with mat and it looks like the next step is on the ops side of things. Wezhou, any chance you could look into this soon? tl;dr; comment 0, comment 2, comment 18, comment 19.
Flags: needinfo?(wezhou)
Hi Philipp, Thanks for pointing out the tl;dr; comments. I'm not familiar with the "reviewer tools" so I have some questions after reading the 4 comments. 1. What are reviewer tools? and can I access them to get a feeling how they work? 2. The comments mentioned "content scripts" and "reviewer tools". What is the relation between the two? or are they just the different names of the same thing? 3. I take it the request for ops to do is to redirect "/firefox/files/" and "/editors/" to reviewers.addons.mozilla.org? 4. What resides on reviewers.a.m.o? 5. What is "mozAddonManager"? Thanks!
Flags: needinfo?(wezhou)
(In reply to :wezhou from comment #22) > Hi Philipp, > > Thanks for pointing out the tl;dr; comments. > > I'm not familiar with the "reviewer tools" so I have some questions after > reading the 4 comments. > > 1. What are reviewer tools? and can I access them to get a feeling how they > work? This is the interface that add-on reviewers use to review add-ons. jorgev/TheOne can give you access. > > 2. The comments mentioned "content scripts" and "reviewer tools". What is > the relation between the two? or are they just the different names of the > same thing? Reviewer tools references the websites on AMO for reviewing add-ons. "content scripts" references developer-created scripts within a WebExtension that customize the behavior of websites to the liking of the developer. Some reviewers have developed such add-ons to improve their efficiency. > > 3. I take it the request for ops to do is to redirect "/firefox/files/" and > "/editors/" to reviewers.addons.mozilla.org? Yes. > > 4. What resides on reviewers.a.m.o? Currently nothing, the plan is is to host the reviewer tools (and file viewer) from this domain so that content scripts are not blocked for security reasons. See next question for more details. > > 5. What is "mozAddonManager"? This is an API in Firefox that allows the website to install add-ons. We have this on addons.mozilla.org and discovery.addons.mozilla.org so that add-on installs can be triggered, I believe in certain cases without confirmation. If content scripts were allowed to run on these websites, then an add-on could install arbitrary add-ons without the user noticing. By moving the reviewer tools to reviewers.a.m.o, content scripts can run on that domain without restrictions, so that reviewers can create/update their own add-ons again to aid with reviewing. > > Thanks! I hope this answers the questions, let me know if you need more information.
Flags: needinfo?(wezhou)
Hi Philipp, Thanks for the reply. It sounds like the "reviewer tools" is one of the functions of the existing a.m.o web site. Since you mentioned that "the plan is is to host the reviewer tools (and file viewer) from this domain", do we know how? I can image that one way is to run addons-server code on the reviewers.a.m.o site, however, that means the reviewers.a.m.o will have all the rest functions a.m.o has to offer, and people can go to either site and see the same thing. (We do that for the "admin tools" btw. Except that "admin tools" is behind VPN, so the public doesn't have access. But I don't think "reviewer tools" is required to be behind VPN.) If we're not going to run the whole addons-server code on the reviewers.a.m.o site, then it means "reviewers tools" function should be deployed and run separately from the rest of a.m.o functions. Is that the plan or something else?
Flags: needinfo?(wezhou)
Yes, reviewer tools is a function of the existing site. I don't know how that would be done best. I don't know if there are any downsides to running the whole AMO on reviewers.a.m.o, engineering/security would have to comment on that. I'm putting mat on CC for some details. In my superficial understanding I'd imagine some sort of internal redirects so that e.g. reviewers.a.m.o/ab-cd/editors/ would internally access the same instance and data that a.m.o/ab-cd/editors/ would.
Flags: needinfo?(mpillard)
I noticed that Firefox's built-in features don't work on AMO either when I tried the new Screenshot tool in FF56 !!?
> If we're not going to run the whole addons-server code on the reviewers.a.m.o site, then it means "reviewers tools" function should be deployed and run separately from the rest of a.m.o functions. We might want/need that separate deploy in the future, but technically, from an engineering point of view, we don't have to do it for the moment to implement this (we could just have a rule to make sure the relevant paths only work on reviewers.a.m.o domain but still use the same instances, as Philipp is pointing out).
Flags: needinfo?(mpillard)
(In reply to Mathieu Pillard [:mat] from comment #27) > We might want/need that separate deploy in the future, but technically, from > an engineering point of view, we don't have to do it for the moment to > implement this (we could just have a rule to make sure the relevant paths > only work on reviewers.a.m.o domain but still use the same instances, as > Philipp is pointing out). OK, so if I understand it correctly, what we'll do is, when reviewers visits "/firefox/files/" and "/editors/" on a.m.o site, they get redirected to reviewers.a.m.o, and then at the proxy layer, we make it so that these requests are sent to the same application instances that serve a.m.o, however, the reviewers.a.m.o site will only serve the above two end points. Does that sound right?
Flags: needinfo?(mpillard)
From my side that sounds like the right thing to do, happy to have mat confirm if it works from an engineering perspective.
I agree we should try and make the least invasive change to get this done. Presumably not running a distinct instance/instances means that we don't get to change the config independently so we will need to see if that has any impact (e.g. CSP, though that might be OK) and potentially make a plan for going that route later on so that we can refine things like session length and other relevant settings.
> OK, so if I understand it correctly, what we'll do is, when reviewers visits "/firefox/files/" and "/editors/" on a.m.o site, they get redirected to reviewers.a.m.o, and then at the proxy layer, we make it so that these requests are sent to the same application instances that serve a.m.o, however, the reviewers.a.m.o site will only serve the above two end points. That sounds right, yes. > Presumably not running a distinct instance/instances means that we don't get to change the config independently so we will need to see if that has any impact (e.g. CSP, though that might be OK) and potentially make a plan for going that route later on so that we can refine things like session length and other relevant settings. Yeah, doing it that way means changing the config independently is a lot harder - this is just a first step to get this done, once we do have the separate domain up and running we can look into completely separate instances.
Flags: needinfo?(mpillard)
Depends on: 1402948
As a side note: While the argument has been made to prevent injecting scripts into AMO, why is CSS blocked? CSS should not pose any security risk.!!??
Unfortunately it can pose a security risk: https://www.owasp.org/index.php/Testing_for_CSS_Injection_(OTG-CLIENT-005) "The impact of such a vulnerability may vary on the basis of the supplied CSS payload: it could lead to Cross-Site Scripting in particular circumstances, to data exfiltration in the sense of extracting sensitive data or to UI modifications." Its typically harder to exploit but it is still possible.
Hi Philipp or Mat, On the reviewers.a.m.o site, do we need to allow /api/v3/ endpoint? For example, does fxa need to hit the redirect uri path after authorization is complete? Thanks.
(In reply to :wezhou from comment #34) > Hi Philipp or Mat, > > On the reviewers.a.m.o site, do we need to allow /api/v3/ endpoint? For > example, does fxa need to hit the redirect uri path after authorization is > complete? > > Thanks. Best for mat to answer this, I'm not sure how auth will work with the new site.
Flags: needinfo?(mpillard)
> On the reviewers.a.m.o site, do we need to allow /api/v3/ endpoint? For example, does fxa need to hit the redirect uri path after authorization is complete? Yes, good point, we will need it.
Flags: needinfo?(mpillard)
I deployed the change to -dev. Please test.
I see that https://github.com/mozilla/addons-server/issues/6736 is verified and closed, thus I deployed the change to -stage. Please test. Philipp, Andreas, when would you like this to go to -prod once -stage passes QA?
I've tested this on -dev and from my perspective it works as expected. I was able to run all the content scripts I need for my add-on and was able to organize those away that were not on the reviewers domain. For me this can go to prod as soon as it passes QA. mat, anything you'd like to check?
Flags: needinfo?(mpillard)
I've been testing it since it landed on dev and it appears to be working just fine.
Flags: needinfo?(mpillard)
:krupa, can your team help test it in -stage please? Ideally, I'd like to push it out by next Tuesday.
Flags: needinfo?(kraj)
(In reply to :wezhou from comment #41) > :krupa, can your team help test it in -stage please? Ideally, I'd like to > push it out by next Tuesday. on it.
Flags: needinfo?(kraj)
Verified as fixed on Stage too. All review actions can be done under the new domain and all links to AMO are working fine, redirecting to the correct domain.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Before this goes to production - is there anything else we need to do or file issues for? Simon - are you happy for this to go to production from a security perspective?
Flags: needinfo?(sbennetts)
Yes, I'm happy for this to go to production.
Flags: needinfo?(sbennetts)
The change for this is on -prod now.
It has been changed today an the new sub-domain seems to be working fine.
The new sub-domain also appears to be more responsive (faster).
https://reviewers.addons.mozilla.org/ redirects to https://addons.mozilla.org/<LOCALE>/firefox/ which it shouldn't, instead it should redirect to https://reviewers.addons.mozilla.org/<LOCALE>/editors/
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Filing a new bug for comment 49.
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Validation is still on AMO domain which wont allow script injection https://addons.mozilla.org/en-US/developers/addon/*/file/*/validation
Can you file a new bug to consider that? The validation would need to be available on both domains, and we need to make sure to not expose all of developers/addon/
New bug filed: bug 1416215
You need to log in before you can comment on or make changes to this bug.