Closed Bug 708688 Opened 13 years ago Closed 12 years ago

Signed MAR files are not version checked and can be used to downgrade an install

Categories

(Toolkit :: Application Update, defect)

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

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla13
Tracking Flags:
Tracking Status
firefox10 --- unaffected
firefox12 --- affected
firefox13 --- verified
firefox-esr10 --- unaffected

People

(Reporter: imelven, Assigned: bbondy)

References

(Blocks 1 open bug)

Details

(Keywords: verified-beta, Whiteboard: [sg:dupe 708690][advisory-tracking+][qa+:simonab]) 

Although MAR files will be signed as part of bug 699700, the version 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 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.
See Also: → 699700
See Also: → 708690, 708697, 708688
Are you suggesting that we should fail to apply a MAR if "it's version" is older than the current version? That would require baking in an understanding of version numbers, and although I don't think we've ever actually done it, we avoided this kind of version check in the past (even as part of AUS) because of the possibility that if we "messed up" and accidentally shipped something like "version *" that we'd never be able to ship another update, because * is the maximum possible version.

I don't think this 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.
With the service, without this protection, an unprivileged user (e.g. the guest user on a public computer) can downgrade the version of Firefox installed on the computer. Currently, without the service, this is not possible. Obviously, a user without admin rights should not be able to control which versions of what software is installed (in C:\Program Files, for all users) on the system. So, this is a security regression in the service-based installer.
It is a regression as far as this possibility wasn't present before the service though the same can be said for most features that are added. In this instance the downgrading of the app to a previous version of the app is IMO not that detrimental since the app will just update itself to the latest version within 24 hours after downgrading. With the risk mitigation capabilities stated in comment #1 I still think that not trying to prevent downgrades since the app will just update itself again within 24 hours after the last update check is acceptable.
Comment 3 is private: false
(In reply to Robert Strong [:rstrong] (do not email) from comment #5)
> With the risk mitigation capabilities stated in comment #1 I still think
> that not trying to prevent downgrades since the app will just update
> itself again within 24 hours after the last update check is acceptable.

Allowing downgrades, even for a short period of time, is not acceptable on a multi-user system that makes distinctions between privileged and unprivileged users. If nothing else, allowing downgrades like this (even for short periods of time) allows one user to roll back security fixes for other users on the same computer. This is not acceptable for any short period of time.

This isn't a problem now (pre-service) because the user that does an upgrade/downgrade must be an administrator, and administrators can do whatever they want.

I do think that bsmedberg's point about the potential downfalls of version checking is valid, and we have to be careful to do this in a way that doesn't cause us to shoot ourselves in the foot.

(FWIW, I brought this up during the initial security review and during the even-earlier security review of the stub installer, and I thought we already had agreement on this issue. It is OK if that agreement is lost. But, this is by no means a new issue, unlike some of the other issues that were brought up this week.)
I don't recall this being brought up in the initial security review and the notes don't reflect this being brought up (I do remember regarding stub installer but that also included the possibility of a MITM attack which makes that case easier and more likely than a compromised local non-privileged user account being compromised).
https://wiki.mozilla.org/Security/Reviews/Firefox10/SilentUpdateOSDialogs

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.

Agreed that the original and ever changing design due to additional security requirements has brought up new issues... that's what happens when new requirements are brought up!
(In reply to Robert Strong [:rstrong] (do not email) from comment #7)
> I don't recall this being brought up in the initial security review and the
> notes don't reflect this being brought up (I do remember regarding stub
> installer but that also included the possibility of a MITM attack which
> makes that case easier and more likely than a compromised local
> non-privileged user account being compromised).

The local (non-privileged) user *is* the MITM here. Think about a public computer at a public library. The attacker is an authorized user of the computer.

In email, Robert pointed out that our MAR files are like Microsoft Installer patches (MSP files). Robert said MSPs may be able to be installed by limited users. I do know that *some* MSPs can be installed this way, but I believe that those are MSPs that only affect per-user configuration, and not the global configuration of the system. I would like a pointer to more information about limited users being able to install MSPs that actually contain patches to the admin-controlled parts of the system (i.e. MSPs that affect files in C:\Program Files\ and registry settings in HKLM). It is likely that we could use a similar design.

And, to be clear, I think this is a must-fix bug, but I don't think it needs to block the service landing in Nightly. We should have a fix before or in Aurora.
status-firefox11: --- → affected
tracking-firefox11: --- → ?
I think not allow silent updates from accounts that are not administrators (most user accounts are administrators) would make the concerns a non issue if this is really only about the service.
(In reply to Brian Smith (:bsmith) from comment #8)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #7)
> > I don't recall this being brought up in the initial security review and the
> > notes don't reflect this being brought up (I do remember regarding stub
> > installer but that also included the possibility of a MITM attack which
> > makes that case easier and more likely than a compromised local
> > non-privileged user account being compromised).
> 
> The local (non-privileged) user *is* the MITM here. Think about a public
> computer at a public library. The attacker is an authorized user of the
> computer.
Besides this not being brought up during the review... the difference being that one is available remotely and the other is available locally. Similarly, locally booting to a Windows installation device allows a person to do all of the above and more.

> In email, Robert pointed out that our MAR files are like Microsoft Installer
> patches (MSP files). Robert said MSPs may be able to be installed by limited
> users. I do know that *some* MSPs can be installed this way, but I believe
> that those are MSPs that only affect per-user configuration, and not the
> global configuration of the system. I would like a pointer to more
> information about limited users being able to install MSPs that actually
> contain patches to the admin-controlled parts of the system (i.e. MSPs that
> affect files in C:\Program Files\ and registry settings in HKLM). It is
> likely that we could use a similar design.
I don't have pointers handy but signed MSP's can update existing installs.

> And, to be clear, I think this is a must-fix bug, but I don't think it needs
> to block the service landing in Nightly. We should have a fix before or in
> Aurora.
I'd like to get the input from the rest of the security team on whether this is a must-fix bug.

(In reply to Brian R. Bondy [:bbondy] from comment #9)
> I think not allow silent updates from accounts that are not administrators
> (most user accounts are administrators) would make the concerns a non issue
> if this is really only about the service.
Agreed. If this is going to be an issue I am sorely tempted to just not allow updates for non-admin users and instead just notify.
> I think not allow silent updates from accounts that are not administrators 
> (most user accounts are administrators) would make the concerns a non issue 
> if this is really only about the service.

I should also mention that even today, with the same attack, just because an administrator gives their approval for updater.exe to run, to me this doesn't imply that they are giving their approval for the installation on the machine to be downgraded.
Since most if not all of the secteam is on this bug, do you think this is a must fix bug or a Firefox 11 blocker? If so, we will remove the ability to update when you are not an administrator of the system.
We are going to go with comment #12 unless the secteam decides that it is ok to not mitigate this bug along with bug 708697 and bug 708690 for Firefox 11.
status-firefox11: affected → ---
tracking-firefox11: ? → ---
Is it reasonable to make a security decision based on the fact that the user is in the Administrators group, when they haven't enabled the administrator role (i.e. when they haven't elevated)? Is there any precedent for this? Windows' security system with UAC is designed to allow users in the Administrators group to be protected/limited to the same extent as limited users, as long as they are not elevated. As far as I understand comment 9, implementing the suggestion in comment 9 would be counter to this. But, perhaps I do not understand comment 9 and Brian wasn't suggesting that we check that the user is in the Administrators group, but that we check something else instead. But, I don't know what we would check.
Yes I meant the user belongs to the administrators group.

You mentioned a thread with a guest user on a public computer being able to downgrade a Firefox installation.
A guest user on a public computer would not belong to this group.

And since they would not belong to this group they could not write an update into an administrative user's app data directory that could apply updates through the service.

I would also appreciate feedback on Comment 11.
You mentioned a *threat...
(In reply to Brian Smith (:bsmith) from comment #14)
> Is it reasonable to make a security decision based on the fact that the user
> is in the Administrators group, when they haven't enabled the administrator
> role (i.e. when they haven't elevated)? Is there any precedent for this?
> Windows' security system with UAC is designed to allow users in the
> Administrators group to be protected/limited to the same extent as limited
> users, as long as they are not elevated. As far as I understand comment 9,
> implementing the suggestion in comment 9 would be counter to this. But,
> perhaps I do not understand comment 9 and Brian wasn't suggesting that we
> check that the user is in the Administrators group, but that we check
> something else instead. But, I don't know what we would check.
This seems to argue that we shouldn't get rid of the UAC dialog at all since we would be making a "security decision". Is that what you are suggesting?
(In reply to Robert Strong [:rstrong] (do not email) from comment #17)
> This seems to argue that we shouldn't get rid of the UAC dialog at all since
> we would be making a "security decision". Is that what you are suggesting?

No. I always thought that the rationale for doing this was that the administrator decided (while elevated) to install the product knowing that it would install such a service that acts on his behalf. (In reality, most users won't know about it or understand it, but that is the responsibility that an administrator has.) The key there is that the service is installed when the administrator is elevated.

When the administrator is not elevated, the expectation is that the account functions the same as a limited user's account; that is, there should be nothing that can happen in his account that could not happen in a limited user's account. Checking for Administrators group membership, instead of checking for elevation, goes against this part of the UAC design.

In Linux terms: In Linux, we would never make a trust decision based on the fact that the current user is merely *listed* in the sudoers file. Instead, for trusted actions, we require that the user has actually executed the sudo command and elevated himself to root-level privileges. When running a command without sudoing, an administrator expects and relies on the fact that his membership in sudoers doesn't affect the security of the command.
(In reply to Brian Smith (:bsmith) from comment #18)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #17)
> > This seems to argue that we shouldn't get rid of the UAC dialog at all since
> > we would be making a "security decision". Is that what you are suggesting?
> 
> No. I always thought that the rationale for doing this was that the
> administrator decided (while elevated) to install the product knowing that
> it would install such a service that acts on his behalf. (In reality, most
> users won't know about it or understand it, but that is the responsibility
> that an administrator has.) The key there is that the service is installed
> when the administrator is elevated.
> 
> When the administrator is not elevated, the expectation is that the account
> functions the same as a limited user's account; that is, there should be
> nothing that can happen in his account that could not happen in a limited
> user's account. Checking for Administrators group membership, instead of
> checking for elevation, goes against this part of the UAC design.
The admin account (since we will only be using the service for admin accounts per comment #13 unless we hear otherwise) will have nothing happen in his account that could not happen in a limited user account. The system account that was installed by the admin account (elevated if the system is configured to use UAC and the admin accepted elevation) will perform the update for the admin account that requested the update.

> 
> In Linux terms: In Linux, we would never make a trust decision based on the
> fact that the current user is merely *listed* in the sudoers file. Instead,
> for trusted actions, we require that the user has actually executed the sudo
> command and elevated himself to root-level privileges. When running a
> command without sudoing, an administrator expects and relies on the fact
> that his membership in sudoers doesn't affect the security of the command.
This again seems that you want the user to accept / initiate / <insert favorite term her> (in the case of Windows this would be the UAC prompt) the action before performing the action since you state "we require that the user has actually executed the sudo command and elevated himself to root-level privileges". It still seems that you want to keep the UAC prompt.
(In reply to Robert Strong [:rstrong] (do not email) from comment #19)
> The admin account (since we will only be using the service for admin
> accounts per comment #13 unless we hear otherwise) will have nothing happen
> in his account that could not happen in a limited user account.

From what I understand, that is not true. An account in the administrator group  would be able to tell the service (in a way that the service would trust) which MAR file to use for the upgrade/downgrade, but an account that is not in the administrators group would not be able to tell the service (in a way that the service would trust) which upgrade/downgrade to install.

> The system
> account that was installed by the admin account (elevated if the system is
> configured to use UAC and the admin accepted elevation) will perform the
> update for the admin account that requested the update.

By the way, how would the service know whether or not the user that wrote the MAR file was in the Administrators group? AFAICT, that was the same kind of problem that we couldn't find a solution for w.r.t. the session ID spoofing.

> It still seems that you want to keep the UAC prompt.

Please trust me when I say I do not want to keep the UAC prompt. I feel like I have failed to describe the issue clearly here. Tonight, I will try to find a new way to describe the issue of being elevated vs member of the group that is allowed to elevate.
(In reply to Brian Smith (:bsmith) from comment #20)
> By the way, how would the service know whether or not the user that wrote
> the MAR file was in the Administrators group? AFAICT, that was the same kind
> of problem that we couldn't find a solution for w.r.t. the session ID
> spoofing.

s/MAR file/workitem file/.
(In reply to Brian Smith (:bsmith) from comment #20)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #19)
> > The admin account (since we will only be using the service for admin
> > accounts per comment #13 unless we hear otherwise) will have nothing happen
> > in his account that could not happen in a limited user account.
> 
> From what I understand, that is not true. An account in the administrator
> group  would be able to tell the service (in a way that the service would
> trust) which MAR file to use for the upgrade/downgrade, but an account that
> is not in the administrators group would not be able to tell the service (in
> a way that the service would trust) which upgrade/downgrade to install.
The system account will be running this and not the admin's (unelevated) account. I guess you are saying that the work item will be written out by the admin account and it will start the service while the work will be done by the system account?

> 
> > The system
> > account that was installed by the admin account (elevated if the system is
> > configured to use UAC and the admin accepted elevation) will perform the
> > update for the admin account that requested the update.
> 
> By the way, how would the service know whether or not the user that wrote
> the MAR file was in the Administrators group? AFAICT, that was the same kind
> of problem that we couldn't find a solution for w.r.t. the session ID
> spoofing.
We have code to check if the user is and admin.

> 
> > It still seems that you want to keep the UAC prompt.
> 
> Please trust me when I say I do not want to keep the UAC prompt. I feel like
> I have failed to describe the issue clearly here. Tonight, I will try to
> find a new way to describe the issue of being elevated vs member of the
> group that is allowed to elevate.
I don't think you "want" to keep the UAC prompt. I do think your examples are representative of keeping the UAC prompt.

can you explain how your example of "we require that the user has actually executed the sudo command and elevated himself to root-level privileges" isn't the same as still displaying the UAC? I am obviously missing your point.
> By the way, how would the service know whether or not the user 
> that wrote the MAR file was in the Administrators group? 
> AFAICT, that was the same kind of problem that we couldn't 
> find a solution for w.r.t. the session ID spoofing.

I can think of at least 3 ways:
1) We set administrators only write access to the work item directory.

or

2) We watch directories inside app user data of administrator accounts instead of programdata for work items.  Limited user accounts cannot write to those directories by default.  

or

3) We only accept MARs that are located in the profile dirs of users that are administrators.

or probably others.
RelEng had no major concerns about this task by the way, only the channel one.
I suggest we include both the version number and buildid here. buildids are basically timestamps, so it would be easy to check that one version is newer/older than another.
Assignee: nobody → netzen
If we are doing this, the version we can use can be found in MOZ_APP_VERSION
When updater.exe wants to check "what is the currently-installed version," where would it get this information? It is important that it gets it from a place that is updated by every upgrade(/downgrade). (Also, taking into account system restore.)
Whiteboard: [sg:moderate]
We have changed the build ID format several times in the past and might change it again in the future so I don't want build ID used so we don't end up locking us in to changing it again in the future. Checking app version should be enough and the only people that could be downgraded would be on a branch where we update to the same version number and not release builds so I am not concerned about that.

We have a very short time until we have to branch and I still want an answer from the secteam regarding comment #12. We have had multiple new requirements from the secteam on this project (even after when we told the secteam we needed land) many of which are for mitigating issues that were brought up several months ago and if not updating except in the admin case as we do today resolves the other issues brought up recently and this then I want to know and if it does not I want to know why it doesn't.

I know there are ways we can add this functionality and if you don't understand why I am still concerned and want to know if that is acceptable these changes add even more complexity in an area where we haven't done this previously and are very late breaking whereas we have done checks for admin in the past. At this time we should not be adding code for additional late breaking security issues and if only updating with the service (e.g. bypassing the UAC dialog) only when the user is admin is not acceptable I think we should hold off on this landing until the Firefox 12 release train.
How would requiring a member of the Administrators group to write the work item file work on Windows XP? Wouldn't that effectively require the administrator to log in (with admin privileges), open firefox (with admin privileges), and wait for it to download the MAR file (with admin privileges), then write the workitem file (with admin privileges). Isn't that exactly what happens now on Windows XP?

Even on Vista and 7, making that change effectively means that an administrator has to log in, run firefox, etc., like above on each box. Think about a corporate network--the administrator *never* almost never logs into any users' boxes to run Firefox.

So, AFAICT, making that change would severely cripple the feature, regardless of whether it is secure or not. It isn't my call as to whether such crippling is acceptable, but it totally changes the feature.
(In reply to Robert Strong [:rstrong] (do not email) from comment #22)
> The system account will be running this and not the admin's (unelevated)
> account. I guess you are saying that the work item will be written out by
> the admin account and it will start the service while the work will be done
> by the system account?

As far as I understand the proposal, the proposal is "Trust workitem files that are written by users who are members of the administrators group, regardless of whether they were elevated when they did it; not not trust workitem files written by users who are not in the administrators group." My point is that "Trust <any action> by users who are members of the administrators group, regardless of whether they are elevated..." is not, AFAICT, in line with Vista/2008/7's security design. I do not know what, if anything, breaks, when you do not follow that design.

> I don't think you "want" to keep the UAC prompt. I do think your examples
> are representative of keeping the UAC prompt.
> 
> can you explain how your example of "we require that the user has actually
> executed the sudo command and elevated himself to root-level privileges"
> isn't the same as still displaying the UAC? I am obviously missing your
> point.

If we were able to safely do the update, regardless of whether the user who wrote the workitem file was a member of the Administrators group, then we would be working with (instead of against) Windows' security architecture.
(In reply to Brian Smith (:bsmith) from comment #29)
> How would requiring a member of the Administrators group to write the work
> item file work on Windows XP? Wouldn't that effectively require the
> administrator to log in (with admin privileges), open firefox (with admin
> privileges), and wait for it to download the MAR file (with admin
> privileges), then write the workitem file (with admin privileges). Isn't
> that exactly what happens now on Windows XP?
We wouldn't use the service on XP since we would only be using the service on systems with UAC to remove the UAC.

> 
> Even on Vista and 7, making that change effectively means that an
> administrator has to log in, run firefox, etc., like above on each box.
> Think about a corporate network--the administrator *never* almost never logs
> into any users' boxes to run Firefox.
As is the case today.

> 
> So, AFAICT, making that change would severely cripple the feature,
> regardless of whether it is secure or not. It isn't my call as to whether
> such crippling is acceptable, but it totally changes the feature.
It will remove the UAC dialog which is the primary reason for this feature.
(In reply to Robert Strong [:rstrong] (do not email) from comment #31)
>...
> > So, AFAICT, making that change would severely cripple the feature,
> > regardless of whether it is secure or not. It isn't my call as to whether
> > such crippling is acceptable, but it totally changes the feature.
> It will remove the UAC dialog which is the primary reason for this feature.
Should have said: It is actually the only reason for this feature and everything else was just gravy that I wanted to get added. We will try to add these gravy items in the future.
OK. Then, somebody just needs to explain why it is secure to give an administrator extra privileges even when he isn't elevated. AFAICT, that goes against end=user expectations that have been set by Microsoft, at least.
If that is the case then we cannot get rid of the UAC since that would be doing the same thing as giving the administrator (hopefully non administrator accounts in the future) extra privileges when their account is not elevated.
(In reply to Robert Strong [:rstrong] (do not email) from comment #34)
> If that is the case then we cannot get rid of the UAC since that would be
> doing the same thing as giving the administrator (hopefully non
> administrator accounts in the future) extra privileges when their account is
> not elevated.

IF we do not do the "a member of administrators wrote the workitem file" test, then we would be treating unelevated administrators equally to non-administrators. So, that would be a non-issue.

Are other issues:
* Do administrators expect that, after the they install Firefox 11, that it will upgrade itself without their intervention/approval, possibly influenced by  untrusted users?
* Do administrators expect that, after they install Firefox 11, that it will downgrade itself, change channels, or corrupt itself*, without their intervention/approval, possibly influenced by untrusted users?

I think that we've been thinking since the beginning that the answer to the first answer is "yes". But, we've never considered the second question before, and I think that the answer would be "no."

* I do not know to what extent we test the updater being given a MAR file for some previous release to do a downgrade. I wouldn't be surprised if it just corrupts the install, but I also wouldn't be surprised if rstrong has already done some magic to support it.
(In reply to Brian Smith (:bsmith) from comment #35)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #34)
> > If that is the case then we cannot get rid of the UAC since that would be
> > doing the same thing as giving the administrator (hopefully non
> > administrator accounts in the future) extra privileges when their account is
> > not elevated.
> 
> IF we do not do the "a member of administrators wrote the workitem file"
> test, then we would be treating unelevated administrators equally to
> non-administrators. So, that would be a non-issue.
We would just be giving both administrators and non administrators the capability you pointed out in comment #33 and thereby we would still have the issue you stated for administrators. On top of that we would also be doing this for non administrators that don't have the expectation to update. Your reply does not negate that fact.

> Are other issues:
> * Do administrators expect that, after the they install Firefox 11, that it
> will upgrade itself without their intervention/approval, possibly influenced
> by  untrusted users?
Administrators only expect that someone that can update today will be able to update tomorrow and by only allowing administrators this will still be the case. If it wasn't we would message the change so they could take appropriate steps.

> * Do administrators expect that, after they install Firefox 11, that it will
> downgrade itself, change channels, or corrupt itself*, without their
> intervention/approval, possibly influenced by untrusted users?
This won't be anymore of a problem than it is today if we go with only allowing administrators to update.

> 
> I think that we've been thinking since the beginning that the answer to the
> first answer is "yes". But, we've never considered the second question
> before, and I think that the answer would be "no."
> 
> * I do not know to what extent we test the updater being given a MAR file
> for some previous release to do a downgrade. I wouldn't be surprised if it
> just corrupts the install, but I also wouldn't be surprised if rstrong has
> already done some magic to support it.
It doesn't corrupt the install. If it is a partial it fails... if it is a complete it succeeds.
(In reply to Brian R. Bondy [:bbondy] from comment #23)
> I can think of at least 3 ways:
> 1) We set administrators only write access to the work item directory.

This would require the administrator to be elevated, based on an experiment I just did.

> 3) We only accept MARs that are located in the profile dirs of users that
> are administrators.

An untrusted user could, at least in theory, write a workitem file that contains path to an old/inappropriate MAR file inside another (administrator) user's profile, to have a similar effect. For example, if the administrator logged in and downloaded a MAR file 6 months ago, and the outdated MAR file is still sitting around in their profile.

> 2) We watch directories inside app user data of administrator accounts
> instead of programdata for work items.  Limited user accounts cannot write
> to those directories by default.  

Of the three options you presented, this seems the most plausible. But, I think it is still wrong because of the reasons I have (tried to have) explained above, regarding treating unelevated administrators specially.
(In reply to Brian Smith (:bsmith) from comment #37)
> (In reply to Brian R. Bondy [:bbondy] from comment #23)
> > I can think of at least 3 ways:
> > 1) We set administrators only write access to the work item directory.
> 
> This would require the administrator to be elevated, based on an experiment
> I just did.
As stated in private email:
"I think you are right on this. We'll try to come up with another method and reply with it after we do."

> > 3) We only accept MARs that are located in the profile dirs of users that
> > are administrators.
> 
> An untrusted user could, at least in theory, write a workitem file that
> contains path to an old/inappropriate MAR file inside another
> (administrator) user's profile, to have a similar effect. For example, if
> the administrator logged in and downloaded a MAR file 6 months ago, and the
> outdated MAR file is still sitting around in their profile.
Already understood and we are working on coming up with a way to mitigate this.

> 
> > 2) We watch directories inside app user data of administrator accounts
> > instead of programdata for work items.  Limited user accounts cannot write
> > to those directories by default.  
> 
> Of the three options you presented, this seems the most plausible. But, I
> think it is still wrong because of the reasons I have (tried to have)
> explained above, regarding treating unelevated administrators specially.
This is the one bbondy are discussing.

I would still like an answer to comment #36. Specifically, how is doing this with an administrator that is not elevated go against the end user expectation set by MS and doing this for an administrator that is not elevated and for limited users not going against the end user expectation set by MS. In addition, we can set the policy as to what conditions we limit allowing the update based on our own security evaluations.
(In reply to Robert Strong [:rstrong] (do not email) from comment #38)
> (In reply to Brian Smith (:bsmith) from comment #37)
> > (In reply to Brian R. Bondy [:bbondy] from comment #23)
> > > I can think of at least 3 ways:
> > > 1) We set administrators only write access to the work item directory.
> > 
> > This would require the administrator to be elevated, based on an experiment
> > I just did.
> As stated in private email:
> "I think you are right on this. We'll try to come up with another method and
> reply with it after we do."
> 
> > > 3) We only accept MARs that are located in the profile dirs of users that
> > > are administrators.
> > 
> > An untrusted user could, at least in theory, write a workitem file that
> > contains path to an old/inappropriate MAR file inside another
> > (administrator) user's profile, to have a similar effect. For example, if
> > the administrator logged in and downloaded a MAR file 6 months ago, and the
> > outdated MAR file is still sitting around in their profile.
> Already understood and we are working on coming up with a way to mitigate
> this.
bah... I misundertood this. We already have version check code in the startup path to prevent this.
(In reply to Robert Strong [:rstrong] (do not email) from comment #39)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #38)
> > (In reply to Brian Smith (:bsmith) from comment #37)
> > > (In reply to Brian R. Bondy [:bbondy] from comment #23)
> > > > I can think of at least 3 ways:
> > > > 1) We set administrators only write access to the work item directory.
> > > 
> > > This would require the administrator to be elevated, based on an experiment
> > > I just did.
> > As stated in private email:
> > "I think you are right on this. We'll try to come up with another method and
> > reply with it after we do."
> > 
> > > > 3) We only accept MARs that are located in the profile dirs of users that
> > > > are administrators.
> > > 
> > > An untrusted user could, at least in theory, write a workitem file that
> > > contains path to an old/inappropriate MAR file inside another
> > > (administrator) user's profile, to have a similar effect. For example, if
> > > the administrator logged in and downloaded a MAR file 6 months ago, and the
> > > outdated MAR file is still sitting around in their profile.
> > Already understood and we are working on coming up with a way to mitigate
> > this.
> bah... I misundertood this. We already have version check code in the
> startup path to prevent this.
I should also mention before we go down discussing this path that the last item in comment #38 will cover the untrusted user part of your statement.
Hrm.

The case of multi-user installs of Firefox with differing privilege levels and rogue local access is, I hope we can agree, a subset of our post-XP Windows install base. I would go so far as to call it a small subset. Shipping the service without a resolution to this bug creates a security exposure for that group, though I'd hasten to point out that it is sg:moderate at best. This is a real bug, we should fix it, but it is a bug that is constrained in both scope and impact.

The problem this service sets out to solve, though, is user resistance to applying updates in general because of an operating system feature that we believe, in making updates more onerous and manual, dissuades users from applying them regularly. The degree of that dissuasion is hard to measure, but what we can say is that it applies to all of our post-XP windows install (modulo those who disable UAC). What's worse, under the current update regime, unprivileged users in this differing-privilege cohort don't get to apply updates at all (they get a notice so they can escalate/inform).

To my mind, both "inability to apply updates while unprivileged" and "less willing to apply updates while privileged" are graver security risks for our users than the downgrade attack represented here. My preference, and the motivation behind this work, would be to address this update lag as soon as humanly possible because it represents a massive attack surface for users at all privilege levels and we have good data pointing to this lag in the field (albeit while speculating about its causes).

Am I off-base in my assessment here? I think we are burning-hot-on-fire-with-flames in terms of putting our users at risk by requiring their active participation in our delivery of updates. My sense is that the path of righteousness is to douse that fire by shipping the service, and then resolve this important and valid bug. Not because of an artificial timeline we've imposed on ourselves, but because I believe we are putting our users at continued risk by not shipping it.
(In reply to Johnathan Nightingale [:johnath] from comment #41)
> Am I off-base in my assessment here? I think we are
> burning-hot-on-fire-with-flames in terms of putting our users at risk by
> requiring their active participation in our delivery of updates. My sense is
> that the path of righteousness is to douse that fire by shipping the
> service, and then resolve this important and valid bug. Not because of an
> artificial timeline we've imposed on ourselves, but because I believe we are
> putting our users at continued risk by not shipping it.

I think that you are mostly right Johnathan. But, I think the same things apply to XP users too--in fact, they are even more affected than Vista/7 users, because UAC is actually *MUCH* less inconvenient than what XP users have to do to install an update. The nice thing about fixing this bug and the related ones is that the automatic, silent updates will work for all users on all versions of Windows. AFAICT, that is what we are going to have to do anyway.

The bad thing about doing the workaround that rstrong and bbondy are proposing is that, we will have to rip it out and fix these bugs later anyway. That means churn in the feature after we've started shipping it to users, which means more risk that we are going to break things and/or paint ourselves into a corner for solving the problem more appropriately later. Also, we don't even know the design of that workaround yet, so we cannot even evaluate its security. And, finally, the workaround greatly restricts the effectiveness of the feature compared with fixing these bugs.
(In reply to Robert Strong [:rstrong] (do not email) from comment #38)
> I would still like an answer to comment #36. Specifically, how is doing this
> with an administrator that is not elevated go against the end user
> expectation set by MS and doing this for an administrator that is not
> elevated and for limited users not going against the end user expectation
> set by MS.

Because, an unevaluated admin is being treated like a limited user. That is the main expectation that makes it reasonable to use your computer to do everyday tasks with an account that is in the Administrators group.

Imagine that we designed things a different way, so that the service just downloaded the MAR file itself. Under what conditions could we automatically install the MAR file? We could do so as long as it is the same product, same channel, and later version. Now, what difference does it really make that some user (admin or otherwise) downloaded the file instead of us? My position is that there is no difference (and should be no difference); they are just playing the part of an untrusted network proxy, so that we don't have to write networking code driven by the service.

(Hopefully that clarifies things more than it confuses things.)
(In reply to Brian Smith (:bsmith) from comment #42)
> (In reply to Johnathan Nightingale [:johnath] from comment #41)
> > Am I off-base in my assessment here? I think we are
> > burning-hot-on-fire-with-flames in terms of putting our users at risk by
> > requiring their active participation in our delivery of updates. My sense is
> > that the path of righteousness is to douse that fire by shipping the
> > service, and then resolve this important and valid bug. Not because of an
> > artificial timeline we've imposed on ourselves, but because I believe we are
> > putting our users at continued risk by not shipping it.
> 
> I think that you are mostly right Johnathan. But, I think the same things
> apply to XP users too--in fact, they are even more affected than Vista/7
> users, because UAC is actually *MUCH* less inconvenient than what XP users
> have to do to install an update. The nice thing about fixing this bug and
> the related ones is that the automatic, silent updates will work for all
> users on all versions of Windows. AFAICT, that is what we are going to have
> to do anyway.
> 
> The bad thing about doing the workaround that rstrong and bbondy are
> proposing is that, we will have to rip it out and fix these bugs later
> anyway. That means churn in the feature after we've started shipping it to
> users, which means more risk that we are going to break things and/or paint
> ourselves into a corner for solving the problem more appropriately later.
> Also, we don't even know the design of that workaround yet, so we cannot
> even evaluate its security. And, finally, the workaround greatly restricts
> the effectiveness of the feature compared with fixing these bugs.
We have already had a metric ton of churn in this last week. What we are proposing is very simple to accomplish when compared to that churn. Also, the churn in the last week is significantly more likely to break users than what we are proposing. We have already considered whether it would paint us into a corner and actually fixing this bug would be much more likely to paint us into a corner due to the extra restrictions that have been repeatedly noted.
(In reply to Brian Smith (:bsmith) from comment #43)
> (In reply to Robert Strong [:rstrong] (do not email) from comment #38)
> > I would still like an answer to comment #36. Specifically, how is doing this
> > with an administrator that is not elevated go against the end user
> > expectation set by MS and doing this for an administrator that is not
> > elevated and for limited users not going against the end user expectation
> > set by MS.
> 
> Because, an unevaluated admin is being treated like a limited user. That is
> the main expectation that makes it reasonable to use your computer to do
> everyday tasks with an account that is in the Administrators group.
So, the unevaluated admin (do you mean unelevated and if not please provide more details) should not be allowed to apply an update without displaying the UAC and the limited user shouldn't be allowed to apply updates? Or are you saying that the admin user could downgrade and we need to protect against that? If so, the admin user can already downgrade.

> 
> Imagine that we designed things a different way, so that the service just
> downloaded the MAR file itself. Under what conditions could we automatically
> install the MAR file? We could do so as long as it is the same product, same
> channel, and later version. Now, what difference does it really make that
> some user (admin or otherwise) downloaded the file instead of us? My
> position is that there is no difference (and should be no difference); they
> are just playing the part of an untrusted network proxy, so that we don't
> have to write networking code driven by the service.
This doesn't apply since we are proposing only allowing an admin or as Johnathan commented not worrying about this bug for the current release. See comment #28 as to why these are the choices at this time.
(In reply to Robert Strong [:rstrong] (do not email) from comment #45)
> Or are
> you saying that the admin user could downgrade and we need to protect
> against that? If so, the admin user can already downgrade.

He *can* but we should not forget that he might not want to do that but something else on running under his account wants so without him noticing. I'm not saying this is critical but malware should not be able to abuse our service for this or any other purpose that helps it to circumvent the UAC.
Glad we finally got that cleared up. Thanks.
Just to make that clear, as my post might have been misleading: I'm not saying that the malware attack is an issue at all for this particular bug (version downgrade), because malware wouldn't benefit that much from downgrading it (exploiting a bug in downgraded Firefox wouldn't elevate privileges), it was rather an example to show that whether the user can do it with or without UAC is something we shouldn't threat equally in general. But I assume you we're not suggesting that and rather only referring to the version downgrade specifically.
(In reply to Robert Strong [:rstrong] (do not email) from comment #45)
> > Because, an unevaluated admin is being treated like a limited user. That is
> > the main expectation that makes it reasonable to use your computer to do
> > everyday tasks with an account that is in the Administrators group.
> So, the unevaluated admin (do you mean unelevated and if not please provide
> more details) should not be allowed to apply an update without displaying
> the UAC and the limited user shouldn't be allowed to apply updates?

Right.

But, all types of users should equally be allowed to submit a MAR file to the service so that the service can apply the updates if the update meet reasonable criteria for automatically, silently, doing the update.

> Or are
> you saying that the admin user could downgrade and we need to protect
> against that? If so, the admin user can already downgrade.

I am saying that the service shouldn't automate downgrading and that downgrading must require explicit administrator intervention. (And, same with channel switching.)

> This doesn't apply since we are proposing only allowing an admin or as
> Johnathan commented not worrying about this bug for the current release. See
> comment #28 as to why these are the choices at this time.

It seems like only allowing an admin to do this is going to reduce attack surface, so that is better than just doing nothing. I hope that people that would make the decision to do that understand that this (a) severely limits the effectiveness of the feature, and (b) is counter to the fundamental design of Windows' UAC mechanism. If there is anything I can do to clarify this for people that don't understand what I am trying to say, I will try harder to clarify it.

We don't have any concrete proposal for restricting the submission of workitems such that only administrators can submit them. If we are going to do something like that (which I am still totally against), please at least share the details so that I can help analyze the security of that workaround. Even though I completely disagree with doing anything like that, I still want to help ensure that whatever you do is as good as possible.
(In reply to Robert Strong [:rstrong] (do not email) from comment #39)
> > (In reply to Brian Smith (:bsmith) from comment #37)
> > > An untrusted user could, at least in theory, write a workitem file that
> > > contains path to an old/inappropriate MAR file inside another
> > > (administrator) user's profile, to have a similar effect. For example, if
> > > the administrator logged in and downloaded a MAR file 6 months ago, and the
> > > outdated MAR file is still sitting around in their profile.
>
> We already have version check code in the
> startup path to prevent this.

How does that work specifically? The version isn't in the MAR or we'd close this bug INVALID/WORKSFORME. There's a "update.version" text file that sits next to the .MAR but that's not signed and unreliable in the attack scenario.
Blocks: 720688
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: 12 years ago
Resolution: --- → FIXED
Marking as WFM as per bsmith's recommendation.
Depends on: 708690
Resolution: FIXED → WORKSFORME
status-firefox-esr10: --- → unaffected
status-firefox10: --- → unaffected
status-firefox12: --- → affected
status-firefox13: --- → fixed
Target Milestone: --- → mozilla13
WFM implies we don't know when/why something went away. This is more either FIXED by or a DUPE of bug 708690
Resolution: WORKSFORME → FIXED
Whiteboard: [sg:moderate] → [sg:dupe 708690]
That's fine with me and that's what I did originally in Comment 51, but I was emailed by bsmith to change it. 

> AFAICT, the "proper" way to resolve this is RESOLVED WORKSFORME, Depends on:
> 708690. That way, the bug with the fix gets backed out, or a follow-up bug is
> filed, we can better see what other bugs are possibly affected. I was told that 
> RESOLVED FIXED is only supposed to be used for bugs with patches.

I don't mind either way but just wanted to show the justification for why it was changed.
Whiteboard: [sg:dupe 708690] → [sg:dupe 708690][advisory-tracking+]
Is there something QA can do to verify this is fixed?
I think it was verified here:
Bug 708690 Comment 96
Status: RESOLVED → VERIFIED
Okay, thanks Brian. Marking this verified based on that.
status-firefox13: fixed → verified
Keywords: verified-beta
Whiteboard: [sg:dupe 708690][advisory-tracking+] → [sg:dupe 708690][advisory-tracking+][qa+:simonab]
Group: core-security
Blocks: 750462
You need to log in before you can comment on or make changes to this bug.