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

Prevent GetType and System.Reflection namespace under interop by default #994

Merged
merged 4 commits into from Nov 3, 2021

Conversation

lahma
Copy link
Collaborator

@lahma lahma commented Oct 31, 2021

Seems that this trips people often enough that we shouldn't allow GetType by default. Created separate property to control so that overwritten member filter won't cause us trouble, GetType will always by filtered based on configuration.

fixes #993

@lahma lahma mentioned this pull request Oct 31, 2021
@lahma lahma force-pushed the hide-gettype-by-default branch 2 times, most recently from 745a6fb to 512d742 Compare October 31, 2021 06:51
@sebastienros
Copy link
Owner

Fine with disabling it by default.
What about just reflection as a whole? Like checking on the System.Reflection namespace.

What about having a bool in AllowClr to also allow reflection. Just to give more ideas, not sure it's better, just maybe easier. No sure if with a custom filter we could actually remove the GetType limitation as a side effect. This bool would be taken into account and block the whole namespace whatever the filter says.

@lahma
Copy link
Collaborator Author

lahma commented Oct 31, 2021

Yeah makes sense to consider separate reflection namespace restriction, but GetType is a gateway to all the magic (GetType().Assembly.GetType("HardDriveFormatter).GetMethod("Format").Invoke()). GetType is defined on object and the Type is under System namespace. So we would then allow access to Type information at least, which might leak something - dunno.

So I guess we could do something extra, maybe another filter to check for Reflection namespace before allowing wrapping, like the AllowClr(bool allowReflectionNamespace) you suggested.

@sebastienros
Copy link
Owner

Didn't realize that GetType() was in System, so obvious.

@koculu
Copy link

koculu commented Oct 31, 2021

I have done following for Topaz:
koculu/Topaz@00bf8f1
Type is under system and it is safe. So any of its members are callable including Assembly.
Since Assembly is under System.Reflection namespace, it's members not callable or accessible through script.
I believe blocking System.Reflection namespace is enough for safety.
Any other dangerous use case for Type?

@koculu
Copy link

koculu commented Oct 31, 2021

ok I found my own answer after throwing the question.
someValue.GetType().GetProperty("xxx").GetValue(someValue);
With above statement script can access private members of a value which are not supposed to be.
And tons of other functions are evil.
Instead of disabling GetType() completely, it is best to write a white list for Type class and allow them.

@lahma
Copy link
Collaborator Author

lahma commented Nov 3, 2021

I've added separate AllowSystemReflection which controls the namespace wrapping, in addition to separate AllowGetType. I've harmonized the naming a bit and moved options under Options.Interop.

If there's need to prevent Type as whole, one can always create a throwing type-converter, but there might be valid use cases to access if, it's under System anyway. Type.GetProperty then return PropertyInfo which is under System.Reflection, and will be prevented.

I didn't alter AllowClr signature for now as it's params and all, maybe it's good to require some configuration work to allow the access.

@lahma lahma changed the title Hide GetType under interop with separate ExposeGetType configuration Prevent GetType and System.Reflection namespace under interop by default Nov 3, 2021
Jint/Runtime/Interop/TypeResolver.cs Outdated Show resolved Hide resolved
@lahma lahma merged commit c117c5e into sebastienros:main Nov 3, 2021
@lahma lahma deleted the hide-gettype-by-default branch November 3, 2021 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Security leak
3 participants