Disable local manifest by default #1453

yao-msft · 2021-09-10T17:21:58Z

Change

Due to security concerns, this change disables local manifest files by default. User can still turn on local manifest files through group policy, or through running 'winget settings --enable LocalManifestFiles' as administrator.

Validation

Added tests. Also tested manually.

Microsoft Reviewers: Open in CodeFlow

denelon · 2021-09-10T18:36:45Z

@ImJoakim
@ItzLevvie
@jedieaston
@KaranKad
@OfficialEsco
@quhxl

Hey, I just wanted to let you know this change is coming. The PowerShell scripts will likely need to be modified for testing local manifest files once this change is released. We had a security review, and the decision was made to make this disabled by default. Most users will not need to execute local manifest files. This requires administrative access to modify, so it's not in the local settings file.

OfficialEsco · 2021-09-10T19:00:50Z

Most users will not need to execute local manifest files.

Seems fair since there is just about max 10 active contributors that might have to test a package locally..
Might have to make it a bit more user friendly for the people who submit one or two manifests

[ ] Have you tested your manifest locally with winget install --manifest <path>?

What if there is a way to temporary allow local manifests instead of toggling it once to use it one time?

denelon · 2021-09-10T20:02:51Z

That's an interesting approach. Are you thinking about having the command just work for the "next" execution rather than an "on/off" switch?

yao-msft · 2021-09-10T20:06:05Z

What if there is a way to temporary allow local manifests instead of toggling it once to use it one time?

For now, the toggle setting stays until it's changed explicitly by user later. If users want to test local manifests, they'll run 'winget settings --enable LocalManifestFiles', after the testing is done, they could use 'winget settings --disable LocalManifestFiles' to disable it.

If there's need in the future, we can add an option to make the toggle be effective only once.

OfficialEsco · 2021-09-10T20:16:20Z

Are you thinking about having the command just work for the "next" execution rather than an "on/off" switch?

I have PowerShell's execution policy on my mind, however since winget is a appx package there is a lot less to worry about, we do need a on/off switch, however i believe it would be nice for people to have a next or temporary switch, i was thinking of Set-ExecutionPolicy -Scope Process, however since it requires Administrator to change the policy winget install would run as Administrator which we do not prefer when users test their manifest..

yao-msft · 2021-09-10T20:24:51Z

Given the current code structure, I think I can easily add an option 'winget settings --enable LocalManifestFiles --once', which will allow next local manifest invocation(could be either non elevated or elevated), then the feature is disabled after invocation.

Does that sort of satisfy the "temporarily enable" ask? Thanks.

jedieaston · 2021-09-10T20:25:53Z

Two questions:

Can we have a GPO to configure this?
What was the security concern brought up by local manifests? I don't understand what you can do with winget that you can't do with Invoke-WebRequest or a browser. It seems like organizations concerned about this vector would already be using AppLocker or something since that applies to all executables a user tries to run, not just ones they wrote a winget manifest for. I guess it doesn't really matter, but I'm curious.

OfficialEsco · 2021-09-10T20:29:25Z

(could be either non elevated or elevated)

Doesn't this defeat the purpose of a security risk?

If can answer, what is the security concerns you guys are thinking about?

yao-msft · 2021-09-10T20:44:00Z

Can we have a GPO to configure this?

Yes, there's an existing one named LocalManifestFiles.

winget-cli/doc/admx/DesktopAppInstaller.admx

Line 46 in fee797b

    
           <policy name="EnableLocalManifestFiles" class="Machine" displayName="$(string.EnableLocalManifestFiles)" explainText="$(string.EnableLocalManifestFilesExplanation)" key="Software\Policies\Microsoft\Windows\AppInstaller" valueName="EnableLocalManifestFiles">

Doesn't this defeat the purpose of a security risk?

The 'winget settings --enable LocalManifestFiles --once' still needs to be invoked as elevated. The next run with local manifest input could be non elevated.

Regarding questions about what security concerns, I think mainly we don't want to be the entrypoint to install random apps potentially without user knowledge, but I guess I'll let @denelon elaborate.

jedieaston · 2021-09-10T20:47:07Z

Shoot, that's what I get for not checking the admx file again. I remember that being there now, thanks!

JohnMcPMS · 2021-09-11T00:11:50Z

Given the current code structure, I think I can easily add an option 'winget settings --enable LocalManifestFiles --once', which will allow next local manifest invocation(could be either non elevated or elevated), then the feature is disabled after invocation.

Does that sort of satisfy the "temporarily enable" ask? Thanks.

That can't work; it would require admin to set the "consumed" state or it would be possible to just set it back to unconsumed.

While we don't run with any more permissions than a potential attacker, we don't want to make it any easier to download and run some binary either. Given that 99% of users will never run winget (even that is optimistic when we we will have a billion machines with the code at some point), and then 99% of the ones who do will never need the local manifest, it is just safer to disable this potential attack surface by default. Happy to make it as easy as possible for anyone to turn it back on if they want, so long as they have the permissions.

jedieaston · 2021-09-11T00:21:33Z

Alrighty, that makes sense. Just going to have to be another consideration when running local manifest tests.

Thanks for looping me in @denelon.

yao-msft · 2021-09-11T00:35:01Z

it would require admin to set the "consumed" state or it would be possible to just set it back to unconsumed.

Edit: Correct, it won't work. I forgot the secure setting part.

Anyway, as you said, given the rare use cases for this feature, I'm totally fine with an on/off switch only.

JohnMcPMS · 2021-09-11T19:48:41Z

src/AppInstallerCLICore/Commands/SettingsCommand.cpp

-            for (const auto& warning : User().GetWarnings())
+            context << Workflow::EnsureRunningAsAdmin;
+
+            if (!context.IsTerminated())


The whole point of the workflow is to never check this; you just make a task that does what you need to do (the block inside the if) and invoke it. #Resolved

JohnMcPMS · 2021-09-11T19:54:48Z

src/AppInstallerCommonCore/AdminSettings.cpp

+
+            if (!valueNode || !valueNode.IsScalar())
+            {
+                AICLI_LOG(Core, Info, << "Admin setting '" << name << "' was not found or did not contain the expected format");


Suggested change

AICLI_LOG(Core, Info, << "Admin setting '" << name << "' was not found or did not contain the expected format");

AICLI_LOG(Core, Verbose, << "Admin setting '" << name << "' was not found or did not contain the expected format");

``` #Resolved

JohnMcPMS · 2021-09-11T19:56:55Z

src/AppInstallerCommonCore/AdminSettings.cpp

+            auto stream = Settings::GetSettingStream(Settings::Streams::AdminSettings);
+            if (!stream)
+            {
+                AICLI_LOG(Core, Info, << "Admin settings was not found");


Suggested change

AICLI_LOG(Core, Info, << "Admin settings was not found");

AICLI_LOG(Core, Verbose, << "Admin settings was not found");

``` #Resolved

JohnMcPMS · 2021-09-11T20:00:35Z

src/AppInstallerCommonCore/AdminSettings.cpp

+        AdminSettingsInternal adminSettingsInternal;
+        bool isEnabled = adminSettingsInternal.GetAdminSettingBoolValue(setting);
+
+        // For some admin settings, even if it's disabled, if the corresponding policy is enabled then override it.


I would put this above the code that just read in the settings stream and return directly so that we don't waste cycles doing that. #Resolved

yao-msft added 2 commits September 9, 2021 23:09

Code

3fef86e

Tests

4a8191a

yao-msft requested a review from a team as a code owner September 10, 2021 17:21

JohnMcPMS reviewed Sep 11, 2021

View reviewed changes

pr comments

1d701c4

JohnMcPMS approved these changes Sep 13, 2021

View reviewed changes

yao-msft merged commit 1edc52b into microsoft:master Sep 13, 2021

yao-msft deleted the disableLocalManifest branch September 13, 2021 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable local manifest by default #1453

Disable local manifest by default #1453

yao-msft commented Sep 10, 2021 •

edited by ghost

Loading

denelon commented Sep 10, 2021

OfficialEsco commented Sep 10, 2021

denelon commented Sep 10, 2021

yao-msft commented Sep 10, 2021 •

edited

Loading

OfficialEsco commented Sep 10, 2021 •

edited

Loading

yao-msft commented Sep 10, 2021

jedieaston commented Sep 10, 2021

OfficialEsco commented Sep 10, 2021

yao-msft commented Sep 10, 2021

jedieaston commented Sep 10, 2021

JohnMcPMS commented Sep 11, 2021

jedieaston commented Sep 11, 2021

yao-msft commented Sep 11, 2021 •

edited

Loading

JohnMcPMS Sep 11, 2021 •

edited by yao-msft

Loading

JohnMcPMS Sep 11, 2021 •

edited by yao-msft

Loading

JohnMcPMS Sep 11, 2021 •

edited by yao-msft

Loading

JohnMcPMS Sep 11, 2021 •

edited by yao-msft

Loading

	AICLI_LOG(Core, Info, << "Admin setting '" << name << "' was not found or did not contain the expected format");
	AICLI_LOG(Core, Verbose, << "Admin setting '" << name << "' was not found or did not contain the expected format");
	``` #Resolved

-                AICLI_LOG(Core, Info, << "Admin settings was not found");
+                AICLI_LOG(Core, Verbose, << "Admin settings was not found");
+``` #Resolved

Disable local manifest by default #1453

Disable local manifest by default #1453

Conversation

yao-msft commented Sep 10, 2021 • edited by ghost Loading

Change

Validation

Microsoft Reviewers: Open in CodeFlow

denelon commented Sep 10, 2021

OfficialEsco commented Sep 10, 2021

denelon commented Sep 10, 2021

yao-msft commented Sep 10, 2021 • edited Loading

OfficialEsco commented Sep 10, 2021 • edited Loading

yao-msft commented Sep 10, 2021

jedieaston commented Sep 10, 2021

OfficialEsco commented Sep 10, 2021

yao-msft commented Sep 10, 2021

jedieaston commented Sep 10, 2021

JohnMcPMS commented Sep 11, 2021

jedieaston commented Sep 11, 2021

yao-msft commented Sep 11, 2021 • edited Loading

JohnMcPMS Sep 11, 2021 • edited by yao-msft Loading

Choose a reason for hiding this comment

JohnMcPMS Sep 11, 2021 • edited by yao-msft Loading

Choose a reason for hiding this comment

JohnMcPMS Sep 11, 2021 • edited by yao-msft Loading

Choose a reason for hiding this comment

JohnMcPMS Sep 11, 2021 • edited by yao-msft Loading

Choose a reason for hiding this comment

yao-msft commented Sep 10, 2021 •

edited by ghost

Loading

yao-msft commented Sep 10, 2021 •

edited

Loading

OfficialEsco commented Sep 10, 2021 •

edited

Loading

yao-msft commented Sep 11, 2021 •

edited

Loading

JohnMcPMS Sep 11, 2021 •

edited by yao-msft

Loading

JohnMcPMS Sep 11, 2021 •

edited by yao-msft

Loading

JohnMcPMS Sep 11, 2021 •

edited by yao-msft

Loading

JohnMcPMS Sep 11, 2021 •

edited by yao-msft

Loading