711505 - Callback application should be locked when doing updates from the service

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Brian R. Bondy [:bbondy]]

When updates run through the maintenance service, we are no longer passing callback information to the maintenance service (as of a recent security bug).  So when the maintenance service runs the updater elevated it does not have the callback information.

For that reason the callback application is not being locked during update, so a user could start it during an update.  if this happens the user will get an error saying that they should download the complete MAR.  It would be better to give the error we do without the service that the file is in use.

To do this;

> callbackFile = CreateFileW(argv[callbackIndex],
>                            DELETE | GENERIC_WRITE,
>                            // allow delete, rename, and write
>                            FILE_SHARE_DELETE | FILE_SHARE_WRITE,
>                            NULL, OPEN_EXISTING, 0, NULL);

This should be done from the unelevated callback though since the maintenance service does not have information about the callback application.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

This makes it so you can't use firefox while the service is applying the update.

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2.

Alternate more safe way of doing locking that won't cause problems as per discussions.

Comment 3

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

Comment on attachment 582371 [details] [diff] [review]
Patch v2.

Yes! Some manual testing would be a good thing but I am as certain as can be this is the best way to do this.

Comment 4

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3.

Patch v2 was fine but it didn't handle when there was an error for launching the callback. In those cases the callback would be launched as session 0 in addition to the user session. I put the check inside the launch callback app function to catch all past, present and future calls to this function.

Comment 5

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

Comment on attachment 582490 [details] [diff] [review]
Patch v3.

Makes sense. Would be good to go through these different code paths but better to do so when fully awake.

Comment 6

[Brian R. Bondy [:bbondy]]

I tested one error condition which was how I seen a firefox in session 0 with the old patch, the new patch fixes it though.  The other error conditions are all the same type of code with launching callback and early return

Comment 7

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/mozilla-central/rev/4b4c20784201