Service is sometimes not used because security does not always get set to allow unelevated user access.

RESOLVED FIXED in mozilla12

Status

()

Toolkit
Application Update
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dao, Assigned: bbondy)

Tracking

Trunk
mozilla12
x86
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

6 years ago
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120106 Firefox/12.0a1

"Use a background service to install updates" is checked.

In case it matters, I selected "Check for updates, but let me choose whether to install them".

- Are you running as an admin?

yes

- Have you ever installed a previous build and manually uninstalled? It will only be installed on update on the first install.

no

- Is the service installed on your machine? From a command prompt run sc query MozillaMaintenance

SERVICE_NAME: MozillaMaintenance
        TYPE               : 10  WIN32_OWN_PROCESS
        STATE              : 1  STOPPED
        WIN32_EXIT_CODE    : 1077  (0x435)
        SERVICE_EXIT_CODE  : 0  (0x0)
        CHECKPOINT         : 0x0
        WAIT_HINT          : 0x0

- Are you using an x86 build or an x64 build?

x86
(Assignee)

Comment 1

6 years ago
Could you zip up and attach the contents of this directory?
C:\ProgramData\Mozilla\logs
(Reporter)

Comment 2

6 years ago
There's only one file, maintenanceservice-install.log:

Upgrading service if installed...
Sending stop request...
Error sending stop request: 1062
Waiting for service stop...
Done waiting for service stop, last service state: 1
The service was upgraded successfully
(Assignee)

Comment 3

6 years ago
Updates get applied when you click Yes on the UAC prompt right?

Can you check about:config for app.update.service.errors is that value non existent?  I'm guessing it will not exist.

It seems like it's not even attempting to use the service for some reason.
(Assignee)

Comment 4

6 years ago
> In case it matters, I selected "Check for updates, but let me choose whether to install them".

I suspect it has to do with this setting.
(Reporter)

Comment 5

6 years ago
(In reply to Brian R. Bondy [:bbondy] from comment #3)
> Updates get applied when you click Yes on the UAC prompt right?

yes

> Can you check about:config for app.update.service.errors is that value non
> existent?  I'm guessing it will not exist.

it doesn't exist
(Assignee)

Comment 6

6 years ago
Could you try installing yesterday's build and change that setting to Automatically install updates (recommended: improved security).

Then try to upgrade to today's nightly. It should use the service.

I think that is the expected behavior if you have "Check for updates, but let me choose whether to install them".
(Reporter)

Comment 7

6 years ago
(In reply to Brian R. Bondy [:bbondy] from comment #6)
> I think that is the expected behavior if you have "Check for updates, but
> let me choose whether to install them".

Why?

I changed that setting and triggered new nightly builds for af6501ede378. Will report back when I get the update.
(Assignee)

Comment 8

6 years ago
If the test I proposed does not work, then we have a bug.
If the test I proposed works:

If you'd like this functionality changed, then please rename the subject of this bug to:
Use service even with "Check for updates, but let me choose whether to install them".  Then I'll take this bug.

If you agree the service should not be used in this case please mark as resolved / invalid.
(Assignee)

Comment 9

6 years ago
> I changed that setting and triggered new nightly builds for af6501ede378. 
> Will report back when I get the update.

I'll try here and report back the result.

Comment 10

6 years ago
Is it possible that using the -no-remote option affects this?
(Assignee)

Comment 11

6 years ago
No I think we only use the service when we have a pending state which is only when automatic apply updates are used. Verifying this shortly though.
(Assignee)

Comment 12

6 years ago
OK I tested and "Check for updates, but let me choose whether to install them" does not automatically stop using the service.  I.e. whether or not to use the service is determined by "Use a background service to install updates.

So it sounds like there is some other reason why your Nightly Firefox is not asking the service to do an update.  The service is not being attempted to be used at all, otherwise you'd have app.update.service.errors in your about:config and you'd have a log named maintenanceservice.log. 

You are using the m-c Nightly right? Not a UX nightly or something similar to do the test that might not have the application specific code to use the installed service?
(Reporter)

Comment 13

6 years ago
(In reply to Brian R. Bondy [:bbondy] from comment #12)
> You are using the m-c Nightly right?

Yes.
(Assignee)

Updated

6 years ago
Summary: Nightly update keeps triggering the UAC prompt → Nightly updates are sometimes not using the service for updates even know the service is installed
(Assignee)

Comment 14

6 years ago
Whenever you get a chance, next time you get an apply update button, please go here:
C:\Users\YOURUSERNAME\AppData\Local\Mozilla\Firefox\Nightly\updates\0

Open up the update.status file in a text editor and let me know what's inside.

If you have time now please:
1. Uninstall Nightly
2. Uninstall Mozilla Maintenance Service
3. Download and install http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-01-05-08-39-33-mozilla-central/firefox-12.0a1.en-US.win32.installer.exe
4. Go to the about dialog so the update downloads.
5. Before restarting Firefox to apply the update, check the directory I mentioned once the update is downloaded.
6. Report back the contents of update.status.
(Assignee)

Comment 15

6 years ago
If you do this test when the next update comes tomorrow, please remember you have to grab the info before the update is applied but after it is downloaded.
(Reporter)

Comment 16

6 years ago
Isn't there a risk that uninstalling and reinstalling could make the bug disappear and leave me with no way to reproduce it?
(Assignee)

Comment 17

6 years ago
You can only uninstall firefox and not the service to reduce this risk to none.  But there is no guarantee we will reproduce this even if we wait until tomorrow.
(Reporter)

Comment 18

6 years ago
(In reply to Brian R. Bondy [:bbondy] from comment #14)
> 6. Report back the contents of update.status.

pending-service
(Assignee)

Comment 19

6 years ago
Still no logs in C:\ProgramData\Mozilla\logs right? 
And service wasn't attempted to be used right? (Went directly to the UAC prompt)
(Assignee)

Comment 20

6 years ago
OK thanks for all the info so far, I would get this info myself but I cannot reproduce. 
A couple more things you can do to give me more info.

1. Could you please attach an export of "HKEY_LOCAL_MACHINE\SOFTWARE\Mozilla\MaintenanceService"
To do this just open regedit, navigate to that key, right click on it and select Export.

2. Could you check if the permissions are set on the service to allow unelevated processes to use it?

To do this simply open up a command prompt and type in:

> net start MozillaMaintenance

If you see the following, then permissions are set fine:

> The Mozilla Maintenance Service service is starting.
> The Mozilla Maintenance Service service could not be started.
> The service did not report an error.
> More help is available by typing NET HELPMSG 3534.

If you see the following, then permissions are not set:

> System error 5 has occurred.
> Access is denied.

Comment 21

6 years ago
I get "access denied" from "net start MozillaMaintenance".

Comment 22

6 years ago
If I use elevated cmd, it says something like "Mozilla Maintenance Service could not be started. The service didn't report an error."
(Assignee)

Comment 23

6 years ago
JK do you get the same as Dao with all of his same answers above?

Dao please confirm if you get the same as JK for the net start command. If so it sounds like the permissions were not set on the service to allow Firefox to use it.
(Reporter)

Comment 24

6 years ago
Created attachment 586708 [details]
HKEY_LOCAL_MACHINE\SOFTWARE\Mozilla\MaintenanceService
(Assignee)

Comment 25

6 years ago
Registry looks good, I suspect you have the same as JK.
(Reporter)

Comment 26

6 years ago
(In reply to JK from comment #22)
> If I use elevated cmd, it says something like "Mozilla Maintenance Service
> could not be started. The service didn't report an error."

I get the same over here.
(Assignee)

Comment 27

6 years ago
Great! :)
OK I'm throwing together a quick patch that we'll get landed on m-c then you can send me your C:\ProgramData\Mozilla\logs\maintenanceservice-install.log file after the next upgrade after that landed.

The new logging will tell us exactly which call the service security cannot be set on.  It'll also try to modify the security of the service back to the normal value on each upgrade.
(Assignee)

Updated

6 years ago
Summary: Nightly updates are sometimes not using the service for updates even know the service is installed → Service is sometimes not used because security does not always get set to allow unelevated user access.
(Assignee)

Comment 28

6 years ago
Created attachment 586710 [details] [diff] [review]
Extra logging and always set.  Patch v1.

- Always set security on service when upgrading service.
- More detailed logging when service security cannot be modified.
Assignee: nobody → netzen
Attachment #586708 - Attachment is obsolete: true
Attachment #586710 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 29

6 years ago
Dao, just to confirm, you said:

> If I use elevated cmd, it says something like "Mozilla Maintenance Service
> could not be started. The service didn't report an error."

I get the same over here.

The original question asked what you get for unelevated cmd.  I assume you also tried that and get the same as JK right?

> JK wrote:
> I get "access denied" from "net start MozillaMaintenance".
(Reporter)

Comment 30

6 years ago
(In reply to Brian R. Bondy [:bbondy] from comment #29)
> The original question asked what you get for unelevated cmd.  I assume you
> also tried that and get the same as JK right?
> 
> > JK wrote:
> > I get "access denied" from "net start MozillaMaintenance".

yes
(Reporter)

Comment 31

6 years ago
Created attachment 586713 [details]
maintenanceservice.log

this file appeared about half an hour ago, apparently
(Assignee)

Comment 32

6 years ago
Yup that is just from you running the command from an elevated prompt.
The relevant line from that log is:

> Not enough command line arguments to execute a service command

Which happens if you try to start it manually without Firefox passing extra args to the service on start.
(Assignee)

Comment 33

6 years ago
Created attachment 586805 [details] [diff] [review]
(already landed) Extra logging. Patch v2.

Minor fix for last patch.
Attachment #586710 - Attachment is obsolete: true
Attachment #586710 - Flags: review?(robert.bugzilla)
Attachment #586805 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

6 years ago
Attachment #586713 - Attachment is obsolete: true
Attachment #586805 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 34

6 years ago
Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e59ed78d1ed4

For whoever merges this into mozilla-central, please do not mark the bug as RESOLVED because this is just an intermediate patch to determine what the error is.
Target Milestone: --- → mozilla12
Created attachment 587333 [details]
maintenance service reg file

After a few successful updates without a UAC prompt, I saw one with today's (x86) nightly on 64-bit Win7.

(In reply to Brian R. Bondy [:bbondy] from comment #20)
> 
> 1. Could you please attach an export of
> "HKEY_LOCAL_MACHINE\SOFTWARE\Mozilla\MaintenanceService"
> To do this just open regedit, navigate to that key, right click on it and
> select Export.

Attached.

> 
> 2. Could you check if the permissions are set on the service to allow
> unelevated processes to use it?

Yes. I tried from an unelevated prompt and permissions seemed to be set correctly.
Attachment #587333 - Attachment description: maintenance service → maintenance service reg file
(Assignee)

Comment 36

6 years ago
Re sid0: 

Registry looks good.
Some notes on sid0's case:

- I looked at his maintenanceservice log files and they all showed success. So for the time it used the UAC prompt, it didn't even attempt to use the service.
- He does have permissions to the service so it seems to be a different issue than this bug.  I'll spin off a new bug ID.  
- The update succeeded with the UAC prompt so there are no errors it just didn't try to use the service for some reason.
- His preferences are set to automatically use updates and use service is still ticked on.
- He mentioned he did not accidentally use an x64 native build for this update nor a different branch such as UX.
(Assignee)

Updated

6 years ago
Attachment #587333 - Attachment is obsolete: true
(Assignee)

Comment 37

6 years ago
Posted about sid0's issue here: Bug 716915

Updated

6 years ago
Whiteboard: [leave open after merge]
(In reply to Brian R. Bondy [:bbondy] from comment #34)
> Pushed to mozilla-inbound:
> http://hg.mozilla.org/integration/mozilla-inbound/rev/e59ed78d1ed4
> 
> For whoever merges this into mozilla-central, please do not mark the bug as
> RESOLVED because this is just an intermediate patch to determine what the
> error is.

https://hg.mozilla.org/mozilla-central/rev/e59ed78d1ed4
(Assignee)

Comment 39

6 years ago
Thank you!
(Assignee)

Comment 40

6 years ago
to Dão:

Please send me your "C:\ProgramData\Mozilla\logs\maintenanceservice-install.log" assuming you can still reproduce on the next update. 

It is possible it will just start working on the update after next because each update now tries to set the permissions on the service.  

In either case I should be able to tell from that log file.
(Reporter)

Comment 41

6 years ago
(In reply to Brian R. Bondy [:bbondy] from comment #40)
> to Dão:
> 
> Please send me your
> "C:\ProgramData\Mozilla\logs\maintenanceservice-install.log" assuming you
> can still reproduce on the next update. 
> 
> It is possible it will just start working on the update after next because
> each update now tries to set the permissions on the service.  
> 
> In either case I should be able to tell from that log file.

Upgrading service if installed...
Sending stop request...
Error sending stop request: 1062
Waiting for service stop...
Done waiting for service stop, last service state: 1
The service was upgraded successfully
(Assignee)

Comment 42

6 years ago
1062 just means the service is not currently started.
last service state 1 is just stopped.

Unfortunately I think in the upgrade code path (which is hard to test) we're closing the file handle so we didn't attempt to modify the security settings again.  I tested this but with the forceinstall cmd line.  I'll need to do another patch that re-opens the service if we don't have a handle to the service (like in the upgrade case).
(Assignee)

Comment 43

6 years ago
Created attachment 587803 [details] [diff] [review]
Set permissions on upgrades. Patch v1.
Attachment #587803 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

6 years ago
Attachment #586805 - Attachment description: Extra logging and always set. Patch v2. → (already landed) Extra logging. Patch v2.
(Assignee)

Comment 44

6 years ago
Note for reviewer (Rob) I pushed this new patch to elm, did 2 nightly builds, and did an upgrade. I confirmed the needed log message is in the logs under the name maintenanceservice-install.log.
Comment on attachment 587803 [details] [diff] [review]
Set permissions on upgrades. Patch v1.

>diff --git a/toolkit/components/maintenanceservice/serviceinstall.cpp b/toolkit/components/maintenanceservice/serviceinstall.cpp
>--- a/toolkit/components/maintenanceservice/serviceinstall.cpp
>+++ b/toolkit/components/maintenanceservice/serviceinstall.cpp
>@@ -130,16 +130,26 @@ SvcInstall(SvcInstallAction action)
>     // The service exists but we couldn't open it
>     LOG(("Could not open service.  (%d)\n", GetLastError()));
>     return FALSE;
>   }
>   
>   if (schService) {
>     serviceAlreadyExists = TRUE;
> 
>+    // The service does exist but it may not have the correct permissions.
nit: s/does exist/exists/

>+    // This could happen if the permissions were not set correctly originally
>+    // or if the user manually messed with the permissions of the service.
or have been changed after the installation.

>+    // This will reset the permissions when the service already exists.
>+    if (!SetUserAccessServiceDACL(schService)) {
>+      LOG(("Could not reset security ACE on service handle, the service may not"
>+           " be able to be started from unelevated processes. (%d)\n", 
nit: I think this is technically more accurate.

Could not reset security ACE on service handle. It might not be possible to start the service. This error should never happen.  (%d)\n

>+           GetLastError()));
>+    }
>+
>     // The service exists and we opened it
>     DWORD bytesNeeded;
>     if (!QueryServiceConfigW(schService, NULL, 0, &bytesNeeded) && 
>         GetLastError() != ERROR_INSUFFICIENT_BUFFER) {
>       LOG(("Could not determine buffer size for query service config.  (%d)\n", 
>            GetLastError()));
>       return FALSE;
>     }
>@@ -234,19 +244,17 @@ SvcInstall(SvcInstallAction action)
>                                   newServiceBinaryPath, NULL, NULL, NULL, 
>                                   NULL, NULL));
>     if (!schService) {
>       LOG(("Could not create Windows service. "
>            "This error should never happen since a service install "
>            "should only be called when elevated. (%d)\n", GetLastError()));
>       return FALSE;
>     } 
>-  }
> 
>-  if (schService) {
Unless I'm mistaken, this change makes it so the first block where the service already exists should just return TRUE, the else if (UpgradeSvc == action) should be an if, and the if (!serviceAlreadyExists) that follows should be removed.

>     if (!SetUserAccessServiceDACL(schService)) {
>       LOG(("Could not set security ACE on service handle, the service will not"
>            "be able to be started from unelevated processes. "
>            "This error should never happen.  (%d)\n", 
nit: also here regarding the text

>            GetLastError()));
>     }
>   }
>
Attachment #587803 - Flags: review?(robert.bugzilla) → review-
(Assignee)

Comment 46

6 years ago
Created attachment 589378 [details] [diff] [review]
Set permissions on upgrades. Patch v2.

Implemented review comments.
Thanks for the suggestions, much cleaner now.
Attachment #587803 - Attachment is obsolete: true
Attachment #589378 - Flags: review?(robert.bugzilla)
Comment on attachment 589378 [details] [diff] [review]
Set permissions on upgrades. Patch v2.

># HG changeset patch
># Parent c9c6e7e10d8287c2faaec7f23809c75bd6042342
># User Brian R. Bondy <netzen@gmail.com>
>Bug 715910 - Set permissions for the service even on upgrades.
>
>diff --git a/toolkit/components/maintenanceservice/serviceinstall.cpp b/toolkit/components/maintenanceservice/serviceinstall.cpp
>--- a/toolkit/components/maintenanceservice/serviceinstall.cpp
>+++ b/toolkit/components/maintenanceservice/serviceinstall.cpp
>@@ -115,30 +115,37 @@ SvcInstall(SvcInstallAction action)
>                           sizeof(newServiceBinaryPath) / 
>                           sizeof(newServiceBinaryPath[0]))) {
>     LOG(("Could not obtain module filename when attempting to "
>          "install service. (%d)\n",
>          GetLastError()));
>     return FALSE;
>   }
> 
>-  // Check if we already have an open service
>-  BOOL serviceAlreadyExists = FALSE;
>+  // Check if we already have the service installed.
>   nsAutoServiceHandle schService(OpenServiceW(schSCManager, 
>                                               SVC_NAME, 
>                                               SERVICE_ALL_ACCESS));
>   DWORD lastError = GetLastError();
>   if (!schService && ERROR_SERVICE_DOES_NOT_EXIST != lastError) {
>     // The service exists but we couldn't open it
>     LOG(("Could not open service.  (%d)\n", GetLastError()));
>     return FALSE;
>   }
>   
>   if (schService) {
>-    serviceAlreadyExists = TRUE;
>+    // The service exists but it may not have the correct permissions.
>+    // This could happen if the permissions were not set correctly originally
>+    // or have been changed after the installation.  This will reset the 
>+    // permissions back to allow limited user accounts.
>+    if (!SetUserAccessServiceDACL(schService)) {
>+      LOG(("Could not reset security ACE on service handle. It might not be "
>+           "possible to start the service. This error should never"
>+           " happen.  (%d)\n", GetLastError()));
nit I'm a tad of a stickler for consistency. Please move the space before "happen" to the previous line.

>+    }
> 
>     // The service exists and we opened it
>     DWORD bytesNeeded;
>     if (!QueryServiceConfigW(schService, NULL, 0, &bytesNeeded) && 
>         GetLastError() != ERROR_INSUFFICIENT_BUFFER) {
>       LOG(("Could not determine buffer size for query service config.  (%d)\n", 
>            GetLastError()));
>       return FALSE;
>@@ -193,66 +200,67 @@ SvcInstall(SvcInstallAction action)
>         LOG(("Could not delete old service binary file %ls.  (%d)\n", 
>              serviceConfig.lpBinaryPathName, GetLastError()));
>         return FALSE;
>       }
> 
>       if (!CopyFileW(newServiceBinaryPath, 
>                     serviceConfig.lpBinaryPathName, FALSE)) {
>         LOG(("Could not overwrite old service binary file. "
>-             "This should never happen, but if it does the next upgrade will fix"
>-             " it, the service is not a critical component that needs to be "
>+             "This should never happen, but if it does the next upgrade will "
>+             "fix it, the service is not a critical component that needs to be"
>              " installed for upgrades to work. (%d)\n", 
nit: same here.

>              GetLastError()));
>         return FALSE;
>       }
> 
>       // We made a copy of ourselves to the existing location.
>       // The tmp file (the process of which we are executing right now) will be
>       // left over.  Attempt to delete the file on the next reboot.
>       MoveFileExW(newServiceBinaryPath, NULL, MOVEFILE_DELAY_UNTIL_REBOOT);
>       
>       // Setup the new module path
>       wcsncpy(newServiceBinaryPath, serviceConfig.lpBinaryPathName, MAX_PATH);
>-      // Fall through so we replace the service
>-    } else {
>-      // We don't need to copy ourselves to the existing location.
>-      // The tmp file (the process of which we are executing right now) will be
>-      // left over.  Attempt to delete the file on the next reboot.
>-      MoveFileExW(newServiceBinaryPath, NULL, MOVEFILE_DELAY_UNTIL_REBOOT);
>-      
>-      return TRUE; // nothing to do, we already have a newer service installed
>+
>+      // We replaced the service and set the permissions again so just return.
>+      return TRUE;
>     }
>-  } else if (UpgradeSvc == action) {
>-    // The service does not exist and we are upgrading, so don't install it
>+
>+    // We don't need to copy ourselves to the existing location.
>+    // The tmp file (the process of which we are executing right now) will be
>+    // left over.  Attempt to delete the file on the next reboot.
>+    MoveFileExW(newServiceBinaryPath, NULL, MOVEFILE_DELAY_UNTIL_REBOOT);
>+    
>+    // nothing to do, we already have a newer service installed
>+    return TRUE; 
>+  }
>+  
>+  // If the service does not exist and we are upgrading, don't install it.
>+  if (UpgradeSvc == action) {
>     return TRUE;
>   }
> 
>-  if (!serviceAlreadyExists) {
>-    // Create the service as on demand
>-    schService.own(CreateServiceW(schSCManager, SVC_NAME, SVC_DISPLAY_NAME,
>-                                  SERVICE_ALL_ACCESS, SERVICE_WIN32_OWN_PROCESS,
>-                                  SERVICE_DEMAND_START, SERVICE_ERROR_NORMAL,
>-                                  newServiceBinaryPath, NULL, NULL, NULL, 
>-                                  NULL, NULL));
>-    if (!schService) {
>-      LOG(("Could not create Windows service. "
>-           "This error should never happen since a service install "
>-           "should only be called when elevated. (%d)\n", GetLastError()));
>-      return FALSE;
>-    } 
>-  }
>+  // The service does not already exist so create the service as on demand
>+  schService.own(CreateServiceW(schSCManager, SVC_NAME, SVC_DISPLAY_NAME,
>+                                SERVICE_ALL_ACCESS, SERVICE_WIN32_OWN_PROCESS,
>+                                SERVICE_DEMAND_START, SERVICE_ERROR_NORMAL,
>+                                newServiceBinaryPath, NULL, NULL, NULL, 
>+                                NULL, NULL));
>+  if (!schService) {
>+    LOG(("Could not create Windows service. "
>+         "This error should never happen since a service install "
>+         "should only be called when elevated. (%d)\n", GetLastError()));
>+    return FALSE;
>+  } 
> 
>-  if (schService) {
>-    if (!SetUserAccessServiceDACL(schService)) {
>-      LOG(("Could not set security ACE on service handle, the service will not"
>-           "be able to be started from unelevated processes. "
>-           "This error should never happen.  (%d)\n", 
>-           GetLastError()));
>-    }
>+  if (!SetUserAccessServiceDACL(schService)) {
>+    LOG(("Could not set security ACE on service handle, the service will not"
space after not

>+         "be able to be started from unelevated processes. "
>+         "This error should never happen.  (%d)\n", 
>+         GetLastError()));
>   }
> 
>   return TRUE;
> }
> 
> /**
>  * Stops the Maintenance service.
>  *
Attachment #589378 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 48

6 years ago
> nit I'm a tad of a stickler for consistency. Please move the space 
> before "happen" to the previous line.

I actually noticed that just before submitting and decided to leave it, heh I deserved it for leaving it :)
(Assignee)

Comment 49

6 years ago
Created attachment 589389 [details] [diff] [review]
Set permissions on upgrades. Patch v3.

Implemented nits
Attachment #589378 - Attachment is obsolete: true
Attachment #589389 - Flags: review+
(Assignee)

Comment 50

6 years ago
Created attachment 589598 [details] [diff] [review]
Set permissions on upgrades. Patch v4.

Had to rebase due to order of landing.
Attachment #589389 - Attachment is obsolete: true
Attachment #589598 - Flags: review+

Comment 51

6 years ago
Looks like this didn't land yet...?
(Assignee)

Comment 52

6 years ago
Will be pushing this out today
(Assignee)

Comment 53

6 years ago
(I had to retest the rebased version on elm)
(Assignee)

Comment 54

6 years ago
Upgrade worked on elm, pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/82ec65fda578
https://hg.mozilla.org/mozilla-central/rev/82ec65fda578
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open after merge]
(Assignee)

Comment 56

6 years ago
Thanks edmorley, I'm going to keep this open for now though until Dao tries out the new patch.  There may be additional work needed in the context of this taks.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 57

6 years ago
*task. 

So this applies again:

> to Dão:
> 
> Please send me your
> "C:\ProgramData\Mozilla\logs\maintenanceservice-install.log" assuming you
> can still reproduce on the next update. 
> 
> It is possible it will just start working on the update after next because
> each update now tries to set the permissions on the service.  
> 
> In either case I should be able to tell from that log file.
(In reply to Brian R. Bondy [:bbondy] from comment #56)
> Thanks edmorley, I'm going to keep this open for now though until Dao tries
> out the new patch.  There may be additional work needed in the context of
> this taks.

Ah sorry, thought the leave open was leftover from the previous push :-)

Comment 59

6 years ago
Hello,

I was following this thread because I have the same problem.
I just updated Nightly. My maintenanceservice-install.log file now tell this :

Upgrading service if installed...
Warning: Could not set entries in ACL.  (1332)
Could not reset security ACE on service handle. It might not be possible to start the service. This error should never happen.  (1332)
Sending stop request...
Waiting for service stop...
Done waiting for service stop, last service state: 1
The new service binary was copied in.
Deleting the old file path on the next reboot: C:\Program Files (x86)\Mozilla Maintenance Service\maintenanceservice_tmp.exe.
The service was upgraded successfully
(Assignee)

Comment 60

6 years ago
Thanks for the info Vincent.  So this means the following function is failing:

SetEntriesInAcl ( http://msdn.microsoft.com/en-us/library/windows/desktop/aa379576%28v=vs.85%29.aspx )

with the following error:

ERROR_NONE_MAPPED - No mapping between account names and security IDs was done.
(Assignee)

Comment 61

6 years ago
Are you guys using an English version of Windows?

Control Panel -> Administrative Tools -> Computer Management
On the left bar:
System Tools -> Local Users and Groups -> Groups

Is there a a "Group Name" named "Users"?

I suspect the group name I'm trying to set needs to be localized before setting it.

Comment 62

6 years ago
I indeed use a French version of windows 7 64 bits.

I have a "group name" named "Utilisateurs" instead of "Users". Groups is also replaced by "Groupes".
(Assignee)

Comment 63

6 years ago
Thanks for confirming, I didn't realize the group names are named differently in localized versions of Windows.  This will be easy to fix by looking up the group name based on the SID. Thanks!
(Assignee)

Comment 64

6 years ago
Created attachment 590567 [details] [diff] [review]
Fix for looking up localized safe Users account name by SID. Patch v1.
Attachment #590567 - Flags: review?(robert.bugzilla)

Comment 65

6 years ago
Just for the record: I have the same issue on a German Win7 32; the log file says:

Upgrading service if installed...
Warning: Could not set entries in ACL.  (1332)
Could not reset security ACE on service handle. It might not be possible to start the service. This error should never happen.  (1332)
Sending stop request...
Waiting for service stop...
Done waiting for service stop, last service state: 1
The new service binary was copied in.
Deleting the old file path on the next reboot: C:\Program Files\Mozilla Maintenance Service\maintenanceservice_tmp.exe.
The service was upgraded successfully
(Assignee)

Comment 66

6 years ago
Thanks yup that's the same as the previous person.  It'll be fixed once the attached patch lands.  Thanks for the info.
Comment on attachment 590567 [details] [diff] [review]
Fix for looking up localized safe Users account name by SID. Patch v1.

damn... I should have caught this
Attachment #590567 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 68

6 years ago
This push to inbound corresponds to this patch "Fix for looking up localized safe Users account name by SID. Patch v1." (attachment 590567 [details] [diff] [review])

http://hg.mozilla.org/integration/mozilla-inbound/rev/2f9840b376a6

This task can be marked as resolved/fixed now once this is landed.
Comment on attachment 590567 [details] [diff] [review]
Fix for looking up localized safe Users account name by SID. Patch v1.

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

Drive by:

::: toolkit/components/maintenanceservice/serviceinstall.cpp
@@ +498,5 @@
> +    LOG(("Could not allocate SID memory.  (%d)\n", GetLastError()));
> +    return GetLastError();
> +  }
> +
> +  if (!CreateWellKnownSid(WinBuiltinUsersSid, NULL, sid, &SIDSize)) {

Are you sure you want the "Users" group and not "Authenticated Users"? (I am not sure either way.)

It seems like both include regular users, domain users, and guest, which is correct. But, does it seems like "Users" might also include "anonymous" logons in some versions of Windows--maybe only versions that are too old to matter?

http://www.windowsitpro.com/article/file-systems/should-you-use-the-authenticated-users-group-

@@ +518,5 @@
> +                         &accountNameSize, 
> +                         domainName, &domainNameSize, &accountType)) {
> +    LOG(("Warning: Could not lookup account Sid, will try Users.  (%d)\n",
> +         GetLastError()));
> +    wcscpy(accountName, L"Users");

It seems like a bad idea to me to do a fallback like this. In a non-English version of Windows, won't names like "Users" or "Authenticated Users", etc. be available (perhaps confusingly) for use by a different group or for a different individual user?
(Assignee)

Comment 70

6 years ago
Regarding users, I don't think there is a huge difference but I don't fully understand the difference. 

Authenticated users seems to be non controllable by admins (only by the OS).  Users seems like it has more control to the admin. 

This site says that domain users are added into Users, I'm not sure if they are added into Authenticated Users though:
http://ss64.com/nt/syntax-security_groups.html


> Users: A built-in group. After the initial installation of the operating
> system, the only member is the Authenticated Users 
> group. When a computer joins a domain, the Domain Users group is added 
> to the Users group on the computer. 
> Users can perform tasks such as running applications, using local and 
> network printers, shutting down the computer, 
> and locking the computer. Users can install applications that only they are
> allowed to use if the installation program 
> of the application supports per-user installation.

If it is a security recommendation to use Authenticated Users instead of Users please post another ticket or let me know and I can post it.  For anyone following along, the Users group does not imply unauthenticated.

--

Regarding removing the fallback to use Users if there is some strange error, I'm indifferent there.  I'll let rs confirm the change, and if he wants it I'll post another bug and put the patch there.
(Assignee)

Comment 71

6 years ago
I do think that changing to authenticated users has a chance of introducing regressions by the way.
https://hg.mozilla.org/mozilla-central/rev/2f9840b376a6
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Hardware: x86_64 → x86
Resolution: --- → FIXED

Comment 73

6 years ago
I don't know if the patch was included in the last update because the problem is still here for now :

Upgrading service if installed...
Warning: Could not set entries in ACL.  (1332)
Could not reset security ACE on service handle. It might not be possible to start the service. This error should never happen.  (1332)
Sending stop request...
Waiting for service stop...
Done waiting for service stop, last service state: 1
The new service binary was copied in.
Deleting the old file path on the next reboot: C:\Program Files (x86)\Mozilla Maintenance Service\maintenanceservice_tmp.exe.
The service was upgraded successfully

I think until we can verify that the patch works correctly this bug shouldn't be marked as resolved/fixed...
(Assignee)

Comment 74

6 years ago
It was not in last nights build.

Also you won't see it fixed until 1 day after the fix is in, so you shouldn't see it fixed on tomorrow's nightly. You should see it fixed after that though.

It is policy to mark bugs that land in mozilla-central as Resolved/Fixed (mozilla-central is where the Nightly builds you are using are from).  There is a Verified status that can be marked once it is verified.  If it doesn't work for some reason it will be set to status Reopened.

Comment 75

6 years ago
Ok thanks for the information. I will give a feedback in 2 days so.
(Assignee)

Comment 76

6 years ago
Great, thank you!

Comment 77

6 years ago
Looks good!

Upgrading service if installed...
User access was set successfully on the service.
Sending stop request...
Waiting for service stop...
Done waiting for service stop, last service state: 1
The new service binary was copied in.
Deleting the old file path on the next reboot: C:\Program Files (x86)\Mozilla Maintenance Service\maintenanceservice_tmp.exe.
The service was upgraded successfully
(Assignee)

Comment 78

6 years ago
Great! So tomorrows update should be silent ;)

Comment 79

6 years ago
I have the exact same log as Marco B. in last nightly (french one).

It seems to be OK!

Updated

6 years ago
Duplicate of this bug: 720230
Why doesn't UX update silently? When I hit "Apply Update" in the about window, UX restarts without applying the update. Then when I again click on "Apply Update" in the about window it applies the update after showing the UAC dialog.
(Assignee)

Comment 82

6 years ago
For security reasons the bins need to be signed for the service to actually be used. Since the UX branch does not sign the binaries (with a cert issued by a trusted root authority), the service fails and falls back to a normal update on the next app restart. Bug 722805 will disable the service completely for the UX branch and others.
You need to log in before you can comment on or make changes to this bug.