Skip to content

Conversation

@daxian-dbw
Copy link
Member

@daxian-dbw daxian-dbw commented Dec 4, 2018

PR Summary

This PR includes some changes and cleanups that will reduce the startup time. Each commit is self-contained, and the commit message serves as a summary of the changes in the commit.

  • Rename s_wasSystemPolicyDebugPolicy to s_allowDebugOverridePolicy to make it less confusing. Also slightly refactor HelperSecurity.psm1 and ConstrainedLanguageDebugger.Tests.ps1 to remove unneeded code. There is no functional change in this commit.
  • Remove the unneeded static property IsInbox, as PowerShell Core won't be shipped in-box with Windows in the foreseeable feature. Even if we do in future, we won't be needing it because Windows PowerShell will probably be gone by that time.
    • This static property was hit at startup time when constructing the value for $env:PSModulePath, and it will in turn cause IsNanoServer and IsIoT to be evaluated, all of which will trigger access to the Registry. By removing IsInbox, that can be all avoided.
  • Update 'BindRunspace' to avoid getting all commands and unneeded method calls.
    • BindRunspace retrieves all available commands from the session state, but for the most common scenario of creating/opening a Runspace, the retrieved commands are never get used afterwards. The method is updated to avoid retrieving all commands unless it's necessary.
    • The method is also updated to add check before calling into a ProcessXXX method, so that we can avoid some unneeded method calls.
  • Avoid creating a IsSafeValueVisitor every time when IsScriptBlockInFactASafeHashtable runs.
    • The static method IsSafeValueVisitor.IsAstSafe creates an instance of IsSafeValueVisitor every time it runs. Given that IsScriptBlockInFactASafeHashtable gets called relatively frequently in PerformSecurityCheck, this will result in some GC pressure as it will generate transient objects. This is updated to reuse a IsSafeValueVisitor singleton with the default SafeValueContext.
    • The Default and _safeValueContext fields are made readonly, so it cannot be changed by reflection.
    • The _visitCount is changed to uint type, so an attacker cannot change the value of it to a negative number in order to practically increase the max visit count limit.

PR Checklist

@daxian-dbw daxian-dbw self-assigned this Dec 4, 2018
@iSazonov
Copy link
Collaborator

iSazonov commented Dec 5, 2018

The _visitCount is changed to uint type, so an attacker cannot change the value of it to a negative number in order to practically increase the max visit count limit.

Currently runtime has Unsafe.As method and similar others. Can these methods be used to change the value with reflection?

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.

LGTM

@daxian-dbw
Copy link
Member Author

Currently runtime has Unsafe.As method and similar others. Can these methods be used to change the value with reflection?

@iSazonov I don't think uint field can be set with a negative value by reflection.

@daxian-dbw daxian-dbw merged commit c1e1716 into PowerShell:master Dec 6, 2018
@daxian-dbw daxian-dbw deleted the debugsyspolicy branch December 6, 2018 02:29
@daxian-dbw daxian-dbw added CL-Engine Indicates that a PR should be marked as an engine change in the Change Log CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log and removed CL-Engine Indicates that a PR should be marked as an engine change in the Change Log labels Dec 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants