452128 - Only try to elevate when updating from the application

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Robert Strong (they/them - no direct email)]

Found this over the weekend while working on adding updater tests for applying mar files.

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch - check argc > 4

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.

Comment 2

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1 - diff -w

Ted, a -w patch should be easier to review

Comment 3

[Robert Strong (they/them - no direct email)]

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.

Comment 4

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/ad9d3380f03a0013c81497790209fe62646fd5b6

Comment 5

[Robert Strong (they/them - no direct email)]

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

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 335483 [details] [diff] [review]
patch rev1 - diff -w

Approved for 1.9.0.4, a=dveditz for release-drivers

Comment 7

[Daniel Veditz [:dveditz]]

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?

Comment 8

[Robert Strong (they/them - no direct email)]

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.

Comment 9

[Robert Strong (they/them - no direct email)]

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

Comment 10

[Robert Strong (they/them - no direct email)]

Dan, I'm not going to remove the sg:low but I really don't think this is a security issue.

Comment 11

[Daniel Veditz [:dveditz]]

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?

Comment 12

[Robert Strong (they/them - no direct email)]

(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.