Njs: new Function() support (strict only mode).

[xeioex]

Function constructor:
1) is a better alternative to eval()
2) It will not be identical to eval() from specs, but similar enough.

var sum = new Function('a', 'b', 'return a + b');

console.log(sum(2, 6));
// expected output: 8

var x = 10;

function createFunction1() {
    var x = 20;
    return new Function('return x;'); // this |x| refers global |x|
}

function createFunction2() {
    var x = 20;
    function f() {
        return x; // this |x| refers local |x| above
    }
    return f;
}

var f1 = createFunction1();
console.log(f1());          // 10
var f2 = createFunction2();
console.log(f2());          // 20

Basically, we need to compile and generate anonymous function in runtime.

njs_vm_compile() does parsing + generating byte-code. It also deletes previous AST tree (of njs_parser_node_t).

The new function should:
1) use a separate parser to parse the code.
2) generate byte-code
3) resolve references to global variables
4) invoke the generated byte code (something like njs_vm_invoke())

[drsm]

Here is the dumb implementation that pass some tests:

diff -r 2f4101c9a608 njs/njs_builtin.c
--- a/njs/njs_builtin.c Wed Mar 27 21:00:20 2019 +0300
+++ b/njs/njs_builtin.c Thu Mar 28 13:45:39 2019 +0300
@@ -119,7 +119,7 @@

 const njs_function_init_t  njs_native_functions[] = {
     /* SunC does not allow empty array initialization. */
-    { njs_eval_function,               { 0 } },
+    { njs_eval_function,               { NJS_SKIP_ARG, NJS_SKIP_ARG } },
     { njs_object_prototype_to_string,  { 0 } },
     { njs_number_global_is_nan,        { NJS_SKIP_ARG, NJS_NUMBER_ARG } },
     { njs_number_is_finite,            { NJS_SKIP_ARG, NJS_NUMBER_ARG } },
diff -r 2f4101c9a608 njs/njs_function.c
--- a/njs/njs_function.c    Wed Mar 27 21:00:20 2019 +0300
+++ b/njs/njs_function.c    Thu Mar 28 13:45:39 2019 +0300
@@ -1077,6 +1077,23 @@
 njs_eval_function(njs_vm_t *vm, njs_value_t *args, nxt_uint_t nargs,
     njs_index_t unused)
 {
+    njs_ret_t          ret;
+    const njs_value_t  *value;
+    nxt_str_t          codes;
+
+    value = njs_arg(args, nargs, 1);
+    if (nxt_slow_path(value->type != NJS_STRING)) {
+        vm->retval = *value;
+        return NXT_OK;
+    }
+
+    njs_string_get(value, &codes);
+
+    ret = njs_vm_compile(vm, &codes.start, codes.start + codes.length);
+    if (ret != NXT_OK) {
+        return ret;
+    }
+
     njs_internal_error(vm, "Not implemented");

     return NXT_ERROR;
diff -r 2f4101c9a608 njs/test/njs_interactive_test.c
--- a/njs/test/njs_interactive_test.c   Wed Mar 27 21:00:20 2019 +0300
+++ b/njs/test/njs_interactive_test.c   Thu Mar 28 13:45:39 2019 +0300
@@ -159,7 +159,7 @@
                  "    at Math.log (native)\n"
                  "    at main (native)\n") },

-    { nxt_string("eval()" ENTER),
+    { nxt_string("eval('')" ENTER),
       nxt_string("InternalError: Not implemented\n"
                  "    at eval (native)\n"
                  "    at main (native)\n") },
diff -r 2f4101c9a608 njs/test/njs_unit_test.c
--- a/njs/test/njs_unit_test.c  Wed Mar 27 21:00:20 2019 +0300
+++ b/njs/test/njs_unit_test.c  Thu Mar 28 13:45:39 2019 +0300
@@ -9408,9 +9408,22 @@
     { nxt_string("eval.constructor === Function"),
       nxt_string("true") },

-    { nxt_string("eval()"),
+    { nxt_string("eval('')"),
       nxt_string("InternalError: Not implemented") },

+    // 18.2.1.1.2 Runtime Semantics: PerformEval
+    { nxt_string("var a = [undefined, null, false, 1, {}, function() {}];"
+                 "a.map(function(x) { return eval(x); })"
+                 ".filter(function(x,i) { return x !== a[i]; })"
+                 ".length == 0"),
+      nxt_string("true") },
+
+    { nxt_string("eval(new String('abc'))"),
+      nxt_string("abc") },
+
+    { nxt_string("eval(':)')"),
+      nxt_string("SyntaxError: Unexpected token \":\"") },
+
     /* Math. */

     { nxt_string("Math.PI"),

[hongzhidao]

@xeioex

Fixed division token in lexer.
Introduced njs_variables_copy().
https://gist.github.com/hongzhidao/b53bb27b61cbfbe3c592cd052a20c57e

An early version for checking design, welcome to suggestion.
https://gist.github.com/hongzhidao/06db09729dc720d5a520ef74db0b3e83

@xeioex can we dump all the tests relate to this feature in test262?
@drsm Welcome more tests. Especially like this, bind.

[drsm]

@hongzhidao

@drsm Welcome more tests. Especially like this, bind.

By default this refers to global this value.
The new Function('return this') hack is the only way to get the global this value inside a module.

// script mode
> var t = this, fn = (function() { return new Function('return this'); }).call({}), o = {};
undefined
> fn() == t && fn.call(o) == o && fn.bind(o).call(t) == o
true

// module mode
> var fn = new Function('return this'); fn().Infinity === Infinity
true

>> (new Function({ toString() { return '...a'; }}, { toString() { return 'return a;' }}))(1,2,3)
SyntaxError: Unexpected token "" in runtime:1
    at native (native)
    at main (native)

[drsm]

@xeioex
@hongzhidao

I think the feature opens RCE hole (a real one, not like already reported :)), so it's better to disable it in sandbox.

[hongzhidao]

@xeioex

It seems there is an issue related to global this accumulative mode. Help to fix.

>  var fn = (function() { return this; }); fn() == this;
false 

[hongzhidao]

@drsm Thanks for reporting.

Updated.
https://gist.github.com/hongzhidao/06db09729dc720d5a520ef74db0b3e83

(new Function({ toString() { return '...a'; }}, { toString() { return 'return a;' }}))(1,2,3)

Fixed.

var fn = new Function('return this'); fn().Infinity === Infinity

See above, there is an issue in accumulative mode.

I think the feature opens RCE hole (a real one, not like already reported :)), so it's better to disable it in sandbox.

@xeioex

[drsm]

@hongzhidao

It seems there is an issue related to global this accumulative mode. Help to fix.

>  var fn = (function() { return this; }); fn() == this;
false 

No, it's OK. It should be true in non-strict mode only:

$ node --use-strict
> var fn = (function() { return this; }); fn() == this;
false

But for the functions created via new Function(code), the code is executed always in the global scope.

[hongzhidao]

OK, I'll deep into it later. Now, I'm on test262.

BTW, I used a tricky.
https://gist.github.com/hongzhidao/06db09729dc720d5a520ef74db0b3e83#file-njs-120-patch-L86

See this.

var a;    // index: 141
// since it used a seperate generator, they may generate the same index. index 141 will be assigned with [Function Object]
new Function('return a'); 

In short word, I regard the mode of the code in new Function as accumulate.

[hongzhidao]

@xeioex

There are more tests in the latest test262.

=== Summary ===
 - Ran 31763 tests
 - Passed 8396 tests (26.4%)
 - Failed 23367 tests (73.6%)
Failed Tests

vs

=== Summary ===
 - Ran 31763 tests
 - Passed 8492 tests (26.7%)
 - Failed 23271 tests (73.3%)
Failed Tests

[xeioex]

@drsm

I think the feature opens RCE hole (a real one, not like already reported :)), so it's better to disable it in sandbox.

I thought about it. Not sure we need a special flag, just using new Function() is not recommended (up to a user to decide). If there is no new Function() in your code is there a possible RCE?

[xeioex]

@hongzhidao

In short word, I regard the mode of the code in new Function as accumulate.

Not sure we can use this. It is not it create a copy of global vars, not reference?

var a;    // index: 141
// since it used a separate generator, they may generate the same index. index 141 will be assigned with [Function Object]
new Function('return a'); 

I think we should reuse the current global indices.

// index 141 will be assigned with [Function Object]
is not we need 142 index here for new Function()?

// 'return a'
a here should refer to 141.

[hongzhidao]

// index 141 will be assigned with [Function Object]
is not we need 142 index here for new Function()?

var a;    // generator A, index is 0141.
new Function('return a');  // generator B, index is 0141, see below (i)

static njs_int_t
njs_generate_function(njs_vm_t *vm, njs_generator_t *generator,
    njs_parser_node_t *node)
{
     ...
    node->index = njs_generate_object_dest_index(vm, generator, node); // i
    function->retval = node->index;
    return NJS_OK;
}

So I set the vm->options.accumulate true, I don't prefer to reuse the (main) vm generator (maybe can be copied).

// 'return a'
a here should refer to 141.

Yes, now.

[drsm]

@xeioex

I think the feature opens RCE hole (a real one, not like already reported :)), so it's better to disable it in sandbox.

I thought about it. Not sure we need a special flag, just using new Function() is not recommended (up to a user to decide). If there is no new Function() in your code is there a possible RCE?

The problem that it leads to a much larger attack surface:

• A specially crafted configuration can be supplied to the enduser which does the main thing, but also has a special :) abilities.
• It may be called indirectly: (function() {}).constructor('return 1+3')()
• Not needed in 99% of cases.

[xeioex]

@drsm

The problem that it leads to a much larger attack surface:
...

good points!

[xeioex]

@hongzhidao

https://gist.github.com/hongzhidao/06db09729dc720d5a520ef74db0b3e83#file-njs-120-patch-L32

We should call njs_value_to_string() only once per each arguments. Because custom toString() can change the string value itself. Like in:

> var i = 0, obj = {toString(){if(i++ == 0) return 'x'; else return 'xxx'}}
undefined
> obj.toString()
'x'
> obj.toString()
'xxx'

args[i] is a mutable copy, so a standard way is (for example njs_object_math_min()):
njs_value_to_string(vm, &args[i], &args[i])

[hongzhidao]

@xeioex

Is there any way to process argument by declaration?

const njs_function_init_t  njs_native_constructors[] = {
    /* SunC does not allow empty array initialization. */
    { njs_object_constructor,     { 0 } },
    { njs_array_constructor,      { 0 } },
    { njs_boolean_constructor,    { 0 } },
    { njs_number_constructor,     { NJS_SKIP_ARG, NJS_NUMBER_ARG } },
    { njs_string_constructor,     { NJS_SKIP_ARG, NJS_STRING_ARG } },
    { njs_function_constructor,   { 0 } },   /* NJS_STRING_ARG, NJS_STRING_ARG, ... */

If not, I'll rework it with your suggestion.

[xeioex]

@hongzhidao

Is there any way to process argument by declaration?

No. Declarations are only suitable for fixed number of arguments (and fixed prototypes). Many js functions have variable number of arguments.

[hongzhidao]

args[i] is a mutable copy

Updated.
https://gist.github.com/hongzhidao/06db09729dc720d5a520ef74db0b3e83

What are left:

• global this.

[xeioex]

@hongzhidao

https://gist.github.com/xeioex/c421b62130d3c7dacf22df77e69875b8

minor improvements for the arguments handling part.

[hongzhidao]

@drsm
Any difference between Function('return this') and new Function('return this')?
What I mean is does new affect Function?

[drsm]

@hongzhidao
No, there is no difference

[hongzhidao]

@drsm

See this.

function foo() { 
    console.log(this); 
}
foo()

What's the value of this? Can you test whether it's right in NJS?
I tried it, its value is undefined, shouldn't it be global this object?

[hongzhidao]

@xeioex

diff --git a/src/njs_variable.c b/src/njs_variable.c
--- a/src/njs_variable.c
+++ b/src/njs_variable.c
@@ -107,8 +107,8 @@ njs_variables_copy(njs_vm_t *vm, njs_lvl
         lhq.key_hash = njs_djb_hash(var->name.start, var->name.length);

         ret = njs_lvlhsh_insert(variables, &lhq);
-        if (njs_slow_path(ret != NJS_OK)) {
-            return NJS_ERROR;
+        if (njs_slow_path(ret == NJS_ERROR)) {
+            return ret;
         }
     }

Right?

[drsm]

@hongzhidao

function foo() { 
    console.log(this); 
}
foo()

What's the value of this? Can you test whether it's right in NJS?
I tried it, its value is undefined, shouldn't it be global this object?

No, in strict mode (njs is always in strict mode) the value of this is undefined in this case.
In non-strict mode (unsupported) it behaves exactly like Function('console.log(this)') and logs global this.
So, new Function() is somewhat legacy stuff IMO.
Also:

$ node --use-strict
> var f1 = (function() { return this; }).bind(this /* global */), f2 = new Function('return this');
undefined
> f1() === f2()
true
> // but
undefined
> var o = {};
undefined
> f1.bind(o)() === f2.bind(o)()
false
> f1.call(o) === f2.call(o)
false

new Function() looks exactly like a normal one bound to global this.
With exception that for real bound functions we can't rebind this again.

[hongzhidao]

@xeioex welcome to suggest how to deal with this in Function.

[drsm]

@hongzhidao
@xeioex

I have a sudden insight, that leaving it as is - is the best option :).

$ node
> (new Function('"use strict";return this'))()
undefined

[hongzhidao]

So do you think the current implementation with this patch is right?

[drsm]

Yes. If not, we need to support all the non-strict mode stuff inside Function('').

[hongzhidao]

Actually, we can do it (support non-strict mode), but I'm not sure worth to do it.
Anyway, what's your suggestion about this?
Is it OK, now?

[xeioex]

@hongzhidao

-        if (njs_slow_path(ret != NJS_OK)) {
-            return NJS_ERROR;
+        if (njs_slow_path(ret == NJS_ERROR)) {
+            return ret;
         }

the difference is minimal. Because here we do not expect NJS_DECLINED.

[hongzhidao]

Is it OK, now?

I forgot to process this.

[drsm]

@hongzhidao

Actually, we can do it (support non-strict mode), but I'm not sure worth to do it.
Anyway, what's your suggestion about this?

It's up to @xeioex to decide.
In my opinion, non-strict mode is a legacy stuff that does not worth effort.

Is it OK, now?

looks good to me, except sandboxing.

[hongzhidao]

By default this refers to global this value.

Does it only happen in non-strict mode? It's not mentioned in spec.

[hongzhidao]

looks good to me, except sandboxing.

=== Summary ===
 - Ran 31763 tests
 - Passed 8396 tests (26.4%)
 - Failed 23367 tests (73.6%)
Failed Tests

vs

=== Summary ===
 - Ran 31763 tests
 - Passed 8492 tests (26.7%)
 - Failed 23271 tests (73.3%)
Failed Tests

[drsm]

By default this refers to global this value.

Does it only happen in non-strict mode? It's not mentioned in spec.

Yes.
22
3;10
6.a

[hongzhidao]

https://tc39.es/ecma262/#sec-ordinarycallbindthis

Good point. Thanks a lot.

[hongzhidao]

@xeioex @drsm

looks good to me, except sandboxing.

I'm not on sandboxing, tell me if there are more things need to process.

[hongzhidao]

@xeioex @drsm

new Function() support.
https://gist.github.com/hongzhidao/06db09729dc720d5a520ef74db0b3e83

[xeioex]

@hongzhidao

debug support is also needed here. (-d shows only main code).

[xeioex]

@drsm @hongzhidao

https://gist.github.com/8dbbffb1c2add33e691ed8044f837915

1) Added "unsafe" option for VM (disabled by default).
Enabled in unit_tests and in CLI.
2) more tests

+ * sandbox      - "sandbox" mode. Disables file access.
+ * unsafe       - enables unsafe language features:
+ *   - Function constructors.

[hongzhidao]

@xeioex

Improved vm options.
https://gist.github.com/hongzhidao/46040585330e376585956f7355bc1f18

debug support is also needed here. (-d shows only main code).

Will do. I'll call disamble here.
https://gist.github.com/xeioex/8dbbffb1c2add33e691ed8044f837915#file-hg-120-patch-L150

Added "unsafe" option for VM

Suggest to review this later, but it seems safe can be added together.

[xeioex]

@hongzhidao

but it seems safe can be added together.

Yes. They are related.

[drsm]

typo

[xeioex]

@hongzhidao

BTW, before committing this, it seems, we need to fix https://github.com/nginx/njs/issues/146 first.
Like new Function("(".repeat(2**13))

[hongzhidao]

@xeioex

Improved vm options. (including unsafe option.)
https://gist.github.com/hongzhidao/46040585330e376585956f7355bc1f18

I think this can be committed first.

"(".repeat(2**13)

BTW, I can't reproduce this issue. Is it OK for you? @drsm

[xeioex]

@hongzhidao

Consider also this flag: https://github.com/nginx/njs/blob/master/src/njs_generator.h#L26
instread of vm->options.accumulative. Or it should be removed.

[hongzhidao]

I prefer to use options accumulate.

[xeioex]

@hongzhidao

After https://github.com/nginx/njs/commit/d2d47eb0e1891b295eb9e2ec7919f9070041c6d2, recursion tests also needed.

[hongzhidao]

After d2d47eb, recursion tests also needed.

On it.

@xeioex

Improved vm options. (including unsafe option.)
https://gist.github.com/hongzhidao/46040585330e376585956f7355bc1f18

Do you plan to commit it first?

[xeioex]

@hongzhidao

I think this https://gist.github.com/hongzhidao/46040585330e376585956f7355bc1f18 can be merged with the main patch.

[hongzhidao]

@xeioex

Final version.
https://gist.github.com/hongzhidao/46040585330e376585956f7355bc1f18

[xeioex]

Committed in: https://github.com/nginx/njs/commit/a838ffa5f9ed380b276883525979606d881278d3