Skip to content

Conversation

@maximburyak
Copy link

No description provided.

@ayende
Copy link

ayende commented Aug 17, 2017

The idea is that this delegate that we inject allow us to verify that the scripts isn't doing BAD THINGS.

See fuller discussion of this and the implications of what it enables here:

https://ayende.com/blog/179457/optimizing-javascript-and-solving-the-halting-problem-part-i?key=cf628fb3c9ae4f4d9dec980d04886248

https://ayende.com/blog/179458/optimizing-javascript-and-solving-the-halting-problem-part-ii?key=c9ab77877d474eb694fc4b901965177f

@paulbartrum
Copy link
Owner

Very interesting, thanks. Being robust to badly behaving and actively malicious scripts is definitely of interest to quite a few people, but I've resisted doing anything about it because the only truly robust method is to run the malicious code in a separate OS process. Unfortunately, this solution is difficult and in a lot of cases (e.g. UWP) it's not even possible. So a solution like yours would definitely be useful as a kind of middle ground, even if it's not 100% robust.

@ayende
Copy link

ayende commented Aug 18, 2017

@paulbartrum We actually tested a bunch of other stuff, including injecting our code on every branch instruction, but that seemed too intrusive.
I can't think of a way that this + recursion limit + no CLR calls gives you a way to damage the parent process.

Note that we assume that actually handling the policy such as timeout / # of allocations is done by the supplied delegate.

Can you think about a way this will still open us some issues?

@paulbartrum
Copy link
Owner

You're hooking into the built-in loop statements, but there are many other ways to execute lots of code.

Some I can think of:

  • Add tons of code to a string variable and eval() it.
  • Use Array.prototype.forEach.
  • Call methods that take a long time to complete (like Array.prototype.sort for example).

if (EmitOnLoopIteration.Target != null)
{
generator.LoadArgument(0);
generator.LoadField(typeof(ScriptEngine).GetField(nameof(ScriptEngine.OnLoopIterationCallTarget)));
Copy link
Owner

Choose a reason for hiding this comment

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

The GetField call can be in ReflectionHelpers.

/// </summary>
public bool EnableILAnalysis { get; set; }

public Action EmitOnLoopIteration { get; set; }
Copy link
Owner

Choose a reason for hiding this comment

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

Missing doc comment.

// Emit on loop iteration (before loop's branch call)
//_________________________________________________________________________________________
private Action _onLoopIterationCall;
public Action OnLoopIterationCall
Copy link
Owner

Choose a reason for hiding this comment

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

Missing doc comment.

OnLoopIterationCallTarget = OnLoopIterationCall?.Target;
}
}
public object OnLoopIterationCallTarget;
Copy link
Owner

Choose a reason for hiding this comment

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

Can this be a property that returns OnLoopIterationCall?.Target?

Copy link

Choose a reason for hiding this comment

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

The idea was to reduce further the amount of work required. In particular, avoid a call in favor of a field load

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, good point, keep it a field then.

OnLoopIterationCallTarget = OnLoopIterationCall?.Target;
}
}
public object OnLoopIterationCallTarget;
Copy link
Owner

Choose a reason for hiding this comment

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

Missing doc comment.

@@ -0,0 +1,207 @@
using System;
Copy link
Owner

Choose a reason for hiding this comment

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

Move this file under the Core folder please.

public void OnLoopCallWithInstanceMethodInDebug()
{
var engine = new ScriptEngine();
engine.EnableDebugging = true;
Copy link
Owner

Choose a reason for hiding this comment

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

Don't use this property, it doesn't work in .NET core.

@kpreisser
Copy link
Collaborator

kpreisser commented Aug 24, 2017

Hi,
note that I did similar changes in my branch implementScriptTimeout3 - see #85: At the beginning of every loop (and at the beginning of user-defined functions) I check a flag, and if it is true, a ScriptCanceledException will be thrown in order to cancel the script (in this case, finally blocks aren't executed any more as they now will only be executed if the exception could have been caught by JS code (if it is a JavaScriptException).

(I originally created code of the ScriptTimeoutHelper in the wiki page as that approach seemed to work for me, until I found that it does not work for JS code in finally clauses where a ThreadAbortException will not be thrown.)

I think that you should also check the delegate when user-defined functions are invoked, in order to handle the Array.prototype.forEach case that @paulbartrum mentioned, and because you can construct functions without a loop that run lots of operations (without exceeding a reasonable recursion depth limit), e.g.

function func(n) {
    if (n < 15) {
        func(n + 1);
        func(n + 1);
        func(n + 1);
        func(n + 1);
        func(n + 1);
        func(n + 1);
    }
}
func(1);

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.

4 participants