Closed Bug 708778 Opened 12 years ago Closed 12 years ago

Updater service used on Windows should drop as many permissions as possible

Categories

(Toolkit :: Application Update, defect)

x86
Windows Vista
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: imelven, Assigned: bbondy)

References

Details

(Whiteboard: [see comment 4 and 5 for follow-up bugs that may need to be filed])

Attachments

(1 file, 4 obsolete files)

During the discussion of the silent update service, the point was raised that the service, although running as SYSTEM, should drop whatever permissions possible - this could be via creating a new token that only has the needed privileges and using this to execute the elevated updater.exe.

This follows the general principle of least privilege.
See Also: → 481815
Note that not only the service should do this, but updater.exe should do this as well. Though, each may have different sets of privileges they require, and thus may have different sets of privileges to drop.
Just to clarify, do you mean token privileges  which are set via AdjustTokenPrivileges such as in this example:
http://msdn.microsoft.com/en-us/library/aa446619%28VS.85%29.aspx

Or do you mean something else?
(In reply to Brian R. Bondy [:bbondy] from comment #2)
> Just to clarify, do you mean token privileges  which are set via
> AdjustTokenPrivileges such as in this example:
> http://msdn.microsoft.com/en-us/library/aa446619%28VS.85%29.aspx
> 
> Or do you mean something else?

i took this to mean token privileges as described in that link, yes
(In reply to Brian R. Bondy [:bbondy] from comment #2)
> Just to clarify, do you mean token privileges  which are set via
> AdjustTokenPrivileges such as in this example:
> http://msdn.microsoft.com/en-us/library/aa446619%28VS.85%29.aspx
> 
> Or do you mean something else?

Token privileges are the thing that I know of. I do not know if there are other things that can be done; if so, we can file follow-up bugs for them.

This doesn't apply only to the service-based installation, though that is the case where we are most concerned about, due to the automatic nature of the updates.

Note also that this applies to other platforms than Windows; e.g. Linux with selinux (at least) has a capability system, and capabilities can be dropped. But, IMO, that should be done in a Linux-specific bug.
Whiteboard: [see comment 4 for follow-up bugs that may need to be filed]
Assignee: nobody → netzen
bsmith wrote:
> Note also that this applies to other platforms than Windows; e.g. Linux with 
> selinux (at least) has a capability system, and capabilities can be dropped. But,
> IMO, that should be done in a Linux-specific bug.

Since other platforms would have completely different solutions than Windows, that should be posted separately.

If security is concerned about other platforms, please post follow up bugs for the other concerns, the future patch in this bug will only address the Windows specific permissions in the service and in updater.exe.
Whiteboard: [see comment 4 for follow-up bugs that may need to be filed] → [see comment 4 and 5 for follow-up bugs that may need to be filed]
Attached patch Patch v1. (obsolete) — Splinter Review
This patch removes all privs from the token on the service.
Any process started from the service currently does not specificy a token, so it will itself use the token of the service. 
Note: some privs may not be able to be removed, but there is nothing we can do but try to remove them.
Attachment #580755 - Flags: review?(robert.bugzilla)
Attachment #580755 - Flags: review?(imelven)
Comment on attachment 580755 [details] [diff] [review]
Patch v1.

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

looks good to me, a few small nits.

::: toolkit/components/maintenanceservice/maintenanceservice.cpp
@@ +233,5 @@
>    // Setup logging, and backup the old logs
>    WCHAR updatePath[MAX_PATH + 1];
>    if (GetLogDirectoryPath(updatePath)) {
>      BackupOldLogs(updatePath, LOGS_TO_KEEP);
>      LogInit(updatePath, L"maintenanceservice.log");

SvcMain(DWORD dwArgc, LPWSTR *lpszArgv) went away ?

::: toolkit/mozapps/update/common/uachelper.cpp
@@ +138,5 @@
>    return hNewLinkedToken;
>  }
> +
> +/**
> + * Enables or disables a privlege for the specified token.

nit : privilege (more below)

@@ +167,5 @@
> +
> +  return GetLastError()  == ERROR_SUCCESS;
> +}
> +
> +/**

consider documenting that if no token is passed in the
current process' token will be used.

@@ +173,5 @@
> + * drop the privilege. 
> + * 
> + * @param  token        The token to adjust the privlege on. 
> + *         Pass NULL for current token.
> + * @param  uneededPrivs An array of uneeded privileges.

nit : unneeded

::: toolkit/mozapps/update/common/uachelper.h
@@ +47,5 @@
> +  static BOOL DropAllPrivileges(HANDLE token);
> +
> +private:
> +  static BOOL SetPrivilege(HANDLE token, LPCTSTR privs, BOOL enable);
> +  static BOOL DropUneededPrivileges(HANDLE token, 

nit: Unneeded (in function name and arg unneededPrivs)
Attachment #580755 - Flags: review?(imelven) → feedback+
> SvcMain(DWORD dwArgc, LPWSTR *lpszArgv) went away ?

No it is still there, the line you are seeing is to give context and is part of the patch file.
> consider documenting that if no token is passed in the
> current process' token will be used.

For that function in particular it is not the case.  For the wrapper functions though it is the case and it is documented for those.
(In reply to Brian R. Bondy [:bbondy] from comment #9)
> > consider documenting that if no token is passed in the
> > current process' token will be used.
> 
> For that function in particular it is not the case.  For the wrapper
> functions though it is the case and it is documented for those.

yes, i see that now, sorry about that - and as for the SvcMain (comment 8) - that was colored differently in splinter - i am still getting used to using that.  apologies again.
Attached patch Patch v2. (obsolete) — Splinter Review
Fixed a couple of spacing issues and spelling.
Attachment #580755 - Attachment is obsolete: true
Attachment #580755 - Flags: review?(robert.bugzilla)
Attachment #581144 - Flags: review?(robert.bugzilla)
Comment on attachment 580755 [details] [diff] [review]
Patch v1.

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

(In reply to Brian R. Bondy [:bbondy] from comment #6)
> This patch removes all privs from the token on the service.
> Any process started from the service currently does not specificy a token,
> so it will itself use the token of the service. 

It is still useful to drop the privileges in updater.exe, for the case where updater.exe is not run from the service, but is instead run the old way or executed by a malicious (unprivileged) application.

The documentation at [1] mentions several types of privileges:
http://msdn.microsoft.com/en-us/library/windows/desktop/aa375728%28v=vs.85%29.aspx#privilege_constants

It seems like this patch does correctly drop the privileges that we can drop at the per-process level. However, we should follow up this work with some analysis of the per-user privileges (i.e. should we be running as LocalSystem or someone else) and per-directory privileges (including mandatory integrity control) to ensure that we are making the proper tradeoffs on those areas. (No action for this bug though.)

::: toolkit/mozapps/update/common/uachelper.cpp
@@ +41,3 @@
>  
>  typedef BOOL (WINAPI *LPWTSQueryUserToken)(ULONG, PHANDLE);
> +LPCTSTR UACHelper::AllPrivs[] = { 

Please sort this list in alphabetical order, so that it can be cross-referenced with the documentation at
http://msdn.microsoft.com/en-us/library/windows/desktop/bb530716%28v=vs.85%29.aspx

@@ +62,5 @@
> +  SE_SHUTDOWN_NAME,
> +  SE_DEBUG_NAME,
> +  SE_AUDIT_NAME,
> +  SE_SYSTEM_ENVIRONMENT_NAME,
> +  SE_CHANGE_NOTIFY_NAME,

Don't you need SE_CHANGE_NOTIFY_NAME in order to monitor the workfile directory?

If so, this would be a privilege that the service needs but updater.exe doesn't.

Either way, please document why you do or do not need this particular privilege.

::: toolkit/mozapps/update/common/uachelper.h
@@ +43,5 @@
>  public:
>    static BOOL IsVistaOrLater();
>    static HANDLE OpenUserToken(DWORD sessionID);
>    static HANDLE OpenLinkedToken(HANDLE token);
> +  static BOOL DropAllPrivileges(HANDLE token);

Please rename this to DropAllKnownPrivileges, because doesn't necessarily drop every privilege. (Later versions of Windows may define new privileges that we don't drop here.)

Though, it seems like "Drop All" might not be appopriate at all; see my other review comments.
Attachment #580755 - Flags: feedback-
By the way, when I looked over this patch, I did not review the code. I only looked at the set of privileges being dropped.
Comment on attachment 581144 [details] [diff] [review]
Patch v2.

Please see the two previous comments.
Attachment #581144 - Flags: feedback-
> It seems like this patch does correctly drop the privileges that we can drop at
> the per-process level. However, we should follow up this work with some analysis 
> of the per-user privileges (i.e. should we be running as LocalSystem or someone 
> else) and per-directory privileges (including mandatory integrity control) to 
> ensure that we are making the proper tradeoffs on those areas. (No action for 
> this bug though.)

Ian could you post 1 or more bugs depending on what the concerns are exactly here?
Or if you are not sure what exactly it entails maybe just post a bug to investigate these things?
> Don't you need SE_CHANGE_NOTIFY_NAME in order to monitor the workfile directory?
> If so, this would be a privilege that the service needs but updater.exe doesn't.
> Either way, please document why you do or do not need this particular privilege.

It sounds like I would need it from the MSDN doc, but even in a user mode process, when I disable this it still allows me to read directory changes to get notifications of changed files.

I know for sure I'm disabling the priv correctly because I tested with disabling SE_BACKUP_NAME
 and then calling RegSaveKey, it fails with privilege not held error code (1314) when I have the priv disabled and succeeds when I have the priv enabled.

I don't really have any inside information on why it's not needed as I don't have access to their source code, but I will comment that from my testing this is not needed for ReadDirectoryChanges.
"but even in a user mode process,"
Sorry I meant to say: 
but even in an unelevated low integrity process with the priv disabled.
Attached patch Patch v3. (obsolete) — Splinter Review
Implemented bsmith's review comments.
Attachment #581144 - Attachment is obsolete: true
Attachment #581144 - Flags: review?(robert.bugzilla)
Attachment #581282 - Flags: review?(robert.bugzilla)
(In reply to Brian R. Bondy [:bbondy] from comment #15)
> > It seems like this patch does correctly drop the privileges that we can drop at
> > the per-process level. However, we should follow up this work with some analysis 
> > of the per-user privileges (i.e. should we be running as LocalSystem or someone 
> > else) and per-directory privileges (including mandatory integrity control) to 
> > ensure that we are making the proper tradeoffs on those areas. (No action for 
> > this bug though.)
> 
> Ian could you post 1 or more bugs depending on what the concerns are exactly
> here?
> Or if you are not sure what exactly it entails maybe just post a bug to
> investigate these things?

My personal opinion is that if we want to not use LocalSystem and worry about what directories the process has access to, i would suggest focusing on an analysis of what it would take to move to an update process that doesn't require elevation of privileges, similar to what some other browsers have implemented.
(In reply to Ian Melven :imelven from comment #19)
> My personal opinion is that if we want to not use LocalSystem and worry
> about what directories the process has access to, i would suggest focusing
> on an analysis of what it would take to move to an update process that
> doesn't require elevation of privileges, similar to what some other browsers
> have implemented.

My understanding is that Chrome installs into a user's profile, negating the need to have elevated permissions to perform updates. I would guess that IE can obtain whatever permission it needs as long as updates are handled by the Windows update mechanism.
I should note that using a system or domain username is not possible because each would be a different username with a different password.
The username/password must be set at service creation time (http://msdn.microsoft.com/en-us/library/windows/desktop/ms682450%28v=vs.85%29.aspx).

So I think the only other options are:
- LocalSystem
- NT AUTHORITY\LocalService
- AUTHORITY\NetworkService

It seems there's also virtual accounts, but I don't know anything about them.
http://technet.microsoft.com/en-us/library/dd548356%28WS.10%29.aspx
(In reply to Lawrence Mandel [:lmandel] from comment #20)
> 
> My understanding is that Chrome installs into a user's profile, negating the
> need to have elevated permissions to perform updates. I would guess that IE
> can obtain whatever permission it needs as long as updates are handled by
> the Windows update mechanism.

yes, that's my understanding also re Chrome - there are some registry keys for Chrome under HKLM, i assume these are created when elevated on initial install. Windows Update either runs elevated via a UAC prompt or uses the Windows Update service, which runs as LocalSystem.
I filed bug 710428 and bug 710436 for the scheduled task idea and the "use a different user account" idea.
Attached patch Patch v4. (obsolete) — Splinter Review
Rebased.
Attachment #581282 - Attachment is obsolete: true
Attachment #581282 - Flags: review?(robert.bugzilla)
Attachment #581888 - Flags: review?(robert.bugzilla)
Comment on attachment 581888 [details] [diff] [review]
Patch v4.

It is nice reviewing smallish patches! 

>diff --git a/toolkit/components/maintenanceservice/maintenanceservice.cpp b/toolkit/components/maintenanceservice/maintenanceservice.cpp
>--- a/toolkit/components/maintenanceservice/maintenanceservice.cpp
>+++ b/toolkit/components/maintenanceservice/maintenanceservice.cpp
>@@ -241,16 +242,20 @@ SvcMain(DWORD dwArgc, LPWSTR *lpszArgv)
> {
>   // Setup logging, and backup the old logs
>   WCHAR updatePath[MAX_PATH + 1];
>   if (GetLogDirectoryPath(updatePath)) {
>     BackupOldLogs(updatePath, LOGS_TO_KEEP);
>     LogInit(updatePath, L"maintenanceservice.log");
>   }
> 
>+  // Disable every priv we don't need. Any process we start 
>+  // from this service with CreateProcess will use our same token.
>+  UACHelper::DropAllKnownPrivileges(NULL);
I kind of prefer DisablePrivileges since we define which privileges to drop and as pointed out new ones can be added and we may not be able to drop some privileges. This way the names won't have to change if this happens.

>diff --git a/toolkit/mozapps/update/common/uachelper.cpp b/toolkit/mozapps/update/common/uachelper.cpp
>--- a/toolkit/mozapps/update/common/uachelper.cpp
>+++ b/toolkit/mozapps/update/common/uachelper.cpp
>@@ -32,19 +32,63 @@
>...
>+// See the MSDN documentation with title: Privilege Constants
>+// At the time of this writing, this documentation is located at: 
>+// http://msdn.microsoft.com/en-us/library/windows/desktop/bb530716%28v=vs.85%29.aspx
>+LPCTSTR UACHelper::AllKnownPrivs[] = { 
nit: How about PrivilegesToDisable or PrivsToDisable.

If you go with this please exchange Drop with Disable elsewhere and adjust comments accordingly. I'm ok with keeping Drop though.

>+  SE_ASSIGNPRIMARYTOKEN_NAME,
>+  SE_AUDIT_NAME,
>+  SE_BACKUP_NAME,
>+  // From testing, even a low integrity process with the following
>+  // priv disabled, ReadDirectoryChanges still succeeds.
nit: From testing, ReadDirectoryChanges still succeeds even with a low integrity process with the following privilege disabled.

>+/**
>+ * Enables or disables a privilege for the specified token.
>+ *
>+ * @param  token  The token to adjust the privilege on.
>+ * @param  piv    The privilege to adjust.
s/piv/priv/

>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
>@@ -1587,16 +1587,20 @@ int NS_main(int argc, NS_tchar **argv)
>   if (NS_tchdir(argv[2]) != 0) {
>     return 1;
>   }
> 
>   // The directory containing the update information.
>   gSourcePath = argv[1];
> 
> #ifdef XP_WIN
>+  // Disable every priv we don't need. Any process we start 
>+  // from with CreateProcess will use our same token.
s/priv/privilege/ comments are cheap

Processes started using CreateProcess will use the same token as this process with these privileges disabled.

Thanks and nicely done!
Attachment #581888 - Flags: review?(robert.bugzilla) → review+
Attached patch Patch v5.Splinter Review
Implemented review comments.
Attachment #581888 - Attachment is obsolete: true
Attachment #581995 - Flags: review+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Blocks: 718450
Blocks: 719066
You need to log in before you can comment on or make changes to this bug.