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

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 and link it here.

NGINX configuration if applicable
```
# Your NGINX configuration here
```
or put the configuration in a gist and link it here.
NGINX logs if applicable
```
# Your NGINX logs here
```
or post the full log to a gist 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:

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 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.

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.

For example

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 --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.

Hi @xeioex You're right, it's hidden, not deleted. I applied the patch and it fixed the problem of the key being hidden. However, the segfault remains(of the last snippet). Perhaps this is a different bug? I wonder if I should've seperate it into two tickets. One for the disappearing key and one for the segfault(i thought it was the same root cause, at least that's the feeling I had)

I'll try to look at it in the next few days & update here if I find anything, hopefully I'll have an idea to why this happens.

Hi @0xbigshaq,

I agree there is a second bug, but I cannot find the root cause right away. I plan to look into it as time permits. One issue it enough I believe.

The bug is in Object.values() and Object.entries() which are implemented by njs_object_own_enumerate_object().

The function erroneously puts a shared object from a shared_hash directly to a local hash of the object. Normally, to support lazy instantiation of properties, when accessing an object with shared field set it should be copied and put to a local hash for all future references. As it is done during normal properties retrieval by njs_prop_private_copy(). The fix is to use normal njs_value_property() in njs_object_own_enumerate_object() for props retrieval.

Here is the test, that shows the issue with ./configure --address-sanitizer=YES --debug-memory=YES

 var crypto = Object.entries(global).filter((v) => v[0] == 'crypto')[0][1]  // this makes a direct copy of the shared crypto object  
crypto.populate_local_hash = 1    // this erroneously modified a local hash of the shared object
var second_crypto = global.crypto   // it makes a copy of a shared object, and inherits a copy of local hash of crypto
second_crypto.subtle    
// this reallocate local hash, the same as in crypto    
// but update here is not reflected in crypto    
// as a result hash in crypto points to freed memory    
second_crypto.getRandomValues     
console.log(crypto) // accesses freed memory though local hash

In case of your test, it can be simplified to:

let all = Object.entries(global);    
let sus = all[3][1];                                                                                                                 

sus.abc = 1    

try {    
    Uint8Array.prototype.map.call(1, undefined)    

} catch (e) {    
    sus.abc    
}

Here we have the same issue, but it is triggered differently. sus is the same as crypto in my test equivalent to second_crypto appears during global object traversal which we do to find the names of the functions in a backtrace here. While looking for function names we recursively traverse all the properties in the global object. As a result we fetch the second copy of crypto which is copied, as explained above.

nginx / njs