Repository: nginx/njs (c)

Issue:
The properties of `global.crypto` can lead to undefined behavior/crash
### Describe the bug
Upon the first access, the `global.crypto` object contains both the `getRandomValues` function and the `subtle` property, which itself contains multiple cryptographic methods. However, on the second access, the `subtle` property is missing, leaving only the `getRandomValues` function.

- [x] The bug is reproducible with the latest version of njs.
- [x] I minimized the code and NGINX configuration to the smallest
possible to reproduce the issue.

### To reproduce

Steps to reproduce the behavior:

- JS script
```js
console.log(Object.keys(global.crypto)); // output: ['getRandomValues','subtle']
global.crypto.subtle;                    // triggering undefined behabior(?)
console.log(Object.keys(global.crypto)); // output: ['getRandomValues']
```
or put the code in a [gist](https://gist.github.com/) and link it here.
- NGINX configuration if applicable
```
# Your NGINX configuration here
```
or put the configuration in a [gist](https://gist.github.com/) and link it here.
- NGINX logs if applicable
```
# Your NGINX logs here
```
or post the full log to a [gist](https://gist.github.com/) and link it here.
- Output of the `nginx -V` command if applicable.
- Exact steps to reproduce the behavior

### Expected behavior

The `global.crypto` object should consistently contain all its properties and methods, including the `subtle` property, regardless of how many times it is accessed or logged.

### Your environment

- Version of njs or specific commit: `v0.8.5` / `d34fcb03cf2378a644a3c7366d58cbddc2771cbd`
- Version of NGINX if applicable: n/a
- List of other enabled nginx modules if applicable: n/a
- OS: `Ubuntu:20.04`(WSL/Docker environment)

### Additional context
I found that it can lead to crashes too, try running the following JS script:
```js
let all = Object.entries(global);
let sus = all[3][1]; // global.crypto

new Uint8Array([sus]);
let v10 = new Uint8Array(100);

try {
    v10.map(undefined);
    } catch (e) {
        /* TypeError: callback argument is not callable */
        console.log('brrr');
}

console.log(sus); // segfault
                  // prolly because we are trying to access the `subtle` prop but it doesnt exist anymore(?? idk)
```

crash at [src/njs_flathsh.c:340](https://github.com/nginx/njs/blob/d34fcb03cf2378a644a3c7366d58cbddc2771cbd/src/njs_flathsh.c#L340) at line `elt_num = njs_hash_cells_end(h)[-cell_num - 1];` because `cell_num` has a value that is too big(looks like a pointer rather than an offset/array index).
gdb output:
```
(gdb)  print cell_num
$2 = 0x5024c420

(gdb)  bt
#0  0x00005555555cb085 in njs_flathsh_find (fh=fh@entry=0x555555649080, fhq=fhq@entry=0x7fffffffcd90) at src/njs_flathsh.c:340
#1  0x00005555555a0371 in njs_get_own_ordered_keys (vm=0x55555563d6d0, object=0x555555649080, parent=0x555555649080, items=0x555555699b00, flags=0x39) at src/njs_object.c:938
#2  0x00005555555a0a26 in njs_object_own_enumerate_object (vm=vm@entry=0x55555563d6d0, object=object@entry=0x555555649080, parent=parent@entry=0x555555649080, items=items@entry=0x555555699b00, flags=flags@entry=0x39) at src/njs_object.c:1158
#3  0x00005555555a24f3 in njs_object_own_enumerate_value (flags=0x39, items=0x555555699b00, parent=0x555555649080, object=0x555555649080, vm=0x55555563d6d0) at src/njs_object.c:522
#4  njs_object_own_enumerate (vm=vm@entry=0x55555563d6d0, object=0x555555649080, flags=flags@entry=0x39) at src/njs_object.c:564
#5  0x000055555556dcca in njs_value_own_enumerate (vm=0x55555563d6d0, value=0x555555649c30, flags=0x39) at src/njs_value.c:185
#6  0x00005555555abda2 in njs_json_push_stringify_state (stringify=stringify@entry=0x7fffffffd100, value=value@entry=0x555555649c30) at src/njs_json.c:988
#7  0x00005555555ad53b in njs_vm_value_dump (vm=vm@entry=0x55555563d6d0, retval=retval@entry=0x7fffffffdab0, value=value@entry=0x555555649c30, console=console@entry=0x1, indent=indent@entry=0x0) at src/njs_json.c:1999
#8  0x000055555556884c in njs_ext_console_log (vm=0x55555563d6d0, args=0x555555649c20, nargs=0x2, magic=<optimized out>, retval=0x55555565c2e8) at external/njs_shell.c:3810
#9  0x00005555555af9b5 in njs_function_native_call (retval=<optimized out>, vm=0x55555563d6d0) at src/njs_function.c:647
#10 njs_function_frame_invoke (vm=vm@entry=0x55555563d6d0, retval=<optimized out>) at src/njs_function.c:683
#11 0x000055555557861b in njs_vmcode_interpreter (vm=0x55555563d6d0, pc=0x555555658b30 "\r", rval=rval@entry=0x55555563b588, promise_cap=promise_cap@entry=0x0, async_ctx=async_ctx@entry=0x0) at src/njs_vmcode.c:1451
#12 0x000055555556f8a8 in njs_vm_start (vm=<optimized out>, retval=retval@entry=0x55555563b588) at src/njs_vm.c:664
#13 0x00005555555692c6 in njs_engine_njs_eval (engine=0x55555563b580, script=<optimized out>) at external/njs_shell.c:1387
#14 0x0000555555567fd5 in njs_process_script (engine=engine@entry=0x55555563b580, console=console@entry=0x5555556286e0 <njs_console>, script=script@entry=0x7fffffffddd0) at external/njs_shell.c:3528
#15 0x000055555556a585 in njs_process_file (opts=0x7fffffffdde0) at external/njs_shell.c:3500
#16 njs_main (opts=0x7fffffffdde0) at external/njs_shell.c:458
#17 main (argc=<optimized out>, argv=<optimized out>) at external/njs_shell.c:488
```

For the last few days I tried to find the root cause but couldn't. So I thought it's worth bringing this to your attention, hopefully you can shed some more light on this and share your insights on why it happens.



Discussion:
Hi @0xbigshaq,

Regarding the first question, what you found is a bug and not an undefined-behaviour.

The 'subtle' property is not deleted, but becomes hidden, due to improper enumerated flag copied [here](https://github.com/nginx/njs/blob/master/src/njs_extern.c#L239).

For example
```js
crypto.subtle;                                                                                                                       
crypto.subtle;    
console.log(crypto.subtle.digest); 
```
outputs:
`[Function: digest]`

2 functions are important here njs_external_add() and njs_external_prop_handler().
We have copy-on-read here: we create properties in the current context lazily and at the moment of the first access from a shared precompiled state. For example, 'subtle' property is stored as `NJS_PROPERTY_HANDLER` in shared hash which at the moment of access creates a modifiable property by calling `njs_external_prop_handler()` which puts the property in the local hash of an object.

```diff
diff --git a/src/njs_extern.c b/src/njs_extern.c
index df51f9b7..9ec1c1c9 100644
--- a/src/njs_extern.c
+++ b/src/njs_extern.c
@@ -236,11 +236,9 @@ njs_external_prop_handler(njs_vm_t *vm, njs_object_prop_t *self,
         return NJS_ERROR;
     }
 
-    if (slots != NULL) {
-        prop->writable = slots->writable;
-        prop->configurable = slots->configurable;
-        prop->enumerable = slots->enumerable;
-    }
+    prop->writable = self->writable;
+    prop->configurable = self->configurable;
+    prop->enumerable = self->enumerable;
 
     lhq.value = prop;
     njs_string_get(&self->name, &lhq.key);
```

should fix the first problem.






Patch:
diff --git a/src/njs_extern.c b/src/njs_extern.c
--- a/src/njs_extern.c
+++ b/src/njs_extern.c
@@ -236,11 +236,9 @@ njs_external_prop_handler(njs_vm_t *vm, njs_object_prop_t *self,
         return NJS_ERROR;
     }
 
-    if (slots != NULL) {
-        prop->writable = slots->writable;
-        prop->configurable = slots->configurable;
-        prop->enumerable = slots->enumerable;
-    }
+    prop->writable = self->writable;
+    prop->configurable = self->configurable;
+    prop->enumerable = self->enumerable;
 
     lhq.value = prop;
     njs_string_get(&self->name, &lhq.key);
diff --git a/src/njs_object.c b/src/njs_object.c
--- a/src/njs_object.c
+++ b/src/njs_object.c
@@ -1076,82 +1076,14 @@ njs_get_own_ordered_keys(njs_vm_t *vm, const njs_object_t *object,
 }
 
 
-static njs_int_t
-njs_add_obj_prop_kind(njs_vm_t *vm, const njs_object_t *object,
-    const njs_lvlhsh_t *hash, njs_lvlhsh_query_t *lhq,
-    uint32_t flags, njs_array_t *items)
-{
-    njs_int_t          ret;
-    njs_value_t        value, *v, value1;
-    njs_array_t        *entry;
-    njs_object_prop_t  *prop;
-
-    ret = njs_lvlhsh_find(hash, lhq);
-    if (ret != NJS_OK) {
-        return NJS_DECLINED;
-    }
-
-    prop = (njs_object_prop_t *) (lhq->value);
-
-    if (prop->type != NJS_ACCESSOR) {
-        v = njs_prop_value(prop);
-
-    } else {
-        if (njs_is_data_descriptor(prop)) {
-            v = njs_prop_value(prop);
-            goto add;
-        }
-
-        if (njs_prop_getter(prop) == NULL) {
-            v =  njs_value_arg(&njs_value_undefined);
-            goto add;
-        }
-
-        v = &value1;
-
-        njs_set_object(&value, (njs_object_t *) object);
-        ret = njs_function_apply(vm, njs_prop_getter(prop), &value, 1, v);
-        if (ret != NJS_OK) {
-            return NJS_ERROR;
-        }
-    }
-
-add:
-    if (njs_object_enum_kind(flags) != NJS_ENUM_VALUES) {
-        entry = njs_array_alloc(vm, 0, 2, 0);
-        if (njs_slow_path(entry == NULL)) {
-            return NJS_ERROR;
-        }
-
-        njs_string_copy(&entry->start[0], &prop->name);
-        njs_value_assign(&entry->start[1], v);
-
-        njs_set_array(&value, entry);
-        v = &value;
-    }
-
-    ret = njs_array_add(vm, items, v);
-    if (njs_slow_path(ret != NJS_OK)) {
-        return NJS_ERROR;
-    }
-
-    return NJS_OK;
-}
-
-
 static njs_int_t
 njs_object_own_enumerate_object(njs_vm_t *vm, const njs_object_t *object,
     const njs_object_t *parent, njs_array_t *items, uint32_t flags)
 {
-    njs_int_t           ret;
-    uint32_t            i;
-    njs_array_t         *items_sorted;
-    njs_lvlhsh_each_t   lhe;
-    njs_lvlhsh_query_t  lhq;
-
-    lhq.proto = &njs_object_hash_proto;
-
-    njs_lvlhsh_each_init(&lhe, &njs_object_hash_proto);
+    uint32_t     i;
+    njs_int_t    ret;
+    njs_array_t  *items_sorted, *entry;
+    njs_value_t  value, retval;
 
     switch (njs_object_enum_kind(flags)) {
     case NJS_ENUM_KEYS:
@@ -1174,26 +1106,31 @@ njs_object_own_enumerate_object(njs_vm_t *vm, const njs_object_t *object,
             return NJS_ERROR;
         }
 
-        for (i = 0; i< items_sorted->length; i++) {
+        njs_set_object(&value, (njs_object_t *) object);
 
-            lhe.key_hash = 0;
-            njs_object_property_key_set(&lhq, &items_sorted->start[i],
-                                        lhe.key_hash);
+        for (i = 0; i< items_sorted->length; i++) {
+            ret = njs_value_property(vm, &value, &items_sorted->start[i],
+                                     &retval);
+            if (njs_slow_path(ret != NJS_OK)) {
+                njs_array_destroy(vm, items_sorted);
+                return NJS_ERROR;
+            }
 
-            ret = njs_add_obj_prop_kind(vm, object, &object->hash, &lhq, flags,
-                                        items);
-            if (ret != NJS_DECLINED) {
-                if (ret != NJS_OK) {
+            if (njs_object_enum_kind(flags) != NJS_ENUM_VALUES) {
+                entry = njs_array_alloc(vm, 0, 2, 0);
+                if (njs_slow_path(entry == NULL)) {
                     return NJS_ERROR;
                 }
 
-            } else {
-                ret = njs_add_obj_prop_kind(vm, object, &object->shared_hash,
-                                            &lhq, flags, items);
-                njs_assert(ret != NJS_DECLINED);
-                if (ret != NJS_OK) {
-                    return NJS_ERROR;
-                }
+                njs_string_copy(&entry->start[0], &items_sorted->start[i]);
+                njs_value_assign(&entry->start[1], &retval);
+
+                njs_set_array(&retval, entry);
+            }
+
+            ret = njs_array_add(vm, items, &retval);
+            if (njs_slow_path(ret != NJS_OK)) {
+                return NJS_ERROR;
             }
         }
 


Test patch:
diff --git a/src/test/njs_unit_test.c b/src/test/njs_unit_test.c
--- a/src/test/njs_unit_test.c
+++ b/src/test/njs_unit_test.c
@@ -21819,6 +21819,11 @@ static njs_unit_test_t  njs_webcrypto_test[] =
     { njs_str("let buf = new Uint32Array(4);"
               "buf === crypto.getRandomValues(buf)"),
       njs_str("true") },
+
+    { njs_str("crypto.subtle;"
+              "var d = Object.getOwnPropertyDescriptor(crypto, 'subtle');"
+              "d.enumerable && !d.configurable && d.writable"),
+      njs_str("true") },
 };
 
 
@@ -22688,6 +22693,12 @@ static njs_unit_test_t  njs_shared_test[] =
     { njs_str("var v = Math.round(Math.random() * 1000); ExternalNull.set(v);"
               "ExternalNull.get() == v"),
       njs_str("true") },
+
+#if (NJS_HAVE_OPENSSL)
+    { njs_str("var cr = Object.entries(global).filter((v) => v[0] == 'crypto')[0][1];"
+              "cr.abc = 1; cr.abc"),
+      njs_str("1") },
+#endif
 };