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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions Jint.Tests/Runtime/Domain/HiddenMembers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,7 @@ public class HiddenMembers
public string Method1() => "Method1";

public string Method2() => "Method2";

public Type Type => GetType();
}
}
41 changes: 23 additions & 18 deletions Jint.Tests/Runtime/InteropTests.MemberAccess.cs
Original file line number Diff line number Diff line change
Expand Up @@ -70,33 +70,38 @@ public void ShouldBeAbleToFilterMembers()
Assert.True(engine.Evaluate("m.Field2").IsString());
Assert.True(engine.Evaluate("m.Member2").IsString());
Assert.True(engine.Evaluate("m.Method2()").IsString());

// we forbid GetType by default
Assert.True(engine.Evaluate("m.GetType").IsUndefined());
}

[Fact]
public void ShouldBeAbleToHideGetType()
public void ShouldBeAbleToExposeGetType()
{
var engine = new Engine(options => options
.SetTypeResolver(new TypeResolver
{
MemberFilter = member => !Attribute.IsDefined(member, typeof(ObsoleteAttribute))
})
);
var engine = new Engine(options =>
{
options.Interop.AllowGetType = true;
options.Interop.AllowSystemReflection = true;
});
engine.SetValue("m", new HiddenMembers());

Assert.True(engine.Evaluate("m.Method1").IsUndefined());
Assert.True(engine.Evaluate("m.GetType").IsCallable());

// reflection could bypass some safeguards
Assert.Equal("Method1", engine.Evaluate("m.GetType().GetMethod('Method1').Invoke(m, [])").AsString());
}

// but not when we forbid GetType
var hiddenGetTypeEngine = new Engine(options => options
.SetTypeResolver(new TypeResolver
{
MemberFilter = member => member.Name != nameof(GetType)
})
);
hiddenGetTypeEngine.SetValue("m", new HiddenMembers());
Assert.True(hiddenGetTypeEngine.Evaluate("m.GetType").IsUndefined());
[Fact]
public void ShouldProtectFromReflectionServiceUsage()
{
var engine = new Engine();
engine.SetValue("m", new HiddenMembers());

// we can get a type reference if it's exposed via property, bypassing GetType
var type = engine.Evaluate("m.Type");
Assert.IsType<ObjectWrapper>(type);

var ex = Assert.Throws<InvalidOperationException>(() => engine.Evaluate("m.Type.Module.GetType().Module.GetType('System.DateTime')"));
Assert.Equal("Cannot access System.Reflection namespace, check Engine's interop options", ex.Message);
}

[Fact]
Expand Down
2 changes: 1 addition & 1 deletion Jint/Options.Extensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ public static Options AllowClrWrite(this Options options, bool allow = true)

public static Options AllowOperatorOverloading(this Options options, bool allow = true)
{
options.Interop.OperatorOverloadingAllowed = allow;
options.Interop.AllowOperatorOverloading = allow;
return options;
}

Expand Down
14 changes: 13 additions & 1 deletion Jint/Options.cs
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,18 @@ public class InteropOptions
/// </summary>
public bool Enabled { get; set; }

/// <summary>
/// Whether to expose <see cref="object.GetType"></see> which can allow bypassing allow lists and open a way to reflection.
/// Defaults to false.
/// </summary>
public bool AllowGetType { get; set; }

/// <summary>
/// Whether Jint should allow wrapping objects from System.Reflection namespace.
/// Defaults to false.
/// </summary>
public bool AllowSystemReflection { get; set; }

/// <summary>
/// Whether writing to CLR objects is allowed (set properties), defaults to true.
/// </summary>
Expand All @@ -195,7 +207,7 @@ public class InteropOptions
/// <summary>
/// Whether operator overloading resolution is allowed, defaults to false.
/// </summary>
public bool OperatorOverloadingAllowed { get; set; }
public bool AllowOperatorOverloading { get; set; }

/// <summary>
/// Types holding extension methods that should be considered when resolving methods.
Expand Down
8 changes: 8 additions & 0 deletions Jint/Runtime/Interop/DefaultObjectConverter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,14 @@ public static bool TryConvert(Engine engine, object value, out JsValue result)
else
{
var t = value.GetType();

if (!engine.Options.Interop.AllowSystemReflection
&& t.Namespace?.StartsWith("System.Reflection") == true)
{
const string message = "Cannot access System.Reflection namespace, check Engine's interop options";
ExceptionHelper.ThrowInvalidOperationException(message);
}

if (t.IsEnum)
{
var ut = Enum.GetUnderlyingType(t);
Expand Down
2 changes: 1 addition & 1 deletion Jint/Runtime/Interop/DefaultTypeConverter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ public virtual object Convert(object value, Type type, IFormatProvider formatPro
return obj;
}

if (_engine.Options.Interop.OperatorOverloadingAllowed)
if (_engine.Options.Interop.AllowOperatorOverloading)
{
#if NETSTANDARD
var key = (valueType, type);
Expand Down
2 changes: 1 addition & 1 deletion Jint/Runtime/Interop/Reflection/IndexerAccessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ IndexerAccessor ComposeIndexerFactory(PropertyInfo candidate, Type paramType)
return null;
}

var filter = engine.Options.Interop.TypeResolver.MemberFilter;
var filter = new Func<MemberInfo, bool>(m => engine.Options.Interop.TypeResolver.Filter(engine, m));

// default indexer wins
if (typeof(IList).IsAssignableFrom(targetType) && filter(_iListIndexer))
Expand Down
2 changes: 1 addition & 1 deletion Jint/Runtime/Interop/TypeReference.cs
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ private ReflectionAccessor ResolveMemberAccessor(Type type, string name)
}

const BindingFlags bindingFlags = BindingFlags.Public | BindingFlags.Static;
return typeResolver.TryFindMemberAccessor(type, name, bindingFlags, indexerToTry: null, out var accessor)
return typeResolver.TryFindMemberAccessor(_engine, type, name, bindingFlags, indexerToTry: null, out var accessor)
? accessor
: ConstantValueAccessor.NullAccessor;
}
Expand Down
24 changes: 16 additions & 8 deletions Jint/Runtime/Interop/TypeResolver.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,22 @@ namespace Jint.Runtime.Interop
/// </summary>
public sealed class TypeResolver
{
public static readonly TypeResolver Default = new TypeResolver();
public static readonly TypeResolver Default = new();

private Dictionary<ClrPropertyDescriptorFactoriesKey, ReflectionAccessor> _reflectionAccessors = new();

/// <summary>
/// Registers a filter that determines whether given member is wrapped to interop or returned as undefined.
/// By default allows all but will also be limited by <see cref="InteropOptions.AllowGetType"/> configuration.
/// </summary>
/// <seealso cref="InteropOptions.AllowGetType"/>
public Predicate<MemberInfo> MemberFilter { get; set; } = _ => true;

internal bool Filter(Engine engine, MemberInfo m)
{
return (engine.Options.Interop.AllowGetType || m.Name != nameof(GetType)) && MemberFilter(m);
}

/// <summary>
/// Gives the exposed names for a member. Allows to expose C# convention following member like IsSelected
/// as more JS idiomatic "selected" for example. Defaults to returning the <see cref="MemberInfo.Name"/> as-is.
Expand Down Expand Up @@ -73,7 +80,7 @@ internal ReflectionAccessor GetAccessor(Engine engine, Type type, string member,
const BindingFlags bindingFlags = BindingFlags.Static | BindingFlags.Instance | BindingFlags.Public;

// properties and fields cannot be numbers
if (!isNumber && TryFindMemberAccessor(type, memberName, bindingFlags, indexer, out var temp))
if (!isNumber && TryFindMemberAccessor(engine, type, memberName, bindingFlags, indexer, out var temp))
{
return temp;
}
Expand All @@ -97,7 +104,7 @@ internal ReflectionAccessor GetAccessor(Engine engine, Type type, string member,
{
foreach (var iprop in iface.GetProperties())
{
if (!MemberFilter(iprop))
if (!Filter(engine, iprop))
{
continue;
}
Expand Down Expand Up @@ -130,7 +137,7 @@ internal ReflectionAccessor GetAccessor(Engine engine, Type type, string member,
{
foreach (var imethod in iface.GetMethods())
{
if (!MemberFilter(imethod))
if (!Filter(engine, imethod))
{
continue;
}
Expand Down Expand Up @@ -165,7 +172,7 @@ internal ReflectionAccessor GetAccessor(Engine engine, Type type, string member,
var matches = new List<MethodInfo>();
foreach (var method in extensionMethods)
{
if (!MemberFilter(method))
if (!Filter(engine, method))
{
continue;
}
Expand All @@ -189,6 +196,7 @@ internal ReflectionAccessor GetAccessor(Engine engine, Type type, string member,
}

internal bool TryFindMemberAccessor(
Engine engine,
Type type,
string memberName,
BindingFlags bindingFlags,
Expand All @@ -201,7 +209,7 @@ internal ReflectionAccessor GetAccessor(Engine engine, Type type, string member,
var typeResolverMemberNameCreator = MemberNameCreator;
foreach (var p in type.GetProperties(bindingFlags))
{
if (!MemberFilter(p))
if (!Filter(engine, p))
{
continue;
}
Expand Down Expand Up @@ -231,7 +239,7 @@ internal ReflectionAccessor GetAccessor(Engine engine, Type type, string member,
FieldInfo field = null;
foreach (var f in type.GetFields(bindingFlags))
{
if (!MemberFilter(f))
if (!Filter(engine, f))
{
continue;
}
Expand All @@ -256,7 +264,7 @@ internal ReflectionAccessor GetAccessor(Engine engine, Type type, string member,
List<MethodInfo> methods = null;
foreach (var m in type.GetMethods(bindingFlags))
{
if (!MemberFilter(m))
if (!Filter(engine, m))
{
continue;
}
Expand Down
2 changes: 1 addition & 1 deletion Jint/Runtime/Interpreter/EvaluationContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ public EvaluationContext(Engine engine, in Completion? resumedCompletion = null)
Engine = engine;
DebugMode = engine._isDebugMode;
ResumedCompletion = resumedCompletion ?? default; // TODO later
OperatorOverloadingAllowed = engine.Options.Interop.OperatorOverloadingAllowed;
OperatorOverloadingAllowed = engine.Options.Interop.AllowOperatorOverloading;
}

public Engine Engine { get; }
Expand Down