updater.exe needs to be verified to be an updater, not just an executable signed by Mozilla

RESOLVED FIXED in Firefox 12

Status

()

defect
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: imelven, Assigned: bbondy)

Tracking

(Blocks 1 bug)

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

Firefox Tracking Flags

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

Details

(Whiteboard: [sg:moderate][qa+])

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

8 years ago
the current implementation of the silent update Windows service (see bug 481815) does a check on the updater.exe before running it to make sure that it is an executable signed by Mozilla - this check doesn't verify that it's actually an updater.exe - it could be Thunderbird, Firefox, etc. 

one check suggested is to check a 'magic token' in the updater.exe which is essentially a string resource that only an updater.exe and no other signed product would contain. another approach suggested is to check the original file name field of the file metadata and verify that it is 'updater.exe' and not another product. 

see also bug 709158 which covers the case that it IS an updater.exe - but not necessarily the right product's/channel's/version of updater.exe

Marking [sg:moderate], please discuss here in the bug if you have other opinions of this rating.
Reporter

Updated

8 years ago
See Also: → 709158
Reporter

Updated

8 years ago
See Also: → 481815
Assignee

Comment 1

8 years ago
Posted patch Patch v1. (obsolete) — Splinter Review
Added a string table value with a unique value.
Only Mozilla updater applications will have this string table item and value.
An update operation will not run from the service if the updater does not contain this magic string value.
Assignee: nobody → netzen
Attachment #580716 - Flags: review?(robert.bugzilla)
Attachment #580716 - Flags: review?(imelven)
Reporter

Comment 2

8 years ago
Comment on attachment 580716 [details] [diff] [review]
Patch v1.

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

looks good to me, one small comment about a possible refactor that doesn't affect the security concerns here.

::: toolkit/components/maintenanceservice/workmonitor.cpp
@@ +405,5 @@
> +
> +  if (updaterModule) {
> +    FreeLibrary(updaterModule);
> +  }
> +

nit: you could refactor the above, move the FreeLibrary inside the preceding
block, make the 2nd block an else of the first if (!updaterModule)
Attachment #580716 - Flags: review?(imelven) → feedback+
Assignee

Comment 3

8 years ago
Posted patch Patch v2. (obsolete) — Splinter Review
Thanks for the review Ian, implemented nits.
Attachment #580716 - Attachment is obsolete: true
Attachment #580716 - Flags: review?(robert.bugzilla)
Attachment #581251 - Flags: review?(robert.bugzilla)
Comment on attachment 581251 [details] [diff] [review]
Patch v2.

Looks good
Attachment #581251 - Flags: review?(robert.bugzilla) → review+
Assignee

Comment 5

8 years ago
Posted patch Patch v3.Splinter Review
Rebased.
Attachment #581251 - Attachment is obsolete: true
Attachment #581887 - Flags: review+
Assignee

Updated

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Is this something QA can verify?
Whiteboard: [sg:moderate] → [sg:moderate][qa?]
Assignee

Comment 8

7 years ago
I think you can copy another signed executable that is not the updater and rename it to updater.exe.  Then try to do an update.  The maintenance service log should contain an error code 28 which is the identity error.  I'm not 100% certain those are the steps but please try and let me know if it doesn't work.
Whiteboard: [sg:moderate][qa?] → [sg:moderate][qa+]
Group: core-security
(In reply to Brian R. Bondy [:bbondy] from comment #8)
> I think you can copy another signed executable that is not the updater and
> rename it to updater.exe.  Then try to do an update.  The maintenance
> service log should contain an error code 28 which is the identity error. 
> I'm not 100% certain those are the steps but please try and let me know if
> it doesn't work.

Tried the above indications:
- renamed the crashreport.exe to updater.exe and after the update was done the Crash Reporter Error dialog was displayed - the maintenance service log was not generated.
- renamed FirefoxSetup.exe to updater.exe and updated, after the restart the Install dialog was opened - the maintenance service log was not generated.

Also, replaced the Firefox updater with the Thunderbird one, the update was done the old way and the maintenance service log was not generated. 

Brian, any other ideas?
Assignee

Comment 10

7 years ago
ah, that's because the unelevated updater.exe needs to execute first in order to start the maintenance service operation.  I'm not sure how else you could reproduce this short of making a custom mozilla signed exe.
Assignee

Comment 11

7 years ago
I think relying on the log message alone is enough to test this task.

So make sure this is not present in the log after an update:
The updater.exe identity string is not valid.

And make sure this is present in the log:
The updater.exe application contains the Mozilla updater identity.
Mozilla/5.0 (Windows NT 6.1; rv:13.0) Gecko/20100101 Firefox/13.0

Verified (on Win 7 and Win Vista x64)by checking the maintenanceservice logs that after updating Firefox 13 beta 4 to Firefox 13 beta 5 that the updater.exe is verified to be an updater and is signed by Mozilla: 

- "The updater.exe identity string is not valid" - is not present in the logs
and
- "The updater.exe application contains the Mozilla updater identity" - is present in the logs.

Thanks Brian for the guidance.
Blocks: 750462
You need to log in before you can comment on or make changes to this bug.