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.

Sign up for GitHub

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

Jump to bottom

Heap overflow in CreateTTDDirectoryAsNeeded #3434

Closed
rajatd opened this issue Jul 26, 2017 · 6 comments
Closed

Heap overflow in CreateTTDDirectoryAsNeeded #3434

rajatd opened this issue Jul 26, 2017 · 6 comments
Assignees
@mike-kaufman
Labels
Bug

Comments

@rajatd
Copy link
Contributor

rajatd commented Jul 26, 2017

Repro:
build chakracore on linux:
./build.sh --cc=/usr/bin/clang --cxx=/usr/bin/clang++ --arch=amd64 --debug --static -j 8 --sanitize=address,undefined,signed-integer-overflow

run ./ch - from /out/Debug/bin/ch

output:

==67392==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200000eec4 at pc 0x7f4ec99064bf bp 0x7ffcc6c0ab30 sp 0x7ffcc6c0ab28
READ of size 2 at 0x60200000eec4 thread T0
#0 0x7f4ec99064be in PAL_wcslen /home/chakraut/ChakraCore/pal/src/cruntime/wchar.cpp:1212:12
#1 0x7f4ec9845b78 in Helpers::CreateTTDDirectoryAsNeeded(unsigned long*, char*, char const*, char16_t const*) /home/chakraut/ChakraCore/bin/ch/Helpers.cpp:440:41
#2 0x7f4ec98462a0 in Helpers::GetTTDDirectory(char16_t const*, unsigned long*, char*, unsigned long) /home/chakraut/ChakraCore/bin/ch/Helpers.cpp:499:5
#3 0x7f4ec9834a79 in main /home/chakraut/ChakraCore/bin/ch/ch.cpp:986:13
#4 0x7f4ec776c2b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)
#5 0x7f4ec975cf79 in _start (/home/chakraut/ChakraCore/out/Debug/bin/ch/ch+0xb60f79)

0x60200000eec4 is located 12 bytes to the left of 10-byte region [0x60200000eed0,0x60200000eeda)
allocated by thread T0 here:
#0 0x7f4ec97fb578 in __interceptor_malloc (/home/chakraut/ChakraCore/out/Debug/bin/ch/ch+0xbff578)
#1 0x7f4ec98cc577 in CorUnix::InternalMalloc(unsigned long) /home/chakraut/ChakraCore/pal/src/cruntime/malloc.cpp:114:20
#2 0x7f4ec98cc544 in PAL_malloc /home/chakraut/ChakraCore/pal/src/cruntime/malloc.cpp:98:12
#3 0x7f4ec9839204 in utf8::malloc_allocator::allocate(unsigned long) /home/chakraut/ChakraCore/lib/Common/Codex/Utf8Helper.h:128:53
#4 0x7f4ec9838cc6 in int utf8::NarrowStringToWide<void* ()(unsigned long)>(void ()(unsigned long), char const, unsigned long, char16_t**, unsigned long*, unsigned long*) /home/chakraut/ChakraCore/lib/Common/Codex/Utf8Helper.h:72:37
#5 0x7f4ec9838c4e in int utf8::NarrowStringToWideutf8::malloc_allocator(char const*, unsigned long, char16_t**, unsigned long*, unsigned long*) /home/chakraut/ChakraCore/lib/Common/Codex/Utf8Helper.h:122:16
#6 0x7f4ec98373a1 in utf8::NarrowStringToWideDynamic(char const*, char16_t**) /home/chakraut/ChakraCore/lib/Common/Codex/Utf8Helper.h:142:16
#7 0x7f4ec9834422 in main /home/chakraut/ChakraCore/bin/ch/ch.cpp:900:9
#8 0x7f4ec776c2b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/chakraut/ChakraCore/pal/src/cruntime/wchar.cpp:1212:12 in PAL_wcslen
Shadow bytes around the buggy address:
0x0c047fff9d80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c047fff9d90: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c047fff9da0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c047fff9db0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c047fff9dc0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c047fff9dd0: fa fa fa fa fa fa 04 fa[fa]fa 00 02 fa fa 00 00
0x0c047fff9de0: fa fa 00 00 fa fa 06 fa fa fa 02 fa fa fa 02 fa
0x0c047fff9df0: fa fa fd fd fa fa 00 fa fa fa 04 fa fa fa 00 fa
0x0c047fff9e00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c047fff9e10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c047fff9e20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
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
Heap right redzone: fb
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack partial redzone: f4
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
==67392==ABORTING
@rajatd rajatd assigned mrkmarron and unassigned mrkmarron Jul 26, 2017
@dilijev dilijev added the Bug label Jul 26, 2017
@mrkmarron
Copy link
Contributor

It looks like this is a bug in the PAL implementation of wcsstr or something in the string conversion.

Getting to a call to GetTTDDirectory requires a check of the form:

else if(wcsstr(argv[i], _u("-TTRecord=")) == argv[i])

The documentation for wcsstr says it returns "A pointer to the first occurrence in wcs1 of the entire sequence of characters specified in wcs2, or a null pointer if the sequence is not present in wcs1." However, if the input is ./ch - then this test should definitely fail.

@kfarnung
Copy link
Contributor

kfarnung commented Aug 8, 2017

This looks similar to a bug I was investigating, let me take a look.

@kfarnung
Copy link
Contributor

kfarnung commented Aug 9, 2017

This is because the PAL implementation of wcsstr doesn't handle the case where the string reaches a null character before strCharSet has fully matched.

https://github.com/Microsoft/ChakraCore/blob/master/pal/src/cruntime/wchar.cpp#L1496

This means that

wchar_t* foo = "-";
wcsstr(foo, "-TTDRecord=") == foo // this should be false, but it's true

@kfarnung
Copy link
Contributor

kfarnung commented Aug 9, 2017

Looks like the same bug exists in coreclr as well: https://github.com/dotnet/coreclr/blob/master/src/pal/src/cruntime/wchar.cpp

mike-kaufman pushed a commit to mike-kaufman/ChakraCore that referenced this issue Aug 17, 2017
fixing bug chakra-core#3434. wcsstr would return a match if the strin…
7c76a29 
…g being searched ended with a prefix of the search string. Fixed this to behave correctly. Also added tests to test_native.sh to verify this.
@mike-kaufman mike-kaufman mentioned this issue Aug 17, 2017
Closed
mike-kaufman referenced this issue in mike-kaufman/ChakraCore Aug 17, 2017
Merge branch 'master' into build/mkaufman/fix-wcsstr-bug-gh-3434-rebased
a042308
mike-kaufman referenced this issue in mike-kaufman/ChakraCore Aug 17, 2017
Merge branch 'master' into build/mkaufman/fix-wcsstr-bug-gh-3434-rebased
e13263c
@mike-kaufman mike-kaufman mentioned this issue Aug 18, 2017
Merged
mike-kaufman pushed a commit to mike-kaufman/ChakraCore that referenced this issue Aug 20, 2017
Fixing bug in pal's wcsstr.
6efc969 
Fixing chakra-core#3434.  Bug in wcsstr
would return a match if the string being searched ended with a prefix of the
search string.  Fixed this to behave correctly.  Also added tests to
test_native.sh to verify this.
mike-kaufman pushed a commit to mike-kaufman/ChakraCore that referenced this issue Aug 20, 2017
Fixing bug in pal's wcsstr.
74c873b 
Fixing chakra-core#3434.  Bug in wcsstr
would return a match if the string being searched ended with a prefix of the
search string.  Fixed this to behave correctly.  Also added tests to
test_native.sh to verify this.

adding additional tests
mike-kaufman pushed a commit to mike-kaufman/ChakraCore that referenced this issue Aug 20, 2017
Fixing bug in pal's wcsstr.
a870f6f 
Fixing chakra-core#3434.  Bug in wcsstr
would return a match if the string being searched ended with a prefix of the
search string. Also, fixing another bug with compat with win32 API.  If
search string is zero length, we should return the input string.  Added tests
to test_native.sh to verify behavior of wcsstr.
mike-kaufman pushed a commit to mike-kaufman/ChakraCore that referenced this issue Aug 20, 2017
Fixing bug in pal's wcsstr.
561b04d 
Fixing chakra-core#3434.  Bug in wcsstr
would return a match if the string being searched ended with a prefix of the
search string. Also, fixing another bug with compat with win32 API.  If
search string is zero length, we should return the input string.  Added tests
to test_native.sh to verify behavior of wcsstr.
mike-kaufman pushed a commit to mike-kaufman/ChakraCore that referenced this issue Aug 21, 2017
Fixing bug in pal's wcsstr.
67b1cb4 
Fixing chakra-core#3434.  Bug in wcsstr
would return a match if the string being searched ended with a prefix of the
search string. Also, fixing another bug with compat with win32 API.  If
search string is zero length, we should return the input string.  Added tests
to test_native.sh to verify behavior of wcsstr.
chakrabot pushed a commit that referenced this issue Aug 21, 2017
[MERGE #3549 @mike-kaufman] fixing bug in wcssstr
ffdeb1d 
Merge pull request #3549 from mike-kaufman:build/mkaufman/fix-wcsstr-bug-gh-3434-rebased

Fixing #3434..  Bug in wcsstr would return a match if the string being searched ended with a prefix of the search string.  Fixed this to behave correctly.  Also added tests to test_native.sh to verify this.

This is re-opened from PR #3542.
chakrabot added a commit to nodejs/node-chakracore that referenced this issue Aug 22, 2017
@chakrabot
[Merge chakra-core/ChakraCore@ffdeb1d54b] [MERGE #3549 @mike-kaufman]…
1cf3e19 
… fixing bug in wcssstr

Merge pull request #3549 from mike-kaufman:build/mkaufman/fix-wcsstr-bug-gh-3434-rebased

Fixing chakra-core/ChakraCore#3434..  Bug in wcsstr would return a match if the string being searched ended with a prefix of the search string.  Fixed this to behave correctly.  Also added tests to test_native.sh to verify this.

This is re-opened from PR #3542.
@mike-kaufman
Copy link
Contributor

Fixed with #3549

@mike-kaufman
Copy link
Contributor

Looks like the same bug exists in coreclr as well: https://github.com/dotnet/coreclr/blob/master/src/pal/src/cruntime/wchar.cpp

Fixed in coreclr with PR dotnet/coreclr#13504.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mike-kaufman mike-kaufman

Labels
Bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@dilijev @kfarnung @mike-kaufman @mrkmarron @rajatd