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

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Ian Melven :imelven]

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.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 2

[Brian R. Bondy [:bbondy]]

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?

Comment 3

[Ian Melven :imelven]

(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

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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.

Comment 5

[Brian R. Bondy [:bbondy]]

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.

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

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.

Comment 7

[Ian Melven :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)

Comment 8

[Brian R. Bondy [:bbondy]]

> 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.

Comment 9

[Brian R. Bondy [:bbondy]]

> 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.

Comment 10

[Ian Melven :imelven]

(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.

Comment 11

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2.

Fixed a couple of spacing issues and spelling.

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Please see the two previous comments.

Comment 15

[Brian R. Bondy [:bbondy]]

> 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?

Comment 16

[Brian R. Bondy [:bbondy]]

> 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.

Comment 17

[Brian R. Bondy [:bbondy]]

"but even in a user mode process,"
Sorry I meant to say: 
but even in an unelevated low integrity process with the priv disabled.

Comment 18

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3.

Implemented bsmith's review comments.

Comment 19

[Ian Melven :imelven]

(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.

Comment 20

[Lawrence Mandel [:lmandel] (use needinfo)]

(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.

Comment 21

[Brian R. Bondy [:bbondy]]

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

Comment 22

[Ian Melven :imelven]

(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.

Comment 23

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I filed bug 710428 and bug 710436 for the scheduled task idea and the "use a different user account" idea.

Comment 24

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4.

Rebased.

Comment 25

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

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!

Comment 26

[Brian R. Bondy [:bbondy]]

Attachment: Patch v5.

Implemented review comments.

Comment 27

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/mozilla-central/rev/c20c8ef8f0f4