Closed Bug 708697 Opened 13 years ago Closed 13 years ago

Signed MAR files do not protect against a cross-channel update

Categories

(Toolkit :: Application Update, defect)

Product:
Toolkit
Component:
Application Update
Platform:
x86
Windows Vista
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WORKSFORME
Tracking Flags:
Tracking Status
firefox-esr10 --- unaffected

People

(Reporter: imelven, Assigned: bbondy)

References

Details

(Whiteboard: [sg:moderate])

Although MAR files will be signed as part of bug 699700, the channel of the application is not part of the MAR file itself. This means a legitimately signed but older MAR file can be used to downgrade OR updgrade an install of an application. There are other checks around MAR files - a SHA-512 hash of the MAR is downloaded over SSL and used to verify the MAR file downloaded over http was not tampered with. Circumventing this protection would require compromising the update server's SSL session via either attacking the server and its private key directly, or forging/hijacking a cert that would be trusted for the SSL session. Bug 708688 is about downgrading the version specifically (ie installing Firefox 7 instead of 8), bug 708690 is about installing the wrong product (Thunderbird vs Firefox) and this bug is about installing the wrong channel - this could be a downgrade that introduces a security fix patched in a later channel/version OR an upgrade that installs e.g. Nightly which contains a new security hole.
I don't understand why this is a problem... the update channel is not embedded in the MAR file by design, so that we can push the same update to multiple channels without switching users, and we have used this in the past. I don't think this is a flaw or needs to be fixed.
I second comment #1. Also, we have checks to prevent us from downloading a malicious mar file so the remote attack is already mitigated. A local evil user process could but that same local evil process can already do things like change the user's prefs (think of homepage), add certificates, change the session restore file, etc. (e.g. anything in the profile and more), and so on. I personally think this is much worse than downgrading to a previous release.
I'd like to know whether security agrees this is wontfix per comment #1 and comment #2 or rationale provided why this should be fixed that takes into account comment #1 and comment #2.
Without protecting against this, then any unprivileged user will be able to replace a Firefox release with a Firefox pre-release. These unprivileged users do not (necessarily) have authority to do so. Now, without the service, they currently are not able to do so. So, this is a security regression. I do not understand the case where a single MAR file can be applied to multiple channels, or why that is common enough to optimize for. But, if it is necessary, then we could just include multiple channel designations in the MAR file, instead of just one.
This could be mitigated by using different certificates for each channel since the certificate check would fail and prevent updating to that channel. catlee: is doing the above ok with you?
(In reply to Robert Strong [:rstrong] (do not email) from comment #5) > This could be mitigated by using different certificates for each channel > since the certificate check would fail and prevent updating to that channel. > > catlee: is doing the above ok with you? This wouldn't handle the case where we are supposed to support using the same MAR file for multiple channels, which bsmedberg indicated was important, unless we implement the multiple-signature support. I feel like this is very similar to the version-downgrade problem (bug 708688), and, IMO, it would be nice if both were handled the same way.
You are right and the current release process is that the beta, release, and release-test channels are required to be exactly the same. I don't think this can be handled without breaking the requirement that those channels be the same. I don't see how supporting multiple signatures would solve this since this would just allow going to other channels based on the additional signatures. Using the same signature for the channels that the current release process requires to be the same would have the same result. The possibility of a user of a system (note: the guest account is disabled by default) doing what you suggest to another user is to say the least rather unlikely. I really don't think the additional work to mitigate this along with risk / reward makes this something we should be all that concerned about.
(In reply to Robert Strong [:rstrong] (do not email) from comment #7) > The possibility of a user of a system (note: the guest account is disabled > by default) doing what you suggest to another user is to say the least > rather unlikely. I really don't think the additional work to mitigate this > along with risk / reward makes this something we should be all that > concerned about. It isn't unlikely. People do this all the time in school computer labs, libraries, corporate computers, etc. It is normal for the owner of a computer to let people use the computer even when they don't trust for installing software. For example, I have an account akin to "guest" that I let friends use to check Facebook, etc., on my Mozilla-issued laptop. By no means do I want them to be able to control what version of Firefox I use on my own account on that computer. This and bug 708688 must be fixed in order for the service to be considered secure enough to be enabled for release.
tracking-firefox11: --- → ?
And in the environments you mentioned (e.g. school computer labs, libraries, corporate computers, etc.) they have the option of not installing the service.
bsmith wrote: > I do not understand the case where a single MAR file can be applied to > multiple channels, or why that is common enough to optimize for. But, if it > is necessary, then we could just include multiple channel designations in > the MAR file, instead of just one. I talked to catlee and bhearsum and there are 3 channels at least that share MARs currently. aurora / aurora-test beta / beta-test release / release-test RelEng also mentioned they use release-test to make sure that the mirrors and bouncer are ready before pushing updates through them. Baking in the channel breaks a lot of assumptions various systems have about how updates works and vastly increases testing load too. At build time, not all of the channels that will share the MAR are known. It is my understanding that this task at least is going to cause a lot of disruption for RelEng and QA.
Also there may be cases we may want to actually change a user's channel. For example in the case of the introduction of a new channel, or in the case of if we ever removed a channel. For example let's say one day in the future we decide to remove aurora and make the beta channel twice as long, we would want to upgrade people from aurora to beta.
Assignee: nobody → netzen
If we are doing this the current channel at build time can be found in: MOZ_UPDATE_CHANNEL. But see Comment 10 and Comment 11 for major concerns with this task.
(In reply to Brian R. Bondy [:bbondy] from comment #11) > Also there may be cases we may want to actually change a user's channel. > For example in the case of the introduction of a new channel, or in the case > of if we ever removed a channel. For example let's say one day in the > future we decide to remove aurora and make the beta channel twice as long, > we would want to upgrade people from aurora to beta. In comment 4 bsmith suggested that we can add multiple channels to the MAR. Is that feasible? If so, are there additional issues with using MARs on multiple channels (release / release-test) or removing/adding channels in the future?
At build time, not all of the channels that will share the MAR are known.
(In reply to Brian R. Bondy [:bbondy] from comment #14) > At build time, not all of the channels that will share the MAR are known. OK. Thinking about this for one additional minute I think we would need to limit the MARs that contain multiple channels so that it is not regular practice to generate them (or we're in the same position that we're currently in where I can force a move between channels - even if the move is from release to beta then from beta to aurora ...). This method would need to be a release valve in case we have a need to move a user to a different channel. I'm not sure how feasible this would be for the regular releng and QA work.
(In reply to Brian R. Bondy [:bbondy] from comment #14) > At build time, not all of the channels that will share the MAR are known. Strawman #1: 1. Put exactly one channel name ("release", "beta", "aurora", "nightly") in the MAR file (somewhere). 2. updater.exe will get the currently-installed channel from somewhere. This will be a string like "aurora" or "aurora-test". Assign this to X. 3. Truncate X at the first "-". For example, "aurora" becomes "aurora", "aurora-test" becomes "aurora". 4. Do the comparison. 5. Require all testing/non-end-user channels to adhere to this naming convention. 6. Do not allow any end-user channels to contain "-". 7. File follow-up bugs, if necessary, to do this in a better way that removes restrictions #5 and/or #6, if they are seen as being bad. Strawman #2: 1. Put exactly one channel name ("release", "beta", "aurora", "nightly") in the MAR file (somewhere). 2. In addition to storing the current channel name in the installation, also store an additional "MAR channel" name. The "MAR channel" would always be one of the values listed in #1. For example, for the aurora-test channel, this would be "aurora-test" and "aurora". For end-user channels, these two values would always be identical. 3. updater.exe will get the currently-installed *"MAR channel"* from somewhere. 4. Do the comparison of the "MAR channel" with the channel name given in the MAR file. 5. File follow-up bugs, if necessary, to do this in a better way.
(In reply to Brian Smith (:bsmith) from comment #16) > 1. Put exactly one channel name ("release", "beta", "aurora", "nightly") in > the MAR file (somewhere). By the way, I don't know if it would be a good idea to change the MAR file format to do this, or if updater.exe could/should just read this value out of some file that is embedded in the MAR file (i.e. read it out of one of the files it is about to install). The same could be said for the product name and version checks.
Whiteboard: [sg:moderate]
I'm very much opposed to requiring that the channel name be baked into the mar. Mar files up until now have been 'channel-less', and we have relied on that behaviour in our regular release testing process, as well as extraordinary circumstances where we've moved users between channels e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=675805 https://bugzilla.mozilla.org/show_bug.cgi?id=518508 Partner repacks also rely on being able to apply mar files from the 'release' channel even though they're on their own partner specific channel. We've also relied on this behaviour to move beta users to the final release build, e.g. we moved users with 8.0 beta builds to 8.0 final build when it was available. In this case the users remained on the 'beta' channel, but were served the MAR files for the final release build. These are just a few cases that we've been able to identify that have relied on being able to treat MARs as 'channel-less'. I'm sure there are quite a few other cases where we've done this, and so the consequences of making this change are potentially quite large, and are at the least not well understood. Please get feedback also from QA, product and release drivers on this. This change can drastically affect how we approach releases.
Valid task: I just wanted to add that there is a valid argument in protecting channel switches in general (a public user at a library can upgrade a v9 release to a v12 unstable release with possible security issues). That is not being argued, so let's not focus on that. Existing problem: The channel switch (as well as product switch and version downgrade) is currently a problem in existing versions with and without the service. Showing a UAC prompt does NOT protects against these things. We are asking a user to entrust that Mozilla code can run with a UAC prompt. We give the user no visibility today into what is in the MAR that will be applied. We don't tell them they will potentially be changing channels. How can a user know not to click on the UAC run dialog? Is it our recommendation that a user should not trust code signed by us? Surely not. Is it our recommendation that a user should manually inspect a MAR? surely not. So it is an existing issue with every version of Firefox that had an updater with MAR files. Proposed: I think that we should eventually do this, but I think as catlee mentions this can have a huge amount of effects that we aren't ready for. I think we should plan this out separately as a new project and take small steps to get there and only land the channel code once that is done. I think the version check and product checks should remain.
I just thought of something extra (again it affects people with and without service). If we do take the version downgrade check code, but do not take the channel check code, then someone could in effect disable updates for several weeks until that channel catches up to the version. I think we need to explicitly identify all of the issues that channel check code would cause.
(In reply to Brian R. Bondy [:bbondy] from comment #19) > Proposed: > I think that we should eventually do this, but I think as catlee mentions > this can have a huge amount of effects that we aren't ready for. I think we > should plan this out separately as a new project and take small steps to get > there and only land the channel code once that is done. I think the version > check and product checks should remain. this sounds entirely reasonable to me, it's clear there's major consequences to doing this. great thoughts re the attack you outline in comment #20 - this sort of 'freeze attack' where a user cannot update is definitely a real security issue IMO. let me know if i can help move this new project along :)
(In reply to Brian R. Bondy [:bbondy] from comment #19) > The channel switch (as well as product switch and version downgrade) is > currently a problem in existing versions with and without the service. > Showing a UAC prompt does NOT protects against these things. We are asking > a user to entrust that Mozilla code can run with a UAC prompt. We give the > user no visibility today into what is in the MAR that will be applied. > We don't tell them they will potentially be changing channels. I agree that this is a separate issue that should be addressed separately. But... > How can a user know not to click on the UAC run dialog? > So it is an existing issue with every version of Firefox that had an > updater with MAR files. Only Administrators can even get to the UAC prompt. One thing that makes this bug worse with the service than without it is that you don't even have to be an administrator to screw up the Firefox installation with the service-based update, like you do with the non-service-based update. It seems like everybody understands that, and it is just a matter of determining severity or the net difference in severity given what we currently are vulnerable to. It is definitely the case that an admin could have some non-elevated malware running in his session that could trigger the non-service update scenerio with a MAR file from a channel of its choice. That is why it is a good idea to have some UI explaining the channel change if we need to support that. Again, that is something for a separate bug, I think. Legit channel change is a *very* rare event, right? The service-based installation doesn't need to support it, AFAICT. So, it seems very reasonable to me to have the service-based installation refuse to change channels, but allow UAC-prompting updates to do so, with the understanding that eventually the prompting updates will have a clearer UI. We should also consider whether we should handle channel changes in different directions differently. e.g. allow beta -> release silently, but not release -> beta. And/or, allow updates between production channels (nightly, aurora, beta, release) but not non-production channels--I think it is definitely the case that we don't want to allow somebody to put the user on a test channel or throwaway channel that will never be updated ever again, and/or which has updates disabled or otherwise broken. So, definitely things are more nuanced than I expected them to be when I filed this bug. We should make a plan that works and execute it. The phrasing that we will "eventually" fix this doesn't sound great though. On what timeframe are we thinking to fix it?
(In reply to Brian Smith (:bsmith) from comment #22) > when I filed this bug. Er, when Ian filed the bug. (Sorry Ian.)
> Only Administrators can even get to the UAC prompt. Limited users get UAC prompts too but it asks for the administrator password. I could imagine someone at a library staging an upgrade and leaving it at the UAC prompt which says Mozilla tusted app rwould like to run updater.exe, allow? > One thing that makes this bug worse with the service than without it is > that you don't even have to be an administrator to screw up the > Firefox installation with the service-based update, like you do with > the non-service-based update. One thing that may be possible now that wasn't before (because before we had work item files in a world writable directory), is we could probably restrict the service to only be started from admin accounts if that's the concern here. Recall a user performs an update by starting the service with specific command line parameters. > legit channel change is a *very* rare event, right? I should clarify we no longer have the ability as of recently to do channel changes from upgrades. But we can apply a MAR of a different channel onto another channel currently and that is what is important to releng. > The phrasing that we will "eventually" fix this doesn't sound > great though. On what timeframe are we thinking to fix it? I do not fully understand the issues that the channel check code would introduce so I hope that catlee and QA can chime in here. I have just heard from catlee that they are severe and I trust his judgement.
(In reply to Brian Smith (:bsmith) from comment #22) > (In reply to Brian R. Bondy [:bbondy] from comment #19) > > Only Administrators can even get to the UAC prompt. I believe all users can get a UAC prompt, but non admin uses have to supply the credentials for an admin account to elevate for that process.
I should also mention that the product, channel check, and version downgrade check code is already implemented in the context of bug 709158. I proposed that we have a whitelist of acceptable substitute channel names for each channel but catlee said that this would not work. Another approach is baking in more than one channel name into the MAR but I think that will not work for the same reasons.
(In reply to Brian Smith (:bsmith) from comment #22) > (In reply to Brian R. Bondy [:bbondy] from comment #19) > > The channel switch (as well as product switch and version downgrade) is > > currently a problem in existing versions with and without the service. > > Showing a UAC prompt does NOT protects against these things. We are asking > > a user to entrust that Mozilla code can run with a UAC prompt. We give the > > user no visibility today into what is in the MAR that will be applied. > > We don't tell them they will potentially be changing channels. > > I agree that this is a separate issue that should be addressed separately. > But... > > > How can a user know not to click on the UAC run dialog? > > So it is an existing issue with every version of Firefox that had an > > updater with MAR files. > > Only Administrators can even get to the UAC prompt. One thing that makes > this bug worse with the service than without it is that you don't even have > to be an administrator to screw up the Firefox installation with the > service-based update, like you do with the non-service-based update. It > seems like everybody understands that, and it is just a matter of > determining severity or the net difference in severity given what we > currently are vulnerable to. > > It is definitely the case that an admin could have some non-elevated malware > running in his session that could trigger the non-service update scenerio > with a MAR file from a channel of its choice. That is why it is a good idea > to have some UI explaining the channel change if we need to support that. > Again, that is something for a separate bug, I think. > > Legit channel change is a *very* rare event, right? The service-based > installation doesn't need to support it, AFAICT. So, it seems very > reasonable to me to have the service-based installation refuse to change > channels, but allow UAC-prompting updates to do so, with the understanding > that eventually the prompting updates will have a clearer UI. > > We should also consider whether we should handle channel changes in > different directions differently. e.g. allow beta -> release silently, but > not release -> beta. And/or, allow updates between production channels > (nightly, aurora, beta, release) but not non-production channels--I think it > is definitely the case that we don't want to allow somebody to put the user > on a test channel or throwaway channel that will never be updated ever > again, and/or which has updates disabled or otherwise broken. > > So, definitely things are more nuanced than I expected them to be when I > filed this bug. We should make a plan that works and execute it. The > phrasing that we will "eventually" fix this doesn't sound great though. On > what timeframe are we thinking to fix it? We don't support channel changing (e.g. a channel is a string that the client update code sends to the aus server to determine the mar to download) in that we don't ever change the channel with an update. We do update the files that are provided in a mar without consideration for the channel though which is the actual concern of this bug. Besides the user experience issue of a user without admin rights of a system updating the files by manually adding a mar, related metadata, etc. wouldn't the security concerns of updating to a build be at the very least mitigated for the most part if not actually addressed by performing a version check to prevent updating to an older build?
Correction on my previous message: bug 708690
(In reply to Brian R. Bondy [:bbondy] from comment #24) > > Only Administrators can even get to the UAC prompt. > > Limited users get UAC prompts too but it asks for the administrator > password. I could imagine someone at a library staging an upgrade and > leaving it at the UAC prompt which says Mozilla tusted app rwould like to > run updater.exe, allow? Only admin users got the UAC prompt before the service and this is something I've been planning on going over with you this week.
> > Limited users get UAC prompts too but it asks for the administrator > > password. I could imagine someone at a library staging an upgrade and > > leaving it at the UAC prompt which says Mozilla tusted app rwould like to > > run updater.exe, allow? > Only admin users got the UAC prompt before the service and this > is something I've been planning on going over with you this week. I think you mean that in the normal update flow, limited users don't try to upgrade an installation inside Program Files. A limited user account could initiate an elevated prompt for updater.exe though manually (since they are trying to stage an attack in the first place). For example ShellExecute with verb runas with the command lines pointing to the bad MAR file.
They would have to initiate the ShellExecute with the runas verb as a different user and get that user to enter the credentials for it to update the files unless they already know credentials with the needed rights in which case it is already game over.
Right it would popup a UAC prompt asking for the credentials of an admin. I just meant a limited user could produce such a prompt and then stop using the computer. An administrator could later go to the computer and see that prompt. The administrator would have no idea that this is an attack because it has the Mozilla signed trust UAC dialog on the action. The admin has no way of knowing this is a case Mozilla signed code should not be trusted. I was bringing this up to try to show somewhat parity of an existing problem by the way.
The main issue unless I am mistaken is that QA tests updating with the following channels and expects to be able to update from Beta to Release. beta betatest release releasetest There may be other channels as well. It might be acceptable to only allow updating as follows without using the channel names explicitly to denote what can update to what. Nightly to Nightly Aurora to Aurora Beta to Beta and Release Release to Beta and Release Keep in mind that we would also have an app id check and app version check. Note that the app version check would allow updating to the same or a greater app version since we don't version bump nightly builds. I don't think this is a problem I am concerned about with release and beta builds since we will be signing the mar files, etc. We would use something along the lines of the following for identifying what could update to what Nightly = nightly Aurora = aurora Beta = release Release = release With the other mitigations such as app id and app version check would this be sufficient from the Security, RelEng and QA perspectives? For anyone concerned about having to create new mars to move people from one channel to another (e.g. Aurora to Beta, Beta to Aurora, etc.) we would need to create a mar to do that already since the channel-prefs.js file which is not included in normal mar files would need to be updated with the new channel. So, when the new mar is created that includes channel-prefs.js the appropriate name would also be set in the mar to allow the update to be applied.
I discussed some more about Rob's comment 33 idea with him on IRC. Here is a summary of my understanding of that conversation. I think this is likely the best way to go and I would like Security, RelEng and QA to give the go-ahead. The file /browser/confvars.sh is different per branch. rstrong is suggesting we put a new define in there. On hg.mozilla.org/releases/mozilla-release: MOZ_UPGRADE_MAR_CHANNEL=release-beta On hg.mozilla.org/releases/mozilla-beta: MOZ_UPGRADE_MAR_CHANNEL=release-beta (note: same value as release) On hg.mozilla.org/releases/mozilla-aurora: MOZ_UPGRADE_MAR_CHANNEL=aurora On hg.mozilla.org/mozilla-central: MOZ_UPGRADE_MAR_CHANNEL=nightly All other branches: (No MOZ_UPGRADE_MAR_CHANNEL defined, which would indicate no restriction on channel) The very first update would not have the define so would have no restriction in place. On the second upgrade, updater.exe would have the MOZ_UPGRADE_MAR_CHANNEL inside of it so would then check the MAR file to make sure it has a matching MOZ_UPGRADE_MAR_CHANNEL. This would protect against things like having beta and betatest, and allow for extra new ones to be defined in the future. release-beta would share the same MAR channel name so would be open to this attack upgrading to each other, but since changes on beta are already signed by our Mozilla certificate, they have a level of trust already implicitly that is equal to the release build.
I am sorry about the confusion I caused regarding the UAC prompt. You guys definitely understand that part better than me. I didn't mean to derail the discussion regarding concerns about the case where a limited user causes a UAC prompt to come up to wait for an administrator--that is trivial for them to do in many kinds of scenerios--including just executing any variant of Firefox's installer (any channel, any version). The point I was trying to make, as far as service vs. non-service update, is that the service update (without channel checking) allows limited users to change which channel is installed, in a way that would be impossible them without the service, because the UAC prompt stops them in the non-service case if they don't know an admin password. Probably I don't need to state such things, but I just try to be as explicit as possible as to my understanding of things when I comment.
(In reply to Brian Smith (:bsmith) from comment #35) ... No problem. Can we get your feedback as to whether the suggested implementation in comment #33 is acceptable? Also, bug 708690 adds another new value for product id. I think it would be simpler just to have the value used to fix this bug and the value to fix bug 708690 in the field so there is only one check and it is simpler to change the value since there would only be one value to change / find.
(In reply to Robert Strong [:rstrong] (do not email) from comment #33) > It might be acceptable to only allow updating as follows without using the > channel names explicitly to denote what can update to what. > > Nightly to Nightly > Aurora to Aurora > Beta to Beta and Release > Release to Beta and Release > channel. So, when the new mar is created that includes channel-prefs.js the > appropriate name would also be set in the mar to allow the update to be > applied. I think we have a little bit of a disconnect in how we are thinking about what practical effect that channel checking should have. Here's my view of how channel checking has to work, in order for it to be well-defined from a security perspective, and for it to meet users' expectations. It might be the case that this model isn't the consensus; if not, we should discuss it via a more high-bandwidth mechanism: When I install a release build, I expect that any automatic updates will keep me on the release channel. So, if I install Firefox 12 release, I should never end up on Firefox 12 beta or Firefox 13 beta or Firefox 14 Aurora, or any other channel unless I explicitly approve the channel change, because I (the user) am the one that should be in control of what channel (stability/security level) of Firefox I use. The only thing that I, the user, expect the *automated* (completely silent) updates to do is take me from Firefox 12 to Firefox 12.0.1 to Firefox 13 to Firefox 14 to Firefox 15, etc., all on the same channel. So, it isn't the case that the MAR file should contain info about what installed channels of Firefox it can update; instead the installed version of Firefox must contain the name of the channel that it can automatically apply updates from, because the choice of what channel can be updated with what other channel on a particular system is a decision that is made by the end-user that installed the software on that system, not by the producer of the MAR file. It might be the case that the user has requested or that we otherwise think the user should change channels (e.g. release -> beta). But, whether or not to do so much be a decision that the administrator explicitly makes--i.e. it must be a prompted update that invokes the UAC prompt. Let me phrase it another way, just to make the point clear: The automated updates should follow the principle of least astonishment. The automated updater shouldn't do anything that isn't obviously correct. Upgrading Firefox 12 release to Firefox 13 release is obviously correct*; by leaving automated updates enabled, the user is implicitly agreeing to get that kind of update. But, nobody who downloads Firefox off the Mozilla home page would be expecting to wake up one day and be on the beta channel, so the service-based installation must ensure that kind of surprise never occurs. And, the reason this all is a security issue is because, generally, we only take into consideration security issues on the release channel. For example, we don't chemspill on the beta channel, no matter how serious the issue, AFAICT. * Disregarding addon compatibility issues and whatnot. If an automatic updater is sensible for major version updates at all, this is what it is sensible for it to do.
(In reply to Robert Strong [:rstrong] (do not email) from comment #36) > Also, bug 708690 adds another new value for product id. I think it would be > simpler just to have the value used to fix this bug and the value to fix bug > 708690 in the field so there is only one check and it is simpler to change > the value since there would only be one value to change / find. I don't see any security issues either way, but I am basing this thought on the line of reasoning I put forth in my previous comment.
(In reply to Brian Smith (:bsmith) from comment #37) i understand your points - the question then becomes how then do we deal with the QA/releng requirements which seem to not be satisfiable with this approach ? the point about beta builds not receiving chemspill updates and it not being secure to change channels from release to a beta is a particularly important one i believe. > So, it isn't the case that the MAR file should contain info about what > installed channels of Firefox it can update; instead the installed version > of Firefox must contain the name of the channel that it can automatically > apply updates from, because the choice of what channel can be updated with > what other channel on a particular system is a decision that is made by the > end-user that installed the software on that system, not by the producer of > the MAR file. isn't this what is happening when the channel of the MAR file (essentially what channel the update within belongs to) is checked by the updater ? (the updater is for the installed version of Firefox and checks to make sure its channel (and hence Firefox's channel) matches the one the MAR file states it is for) i don't see why for some of the examples from Releng we couldn't modify processes slightly and avoid needing to do a cross channel update - for example, keeping users on the Beta channel and upgrading them to the final release of that beta - i'm not clear on why they wouldn't be updated to the next beta - for example they have beta 8, 8 goes to release, why do they get 8 release and not 9 beta ? and even if we need to do this, build a MAR of the 8 release and mark it as belonging to the 'beta' channel (since we are keeping them on the beta channel anyways). Partner repacks seems like it could be an issue - but maybe the channel needing to match should only be applied if the installed Firefox being updated is on a Mozilla supported channel ie beta, release, aurora, nightly. i think in general the approach suggested is a good one - if we stop release being able to be changed to beta that addresses the biggest concern i have at the moment.
It sounds like from your Comment 37 that my Comment 34 (based on rs' suggestion) meets your needs. The only change is: > - On hg.mozilla.org/releases/mozilla-release: > - MOZ_UPGRADE_MAR_CHANNEL=release-beta > - > -On hg.mozilla.org/releases/mozilla-beta: > - MOZ_UPGRADE_MAR_CHANNEL=release-beta (note: same value as release) > + On hg.mozilla.org/releases/mozilla-release: > + MOZ_UPGRADE_MAR_CHANNEL=release > + > + On hg.mozilla.org/releases/mozilla-beta: > + MOZ_UPGRADE_MAR_CHANNEL=release Please read and confirm. > It might be the case that the user has requested or that we otherwise think > the user should change channels (e.g. release -> beta). But, whether or not > to do so much be a decision that the administrator explicitly makes--i.e. > it must be a prompted update that invokes the UAC prompt. This sounds like a policy change, not something that needs to be implemented. Also the UAC prompt gives no indication that a channel change would occur in this case and is therefore not helpful to the administrator :( > But, nobody who downloads Firefox off the Mozilla home page would be > expecting to wake up one day and be on the beta channel, so the > service-based installation must ensure that kind of surprise never occurs. If that is the case we should never product MARs that are designed for a particular channel that do the change that we sign. This has nothing to do with the service.
Correction: > + On hg.mozilla.org/releases/mozilla-beta: > + MOZ_UPGRADE_MAR_CHANNEL=beta
(In reply to Brian Smith (:bsmith) from comment #37) > (In reply to Robert Strong [:rstrong] (do not email) from comment #33) > > It might be acceptable to only allow updating as follows without using the > > channel names explicitly to denote what can update to what. > > > > Nightly to Nightly > > Aurora to Aurora > > Beta to Beta and Release > > Release to Beta and Release > > > channel. So, when the new mar is created that includes channel-prefs.js the > > appropriate name would also be set in the mar to allow the update to be > > applied. > > I think we have a little bit of a disconnect in how we are thinking about > what practical effect that channel checking should have. Here's my view of > how channel checking has to work, in order for it to be well-defined from a > security perspective, and for it to meet users' expectations. It might be > the case that this model isn't the consensus; if not, we should discuss it > via a more high-bandwidth mechanism: > > When I install a release build, I expect that any automatic updates will > keep me on the release channel. So, if I install Firefox 12 release, I > should never end up on Firefox 12 beta or Firefox 13 beta or Firefox 14 > Aurora, or any other channel unless I explicitly approve the channel change, > because I (the user) am the one that should be in control of what channel > (stability/security level) of Firefox I use. The only thing that I, the > user, expect the *automated* (completely silent) updates to do is take me > from Firefox 12 to Firefox 12.0.1 to Firefox 13 to Firefox 14 to Firefox 15, > etc., all on the same channel. The automatic update mechanism will keep you on the same channel unless product drivers explicitly ask for a custom mar to be generated, releng generates the custom mar, and releng advertises the mar. With the proposed solution in comment #33 there is the possibility for a user to manually circumvent these checks by placing a mar file and its related metadata on the file system and update for beta and release builds and update a user on a release to a newer or same version beta or update a user on beta to a newer or same version release (the version check is a different bug than this one). The reason for this is so QA can test updates from beta to release. The only way for the user to channel change is to manually install a build for that channel. > > So, it isn't the case that the MAR file should contain info about what > installed channels of Firefox it can update; instead the installed version > of Firefox must contain the name of the channel that it can automatically > apply updates from, because the choice of what channel can be updated with > what other channel on a particular system is a decision that is made by the > end-user that installed the software on that system, not by the producer of > the MAR file. The installed version of Firefox will have an updater that contains this information and the mar file contains similar information about the update itself so we can make the decision as to whether it is the same id where beta and release share the same id per comment #33 so QA can still test updates and the version so we prevent downgrades. > > It might be the case that the user has requested or that we otherwise think > the user should change channels (e.g. release -> beta). But, whether or not > to do so much be a decision that the administrator explicitly makes--i.e. it > must be a prompted update that invokes the UAC prompt. Then QA won't be able to test the service when testing updating from beta to release. I also don't believe prompting with the UAC is in anyway a solution to this since people often just click the equivelent of ok when the UAC without user name and password is displayed just as they clicked ok with the add-on installation UI where we warned them about signing xpi's or for that matter other security ui. > Let me phrase it another way, just to make the point clear: The automated > updates should follow the principle of least astonishment. The automated > updater shouldn't do anything that isn't obviously correct. Upgrading > Firefox 12 release to Firefox 13 release is obviously correct*; by leaving > automated updates enabled, the user is implicitly agreeing to get that kind > of update. But, nobody who downloads Firefox off the Mozilla home page would > be expecting to wake up one day and be on the beta channel, so the > service-based installation must ensure that kind of surprise never occurs. As I see it, that is a decision for product drivers and can be enforced at that level vs. code except if we are going to allow beta and release to update to a newer beta or release build. In other words, even if we disallow updates between beta and release we are not going to remove the capability to force a user to a different channel with a custom mar, the decision to do so would be driven by product drivers, and it would be just as secure due to the mar signing. > And, the reason this all is a security issue is because, generally, we only > take into consideration security issues on the release channel. For example, > we don't chemspill on the beta channel, no matter how serious the issue, > AFAICT. It will only allow updating to a newer version and chemspills do land in newer builds. > * Disregarding addon compatibility issues and whatnot. If an automatic > updater is sensible for major version updates at all, this is what it is > sensible for it to do. There are no add-on compatibility issues. So, how about if we store the equivelent of channel (e.g. nightly, aurora, beta, and release), app id, and version in a separate file in the installation directory. Then QA would be able to modify both this file and the channel-pref.js file to continue to be able to test updates?
Whether or not we include channel-prefs.js inside a MAR file has nothing to do with this task in my opinion. This task is to ensure that a particular Firefox channel can only consume MARs that are designed for that channel in particular. I personally think the discussions on channel-prefs.js would be better in another bug where you suggest to enforce a policy change. Perhaps there could be a restriction on even adding a filename by the name of channel-prefs.js inside MARs in that task if the policy was in the end enforced.
I didn't mention channel-prefs.js inside a MAR in comment #42 and I am fine with no longer discussing it in that context except as it relates to releng's concern with repackaging and with the QA process for testing updates. Are you ok with discussing channel-prefs.js as it relates to those issues as I did in comment #42 so we can come up with a solution that QA can continue to test and releng can repackage? ;)
:) Yes I'm in agreement with that, my Comment 43 was directed towards bsmith relating to this comment and others in Comment 37: > because the choice of what channel can be updated with what other channel > on a particular system is a decision that is made by the end-user that > installed the software on that system, not by the producer of the MAR file.
imelven and bsmith, would it be acceptable if we store the equivalent of channel (e.g. nightly, aurora, beta, and release), app id, and version in a separate file in the installation directory? Then QA would be able to modify both this file and the channel-pref.js file to continue to be able to test updates and repackaging could change these values if necessary (I don't think it will be). Thanks!
In case it isn't clear, anyone that was able to modify the file in the installation directory that contains the equivalent of channel (e.g. nightly, aurora, beta, and release), app id, and version would be able to replace the application files and thereby replace the existing installation with whatever build they wanted to.
I just want to clarify a few small points here, from a RelEng perspective: (In reply to Ian Melven :imelven from comment #39) We can't exclude partner repacks here - these types of builds range from builds used only internally, to builds distributed publicly by Yahoo and other partners - and even the EU Ballot builds (the ones Europeans get when they choose Firefox from the Browser Choice page on a new computer) are partner builds. If we decide this is the type of protection we need, these builds need it too IMO. (In reply to Robert Strong [:rstrong] (do not email) from comment #42) > With the proposed solution in comment #33 there is the possibility for a > user to manually circumvent these checks by placing a mar file and its > related metadata on the file system and update for beta and release builds > and update a user on a release to a newer or same version beta or update a > user on beta to a newer or same version release (the version check is a > different bug than this one). The reason for this is so QA can test updates > from beta to release. What do you mean by "updates from beta to release" here? AFAIK we've never converted beta users to release. If you're talking about things like, eg, 7.0b6 -> 7.0 updates - those are done with the 7.0 MARs built with the final release builds, and don't change the user's update channel. > The only way for the user to channel change is to manually install a build > for that channel. Not quite true - you can manually edit defaults/pref/channel-prefs.js in your app directory to change the channel. This is how we put builds on the betatest and releasetest channels, for example. (Note that while this file references the app.update.channel pref, it's not overridable in the profile). > So, how about if we store the equivelent of channel (e.g. nightly, aurora, > beta, and release), app id, and version in a separate file in the > installation directory. Then QA would be able to modify both this file and > the channel-pref.js file to continue to be able to test updates? It should be noted that we need at least two equivalent channels - betatest and releasetest. Sometimes we use additional ones when doing one-off tests for misc. things, too. Eg, we used to use "majortest" for testing major updates.
> Sometimes we use additional ones when doing one-off tests for misc. things, > too. Eg, we used to use "majortest" for testing major updates. As long as they are from the same branch repository, you can have any amount of actual channels in the proposed solution. So upgrading from the hg.mozilla.org/releases/mozilla-release before migration to the hg.mozilla.org/releases/mozilla-release after migration would be fine and would have the same file in the installation directory that rstrong is suggesting.
Even a different repository URL would be fine as long as you named the define that produces/checks the file in the installation directory as "release".
I just read through about 35 comments but... ...the way I'm reading this, it is very unusual for a typical Firefox user to change channels. If this is the case, can we simply pop up the UAC dialog if for some reason the service detects that we're changing channels? Will any of our users actually see the UAC dialog if we take this approach? Will this allow our existing process to continue as is?
(In reply to Ben Hearsum [:bhearsum] from comment #48) > I just want to clarify a few small points here, from a RelEng perspective: > > (In reply to Ian Melven :imelven from comment #39) > > We can't exclude partner repacks here - these types of builds range from > builds used only internally, to builds distributed publicly by Yahoo and > other partners - and even the EU Ballot builds (the ones Europeans get when > they choose Firefox from the Browser Choice page on a new computer) are > partner builds. If we decide this is the type of protection we need, these > builds need it too IMO. > > (In reply to Robert Strong [:rstrong] (do not email) from comment #42) > > > With the proposed solution in comment #33 there is the possibility for a > > user to manually circumvent these checks by placing a mar file and its > > related metadata on the file system and update for beta and release builds > > and update a user on a release to a newer or same version beta or update a > > user on beta to a newer or same version release (the version check is a > > different bug than this one). The reason for this is so QA can test updates > > from beta to release. > > What do you mean by "updates from beta to release" here? AFAIK we've never > converted beta users to release. If you're talking about things like, eg, > 7.0b6 -> 7.0 updates - those are done with the 7.0 MARs built with the final > release builds, and don't change the user's update channel. Understood. I am referring to having the ability to test updating to release build from a beta build by changing the channel and not converting users. The issue is that allowing this would allow users to manually change channels as noted elsewhere. > > The only way for the user to channel change is to manually install a build > > for that channel. > > Not quite true - you can manually edit defaults/pref/channel-prefs.js in > your app directory to change the channel. This is how we put builds on the > betatest and releasetest channels, for example. (Note that while this file > references the app.update.channel pref, it's not overridable in the profile). I did not provide all of the possible approaches where a user can change channels. As far as this bug is concerned it is true. > > So, how about if we store the equivelent of channel (e.g. nightly, aurora, > > beta, and release), app id, and version in a separate file in the > > installation directory. Then QA would be able to modify both this file and > > the channel-pref.js file to continue to be able to test updates? > > It should be noted that we need at least two equivalent channels - betatest > and releasetest. Sometimes we use additional ones when doing one-off tests > for misc. things, too. Eg, we used to use "majortest" for testing major > updates. As bbondy noted this is an equivelent to the channel except it is to be used in relation to this bug. So, this would allow the update channel (e.g. release, releasetest, <insert any update channel name>) to download the mar and the new value would be used to verify the mar is for the update.
(In reply to Lawrence Mandel [:lmandel] from comment #51) > I just read through about 35 comments but... > > ...the way I'm reading this, it is very unusual for a typical Firefox user > to change channels. If this is the case, can we simply pop up the UAC dialog > if for some reason the service detects that we're changing channels? Will > any of our users actually see the UAC dialog if we take this approach? Will > this allow our existing process to continue as is? The main concern we are trying to resolve is in regards to allowing QA to be able to continue to test updates when we release and repacks (which I am fairly certain is not an issue with the approach in comment #47). In regards to Firefox users they would need to modify files in the installation directory and if they have that capability they could just as easily install a different version so it is a non-issue.
(In reply to Robert Strong [:rstrong] (do not email) from comment #47) > In case it isn't clear, anyone that was able to modify the file in the > installation directory that contains the equivalent of channel (e.g. > nightly, aurora, beta, and release), app id, and version would be able to > replace the application files and thereby replace the existing installation > with whatever build they wanted to. i'm fine with your proposed solution in comment #46 - i agree with your assessment in comment #47 as well. i discussed briefly with bbondy just now on IRC, also.
Blocks: 720688
catlee said: > - What channel names will be used here for partner repacks? > - How will we be able to give a beta user a release mar? I think you'd use the same MAR channel name for partner repacks. Relating to beta / release mar question, QA can adjust the mar-channel.ini file themselves. If you need to distribute the same MAR to a beta user though you can strip the signature, update the MAR channel info / version upgrade info, and resign the mar. You can do all of this with the signmar program.
catlee wrote: > Actually I was hoping for a summarized proposal somewhere :) > There are lots of proposals on that bug already, and it's not > clear if 46 and 47 are "complete", or rely on information above. Each installation of Firefox will now have a new file named mar-channel.ini in the root installation directory. Example: > C:\Program Files (x86)\Nightly\mar-channel.ini Here is an example contents of mar-channel.ini: > [Channel] > MAR_CHANNEL=mozilla-central It is up to RelEng exactly how this value is used, but they should stay within bounds of what security advises. In particular this value should contain information about the product and the channel that this MAR can be applied to. I believe that it's security's recommendation that this value be unique for each branch. The value can be controlled by modifying this file: > /browser/confvars.sh This file is unique for every branch and does not get overwritten on migration days. The contents of this file that should be changed to affect this ini file is the following line: > MOZ_MAR_CHANNEL=mozilla-central Independent value: The MAR_CHANNEL value of the mar-channel.ini file is independent of the actual channel name in all ways. Its purpose is to restrict the installation of a MAR file cross product and cross channel. If no mar-channel.ini exists, or if the value is blank, then no restriction will be in place. Note: there is also a version check to make sure you are not downgrading, but it is not related to this value. MAR creation: When a MAR is created using the mar program, it will automatically have an embedded MAR channel inside the MAR matching whatever was set in confvars.sh. The MAR that is created will also have an embedded version number which is equal to the built version. A specific mar channel value can be specified via command line to override the default. A specific version number can also be set via command line to override the default. Modifying existing MAR files: The mar program (and every time I say mar I also mean signmar) can be used to update existing MARs also. You can use the -r command line on a signed MAR file to strip its signature. You can use the -i command line on an un-signed MAR file to refresh its product information block, you can also override the values as above when using the -i command line. You can then resign it after changing the product information block. Example new mar command line usage: - List what's in a MAR, tell you if the MAR is signed, and what's in its product information block: mar -t c:\Users\bbondy\Desktop\test.mar - Refresh a MAR file with specific values: mar -V 99.0.0 -H "new channel name" -i c:\Users\bbondy\Desktop\test.mar - Refresh a MAR file with the info that the mar program was built with: mar -i c:\Users\bbondy\Desktop\test.mar - Strip signatures from an existing MAR: mar -r c:\Users\bbondy\Desktop\test.mar c:\Users\bbondy\Desktop\test_raw.mar - Create a MAR with the default MAR channel and version: mar -c c:\Users\bbondy\Desktop\test.mar c:\martest\1.txt c:\martest\2.txt c:\martest\0.txt - Create a MAR with a specific MAR channel and version: mar -V 99.0.0 -H "new channel name" -c c:\Users\bbondy\Desktop\test.mar c:\martest\1.txt c:\martest\2.txt c:\martest\0.txt QA: QA is free to modify the mar-channel.ini in their installation directory at will for testing if they want to apply an existing MAR from a different channel. Security concerns: I do not believe it to be a security concern because this file resides in the installation directory which is a protected directory inside program files. I believe that RelEng will be using a distinct value for MAR_CHANNEL inside mar-channel.ini. If that assumption is not correct, please speak up. Implemented: This is all already implemented in the attached patches. Modulo review comments, the development is complete on these tasks.
(In reply to Brian R. Bondy [:bbondy] from comment #55) > catlee said: > > > - What channel names will be used here for partner repacks? > > - How will we be able to give a beta user a release mar? > > I think you'd use the same MAR channel name for partner repacks. It would contain the same info. > Relating to beta / release mar question, QA can adjust the mar-channel.ini > file themselves. If you need to distribute the same MAR to a beta user > though you can strip the signature, update the MAR channel info / version > upgrade info, and resign the mar. You can do all of this with the signmar > program. Since this will contain more than just the channel name just go with update.ini. For example, we could move the aus channel name into this file and remove channel-prefs.js in the future.
I'm afraid update.ini might be confusing with updater.ini. I'll wait until the review of the patches and then change the name since I'll need to update the patches at that point anyway and since that change is just a filename change.
I'm fine with a different name that is more generalized than mar-channel.ini so the name reflects that it contains update settings.
I'll go with update-settings.ini if there are no objections on the next set of patches once you put in your review comments. Thanks!
(In reply to Brian R. Bondy [:bbondy] from comment #56) > > Security concerns: > I do not believe it to be a security concern because this file resides in > the installation directory which is a protected directory inside program > files. > I believe that RelEng will be using a distinct value for MAR_CHANNEL inside > mar-channel.ini. > If that assumption is not correct, please speak up. that seems ok to me. If it was embedded in the binary (ie firefox.exe), even if this was signed, if an attacker could write to the install directory, they could replace firefox.exe with a signed, other channel version and then have bogus updates applied (at the very least - they could probably do much worse things if they can write to the install dir as has been discussed repeatedly throughout this work).
Blocks: 721758
Blocks: 725180
Depends on: 728301
The work for this task landed here: Bug 708690 - Signed MAR files do not protect against applying an update for the wrong product
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Marking as WFM as per bsmith's recommendation.
Depends on: 708690
Resolution: FIXED → WORKSFORME
Group: core-security → core-security-release
Blocks: 750462
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.