Update winget server com security #4577

yao-msft · 2024-06-22T19:36:27Z

Change:

Explicitly set COM access permissions for packaged com invocations. Leave access permissions as default and do not register COM objects for manual invocation so that only RPC channel can be used for manual activation.
Update LaunchAndActivationString to allow Self, System, Built-in Admin and AppContainer
only, require at least MediumIL for non-AC.
Move Configuration to a separate COM server, use default permission.

A separate pr will be sent to update AppInstaller manifest.

Validation:
Validated manually with Microsoft Store invocation, Powershell invocation (elevated and non elevated), test sample code and Devhome invocation (on package management and configuration).

Also specifically validated Store invocation with Built-in Administrator sign-in (previously not working).

Microsoft Reviewers: Open in CodeFlow

JohnMcPMS

Looks good to me; will forward to experts.

brialmsft · 2024-06-24T17:59:08Z

src/WinGetServer/WinMain.cpp

+        nullptr, // Authentication services array
+        nullptr, // Reserved
+        RPC_C_AUTHN_LEVEL_PKT_INTEGRITY, // Authentication level. Validate client and data integrity.
+        RPC_C_IMP_LEVEL_IMPERSONATE, // Impersonation level. Can impersonate client.


To be clear: this says you're fine with whoever you call through COM impersonating you. Still subject to kernel-level restrictions on who's allowed to have an impersonate-level token, but it's unusually permissive to put this on your CoInitializeSecurity.

brialmsft · 2024-06-24T18:00:12Z

src/WinGetServer/WinMain.cpp

    // (A;;GA;;;UserSID) specifies access only for the user with the user SID (i.e. self).
    wil::unique_hlocal_security_descriptor securityDescriptor;
-    std::string securityDescriptorString = "S:(ML;;NW;;;HI)D:(A;;GA;;;" + userSID + ")";
+    std::string securityDescriptorString = "S:(ML;;NRNWNX;;;HI)D:(A;;GA;;;" + userSID + ")";


Can't you use PS instead of calculating an explicit userSID?

That depends on whether/how RPC (and ALPC if this is passed through for the port object) call an AccessCheck* API that supports mapping a SID to PS. If they don't use one of these APIs and/or they fail to provide a mapping for PS, then PS isn't meaningful in the SD.

brialmsft · 2024-06-24T18:01:08Z

src/AppInstallerCLIPackage/Package.appxmanifest

@@ -61,7 +61,7 @@
        </uap5:Extension>
        <com:Extension Category="windows.comServer">
          <com:ComServer>
-            <com:ExeServer Executable="WinGetServer\WindowsPackageManagerServer.exe" DisplayName="Windows Package Manager Server" LaunchAndActivationPermission="O:SYG:SYD:(A;;11;;;WD)(A;;11;;;RC)(A;;11;;;AC)(A;;11;;;AN)S:P(ML;;NX;;;S-1-16-0)">
+            <com:ExeServer Executable="WinGetServer\WindowsPackageManagerServer.exe" DisplayName="Windows Package Manager Server" LaunchAndActivationPermission="O:SYG:SYD:(A;;11;;;WD)(A;;11;;;AC)S:P(ML;;NX;;;S-1-16-4096)">


Sanity check: you're fine with allowing non-AppContainer low IL to activate this?

This is needed to allow AppContainer to activate this, right? (due to COM's special handling of LaunchPermission without a SACL to achieve no execute up medium by default for >= medium IL servers)

You can configure a SACL with a Medium mandatory label. This will block non-AppContainer low IL, but still allow AppContainer because you have a packaged SID in the DACL (ALL_APPLICATION_PACKAGES).

I just tested setting to 8192 and calls from AC still works. I'll set to 8192 as suggested.

Ah, I forgot we just need a SACL to tell activation "we're handling that detail", using medium (8192) to allow only medium and above non-AC processes makes sense. Note that this is a new restriction though, existing non-AC < medium IL clients will be broken.

JohnMcPMS · 2024-06-24T18:36:27Z

src/AppInstallerCLIPackage/Package.appxmanifest

@@ -61,7 +61,7 @@
        </uap5:Extension>
        <com:Extension Category="windows.comServer">
          <com:ComServer>
-            <com:ExeServer Executable="WinGetServer\WindowsPackageManagerServer.exe" DisplayName="Windows Package Manager Server" LaunchAndActivationPermission="O:SYG:SYD:(A;;11;;;WD)(A;;11;;;RC)(A;;11;;;AC)(A;;11;;;AN)S:P(ML;;NX;;;S-1-16-0)">
+            <com:ExeServer Executable="WinGetServer\WindowsPackageManagerServer.exe" DisplayName="Windows Package Manager Server" LaunchAndActivationPermission="O:SYG:SYD:(A;;11;;;WD)(A;;11;;;AC)S:P(ML;;NX;;;S-1-16-4096)">


Set to medium, discuss in person.

hart-wilson · 2024-06-24T18:37:17Z

src/AppInstallerCLIPackage/Package.appxmanifest

@@ -61,7 +61,7 @@
        </uap5:Extension>
        <com:Extension Category="windows.comServer">
          <com:ComServer>
-            <com:ExeServer Executable="WinGetServer\WindowsPackageManagerServer.exe" DisplayName="Windows Package Manager Server" LaunchAndActivationPermission="O:SYG:SYD:(A;;11;;;WD)(A;;11;;;RC)(A;;11;;;AC)(A;;11;;;AN)S:P(ML;;NX;;;S-1-16-0)">
+            <com:ExeServer Executable="WinGetServer\WindowsPackageManagerServer.exe" DisplayName="Windows Package Manager Server" LaunchAndActivationPermission="O:SYG:SYD:(A;;11;;;WD)(A;;11;;;AC)S:P(ML;;NX;;;S-1-16-4096)">


WD = Everyone has always been wrong here. The default access permissions without a call to CoInitializeSecurity grant access to PS, SY, and BA for all server processes, plus AC for <= Medium IL server processes, so despite the extremely permissive LaunchAndActivationPermission the effective permissions of activation + calls only let this work for PS, SY, or BA (including AppContainers running as any account that matched one of these) for the usual case of Medium IL server process. My main concern is not to duplicate the overly permissive WD to the access permissions, but:

Choosing LaunchAndActivationPermission to be as narrow as possible would reduce the risk of accidentally granting access to callers who shouldn't have it if/when you find a reason the access permissions need to be loosened.

Setting this closer to the default would make it clearer what differences this server needs relative to the default.

My recommendation would be "O:SYG:SYD:(A;;11;;;IU)(A;;11;;;SY)(A;;11;;;BA)(A;;11;;;AC)S:P(ML;;NX;;;S-1-16-4096)". If Packaged COM supported the use of PS in these security descriptors that would be even better (matching the access permissions), but they don't, so the default LaunchAndActivationPermissions granting access to IU, SY, and BA are a reasonable and longstanding compromise given this known limitation. Defaults + allow AC would be the standard permissions for servers that allow AC, and the recommendation is what you need in LaunchAndActivationPermission to achieve this.

hart-wilson · 2024-06-24T18:40:12Z

src/AppInstallerCLIPackage/Package.appxmanifest

@@ -61,7 +61,7 @@
        </uap5:Extension>
        <com:Extension Category="windows.comServer">
          <com:ComServer>
-            <com:ExeServer Executable="WinGetServer\WindowsPackageManagerServer.exe" DisplayName="Windows Package Manager Server" LaunchAndActivationPermission="O:SYG:SYD:(A;;11;;;WD)(A;;11;;;RC)(A;;11;;;AC)(A;;11;;;AN)S:P(ML;;NX;;;S-1-16-0)">
+            <com:ExeServer Executable="WinGetServer\WindowsPackageManagerServer.exe" DisplayName="Windows Package Manager Server" LaunchAndActivationPermission="O:SYG:SYD:(A;;11;;;WD)(A;;11;;;AC)S:P(ML;;NX;;;S-1-16-4096)">


This is needed to allow AppContainer to activate this, right? (due to COM's special handling of LaunchPermission without a SACL to achieve no execute up medium by default for >= medium IL servers)

hart-wilson · 2024-06-24T18:48:29Z

src/AppInstallerCLIPackage/Package.appxmanifest

              <com:Class Id ="8EF324ED-367C-4880-83E5-BB2ABD0B72F6" DisplayName="DownloadOptions Server">
              </com:Class>
              <com:Class Id ="6484A61D-50FA-41F0-B71E-F4370C6EB37C" DisplayName="AuthenticationArguments Server">
              </com:Class>
            </com:ExeServer>
          </com:ComServer>
        </com:Extension>
+        <com:Extension Category="windows.comServer">
+          <com:ComServer>
+            <com:ExeServer Executable="WinGetServer\WindowsPackageManagerServer.exe" DisplayName="Windows Package Manager Configuration Server" LaunchAndActivationPermission="O:SYG:SYD:(A;;11;;;WD)S:P(ML;;NX;;;S-1-16-8192)">


My recommendation for servers that don't allow AC would be to either omit the LaunchAndActivationPermission attribute to get the default, or explicitly specify a security descriptor that is the same as the default. (This is due to the same concerns about WD I mentioned for the other one.) Default here (but restricted down to local activation permissions, which are the only possible effective permissions for Packaged COM in per-user packages anyway) would be "O:SYG:SYD:(A;;11;;;IU)(A;;11;;;SY)(A;;11;;;BA)". Adding the NX ACE in the SACL has limited effect because the binding of servers without a SACL is specially handled, but it does prevent < Medium IL non-AC clients from launching a server instance that they will never be able to talk to.

hart-wilson · 2024-06-24T18:57:47Z

src/WinGetServer/WinMain.cpp

    // (A;;GA;;;UserSID) specifies access only for the user with the user SID (i.e. self).
    wil::unique_hlocal_security_descriptor securityDescriptor;
-    std::string securityDescriptorString = "S:(ML;;NW;;;HI)D:(A;;GA;;;" + userSID + ")";
+    std::string securityDescriptorString = "S:(ML;;NRNWNX;;;HI)D:(A;;GA;;;" + userSID + ")";


Nit: I would check which of read/write/execute rights RPC maps to the object-specific rights flags it checks for in access checks of clients. Any of these you have added here but which are not meaningful in these access checks are just introducing confusion about how this is locked down as an RPC server.

Alternatively, can the whole thing just be "D:(A;;GA;;;BA)"? I.e. are there any clients that need to be allowed that are High IL, and also the same user, but don't have the BA group effective? That would have to be due to a very unusual use of token restriction on an admin account's full token, e.g. explicitly marking BA DenyOnly without reducing the IL (normally restriction reduces IL and gets more privileged groups automatically marked DenyOnly as a side effect).

I'll revert the added access rights. Regarding why we use specific sid, the original intention was to ensure only whoever manually activated the server can access it.

hart-wilson · 2024-06-24T18:59:27Z

src/WinGetServer/WinMain.cpp

    // (A;;GA;;;UserSID) specifies access only for the user with the user SID (i.e. self).
    wil::unique_hlocal_security_descriptor securityDescriptor;
-    std::string securityDescriptorString = "S:(ML;;NW;;;HI)D:(A;;GA;;;" + userSID + ")";
+    std::string securityDescriptorString = "S:(ML;;NRNWNX;;;HI)D:(A;;GA;;;" + userSID + ")";


That depends on whether/how RPC (and ALPC if this is passed through for the port object) call an AccessCheck* API that supports mapping a SID to PS. If they don't use one of these APIs and/or they fail to provide a mapping for PS, then PS isn't meaningful in the SD.

hart-wilson · 2024-06-24T19:06:17Z

src/WinGetServer/WinMain.cpp

+{
+    wil::unique_hlocal_security_descriptor securityDescriptor;
+    // Allow Everyone and AppContainer access. 3 is COM_RIGHTS_EXECUTE | COM_RIGHTS_EXECUTE_LOCAL
+    std::string securityDescriptorString = "O:SYG:SYD:(A;;3;;;WD)(A;;3;;;AC)";


This is opening up permissions far more than is necessary to fix the bug as I understand it (High IL server processes that ran that way in a UAC-disabled account like the built-in Administrator account if enabled, rather than due to elevation, are not granting AC the way they would due to the defaults if they were running at Medium IL as usual). COM defaults for Medium IL processes grant PS, SY, BA, and AC. If you want the same permissions for both the usual Medium IL case and the less common High IL case, grant access to only those SIDs.

JohnMcPMS · 2024-06-24T22:09:54Z

src/WinGetServer/WinMain.cpp

+HRESULT InitializeComSecurity()
+{
+    wil::unique_hlocal_security_descriptor securityDescriptor;
+    // Allow Everyone and AppContainer access. 3 is COM_RIGHTS_EXECUTE | COM_RIGHTS_EXECUTE_LOCAL


Comment is no longer correct.

hart-wilson · 2024-06-24T22:10:39Z

src/WinGetServer/WinMain.cpp

+HRESULT InitializeComSecurity()
+{
+    wil::unique_hlocal_security_descriptor securityDescriptor;
+    // Allow Everyone and AppContainer access. 3 is COM_RIGHTS_EXECUTE | COM_RIGHTS_EXECUTE_LOCAL


Comment is out of date

yao-msft · 2024-06-24T22:37:45Z

To unblock 1.8 servicing release process, I'll merge and not wait for build since it's only updating a comment from last success build and spelling task passed.

Change: - Explicitly set COM access permissions for packaged com invocations. Leave access permissions as default and do not register COM objects for manual invocation so that only RPC channel can be used for manual activation. - Update LaunchAndActivationString to allow Self, System, Built-in Admin and AppContainer only, require at least MediumIL for non-AC. - Move Configuration to a separate COM server, use default permission. A separate pr will be sent to update AppInstaller manifest. Validation: Validated manually with Microsoft Store invocation, Powershell invocation (elevated and non elevated), test sample code and Devhome invocation (on package management and configuration). Also specifically validated Store invocation with Built-in Administrator sign-in (previously not working).

Update winget server com security

12d58cc

yao-msft requested a review from a team as a code owner June 22, 2024 19:36

This comment has been minimized.

Sign in to view

spelling

dd5b289

JohnMcPMS reviewed Jun 24, 2024

View reviewed changes

brialmsft reviewed Jun 24, 2024

View reviewed changes

JohnMcPMS reviewed Jun 24, 2024

View reviewed changes

hart-wilson suggested changes Jun 24, 2024

View reviewed changes

microsoft-github-policy-service bot assigned yao-msft Jun 24, 2024

microsoft-github-policy-service bot added Needs-Author-Feedback Issue needs attention from issue or PR author Needs-Attention Issue needs attention from Microsoft and removed Needs-Author-Feedback Issue needs attention from issue or PR author labels Jun 24, 2024

yao-msft added 2 commits June 24, 2024 12:47

Comments. Set recommended acl.

2e6410f

8192

a2819ae

JohnMcPMS reviewed Jun 24, 2024

View reviewed changes

hart-wilson approved these changes Jun 24, 2024

View reviewed changes

brialmsft approved these changes Jun 24, 2024

View reviewed changes

Update comments

62907a8

JohnMcPMS approved these changes Jun 24, 2024

View reviewed changes

yao-msft merged commit c555273 into microsoft:master Jun 24, 2024
4 of 7 checks passed

microsoft-github-policy-service bot removed the Needs-Attention Issue needs attention from Microsoft label Jun 24, 2024

ryfu-msft mentioned this pull request Jun 25, 2024

Cherry pick servicing fixes to 1.8 #4580

Merged

yao-msft deleted the comsec branch June 25, 2024 19:05

yao-msft mentioned this pull request Jun 28, 2024

WebView2 dependency in MSIX fails to install on Azure Windows 11 22H2 Pro Gen2 #4473

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update winget server com security #4577

Update winget server com security #4577

yao-msft commented Jun 22, 2024 •

edited

Loading

This comment has been minimized.

JohnMcPMS left a comment

brialmsft Jun 24, 2024

brialmsft Jun 24, 2024

hart-wilson Jun 24, 2024

brialmsft Jun 24, 2024

hart-wilson Jun 24, 2024

brialmsft Jun 24, 2024

yao-msft Jun 24, 2024

hart-wilson Jun 24, 2024

JohnMcPMS Jun 24, 2024

hart-wilson Jun 24, 2024

hart-wilson Jun 24, 2024

hart-wilson Jun 24, 2024

hart-wilson Jun 24, 2024

yao-msft Jun 24, 2024

hart-wilson Jun 24, 2024

hart-wilson Jun 24, 2024

JohnMcPMS Jun 24, 2024

hart-wilson Jun 24, 2024

yao-msft commented Jun 24, 2024

Update winget server com security #4577

Update winget server com security #4577

Conversation

yao-msft commented Jun 22, 2024 • edited Loading

Microsoft Reviewers: Open in CodeFlow

This comment has been minimized.

JohnMcPMS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yao-msft commented Jun 24, 2024

yao-msft commented Jun 22, 2024 •

edited

Loading