Closed Bug 452128 Opened 12 years ago Closed 12 years ago

Only try to elevate when updating from the application

Categories

(Toolkit :: Application Update, defect)

x86
Windows Vista
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1b1

People

(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)

References

Details

(Keywords: fixed1.9.0.4, Whiteboard: [sg:low] local elevation of privilege)

Attachments

(2 files)

Found this over the weekend while working on adding updater tests for applying mar files.
Ted, this just checks if argc > 4 to determine whether a lock file should be created, etc. - Sorry, I wasn't able to get diff -w to not show the whitespace changes.
Attachment #335428 - Flags: review?(ted.mielczarek)
Ted, a -w patch should be easier to review
Attachment #335483 - Flags: review?(ted.mielczarek)
Attachment #335428 - Flags: review?(ted.mielczarek)
Attachment #335483 - Flags: review?(ted.mielczarek) → review?(benjamin)
Comment on attachment 335483 [details] [diff] [review]
patch rev1 - diff -w

Just an FYI... there are tests for verifying the files added, modified, and deleted by the updater binary in bug 451085. These use the command line to apply the mar.
Attachment #335483 - Flags: review?(benjamin) → review?(ted.mielczarek)
Attachment #335483 - Flags: review?(ted.mielczarek) → review+
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/ad9d3380f03a0013c81497790209fe62646fd5b6
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
Comment on attachment 335483 [details] [diff] [review]
patch rev1 - diff -w

Drivers, this prevents attempts to elevate when updating from the command line and is a very simple fix I'd like to get for 1.9.0.3
Attachment #335483 - Flags: approval1.9.0.3?
Attachment #335483 - Flags: approval1.9.0.4? → approval1.9.0.4+
Comment on attachment 335483 [details] [diff] [review]
patch rev1 - diff -w

Approved for 1.9.0.4, a=dveditz for release-drivers
This sounds like a non-remote security problem -- on a shared machine a user with limited rights could modify the installed Firefox on that machine?

I haven't read the code not included in the patch context, how do you make sure the updater is launched by a valid Firefox process?
Whiteboard: [sg:low] local elevation of privilege
When the updater.exe is launched by the application there are additional command line arguments. For the manual update case the previous code assumes that the user already has write access to update the installation and with this change the updater.exe won't attempt elevation for this case.

For the non-remote case with limited rights the updater.exe could be launched by something other than a valid Firefox process with the request for elevation (e.g. runas, shellexecute with the runas verb, etc.) so I don't believe that a check within updater.exe for a valid Firefox process would provide any value.
Checking in mozilla/toolkit/mozapps/update/src/updater/updater.cpp;
/cvsroot/mozilla/toolkit/mozapps/update/src/updater/updater.cpp,v  <--  updater.
cpp
new revision: 1.38; previous revision: 1.37
done
Keywords: fixed1.9.0.4
Dan, I'm not going to remove the sg:low but I really don't think this is a security issue.
How do you tell the difference between the updater being called by Firefox and the user manually passing the required number of arguments? Or writing a stub program to call it with the right number of arguments?

The updates aren't signed, though, so I guess the equivalent attack would be to alter the update files after they're downloaded and before Firefox launches the updater.

Either way doesn't that allow a non-admin user to replace/remove/add files that would otherwise require admin privs to do?
(In reply to comment #11)
> How do you tell the difference between the updater being called by Firefox and
> the user manually passing the required number of arguments? Or writing a stub
> program to call it with the right number of arguments?
The number of arguments passed to updater.exe

> The updates aren't signed, though, so I guess the equivalent attack would be to
> alter the update files after they're downloaded and before Firefox launches the
> updater.
Or adding a mar file along with the associated files used to determine an update is pending installation.

> Either way doesn't that allow a non-admin user to replace/remove/add files that
> would otherwise require admin privs to do?
We present the user with the UAC dialog and in the future for systems without UAC or UAC is turned off with the runas dialog. If the user grants elevation the updater.exe will then apply the mar file.
You need to log in before you can comment on or make changes to this bug.