Consider deserializing serialized script blocks as such (remoting, background jobs)

Bruce Payette (@BrucePay)

Currently and historically, [scriptblock] instances are deserialized as [string] rather than as type [scriptblock]:

This leads to surprising behavior:

# FAILS, because the script block deserializes to a string parameter
PS> pwsh -nop -c { param($sb) & $sb } -args { 'hi' }
&: The term ' 'hi' ' is not recognized as the name of a cmdlet, function, script file, or operable program.

# FAILS, because the background job sees the script block as a string after deserialization.
PS> $sb = { 'hi' }; (Measure-Command $using:sb &) | Receive-Job -Wait -AutoRemoveJob
Measure-Command: Cannot bind parameter 'Expression'. Cannot convert the " 'hi' " value of type "System.String" to type "System.Management.Automation.ScriptBlock".

The serialization part already contains all the required information - e.g. the above script block serializes as XML text <SBK> 'hi' </SBK>, but, surprisingly, during deserialization [scriptblock]::Create() is not called on this string.

Note: This simple plain-text serialization of the script block's content means that the $using:-scope feature for embedding values from the caller's scope doesn't work - references such as $using:foo are serialized as-is and would fail on execution of the recreated script block in the caller's scope.

The historical design rationale for this behavior was given by Bruce Payette (@BrucePay) in #4218 (comment):

Historically this is by design. Serializing scriptblocks with fidelity resulted in too many places where there was automatic code execution so to facilitate secure restricted runspaces, scriptblocks are always deserialized to strings.

To safely avoid this problem and to correct the surprising behavior, it sounds like we'd need to simply make sure that PowerShell-internally no accidental execution (not sure when and why that would happen; simply parsing strings into script blocks seems to be allowed even in restricted runspaces).

From the user's perspective, a script block should deserialize as such, and it is their responsibility, as always, to decide if and when to invoke it.

For old code that worked around the limitation, this change should have little to no impact, given how [scriptblock] instances stringify:

# The following should be equivalent
[scriptblock]::create(" 'foo' ") # convert from string
[scriptblock]::create({ 'foo' }) # redundant conversion from script block, which old code may end up doing

Hypothetically, old code that tries to access [string] properties on a deserialized script block could break - hopefully, this scenario falls into bucket 3.

Remoting working group:
We could have a conversion that allows a deserialized script block to be re-parsed into a scriptblock. But we will always just pass the script block string over the wire. One reason for this is it is much lighter weight for performance reasons. The other is that scriptblocks contain trust information that needs to be re-evaluated in the target session when the string is re-parsed into a script block.

Remoting working group:
On further consideration, this is by design. We don't want to be able to cast a deserialized scriptblock (or string) into a scriptblock, because this opens up security issues. It is essentially a 'bad' constructor and is why scriptblock has a 'Create' function.

because this opens up security issues.

It doesn't open up security issues per se, because a script block is just a piece of code that must be invoked explicitly in order to do anything (including potential harm).

By not deserializing to a script block, all you're doing is to create extra work for the user, by forcing them to call [scriptblock]::Create() themselves or - God forbid - Invoke-Expression - the need for which they won't expect, as demonstrated by the examples in the initial post.

It is funny how pen testers can change your mind :). The serialization system is used in remoting and we've had enough security reviews to know we don't want to do this.

Paul Higinbotham (@PaulHigin)

Paul Higinbotham (@PaulHigin), what are pen testers?

And how does "trust me, we've looked into this before" address the specific argument I've made?

Penetration testers. We will not implement automatic conversion of string to scriptblock via constructor as that can open up injection attacks as shown by our penetration testers.

Thanks for clarifying the terminology.

So, how does deserialization to a script block enable an injection attack if it requires an explicit, deliberate act of invocation to execute the deserialized script block?

Given how this conversation has unfolded, it is to be feared that this fiat decision stands, even though its sole justification is the abstract claim that there is an injection-attack security concern, without supporting that claim or being willing to debate it.

Therefore, the following is likely only for the benefit of future readers (disclaimer: I am by no means a security expert):

From what I gather, an injection attack can mean one of two things:

Execution of (malicious) code where no execution of code is expected at all.
Execution of maliciously modified code.

Neither scenario applies in the situation at hand:

If what is serialized as a script block (in type-specific <SBK> elements, by mere stringification, i.e. the block's verbatim source code),
then deserializing it as such (via [scriptblock]::Create()) cannot possibly result in execution of unwanted code, because constructing a script block doesn't result in its execution; it takes explicit invocation (via &, .Invoke() methods) for actual execution to occur.
- That a stringified script block representation cannot fully capture all aspects of the original script block is incidental to this discussion; (partial) loss of type fidelity is inherent in serialization, whether applied cross-process locally or in remoting.

Note that concern about the serialized data itself getting maliciously modified is an entirely separate debate that is not specific to this use case.

To put it differently: If the user's intent in trying something like pwsh -nop -c { param($sb) & $sb } -args { 'hi' } is to execute the script block being passed, then they can make that happen even with the current limitation:
pwsh -nop -c { param($sb) Invoke-Expression "& { $sb }" } -args { 'hi' }

That it requires an explicit act to recreate a serialized script block is nothing but an unexpected annoyance - it provides no security.

Conversely, if someone managed to tamper with the serialized data itself - an entirely separate, fundamental problem - all bets would be off even with respect to the script block that is directly executed, { param($sb) & $sb } / { param($sb) Invoke-Expression "& { $sb }" }{ param($sb) Invoke-Expression "& { $sb }" }.

Paul Higinbotham (@PaulHigin)

I just ran into this issue myself. Paul Higinbotham (@PaulHigin) Can you shed some light on how/why code could get executed if the ScriptBlock was deserialized automatically?

Has the situation improved since this was declared "as designed"? I'm trying to move some PowerShell from an in-process session to an out-of-process session, but I can't invoke Invoke-Command -ScriptBlock $sb in the out-of-process session:

Cannot bind parameter 'ScriptBlock'. Cannot convert the "<script body>" value of type "System.String" to
type "System.Management.Automation.ScriptBlock".

It'd be great if the parameter value emerged from serialization/deserialization as a ScriptBlock. It'd be tolerable (in my current situation, at least) if Invoke-Command accepted -ScriptBlock {string} and internally called ScriptBlock.Create(). If neither of those are going to work, is there a documented workaround (with useful examples)?

Michael Klement (@mklement0)

Michael Klement (@mklement0) (or others), it is possible to replicate the behavior of Import/Export-Clixml as well as PSSerializer, but with scriptblocks being roundtripped correctly.

It requires some reflection, works on PS 5.1 and Core, and if you're in a pinch, it may just do,

# ideally we could use the [System.Management.Automation.PSSerializer]::Deserialize
# however the public implementation (and as exposed by Import-CliXml) always deserializes
# scriptblocks as string, whereas we need them to remain scriptblocks after the roundtrip
# turns out in the innards of the implementation, there is an option to deserialize correctly
# so below is a bunch reflection to achieve that
$x = [System.Management.Automation.PSSerializer]::Serialize(@{ X = { "Y" }})
$psAssembly = [System.Reflection.Assembly]::GetAssembly([psobject])
$xmlReaderSettings = $psAssembly.GetType('System.Management.Automation.InternalDeserializer').GetProperty('XmlReaderSettingsForCliXml', ([System.Reflection.BindingFlags]::Static -bor [System.Reflection.BindingFlags]::NonPublic)).GetValue($null)

$xmlReader = [System.Xml.XmlReader]::Create([System.IO.StringReader]::new($x), $xmlReaderSettings)

$options = [enum]::ToObject($psAssembly.GetType('System.Management.Automation.DeserializationOptions'), 0x400)

$context = $psAssembly.CreateInstance('System.Management.Automation.DeserializationContext', $false, ([System.Reflection.BindingFlags]::NonPublic -bor [System.Reflection.BindingFlags]::Instance), $null, @($options, $null), $null, $null)

$deserializer = $psAssembly.CreateInstance('System.Management.Automation.Deserializer', $false, ([System.Reflection.BindingFlags]::NonPublic -bor [System.Reflection.BindingFlags]::Instance), $null, @($xmlReader, $context), $null, $null)

$deserialize = $deserializer.GetType().GetMethod('Deserialize', ([System.Reflection.BindingFlags]::NonPublic -bor [System.Reflection.BindingFlags]::Instance), $null, @(), $null)

$y = $deserialize.Invoke($deserializer, $null)

# validates output
$y
$y.X.GetType()

Based on advice from here,
https://stackoverflow.com/a/64209181

The workaround is much simpler: use [scriptblock]::Create() (e.g. $sb = { 'hi' }; Start-Job { & ([scriptblock]::Create($using:sb)) } | Receive-Job -Wait -AutoRemove); however, the point of this issue is that no workaround should be needed.

I agree. But given the stance and the digging that I did, the above is what unblocked my scenario.

Zeus (@suprak)

Zeus (@suprak), I see.

Nice job on getting this to work without the need to modify the code in the script block itself (which is what my ad hoc workaround is based on), but how would you make this work in practice, given that you'd need access to the raw serialization data in order to apply your custom deserialization?
Or are you sending the raw serialization data as a string?

To me it looks like some workaround that requires extra work on a per-call basis is needed, or am I missing something?
Afterthought: your workaround does have the advantage of not needing to modify and therefore not needing to know the contents of the script block.

Right, in my scenario I needed to serialize PS object state and deserialize it back reliably. Things like hashtables, and scriptblocks.
I would have just used Import/Export-Clixml except for the scriptblock deserializing back as strings and not scriptblocks.
After studying how clixml is generated, I realized that deserializing scriptblocks back into scriptblocks is supported just not exposed.
So what I pasted is the solution for my scenario.

Makes sense; thanks for sharing.

Navigation Menu

Repository navigation

Consider deserializing serialized script blocks as such (remoting, background jobs) #11698

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Consider deserializing serialized script blocks as such (remoting, background jobs) #11698

Description

Activity

PaulHigin commented on Mar 17, 2021

PaulHigin commented on Mar 17, 2021

mklement0 commented on Mar 17, 2021

PaulHigin commented on Mar 17, 2021

mklement0 commented on Mar 17, 2021

PaulHigin commented on Mar 17, 2021

mklement0 commented on Mar 17, 2021

mklement0 commented on Mar 18, 2021

dkattan commented on Apr 13, 2021

oz53719 commented on Jan 4, 2022

suprak commented on Nov 4, 2022

mklement0 commented on Nov 4, 2022

suprak commented on Nov 4, 2022

mklement0 commented on Nov 4, 2022

suprak commented on Nov 4, 2022

mklement0 commented on Nov 4, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions