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

Add deserialization type denylist #242

Merged
merged 28 commits into from
Aug 16, 2021
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
d6f8418
Merge pull request #68 from heynickc/master
Aaronontheweb Aug 11, 2017
ff0a22c
Fix nuget publish when symbols aren't required to nuget push (#69) (#70)
Aaronontheweb Aug 11, 2017
cf20153
Merge pull request #74 from akkadotnet/dev
Aaronontheweb Aug 17, 2017
84eb198
Merge pull request #88 from akkadotnet/dev
Aaronontheweb Jan 19, 2018
72d6188
Merge pull request #94 from akkadotnet/dev
Aaronontheweb Jan 31, 2018
b0075b4
Merge pull request #131 from akkadotnet/dev
Aaronontheweb Oct 4, 2019
d9a9319
Merge pull request #136 from akkadotnet/dev
Aaronontheweb Oct 16, 2019
326f89f
Merge pull request #143 from akkadotnet/dev
Aaronontheweb Nov 13, 2019
2cf4598
Merge pull request #154 from akkadotnet/dev
Aaronontheweb Jan 20, 2020
457a5e7
Merge pull request #160 from akkadotnet/dev
Aaronontheweb Feb 10, 2020
b6a4f25
Merge pull request #164 from akkadotnet/dev
Aaronontheweb Feb 13, 2020
0f2044d
Merge pull request #168 from akkadotnet/dev
Aaronontheweb Feb 27, 2020
204e3c6
Merge pull request #179 from akkadotnet/dev
Aaronontheweb Jun 17, 2020
255948e
Merge pull request #207 from akkadotnet/dev
Aaronontheweb Mar 24, 2021
7b6f992
Merge pull request #211 from akkadotnet/dev
Aaronontheweb Apr 13, 2021
615d9d1
Merge pull request #216 from akkadotnet/dev
Aaronontheweb Apr 19, 2021
1c7a6d2
Merge branch 'dev'
Arkatufus Jun 30, 2021
071c880
Version 0.10.2 release
Arkatufus Jun 30, 2021
cbe964e
Merge branch 'dev'
Arkatufus Jul 8, 2021
1f40195
Version 0.11.0 Release
Arkatufus Jul 8, 2021
781eaee
Add a saftey check to prevent deserialization of known evil types
to11mtm Jul 17, 2021
2fb434e
fix API Spec
to11mtm Jul 18, 2021
09185c7
Merge branch 'dev' into add-deserialization-type-denylist
Aaronontheweb Aug 3, 2021
3d5c626
Merge branch 'dev' into add-deserialization-type-denylist
Arkatufus Aug 16, 2021
984f6ac
Update API Approver list
Arkatufus Aug 16, 2021
f491d48
Merge branch 'dev' into add-deserialization-type-denylist
Aaronontheweb Aug 16, 2021
c57c37d
Add null checking
Arkatufus Aug 16, 2021
089f6ef
Merge branch 'add-deserialization-type-denylist' of github.com:to11mt…
Arkatufus Aug 16, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/Hyperion.API.Tests/CoreApiSpec.ApproveApi.approved.txt
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,11 @@ namespace Hyperion.Internal
public string Contract { get; }
public bool ForceFullStates { get; }
}
public class EvilDeserializationException : System.Security.SecurityException
{
public EvilDeserializationException(string message, string typeString) { }
public string BadTypeString { get; }
}
[System.AttributeUsage(System.AttributeTargets.Property | System.AttributeTargets.Field | System.AttributeTargets.Parameter | System.AttributeTargets.All)]
public sealed class HtmlAttributeValueAttribute : System.Attribute
{
Expand Down
1 change: 1 addition & 0 deletions src/Hyperion.Tests/ExpressionTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Linq;
using System.Linq.Expressions;
Expand Down
26 changes: 26 additions & 0 deletions src/Hyperion.Tests/UnsafeDeserializationExclusionTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
using System.IO;
using Hyperion.Extensions;
using Hyperion.Internal;
using Xunit;

namespace Hyperion.Tests
{
public class UnsafeDeserializationExclusionTests
{
[Fact]
public void CantDeserializeANaughtyType()
{
//System.Diagnostics.Process p = new Process();
var serializer = new Hyperion.Serializer();
var di =new System.IO.DirectoryInfo(@"c:\");
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if this unit test will play well in other OS runs... May need to make this something more platform friendly.


using (var stream = new MemoryStream())
{
serializer.Serialize(di, stream);
stream.Position = 0;
Assert.Throws<EvilDeserializationException>(() =>
serializer.Deserialize<DirectoryInfo>(stream));
}
}
}
}
69 changes: 67 additions & 2 deletions src/Hyperion/Extensions/TypeEx.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.IO;
using System.Linq;
using System.Reflection;
using System.Text.RegularExpressions;
using Hyperion.Internal;

namespace Hyperion.Extensions
{
Expand Down Expand Up @@ -132,19 +134,82 @@ private static Type GetTypeFromManifestName(Stream stream, DeserializerSession s
});
}

public static bool disallowUnsafeTypes = true;
Copy link
Member Author

Choose a reason for hiding this comment

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

So, you can't -really- get to this since TypeEx is internal. This should probably be either moved or left out (because really, -why- would you want to do any of these? it's just a bad idea.)


private static ReadOnlyCollection<string> unsafeTypesDenySet =
Copy link
Member Author

Choose a reason for hiding this comment

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

This list was amalgamated from:

new ReadOnlyCollection<string>(new[]
{
"System.Security.Claims.ClaimsIdentity",
"System.Windows.Forms.AxHost.State",
"System.Windows.Data.ObjectDataProvider",
"System.Management.Automation.PSObject",
"System.Web.Security.RolePrincipal",
"System.IdentityModel.Tokens.SessionSecurityToken",
"SessionViewStateHistoryItem",
"TextFormattingRunProperties",
"ToolboxItemContainer",
"System.Security.Principal.WindowsClaimsIdentity",
"System.Security.Principal.WindowsIdentity",
"System.Security.Principal.WindowsPrincipal",
"System.CodeDom.Compiler.TempFileCollection",
"System.IO.FileSystemInfo",
"System.Activities.Presentation.WorkflowDesigner",
"System.Windows.ResourceDictionary",
"System.Windows.Forms.BindingSource",
"Microsoft.Exchange.Management.SystemManager.WinForms.ExchangeSettingsProvider",
"System.Diagnostics.Process",
"System.Management.IWbemClassObjectFreeThreaded"
});

#if NETSTANDARD1_6
#else
public static bool UnsafeInheritanceCheck(Type type)
{
if (type.IsValueType)
return false;
var currentBase = type.BaseType;
Copy link
Member Author

Choose a reason for hiding this comment

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

should we actually start this off with type, just to be safe?

Copy link
Member

Choose a reason for hiding this comment

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

In terms of security scanning?

Copy link
Member

Choose a reason for hiding this comment

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

If that's your question, then yes I would - want to work our way from bottom to top

Copy link
Contributor

Choose a reason for hiding this comment

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

No, its fine, its the while...loop check that needs to be changed

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, I misunderstood what was being asked here

while (currentBase != typeof(object))
{
if (unsafeTypesDenySet.Any(r => currentBase.FullName.Contains(r)))
return true;
currentBase = currentBase.BaseType;
}

return false;
}
#endif
public static Type LoadTypeByName(string name)
{
if (disallowUnsafeTypes && unsafeTypesDenySet.Any(r => name.Contains(r)))
{
throw new EvilDeserializationException(
"Unsafe Type Deserialization Detected!", name);
}
try
{
// Try to load type name using strict version to avoid possible conflicts
// i.e. if there are different version available in GAC and locally
var typename = ToQualifiedAssemblyName(name, ignoreAssemblyVersion: false);
return Type.GetType(typename, true);
var type = Type.GetType(typename, true);
#if NETSTANDARD1_6
#else
if (UnsafeInheritanceCheck(type))
throw new EvilDeserializationException(
"Unsafe Type Deserialization Detected!", name);
#endif
return type;
}
catch (FileLoadException)
{
var typename = ToQualifiedAssemblyName(name, ignoreAssemblyVersion: true);
return Type.GetType(typename, true);
var type = Type.GetType(typename, true);
#if NETSTANDARD1_6
#else
if (UnsafeInheritanceCheck(type))
throw new EvilDeserializationException(
"Unsafe Type Deserialization Detected!", name);
#endif
return type;
}
}

Expand Down
11 changes: 11 additions & 0 deletions src/Hyperion/Internal/Annotations.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#endregion

using System;
using System.Security;

#pragma warning disable 1591
// ReSharper disable UnusedMember.Global
Expand All @@ -19,6 +20,16 @@

namespace Hyperion.Internal
{
public class EvilDeserializationException : SecurityException
{
public EvilDeserializationException(string message,
string typeString) : base(message)
{
BadTypeString = typeString;
}

public string BadTypeString { get; }
}
/// <summary>
/// Indicates that the value of the marked element could be <c>null</c> sometimes,
/// so the check for <c>null</c> is necessary before its usage.
Expand Down
6 changes: 5 additions & 1 deletion src/Hyperion/ValueSerializers/TypeSerializer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#endregion

using System;
using System.Collections.Concurrent;
using System.IO;
using Hyperion.Extensions;

Expand Down Expand Up @@ -60,13 +61,16 @@ public override void WriteValue(Stream stream, object value, SerializerSession s
}
}

private static readonly ConcurrentDictionary<string, Type> TypeNameLookup =
new ConcurrentDictionary<string, Type>();
public override object ReadValue(Stream stream, DeserializerSession session)
{
var shortname = stream.ReadString(session);
if (shortname == null)
return null;

var type = TypeEx.LoadTypeByName(shortname);
var type = TypeNameLookup.GetOrAdd(shortname,
name => TypeEx.LoadTypeByName(shortname));

//add the deserialized type to lookup
if (session.Serializer.Options.PreserveObjectReferences)
Expand Down