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

stack-buffer-overflow src/jsutils.c:751 in vcbprintf #2114

Closed
hope-fly opened this issue Dec 15, 2021 · 6 comments
Closed

stack-buffer-overflow src/jsutils.c:751 in vcbprintf #2114

hope-fly opened this issue Dec 15, 2021 · 6 comments

Comments

@hope-fly
Copy link

hope-fly commented Dec 15, 2021

Espruino revision

Commit: 0a9f07a0
Version: 2v10.246

Build environment

Ubuntu 18.04.5 LTS (Linux 5.4.0-44-generic x86_64)

Build steps
export CCFLAGS='-g -fsanitize=address -fno-omit-frame-pointer'
make clean 
make
Test case
function JSEtest() {
    let v1 = 0;
    const v5 = [1337];
    const v6 = [];
    const v7 = {
        constructor: v6,
        d: parseInt,
        __proto__: v5,
        valueOf: 1782977947,
        e: v5
    };
    let v8 = v7;
    const v10 = [
        v1,
        1337,
        1337,
        1337,
        1337
    ];
    let v11 = v10;
    let v13 = v8;
    let v16 = 5;
    const v20 = JSON.stringify(v13, v11, v13);
    const v21 = JSON.parse(v20, v11);
}
JSEtest();
Execution & Output
./Espruino/espruino poc.js

=================================================================
ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffe05f1053b at pc 0x562494da412c bp 0x7ffe05f0fdc0 sp 0x7ffe05f0fdb0
READ of size 1 at 0x7ffe05f1053b thread T0
    #0 0x562494da412b in vcbprintf src/jsutils.c:751
    #1 0x562494da532a in cbprintf src/jsutils.c:850
    #2 0x562494ff94e0 in jsonNewLine src/jswrap_json.c:243
    #3 0x562494ff94e0 in jsfGetJSONForObjectItWithCallback src/jswrap_json.c:267
    #4 0x562494ff6572 in jsfGetJSONWithCallback src/jswrap_json.c:450
    #5 0x562494ff9b52 in jsfGetJSONWhitespace src/jswrap_json.c:490
    #6 0x562494ff9b52 in jswrap_json_stringify src/jswrap_json.c:73
    #7 0x562494dad1e7 in jsnCallFunction src/jsnative.c:220
    #8 0x562494db69db in jspeFunctionCall src/jsparse.c:609
    #9 0x562494db7cc7 in jspeFactorFunctionCall src/jsparse.c:1184
    #10 0x562494db8651 in jspePostfixExpression src/jsparse.c:1786
    #11 0x562494dbc208 in jspeBinaryExpression src/jsparse.c:1955
    #12 0x562494dbc208 in jspeConditionalExpression src/jsparse.c:1991
    #13 0x562494dbc208 in jspeAssignmentExpression src/jsparse.c:2050
    #14 0x562494dbc208 in jspeStatementVar src/jsparse.c:2165
    #15 0x562494dc080c in jspeBlockNoBrackets src/jsparse.c:2084
    #16 0x562494db6f3f in jspeFunctionCall src/jsparse.c:796
    #17 0x562494db7cc7 in jspeFactorFunctionCall src/jsparse.c:1184                   
    #18 0x562494db8651 in jspePostfixExpression src/jsparse.c:1786
    #19 0x562494dba35a in jspeBinaryExpression src/jsparse.c:1955
    #20 0x562494dba35a in jspeConditionalExpression src/jsparse.c:1991                 
    #21 0x562494dba35a in jspeAssignmentExpression src/jsparse.c:2050
    #22 0x562494dba35a in jspeExpression src/jsparse.c:2056
    #23 0x562494dc1488 in jspeBlockOrStatement src/jsparse.c:2124
    #24 0x562494dc262e in jspParse src/jsparse.c:2136
    #25 0x562494dca2fd in jspEvaluateVar src/jsparse.c:2992
    #26 0x562494dca2fd in jspEvaluate src/jsparse.c:3022
    #27 0x562494cd0acd in main targets/linux/main.c:460
    #28 0x7f11add79bf6 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21bf6)
    #29 0x562494cd33b9 in _start (/root/Espruino/espruino+0x4c3b9)
Address 0x7ffe05f1053b is located in stack of thread T0 at offset 139 in frame
    #0 0x562494ff98ef in jswrap_json_stringify src/jswrap_json.c:56
This frame has 2 object(s):
[32, 88) 'it'[128, 139) 'whitespace' <== Memory access at offset 139 overflows this variable

SUMMARY: AddressSanitizer: stack-buffer-overflow src/jsutils.c:751 in vcbprintf
Shadow bytes around the buggy address:
  0x100040bda050: 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 f2
  0x100040bda060: f2 f2 f2 f2 f2 f2 00 f2 f2 f2 f2 f2 f2 f2 00 f2
  0x100040bda070: f2 f2 f2 f2 f2 f2 00 00 00 00 00 00 00 00 00 00
  0x100040bda080: 00 00 f3 f3 f3 f3 00 00 00 00 00 00 00 00 00 00
  0x100040bda090: 00 00 00 00 00 00 f1 f1 f1 f1 00 00 00 00 00 00
=>0x100040bda0a0: 00 f2 f2 f2 f2 f2 00[03]f2 f2 00 00 00 00 00 00
  0x100040bda0b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100040bda0c0: f1 f1 f1 f1 00 00 00 00 00 00 00 00 00 00 00 00
  0x100040bda0d0: f2 f2 f2 f2 00 00 00 00 00 00 00 00 00 00 00 00
  0x100040bda0e0: f3 f3 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
  0x100040bda0f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
====ABORTING

@gfwilliams
Copy link
Member

Great - thanks for the report!

@jes
Copy link
Contributor

jes commented Jan 25, 2022

This could be because there are cases where the loop can accidentally skip over the trailing nul in the format string. E.g. line 771 and 778 both increment fmt, but there isn't a nul check in between.

I think this should only happen on non-well-formed format strings (e.g. the string "%0") so I don't know how the problem came up in practice. We should probably add "if (!*fmt) break" every time fmt is about to be incremented, in cases where it could conceivably have already been incremented without being checked.

From a quick check, I think strings ending with "%0" are the only way to accidentally walk past the trailing nul.

@jes
Copy link
Contributor

jes commented Jan 25, 2022

I have not succeeded in reproducing the problem with this proof-of-concept. I'm on Ubuntu 20.04 LTS.

Here's a recording: https://asciinema.org/a/V11UdZqa4GXbbL5eLDFjZYtMn

It's not made explicit in the recording, but I checked out commit 0a9f07a.

Changes I made were:
1.) I had to have -faddress=sanitize in LDFLAGS as well as in CCFLAGS, otherwise it can't link.
2.) I modified the proof-of-concept to remove the JSON.parse() call, just to clarify that the reported syntax error is from JSON.parse() rather than from the poc.js source.

I don't get any output from AddressSanitizer.

@hope-fly can you give more information on how to reproduce?

@gfwilliams
Copy link
Member

Ok, just got it. It's actually jswrap_json_stringify (further up the stack) that sets the whitespace string up.

Looks like if you pass an object for the whitespace arg of stringify it's converted to a string, but isn't zero-terminated correctly so then later on cbprintf reads past the end of the string.

print(JSON.stringify({a:{b:1}},null,{ thisx: 324325235325 }))

The one this issue this missed which is really scary is what happens if you put in "%d" as the padding string. I'm putting in a fix now.

But I should note I couldn't reproduce either - I just saw the line using whitespace and checked back to where it was set.

@jes
Copy link
Contributor

jes commented Jan 25, 2022

Note the bug with a format string of "%0" is still there, although it's not obvious to me that it's possible to trigger it from JS, every cbprintf call I can find has the format string coming from C.

@gfwilliams
Copy link
Member

Thanks - good point. I'll get a fix in for that

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

No branches or pull requests

3 participants