-
Notifications
You must be signed in to change notification settings - Fork 62
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
Add deserialization type denylist #242
Conversation
v0.9.5 production release
…net#69) (akkadotnet#70) * add option for not publishing symbols for production release * bad folder name
Hyperion v0.9.6 master release
Hyperion v0.9.7 production release
v0.9.8 production release
v0.9.9 Release
v0.9.10 Release
v0.9.11 Release
v0.9.12 Release
v0.9.13 Release
v0.9.14 Release
v0.9.15 Release
v0.9.16 Release
v0.9.17 Release
v0.10.0 Release
Hyperion v0.10.1 Release
Version 0.10.2 release
Version 0.11.0 Release
@@ -132,19 +144,82 @@ private static Type GetTypeFromManifestName(Stream stream, DeserializerSession s | |||
}); | |||
} | |||
|
|||
public static bool disallowUnsafeTypes = true; | |||
|
|||
private static ReadOnlyCollection<string> unsafeTypesDenySet = |
There was a problem hiding this comment.
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:
- Messagepack-csharp's type exclusion set
- ysoserial.net's set of gadgets
@@ -132,19 +144,82 @@ private static Type GetTypeFromManifestName(Stream stream, DeserializerSession s | |||
}); | |||
} | |||
|
|||
public static bool disallowUnsafeTypes = true; |
There was a problem hiding this comment.
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.)
{ | ||
//System.Diagnostics.Process p = new Process(); | ||
var serializer = new Hyperion.Serializer(); | ||
var di =new System.IO.DirectoryInfo(@"c:\"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the contribution :)
Merging in #250 and then running some benchmarks on this PR |
{ | ||
if (type.IsValueType) | ||
return false; | ||
var currentBase = type.BaseType; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
…m/Hyperion into add-deserialization-type-denylist
First step of resolving #226 and #239 . For now this just looks for the types we know are a bad idea and throws an exception if we encounter them.
@Aaronontheweb I won't have time to get this finished in the next week but hopefully you or @Arkatufus can take it from here.
Notes:
TypeSerializer
to avoid penalties on repeatedly traversing the type hierarchy