Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move bypass execution policy check after AppLocker Deny check #15035

Merged
merged 3 commits into from
Mar 22, 2021

Conversation

TravisEz13
Copy link
Member

@TravisEz13 TravisEz13 commented Mar 15, 2021

PR Summary

Move bypass execution policy check after AppLocker Deny check

PR Context

This only affects a corner case where you only have Deny rules.

PR Checklist

@ghost ghost assigned iSazonov Mar 15, 2021
@TravisEz13 TravisEz13 added the WG-Security security related areas such as JEA label Mar 15, 2021
@TravisEz13 TravisEz13 assigned rjmholt and unassigned iSazonov Mar 15, 2021
@TravisEz13 TravisEz13 added CL-Engine Indicates that a PR should be marked as an engine change in the Change Log OS-Windows labels Mar 15, 2021
Copy link
Contributor

@PaulHigin PaulHigin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment.

@TravisEz13 TravisEz13 marked this pull request as ready for review March 16, 2021 18:05
@rjmholt
Copy link
Collaborator

rjmholt commented Mar 16, 2021

@PaulHigin can you refresh your review?

@PaulHigin PaulHigin self-requested a review March 16, 2021 20:46
@rjmholt rjmholt merged commit f1b8798 into PowerShell:master Mar 22, 2021
@TravisEz13 TravisEz13 deleted the port-applocker-fix branch March 22, 2021 18:39
@strawgate
Copy link
Contributor

strawgate commented Mar 27, 2021

This seems like it would be a breaking change.... this change appears to make it so you cannot use -executionpolicy bypass to run a script in an applocker environment that is denying script execution

@iSazonov
Copy link
Collaborator

This seems like it would be a breaking change

It is a bug fix - no one can disable the lock set by a system administrator.

@strawgate
Copy link
Contributor

strawgate commented Mar 27, 2021

Does this being a bugfix effect whether it's a breaking change?

https://github.com/PowerShell/PowerShell/blob/master/docs/dev-process/breaking-change-contract.md

It doesn't matter if the existing behavior is "wrong", we still need to think through the implications. PowerShell has been in broad use for over 10 years so we be must be extremely sensitive to breaking existing users and scripts.

This would seem to me to be a bucket 2/3 change which previously has required a breaking change conversation even when related to bug fixes: #12766 (comment)

@iSazonov
Copy link
Collaborator

iSazonov commented Mar 27, 2021

@strawgate Any bug fix could be considered as a breaking change :-)

This is a security related bug fix - such things are never discussed publicly (it is Microsoft policy).

@TravisEz13 TravisEz13 added the CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log label Mar 27, 2021
@TravisEz13
Copy link
Member Author

TravisEz13 commented Mar 27, 2021

@strawgate I've marked the issue a breaking change. As @iSazonov has said, AppLocker is considered the security feature with the stronger promise here. Execution policy was never intended as a bypass.

See our security policy: https://github.com/PowerShell/PowerShell/security/policy

Security issues are treated very seriously and will, by default, takes precedence over other considerations including usability, performance, etc... Best effort will be used to mitigate side effects of a security change, but PowerShell must be secure by default.

FYI, this scenario was is even a corner case of AppLocker, where you only have Deny rules and constrained language mode is not used to enforce the policy.

@ghost
Copy link

ghost commented Apr 14, 2021

🎉v7.2.0-preview.5 has been released which incorporates this pull request.:tada:

Handy links:

@iSazonov iSazonov added this to the 7.2.0-preview.5 milestone Apr 15, 2021
TravisEz13 added a commit that referenced this pull request Apr 16, 2021
[7.2.0-preview.5] - 2021-04-14

* Breaking Changes

- Make PowerShell Linux deb and RPM packages universal (#15109)
- Enforce AppLocker Deny configuration before Execution Policy Bypass configuration (#15035)
- Disallow mixed dash and slash in command line parameter prefix (#15142) (Thanks @davidBar-On!)

* Experimental Features

- `PSNativeCommandArgumentPassing`: Use `ArgumentList` for native executable invocation (breaking change) (#14692)

* Engine Updates and Fixes

- Add `IArgumentCompleterFactory` for parameterized `ArgumentCompleters` (#12605) (Thanks @powercode!)

* General Cmdlet Updates and Fixes

- Fix SSH remoting connection never finishing with misconfigured endpoint (#15175)
- Respect `TERM` and `NO_COLOR` environment variables for `$PSStyle` rendering (#14969)
- Use `ProgressView.Classic` when Virtual Terminal is not supported (#15048)
- Fix `Get-Counter` issue with `-Computer` parameter (#15166) (Thanks @krishnayalavarthi!)
- Fix redundant iteration while splitting lines (#14851) (Thanks @hez2010!)
- Enhance `Remove-Item -Recurse` to work with OneDrive (#14902) (Thanks @iSazonov!)
- Change minimum depth to 0 for `ConvertTo-Json` (#14830) (Thanks @kvprasoon!)
- Allow `Set-Clipboard` to accept empty string (#14579)
- Turn on and off `DECCKM` to modify keyboard mode for Unix native commands to work correctly (#14943)
- Fall back to `CopyAndDelete()` when `MoveTo()` fails due to an `IOException` (#15077)

* Code Cleanup

<details>

<summary>

<p>We thank the following contributors!</p>
<p>@xtqqczze, @iSazonov, @ZhiZe-ZG</p>

</summary>

<ul>
<li>Update .NET to <code>6.0.0-preview.3</code> (#15221)</li>
<li>Add space before comma to hosting test to fix error reported by <code>SA1001</code> (#15224)</li>
<li>Add <code>SecureStringHelper.FromPlainTextString</code> helper method for efficient secure string creation (#14124) (Thanks @xtqqczze!)</li>
<li>Use static lambda keyword (#15154) (Thanks @iSazonov!)</li>
<li>Remove unnecessary <code>Array</code> -&gt; <code>List</code> -&gt; <code>Array</code> conversion in <code>ProcessBaseCommand.AllProcesses</code> (#15052) (Thanks @xtqqczze!)</li>
<li>Standardize grammar comments in Parser.cs (#15114) (Thanks @ZhiZe-ZG!)</li>
<li>Enable <code>SA1001</code>: Commas should be spaced correctly (#14171) (Thanks @xtqqczze!)</li>
<li>Refactor <code>MultipleServiceCommandBase.AllServices</code> (#15053) (Thanks @xtqqczze!)</li>
</ul>

</details>

* Tools

- Use Unix line endings for shell scripts (#15180) (Thanks @xtqqczze!)

* Tests

- Add the missing tag in Host Utilities tests (#14983)
- Update `copy-props` version in `package.json` (#15124)

* Build and Packaging Improvements

<details>

<summary>

<p>We thank the following contributors!</p>
<p>@JustinGrote</p>

</summary>

<ul>
<li>Fix <code>yarn-lock</code> for <code>copy-props</code> (#15225)</li>
<li>Make package validation regex accept universal Linux packages (#15226)</li>
<li>Bump NJsonSchema from 10.4.0 to 10.4.1 (#15190)</li>
<li>Make MSI and EXE signing always copy to fix daily build (#15191)</li>
<li>Sign internals of EXE package so that it works correctly when signed (#15132)</li>
<li>Bump Microsoft.NET.Test.Sdk from 16.9.1 to 16.9.4 (#15141)</li>
<li>Update daily release tag format to  work with new Microsoft Update work (#15164)</li>
<li>Feature: Add Ubuntu 20.04 Support to install-powershell.sh (#15095) (Thanks @JustinGrote!)</li>
<li>Treat rebuild branches like release branches (#15099)</li>
<li>Update WiX to 3.11.2 (#15097)</li>
<li>Bump NJsonSchema from 10.3.11 to 10.4.0 (#15092)</li>
<li>Allow patching of preview releases (#15074)</li>
<li>Bump Newtonsoft.Json from 12.0.3 to 13.0.1 (#15084, #15085)</li>
<li>Update the <code>minSize</code> build package filter to be explicit (#15055)</li>
<li>Bump NJsonSchema from 10.3.10 to 10.3.11 (#14965)</li>
</ul>

</details>

* Documentation and Help Content

- Merge `7.2.0-preview.4` changes to master (#15056)
- Update `README` and `metadata.json` (#15046)
- Fix broken links for `dotnet` CLI (#14937)

[7.2.0-preview.5]: v7.2.0-preview.4...v7.2.0-preview.5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log CL-Engine Indicates that a PR should be marked as an engine change in the Change Log OS-Windows WG-Security security related areas such as JEA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants