Closed Bug 715910 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: Application Update, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: dao, Assigned: bbondy)

References

Details

Attachments

(3 files, 7 obsolete files)

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
Could you zip up and attach the contents of this directory?
C:\ProgramData\Mozilla\logs
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
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.
> 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.
(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
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".
(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.
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.
> 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.
Is it possible that using the -no-remote option affects this?
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.
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?
(In reply to Brian R. Bondy [:bbondy] from comment #12)
> You are using the m-c Nightly right?

Yes.
Summary: Nightly update keeps triggering the UAC prompt → Nightly updates are sometimes not using the service for updates even know the service is installed
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.
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.
Isn't there a risk that uninstalling and reinstalling could make the bug disappear and leave me with no way to reproduce it?
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.
(In reply to Brian R. Bondy [:bbondy] from comment #14)
> 6. Report back the contents of update.status.

pending-service
Still no logs in C:\ProgramData\Mozilla\logs right? 
And service wasn't attempted to be used right? (Went directly to the UAC prompt)
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.
I get "access denied" from "net start MozillaMaintenance".
If I use elevated cmd, it says something like "Mozilla Maintenance Service could not be started. The service didn't report an error."
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.
Registry looks good, I suspect you have the same as JK.
(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.
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.
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.
- 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)
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".
(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
Attached file maintenanceservice.log (obsolete) —
this file appeared about half an hour ago, apparently
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.
Minor fix for last patch.
Attachment #586710 - Attachment is obsolete: true
Attachment #586710 - Flags: review?(robert.bugzilla)
Attachment #586805 - Flags: review?(robert.bugzilla)
Attachment #586713 - Attachment is obsolete: true
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
Attached file maintenance service reg file (obsolete) —
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
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.
Attachment #587333 - Attachment is obsolete: true
Posted about sid0's issue here: Bug 716915
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
Thank you!
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 #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
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).
Attachment #587803 - Flags: review?(robert.bugzilla)
Attachment #586805 - Attachment description: Extra logging and always set. Patch v2. → (already landed) Extra logging. Patch v2.
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-
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+
> 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 :)
Implemented nits
Attachment #589378 - Attachment is obsolete: true
Attachment #589389 - Flags: review+
Had to rebase due to order of landing.
Attachment #589389 - Attachment is obsolete: true
Attachment #589598 - Flags: review+
Looks like this didn't land yet...?
Will be pushing this out today
(I had to retest the rebased version on elm)
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
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [leave open after merge]
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 → ---
*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 :-)
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
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.
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.
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".
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!
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
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+
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?
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.
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
Closed: 8 years ago8 years ago
Hardware: x86_64 → x86
Resolution: --- → FIXED
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...
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.
Ok thanks for the information. I will give a feedback in 2 days so.
Great, thank you!
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
Great! So tomorrows update should be silent ;)
I have the exact same log as Marco B. in last nightly (french one).

It seems to be OK!
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.
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.