Runtime: Unintended interactions of SerializationGuard and other features

[jkotas]

The SerializationGuard feature is trying to prohibit certain types of operations during deserialization. However, these prohibited operations can happen and are valid for as features indirectly involved by deserialization. Example of such features include tracing or on-demand assembly loading:

• The tracing often create a new file as side-effect of tracing operations done while the deserialization is in progress.
• On-demand assembly loading systems sometimes that materialize the files on disk or even start a compiler in separate process to create the file.

There are likely unintended interations like this that we cannot think about yet. The SerializationGuard feature violates encapsulation that is one of the key principles of good software design.

[jkotas]

The serialization guard is on by default: https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/shared/System/LocalAppContextSwitches.Common.cs#L45 . This issue should be addressed for 3.0.

[bartonjs]

The ultimate answer is that anything that is legitimate, but negatively affected, can disable the feature via the appcontext switch and file a bug. If things are too egregious, we'll turn it off.

While tracing systems might be legitimately creating new files (and negatively carrying the state with them) the notion of a deserialization causing a compiler to be invoked is actually the sort of thing we're trying to block with Serialization Guard. (Again, there may be legitimate usage, but in general it's dangerous)

[jkotas]

it is true that anybody can disable this once they hit the problem; however they need to pay to cost to diagnose this and get to the root cause; and then they are faced with unconformable decision whether it is better to be more secure or whether it is better to make stuff work.

What do you think about my alternate proposal discussed offline to instead introduce a API that exposes list of types that are blacklisted for deserialization that would be checked by participating deserializers?

[bartonjs]

The concern with a deny-list is that it only affects things that are directly deserialized. It misses the case where the serialization gadget was some component which then on a "non-serialization" chain then still delivers serialization payload information to File.Write, Assembly.Load(byte[]) or Process.Start.

For example, one slide in a talk from abusing .NET serialization involved being able to indirectly create a thing to call the third-party method DotNetNuke.Common.Utilities.FileSystemUtils.PullFile to download a file from the Internet and write it to disk. The thing they were after was writing a thing to disk (which we provide) and they got there by a serializer, but not directly.

[MichalStrehovsky]

The more attack vectors we try to guard against in the future, the more unintended interactions this feature is going to have (e.g. writing to registry can be equally dangerous as Process.Start and WriteFile...).

Doesn't leaving things trivially exploitable (so that anyone can write their own POC in a matter of minutes) send a better message about how incredibly dangerous arbitrary deserialization is than introducing a feature that makes it look like we're making arbitrary deserialization safe?

[bartonjs]

Doesn't leaving things trivially exploitable ... send a better message ...?

For years docs have said to not deserialize untrusted input. And for years we've mopped up onesey twosey places with security bugs. The conclusion was we need to be more proactive.

[jkotas]

The conclusion was we need to be more proactive.

I agree with that. However, it still need to work properly. This feature does not work properly and it is a dead-end design as @MichalStrehovsky pointed out.

[GrabYourPitchforks]

Re: assembly load, I don't think the Serialization Guard feature blocks all assembly loads. I think it's just the Assembly.Load(byte[])-related overloads that are blocked. Is this impacting a mainline scenario?

[MichalStrehovsky]

I think it's just the Assembly.Load(byte[])-related overloads that are blocked. Is this impacting a mainline scenario?

Load from byte array is how people do "single-file" deployments right now. E.g. the NuGet credential provider for Azure embeds its dependent assemblies as resources, hooks up AppDomain.AssemblyResolve, and satisfies assembly resolution requests from the embedded assemblies. The embedded assemblies are loaded from a byte array.

[GrabYourPitchforks]

I bet we could make that scenario work. Should we use this issue to track it?

[MichalStrehovsky]

I bet we could make that scenario work. Should we use this issue to track it?

I think the point that Jan is making is that every single blocking we add is going to have unintended interactions (including the blocking of file writes, and whatever else we decide to block later - it's not just about Assembly.Load). This issue tracks the whole category.

It will be especially problematic if this blocking is looked at as a security feature. E.g. we didn't block registry writes now, but an attacker can use that to do indirect file writes, including a (delayed) process start. Let's say we find this "exploit" later and decide to do a "security fix" to block it. Now the security fix is going to have unintended impacts on scenarios we can't estimate risks on because it impacts arbitrary code paths in user code that might be executing while a deserialization is in progress. The result is that a security fix is going to break customers in code paths unrelated to the fixed area.

[GrabYourPitchforks]

We're not interested in closing every possible exploit vector. We're just interested in closing the ones that keep coming up over and over and over again in the security cases reported to us. File writes, assembly loading, and process start tend to be the ones we see in practice since for better or worse developers love writing deserialization ctors that directly or indirectly kick off one of those operations.

You're right in that nothing stops somebody from writing a deserialization ctor that performs an arbitrary registry write. At the same time nothing stops somebody from writing a deserialization ctor that performs an arbitrary write to memory. But we don't have evidence that such types exist in quantities that pose a threat to our ecosystem. And until that evidence appears I don't see us doing anything about it.

[MichalStrehovsky]

But we don't have evidence that such types exist in quantities that pose a threat to our ecosystem. And until that evidence appears I don't see us doing anything about it.

People use Process.Start and file writes because it's the most straightforward thing. Take that away, and people will just find the next straightforward thing (registry, WMI, whatever). It's easy to anticipate the next step here - this feature doesn't have a broad enough stroke.

If we go and block the next straighforward thing too, it adds to the argument of getting unintended interactions in many places. If we don't block it, this feature becomes pretty much useless (a speed bump that everyone knows how to drive around). Neither of those options make me feel like this is a sustainable design.

[danmosemsft]

We need to resolve this before shipping.

[danmosemsft]

I spoke to @GrabYourPitchforks. He is going to remove the public surface area and reach consensus on the rest for 3.0 (ie this month I hope).

[joperezr]

@GrabYourPitchforks given that you merged dotnet/corefx#38739, can this be closed now?

[ViktorHofer]

Serialization guard is still on by default in 3.0 and master: https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/shared/System/LocalAppContextSwitches.Common.cs#L48

Did we agree on keep it enabled?

[bartonjs]

Yes.