Fixed safe mode bypass in Function constructor.

This closes #367 pull request on Github.

Thanks bux.patryk@gmail.com for prodding it.

diff --git a/src/njs_function.c b/src/njs_function.c
index db1ef31f2992a00ad71227b58850ba07e0d83757..a6653a1d5272e6050e2408671aa0896295e1c235 100644 (file)
--- a/src/njs_function.c
+++ b/src/njs_function.c
@@ -869,41 +869,31 @@ static njs_int_t
 njs_function_constructor(njs_vm_t *vm, njs_value_t *args, njs_uint_t nargs,
     njs_index_t unused)
 {
-    njs_chb_t              chain;
-    njs_int_t              ret;
-    njs_str_t              str, file;
-    njs_uint_t             i;
-    njs_value_t            *body;
-    njs_lexer_t            lexer;
-    njs_parser_t           *parser;
-    njs_vm_code_t          *code;
-    njs_function_t         *function;
-    njs_generator_t        generator;
-    njs_parser_scope_t     *scope;
-    njs_function_lambda_t  *lambda;
-
-    if (!vm->options.unsafe) {
-        body = njs_argument(args, nargs - 1);
-        ret = njs_value_to_string(vm, body, body);
-        if (njs_slow_path(ret != NJS_OK)) {
-            return ret;
-        }
-
-        njs_string_get(body, &str);
-
-        /*
-         * Safe mode exception:
-         * "(new Function('return this'))" is often used to get
-         * the global object in a portable way.
-         */
-
-        if (str.length != njs_length("return this")
-            || memcmp(str.start, "return this", 11) != 0)
-        {
-            njs_type_error(vm, "function constructor is disabled"
-                           " in \"safe\" mode");
-            return NJS_ERROR;
-        }
+    njs_chb_t               chain;
+    njs_int_t               ret;
+    njs_str_t               str, file;
+    njs_uint_t              i;
+    njs_lexer_t             lexer;
+    njs_parser_t            *parser;
+    njs_vm_code_t           *code;
+    njs_function_t          *function;
+    njs_generator_t         generator;
+    njs_parser_node_t       *node;
+    njs_parser_scope_t      *scope;
+    njs_function_lambda_t   *lambda;
+    const njs_token_type_t  *type;
+
+    static const njs_token_type_t  safe_ast[] = {
+        NJS_TOKEN_END,
+        NJS_TOKEN_FUNCTION_EXPRESSION,
+        NJS_TOKEN_STATEMENT,
+        NJS_TOKEN_RETURN,
+        NJS_TOKEN_THIS,
+        NJS_TOKEN_ILLEGAL
+    };
+
+    if (!vm->options.unsafe && nargs != 2) {
+        goto fail;
     }
 
     njs_chb_init(&chain, vm->mem_pool);
@@ -960,6 +950,27 @@ njs_function_constructor(njs_vm_t *vm, njs_value_t *args, njs_uint_t nargs,
         return ret;
     }
 
+    if (!vm->options.unsafe) {
+        /*
+         * Safe mode exception:
+         * "(new Function('return this'))" is often used to get
+         * the global object in a portable way.
+         */
+
+        node = parser->node;
+        type = &safe_ast[0];
+
+        for (; *type != NJS_TOKEN_ILLEGAL; type++, node = node->right) {
+            if (node == NULL || node->left != NULL) {
+                goto fail;
+            }
+
+            if (node->token_type != *type) {
+                goto fail;
+            }
+        }
+    }
+
     scope = parser->scope;
 
     ret = njs_variables_copy(vm, &scope->variables, vm->variables_hash);
@@ -998,6 +1009,11 @@ njs_function_constructor(njs_vm_t *vm, njs_value_t *args, njs_uint_t nargs,
     njs_set_function(&vm->retval, function);
 
     return NJS_OK;
+
+fail:
+
+    njs_type_error(vm, "function constructor is disabled in \"safe\" mode");
+    return NJS_ERROR;
 }
 
 

diff --git a/src/test/njs_unit_test.c b/src/test/njs_unit_test.c
index 4cd63257b6815278b993c065e751a02e9654c794..1c1b0d587a1cf0cef9a5f3923a2fad14671cb932 100644 (file)
--- a/src/test/njs_unit_test.c
+++ b/src/test/njs_unit_test.c
@@ -19518,6 +19518,25 @@ static njs_unit_test_t  njs_test[] =
 };
 
 
+static njs_unit_test_t  njs_safe_test[] =
+{
+    { njs_str("(new Function('return this'))() === globalThis"),
+      njs_str("true") },
+
+    { njs_str("(new Function('return this;'))() === globalThis"),
+      njs_str("true") },
+
+    { njs_str("(new Function('return    this  '))() === globalThis"),
+      njs_str("true") },
+
+    { njs_str("(new Function('return thi'))()"),
+      njs_str("TypeError: function constructor is disabled in \"safe\" mode") },
+
+    { njs_str("(new Function('){return 1337})//', 'return this'))()"),
+      njs_str("TypeError: function constructor is disabled in \"safe\" mode") },
+};
+
+
 static njs_unit_test_t  njs_denormals_test[] =
 {
     { njs_str("2.2250738585072014e-308"),
@@ -21777,6 +21796,12 @@ static njs_test_suite_t  njs_suites[] =
       njs_nitems(njs_test),
       njs_unit_test },
 
+    { njs_str("safe script"),
+      { .repeat = 1},
+      njs_safe_test,
+      njs_nitems(njs_safe_test),
+      njs_unit_test },
+
     { njs_str("denormals"),
       { .repeat = 1, .unsafe = 1 },
       njs_denormals_test,