The default bug view has changed. See this FAQ.

Callback path and arguments are taken from the service work item, which is untrusted

RESOLVED FIXED in Firefox 12

Status

()

Toolkit
Application Update
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: imelven, Assigned: bbondy)

Tracking

Trunk
mozilla12
x86
Windows Vista
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox12 fixed, firefox13 fixed, firefox-esr10 unaffected)

Details

(Whiteboard: [sg:high][qa-])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
The service work item for the work in bug 481185 contains the callback application to execute after the update has occurred and the arguments to pass to this application. The work item is created in a world writeable directory that other valid users on the machine will be able to create files (work items) in. 

Although the callback executable's signature is checked and it is verified to be signed by Mozilla - it could be any executable signed by Mozilla, not necessarily the newly installed version of Firefox. Additionally, arbitrary arguments can be passed to the executable - the command line options to Mozilla signed executables may have all sorts of powers/behaviours that can't be assumed to be safe or low risk. 

Marking [sg:high] due to the possibility of there being command line args to Firefox (and other Mozilla signed executables) that could cause real security problems beyond opening an arbitrary URL. Also marking as blocking bug 481185. Please discuss here in the bug if you have other opinions :)
(Reporter)

Comment 1

5 years ago
The proposed design is to have the unelevated updater.exe perform the callback - this removes the necessity to pass the callback path and args to the service at all, and removes the need to have them in the work item. 

See also bug 708854 which is around the session ID spoofing issue possible to the session ID being in the work item - the same fix to use the unelevated updater.exe to perform the callback should address both that bug and this bug.
See Also: → bug 708854
(Assignee)

Comment 2

5 years ago
Created attachment 580665 [details] [diff] [review]
pach v1.

Note: In bug 708854 we stopped executing the callback application in the service.  In this bug we no longer pass the info of the callback, nor do we use it to determine anything else.  For example we used it to determine where the post update process was located. 

Changes:
- We no longer pass this information to the work item file
- We no longer use this information to determine the path of the post update process which we execute.
- Added some extra logging.
- Tested and pushed to elm as well.
Attachment #580665 - Flags: review?(robert.bugzilla)
Attachment #580665 - Flags: review?(imelven)
(Assignee)

Updated

5 years ago
Blocks: 481815, 708854
No longer blocks: 481185
(Assignee)

Updated

5 years ago
No longer blocks: 708854
Depends on: 708854
(Reporter)

Comment 3

5 years ago
Comment on attachment 580665 [details] [diff] [review]
pach v1.

Review of attachment 580665 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good !

so the callback is now executed from the updater.exe like it sometimes was previously
(that's the this patch is mostly removing code from the service, correct ? )
Attachment #580665 - Flags: review?(imelven) → feedback+
(Reporter)

Comment 4

5 years ago
(In reply to Ian Melven :imelven from comment #3)
> Comment on attachment 580665 [details] [diff] [review]
> pach v1.
> 
> Review of attachment 580665 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good !
> 
> so the callback is now executed from the updater.exe like it sometimes was
> previously
> (that's the this patch is mostly removing code from the service, correct ? )

er 'that's why this patch is' - sorry.

mostly i'm just clarifying my understanding of why this patch is pretty small, the callback stuff from updater.exe (unelevated) was already there.
(Assignee)

Comment 5

5 years ago
> so the callback is now executed from the updater.exe 
> like it sometimes was previously

It is started in the same way as the callback exe (Firefox.exe) is executed without the service (by the unelevated updater.exe).

> mostly i'm just clarifying my understanding of why this patch is pretty small, 
> the callback stuff from updater.exe (unelevated) was already there.

Ya most of the work here was done in bug 708854.  This bug is just removing the passing of the callback info.  And we used that info for determining the location of helper.exe which now we use the install dir instead.
(Assignee)

Comment 6

5 years ago
Created attachment 581885 [details] [diff] [review]
Patch v2.

Rebased
Assignee: nobody → netzen
Attachment #580665 - Attachment is obsolete: true
Attachment #580665 - Flags: review?(robert.bugzilla)
Attachment #581885 - Flags: review?(robert.bugzilla)
Comment on attachment 581885 [details] [diff] [review]
Patch v2.

>diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
>--- a/toolkit/mozapps/update/updater/updater.cpp
>+++ b/toolkit/mozapps/update/updater/updater.cpp
>@@ -1749,17 +1749,21 @@ int NS_main(int argc, NS_tchar **argv)
>       // update.status file to make sure it succeeded, and if it did
>       // we need to manually start the PostUpdate process from the
>       // current user's session of this unelevated updater.exe the
>       // current process is running as.
>       if (useService) {
>         bool updateStatusSucceeded = false;
>         if (IsUpdateStatusSucceeded(updateStatusSucceeded) && 
>             updateStatusSucceeded) {
>-          LaunchWinPostProcess(argv[callbackIndex], gSourcePath, false, NULL);
>+          if (!LaunchWinPostProcess(argv[2], gSourcePath, false, NULL)) {
>+            fprintf(stderr, "The post update process which is only run for "
>+                    "service updates, and is run unelevated, could not be "
>+                    "launched.");
nit: which runs as the user for a service update could not be launched.

Nice!
Attachment #581885 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 8

5 years ago
Created attachment 581993 [details] [diff] [review]
Patch v3.

Implemented review comments.
Attachment #581885 - Attachment is obsolete: true
Attachment #581993 - Flags: review+
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/ee4def38eede
status-firefox-esr10: --- → unaffected
status-firefox12: --- → fixed
status-firefox13: --- → fixed
Whiteboard: [sg:high] → [sg:high][qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.