Separate Archive Scan argument related to security from `--force` #2622

Trenly · 2022-10-22T04:39:31Z

Have you signed the Contributor License Agreement?
Are you working against an Issue?

A change was recently made to make the hash override argument different from the --force argument on the basis that it has heavy implications on security and that those security implications should not be as easy to bypass as just adding on a "standard" parameter. This PR does the same thing for the archive malware scan.

This is implemented as a more generic ignore-security-checks argument. The reason for this is twofold - 1) The archive scan comes after the hash validation. The security implications of skipping this check (as opposed to the hash check) are lower, and therefore a less-specific argument can be used (just something less generic than --force) and 2) This argument will be expansible to other security-related actions. One such example would be if/when CVE data is integrated, a user would be able to use the new argument to confirm they wish to install a package with a known vulnerability.

This is not a breaking change as the archive scan behavior has only been included in pre-release, where behavior is subject to change.

Microsoft Reviewers: Open in CodeFlow

denelon · 2022-10-25T18:55:20Z

How about --ignore-archive-malware-scan ?

I'd also like to know why we're thinking about enabling this behavior. Do we think there are false positives? Or is this just to be "more" complete? I believe we would want another administrator required setting to be able to "enable" a user to be able to pass this argument as well.

We're adding a policy for this specific scenario related to the certificate pinning for the "msstore" source. We would likely need a policy for this scenario as well.

Mentioning @AmelBawa-msft since he's looking at the other policy.

denelon · 2022-10-25T18:56:19Z

Or maybe "bypass" as the other policy is written.

Trenly · 2022-10-25T19:13:18Z

I'd also like to know why we're thinking about enabling this behavior. Do we think there are false positives? Or is this just to be "more" complete?

Using the latest dev build on the latest release of paint.net (local manifest) flags it as malware even though it is not, likely due to the inclusion of the dll's in the zip file

I believe we would want another administrator required setting to be able to "enable" a user to be able to pass this argument as well.

Understandable. When --force was changed to no longer be the hash override, though, it no longer requires an admin setting. I can certainly see value in adding it, but I would imagine it more as a separate feature

Trenly · 2022-10-25T19:21:55Z

How about --ignore-archive-malware-scan ?

I suppose this gets back to user experience. Would there be a necessity for having individual arguments for all security checks? I'm imagining a case where a user is attempting to install a .zip package with a hash mismatch, fails malware scan (false positive), and has known CVE. The implementation in this PR would allow winget install <package> --ignore-security-hash --ignore-security-checks. Using an individual argument would necessitate winget install <package> --ignore-security-hash --ignore-archive-scan --ignore-cves. I think either way would be fine, but I was presuming that if a user was willing to bypass an archive scan or CVE data, they would want to bypass the other security checks as well

Or maybe "bypass" as the other policy is written.

I wrote this with the same concept as hash override. If bypass is preferred, I would suggest all security related arguments use bypass. This can still be done, as 1.4 is not in stable yet

winget-cli/src/AppInstallerCLICore/Argument.cpp

Lines 60 to 61 in edfc884

    
           case Args::Type::HashOverride: 
        
               return Argument{ "ignore-security-hash"_liv, NoAlias, Args::Type::HashOverride, Resource::String::HashOverrideArgumentDescription, ArgumentType::Flag, Settings::TogglePolicy::Policy::HashOverride };

yao-msft · 2022-10-25T20:38:35Z

I would say for this one, it's not actually "bypassing", we still perform the check but "ignore" the results.

JohnMcPMS · 2022-10-27T00:16:44Z

I'd also like to know why we're thinking about enabling this behavior. Do we think there are false positives? Or is this just to be "more" complete?

Using the latest dev build on the latest release of paint.net (local manifest) flags it as malware even though it is not, likely due to the inclusion of the dll's in the zip file

This seems like a problem; @ryfu-msft should look at that. From my understanding, Pure has a fairly extensive set of "bad" it will detect. We should filter that to the appropriate ones for our scenario.

Understandable. When --force was changed to no longer be the hash override, though, it no longer requires an admin setting. I can certainly see value in adding it, but I would imagine it more as a separate feature

And @yao-msft should look into that if true. The admin setting should still be required.

How about --ignore-archive-malware-scan ?

I prefer a specific flag per security related thing. Yes, it means more flags to pass, but it also means that you opted in to ignoring each one failing, rather than opting in to the ones that existed when you wrote your script.

yao-msft · 2022-10-27T00:46:18Z

Understandable. When --force was changed to no longer be the hash override, though, it no longer requires an admin setting. I can certainly see value in adding it, but I would imagine it more as a separate feature

And @yao-msft should look into that if true. The admin setting should still be required.

I think there might be some misunderstanding here? The HashOverride was never behind an admin setting, the only admin settings we have are EnableLocalManifests and EnableBypassStoreCertPinning. HashOverride was behind a group policy, and it still is behind it after the change. And we block HashOverride if it's running as admin.

Adding group policy for ignore-malware-scan is new work though.

Trenly · 2022-10-27T02:06:20Z

I prefer a specific flag per security related thing. Yes, it means more flags to pass, but it also means that you opted in to ignoring each one failing, rather than opting in to the ones that existed when you wrote your script.

I switched to the more specific --ignore-malware-scan

Adding group policy for ignore-malware-scan is new work though.

I believe I added everything needed for group policy

doc/admx/en-US/DesktopAppInstaller.adml

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

src/Microsoft.Management.Deployment/PackageManager.idl

src/PowerShell/Microsoft.WinGet.Client/Common/BaseInstallCommand.cs

doc/admx/en-US/DesktopAppInstaller.adml

yao-msft · 2022-11-02T03:16:22Z

@Trenly as fyi, all group policy values and strings will need to be reviewed by some Microsoft internal team, so these naming/description may need to be changed after review (we'll put comments to the pr).

Also group policy changes require OS side changes, we'll merge this pr after OS side changes are merged.

Trenly · 2022-11-02T03:38:22Z

@Trenly as fyi, all group policy values and strings will need to be reviewed by some Microsoft internal team, so these naming/description may need to be changed after review (we'll put comments to the pr).

Also group policy changes require OS side changes, we'll merge this pr after OS side changes are merged.

Thank you for the info!

doc/admx/DesktopAppInstaller.admx

doc/admx/en-US/DesktopAppInstaller.adml

src/AppInstallerCLICore/Argument.cpp

src/AppInstallerCLICore/Commands/InstallCommand.cpp

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

src/AppInstallerCLITests/GroupPolicy.cpp

src/AppInstallerCommonCore/Public/winget/Resources.h

src/AppInstallerCommonCore/Public/winget/GroupPolicy.h

yao-msft · 2022-12-13T22:10:32Z

Hi @Trenly

The group policy review has finished. The review team has made some naming suggestions. Mainly we should be more specific about what this policy does.

Can you apply the changes and merge with latest code base? If you don't have time, may I push the changes to your branch?

Thanks

Co-authored-by: yao-msft <50888816+yao-msft@users.noreply.github.com>

Trenly · 2022-12-13T23:49:44Z

Hi @Trenly

The group policy review has finished. The review team has made some naming suggestions. Mainly we should be more specific about what this policy does.

Can you apply the changes and merge with latest code base? If you don't have time, may I push the changes to your branch?

Thanks

I think I applied them. There were several merge conflicts with the latest code base. I think I fixed everything now, but I wouldn't be surprised if I made a mistake. If something fails, just give me a nudge (or feel free to commit right to the branch, whichever is easiest for you)

yao-msft · 2022-12-14T00:28:31Z

/azp run

azure-pipelines · 2022-12-14T00:28:51Z

Azure Pipelines successfully started running 1 pipeline(s).

Trenly · 2022-12-14T01:20:12Z

Yep, I definitely did something wrong

    <testcase classname="AppInstallerCLITests.exe.global" name="InstallFlow_Zip_ArchiveScanOverride" time="0.0">
      <failure message="installOutput.str().find(Resource::LocString(Resource::String::ArchiveFailedMalwareScanOverridden).get()) != std::string::npos" type="REQUIRE">
FAILED:
  REQUIRE( installOutput.str().find(Resource::LocString(Resource::String::ArchiveFailedMalwareScanOverridden).get()) != std::string::npos )
with expansion:
  4294967295 (0xffffffff)
  !=
  4294967295 (0xffffffff)
\x1B[0mFound \x1B[96mAppInstaller Test Zip Installer\x1B[0m [\x1B[96mAppInstallerCliTest.TestZipInstaller\x1B[0m] Version 1.0.0.0
\x1B[0mThis application is licensed to you by its owner.
Microsoft is not responsible for, nor does it grant any licenses to, third-party packages.
\x1B[0mSuccessfully verified installer hash
\x1B[91mArchive scan detected malware. To override this check use --ignore-local-archive-malware-scan

at D:\a\1\s\src\AppInstallerCLITests\WorkFlow.cpp(1311)
      </failure>
      <failure type="FAIL_CHECK">
FAILED:
Unused override 
\x1B[0mFound \x1B[96mAppInstaller Test Zip Installer\x1B[0m [\x1B[96mAppInstallerCliTest.TestZipInstaller\x1B[0m] Version 1.0.0.0
\x1B[0mThis application is licensed to you by its owner.
Microsoft is not responsible for, nor does it grant any licenses to, third-party packages.
\x1B[0mSuccessfully verified installer hash
\x1B[91mArchive scan detected malware. To override this check use --ignore-local-archive-malware-scan

at D:\a\1\s\src\AppInstallerCLITests\WorkFlow.cpp(504)
      </failure>
      <failure type="FAIL_CHECK">
FAILED:
Unused override 
\x1B[0mFound \x1B[96mAppInstaller Test Zip Installer\x1B[0m [\x1B[96mAppInstallerCliTest.TestZipInstaller\x1B[0m] Version 1.0.0.0
\x1B[0mThis application is licensed to you by its owner.
Microsoft is not responsible for, nor does it grant any licenses to, third-party packages.
\x1B[0mSuccessfully verified installer hash
\x1B[91mArchive scan detected malware. To override this check use --ignore-local-archive-malware-scan

at D:\a\1\s\src\AppInstallerCLITests\WorkFlow.cpp(504)
      </failure>
    </testcase>

yao-msft · 2022-12-14T01:37:39Z

/azp run

azure-pipelines · 2022-12-14T01:37:49Z

Azure Pipelines successfully started running 1 pipeline(s).

Trenly · 2022-12-14T02:19:04Z

/azp run

Same error; I don't quite understand it 😞

    <testcase classname="AppInstallerCLITests.exe.global" name="InstallFlow_Zip_ArchiveScanOverride" time="0.0">
      <failure message="installOutput.str().find(Resource::LocString(Resource::String::ArchiveFailedMalwareScanOverridden).get()) != std::string::npos" type="REQUIRE">
FAILED:
  REQUIRE( installOutput.str().find(Resource::LocString(Resource::String::ArchiveFailedMalwareScanOverridden).get()) != std::string::npos )
with expansion:
  4294967295 (0xffffffff)
  !=
  4294967295 (0xffffffff)
\x1B[0mFound \x1B[96mAppInstaller Test Zip Installer\x1B[0m [\x1B[96mAppInstallerCliTest.TestZipInstaller\x1B[0m] Version 1.0.0.0
\x1B[0mThis application is licensed to you by its owner.
Microsoft is not responsible for, nor does it grant any licenses to, third-party packages.
\x1B[0mSuccessfully verified installer hash
\x1B[91mArchive scan detected malware. To override this check use --ignore-local-archive-malware-scan

at D:\a\1\s\src\AppInstallerCLITests\WorkFlow.cpp(1311)
      </failure>
      <failure type="FAIL_CHECK">
FAILED:
Unused override 
\x1B[0mFound \x1B[96mAppInstaller Test Zip Installer\x1B[0m [\x1B[96mAppInstallerCliTest.TestZipInstaller\x1B[0m] Version 1.0.0.0
\x1B[0mThis application is licensed to you by its owner.
Microsoft is not responsible for, nor does it grant any licenses to, third-party packages.
\x1B[0mSuccessfully verified installer hash
\x1B[91mArchive scan detected malware. To override this check use --ignore-local-archive-malware-scan

at D:\a\1\s\src\AppInstallerCLITests\WorkFlow.cpp(504)
      </failure>
      <failure type="FAIL_CHECK">
FAILED:
Unused override 
\x1B[0mFound \x1B[96mAppInstaller Test Zip Installer\x1B[0m [\x1B[96mAppInstallerCliTest.TestZipInstaller\x1B[0m] Version 1.0.0.0
\x1B[0mThis application is licensed to you by its owner.
Microsoft is not responsible for, nor does it grant any licenses to, third-party packages.
\x1B[0mSuccessfully verified installer hash
\x1B[91mArchive scan detected malware. To override this check use --ignore-local-archive-malware-scan

at D:\a\1\s\src\AppInstallerCLITests\WorkFlow.cpp(504)
      </failure>
    </testcase>

yao-msft · 2022-12-14T05:05:17Z

The parameter here needs to be updated with the new arg

winget-cli/src/AppInstallerCLITests/WorkFlow.cpp

Line 1301 in 050056a

context.Args.AddArg(Execution::Args::Type::Force);

yao-msft · 2022-12-14T22:16:27Z

/azp run

azure-pipelines · 2022-12-14T22:16:38Z

Azure Pipelines successfully started running 1 pipeline(s).

Trenly · 2022-12-14T22:27:17Z

The parameter here needs to be updated with the new arg

winget-cli/src/AppInstallerCLITests/WorkFlow.cpp

Line 1301 in 050056a

context.Args.AddArg(Execution::Args::Type::Force);

I must be blind, though I do see you got to it before I did; Thank you

Trenly requested a review from a team as a code owner October 22, 2022 04:39

Trenly changed the title ~~Separate argument related to security from --force~~ Separate Archive Scan argument related to security from --force Oct 22, 2022

yao-msft reviewed Nov 1, 2022

View reviewed changes

doc/admx/en-US/DesktopAppInstaller.adml Outdated Show resolved Hide resolved

3m29xp2 mentioned this pull request Nov 2, 2022

Reverted #2656

Closed

yao-msft reviewed Dec 13, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

Trenly and others added 9 commits December 13, 2022 17:21

Use Security Related argument

b032d35

Change to dedicated argument

6d7ed71

Add group policy and connect to COM

9ab73e0

Cleanup

38b7802

Update spec to match

7e0fff8

Apply suggestions from code review

6d6ce7b

Co-authored-by: yao-msft <50888816+yao-msft@users.noreply.github.com>

Revert based on PR comments

0ebb663

Apply suggestions from code review

7e20ff8

Co-authored-by: yao-msft <50888816+yao-msft@users.noreply.github.com>

Update Group Policy description

3132690

Trenly force-pushed the SecurityRelatedCheck branch from 88d91fb to 3132690 Compare December 13, 2022 23:28

Trenly added 2 commits December 13, 2022 17:28

Fix string names

f5be078

Fix last update date

f21c8d3

Trenly added 3 commits December 13, 2022 17:44

Fix String Name

fad29ca

Fix more strings

9a1bb6e

Hope this is everything

9de9d63

Update string printed to user

c71290a

yao-msft added 12 commits December 14, 2022 14:03

Update DesktopAppInstaller.admx

eeb8ab2

Update DesktopAppInstaller.adml

785be2f

Update Argument.cpp

c919a45

Update Resources.h

5c4dc10

Update InstallCommand.cpp

4476dd5

Update GroupPolicy.cs

1769a83

Update GroupPolicyHelper.cs

c7cd9b3

Update winget.resw

e64ff54

Update GroupPolicy.cpp

c29ec0c

Update GroupPolicy.h

fedee7e

Update Resources.h

c827f20

Update WorkFlow.cpp

198091e

yao-msft approved these changes Dec 14, 2022

View reviewed changes

yao-msft merged commit 23f9942 into microsoft:master Dec 14, 2022

Trenly deleted the SecurityRelatedCheck branch December 14, 2022 23:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate Archive Scan argument related to security from `--force` #2622

Separate Archive Scan argument related to security from `--force` #2622

Trenly commented Oct 22, 2022 •

edited by ghost

Loading

denelon commented Oct 25, 2022

denelon commented Oct 25, 2022

Trenly commented Oct 25, 2022

Trenly commented Oct 25, 2022

yao-msft commented Oct 25, 2022

JohnMcPMS commented Oct 27, 2022

yao-msft commented Oct 27, 2022

Trenly commented Oct 27, 2022

yao-msft commented Nov 2, 2022

Trenly commented Nov 2, 2022

yao-msft commented Dec 13, 2022

This comment has been minimized.

Trenly commented Dec 13, 2022

yao-msft commented Dec 14, 2022

azure-pipelines bot commented Dec 14, 2022

Trenly commented Dec 14, 2022 •

edited

Loading

yao-msft commented Dec 14, 2022

azure-pipelines bot commented Dec 14, 2022

Trenly commented Dec 14, 2022

yao-msft commented Dec 14, 2022

yao-msft commented Dec 14, 2022

azure-pipelines bot commented Dec 14, 2022

Trenly commented Dec 14, 2022

Separate Archive Scan argument related to security from --force #2622

Separate Archive Scan argument related to security from --force #2622

Conversation

Trenly commented Oct 22, 2022 • edited by ghost Loading

Microsoft Reviewers: Open in CodeFlow

denelon commented Oct 25, 2022

denelon commented Oct 25, 2022

Trenly commented Oct 25, 2022

Trenly commented Oct 25, 2022

yao-msft commented Oct 25, 2022

JohnMcPMS commented Oct 27, 2022

yao-msft commented Oct 27, 2022

Trenly commented Oct 27, 2022

yao-msft commented Nov 2, 2022

Trenly commented Nov 2, 2022

yao-msft commented Dec 13, 2022

This comment has been minimized.

Trenly commented Dec 13, 2022

yao-msft commented Dec 14, 2022

azure-pipelines bot commented Dec 14, 2022

Trenly commented Dec 14, 2022 • edited Loading

yao-msft commented Dec 14, 2022

azure-pipelines bot commented Dec 14, 2022

Trenly commented Dec 14, 2022

yao-msft commented Dec 14, 2022

yao-msft commented Dec 14, 2022

azure-pipelines bot commented Dec 14, 2022

Trenly commented Dec 14, 2022

Separate Archive Scan argument related to security from `--force` #2622

Separate Archive Scan argument related to security from `--force` #2622

Trenly commented Oct 22, 2022 •

edited by ghost

Loading

Trenly commented Dec 14, 2022 •

edited

Loading