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

Fix oob in X86_insn_reg_intel #725

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
7 participants
@alvarofe
Copy link
Contributor

commented Jun 29, 2016

I just detected an oob when doing things with radare. I can't help but wonder why isn't there test for capstone? I couldn't find anywhere.

@alvarofe alvarofe force-pushed the alvarofe:fix branch from b00c910 to 8ef7b2c Jun 29, 2016

@aquynh

This comment has been minimized.

Copy link
Owner

commented Jun 29, 2016

Why do we have OOB bug here?

@alvarofe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2016

I don't know the reason exactly why but access 2147483647 max 115 first 0 last -1 means that for an array of 115 (insn_regs_intel) was accessing to UT32_MAX because last -1. But with the use of unsigned int the while loop condition was always true. The reason why last == -1 (the size of insn_regs_intel == 0?) i don't know I didn't research further I just fixed it to try to fix a bug in radare2

@danghvu

This comment has been minimized.

Copy link
Collaborator

commented Jun 29, 2016

So something is wrong with ARR_SIZE(insn_regs_intel) ? why is it returning 0 ? Can you confirm it is returning 0 ? If it is the case, this is not the right patch.

@aquynh

This comment has been minimized.

Copy link
Owner

commented Jun 30, 2016

how to reproduce this bug?

@alvarofe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2016

clone radare and remove the patch of capstone in the folder shlr/capstone-fixes and load this binary http://delphiforfun.org/Programs/Download/CountGridCells.zip after that aac

@aquynh

This comment has been minimized.

Copy link
Owner

commented Jul 8, 2016

@crowell, can you confirm this issue?

@crowell

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2016

Yes I see the crash too, the patch is safe and we're using in radare
On Jul 7, 2016 11:42 PM, "Nguyen Anh Quynh" notifications@github.com
wrote:

@crowell https://github.com/crowell, can you confirm this issue?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#725 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAlnYG2A9fVzppLAn8hk8ZhGnLZv1B69ks5qTcb7gaJpZM4JBjI0
.

@aquynh

This comment has been minimized.

Copy link
Owner

commented Jul 8, 2016

can you explain how this is triggered? i dont see how ARR_SIZE can return 0 here.

@ghost

This comment has been minimized.

Copy link

commented Jul 10, 2016

==30051==ERROR: AddressSanitizer: SEGV on unknown address 0x7f11300b33b4 (pc 0x7f0b2f827c2d bp 0x7ffdc67f0910 sp 0x7ffdc67f0880 T0)
==30051==The signal is caused by a READ memory access.
    #0 0x7f0b2f827c2c in X86_insn_reg_intel /home/fuzzer/Fuzzers/capstone-next/arch/X86/X86Mapping.c:2784:7
    #1 0x7f0b2f7ecad2 in X86_Intel_printInst /home/fuzzer/Fuzzers/capstone-next/arch/X86/X86IntelInstPrinter.c:729:8
    #2 0x7f0b2f466a51 in cs_disasm /home/fuzzer/Fuzzers/capstone-next/cs.c:664:4
    #3 0x4f2b9c in LLVMFuzzerTestOneInput (/home/fuzzer/Fuzzers/capstone_fuzzer+0x4f2b9c)
    #4 0x4fa02c in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/fuzzer/Fuzzers/Fuzzer/FuzzerLoop.cpp:488:13
    #5 0x4f9870 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long) /home/fuzzer/Fuzzers/Fuzzer/FuzzerLoop.cpp:444:3
    #6 0x4fa0e2 in fuzzer::Fuzzer::RunOneAndUpdateCorpus(unsigned char const*, unsigned long) /home/fuzzer/Fuzzers/Fuzzer/FuzzerLoop.cpp:465:7
    #7 0x4fb583 in fuzzer::Fuzzer::MutateAndTestOne() /home/fuzzer/Fuzzers/Fuzzer/FuzzerLoop.cpp:678:5
    #8 0x4fbae7 in fuzzer::Fuzzer::Loop() /home/fuzzer/Fuzzers/Fuzzer/FuzzerLoop.cpp:766:5
    #9 0x4f4cd2 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /home/fuzzer/Fuzzers/Fuzzer/FuzzerDriver.cpp:416:5
    #10 0x4ff530 in main /home/fuzzer/Fuzzers/Fuzzer/FuzzerMain.cpp:21:10
    #11 0x7f0b2e067abf in __libc_start_main /build/glibc-qbmteM/glibc-2.21/csu/libc-start.c:289
    #12 0x41aa78 in _start (/home/fuzzer/Fuzzers/capstone_fuzzer+0x41aa78)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/fuzzer/Fuzzers/capstone-next/arch/X86/X86Mapping.c:2784:7 in X86_insn_reg_intel
==30051==ABORTING
0x46,0xf2,0xf,0xc2,0xb0,0x54,0xa1,0xcc,0x0,0xde,0xd9,0xe1,0x80,0x31,0xa1,0xde,0xb0,0x34,0x4e,0x40,0xcd,0xcc,0x97,0x69,0xe4,0x30,0xe1,0x97,0x44,0xde,0x69,0x6e,0x80,0xb0,0x31,0xa7,0xcd,0xde,
F\xf2\x0f\xc2\xb0T\xa1\xcc\x00\xde\xd9\xe1\x801\xa1\xde\xb04N@\xcd\xcc\x97i\xe40\xe1\x97D\xdein\x80\xb01\xa7\xcd\xde
artifact_prefix='./'; Test unit written to ./crash-b84724713cb49e1456f93ba73536584e63a4061e
Base64: RvIPwrBUocwA3tnhgDGh3rA0TkDNzJdp5DDhl0TeaW6AsDGnzd4=
@Maijin

This comment has been minimized.

Copy link

commented Jul 26, 2016

Ping

@aquynh

This comment has been minimized.

Copy link
Owner

commented Jul 26, 2016

this report is pretty confused. can you answer the question of @danghvu above?

@ghost

This comment has been minimized.

Copy link

commented Jul 26, 2016

@aquynh just replace CODE with RvIPwrBUocwA3tnhgDGh3rA0TkDNzJdp5DDhl0TeaW6AsDGnzd4= pasted in my comment in your C example code http://www.capstone-engine.org/lang_c.html. Obviously not in base64, and compile with -fsanitize=address

@radare

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2016

tumblr_ma98p4y6gt1qjdbzao1_500

@aquynh

This comment has been minimized.

Copy link
Owner

commented Aug 27, 2016

this is a wrong fix. i just pushed a commit for this issue, can you confirm?

@ghost

This comment has been minimized.

Copy link

commented Sep 6, 2016

Mitre assigned CVE-2016-7151 to 87a25bb

@aquynh

This comment has been minimized.

Copy link
Owner

commented Sep 6, 2016

interesting! just wondering how they stick that to this bug in the "next" branch, which has no actual version.

@ghost

This comment has been minimized.

Copy link

commented Sep 6, 2016

No need to have a X version of there, hence a CVE link to this commit for example in a situation that someone uses a fork or your next branch or as upstream. Anyway still waiting your release after months in your stable branch, and there are already some CVEs assigned that should be notified in a future changelog in case people wonder to upgrade or move to 'next' branch.

@radare

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2017

still waiting for this thing to be merged.. almost a year have passed and this issue has been confirmed by a bunch of peeps. any interest in fixing or should we keep maintaining forks and patches?

@aquynh

This comment has been minimized.

Copy link
Owner

commented Apr 14, 2017

@radare

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2017

it does fixes the crash, the code has been already provided in hexa and base64 by @revskills , and having this out of the crash is also easy. what else do you need?

i cant reproduce the issue with last capstone, in fact i removed the patch in r2 after upgrading to capstone d3155db . so i think it was silently fixed by someone else.

@revskills @alvarofe can you confirm this? i cant reproduce with this:

valgrind cstool/cstool x64 "46 f2 0f c2 b0 54 a1 cc00 de d9 e1 80 31 a1 de b0 34 4e 40 cd cc 97 69e4 30 e1 97 44 de 69 6e80b031a7cdde"
@vit9696

This comment has been minimized.

Copy link
Contributor

commented May 16, 2017

TL;DR The current implementation is less efficient speed-wise but it does not have this bug.
I feel pretty frightened to discover such issues given that I use capstone in the kernel but I guess a security audit is what you have to do on your own with opensource sw :/

Explanation:
Assume insn_regs_intel_sorted has 2 instructions with ids 1 and 2 and id passed is 0.

Before the first iteration the variables will be: first = 0; last = 1; mid = 1;
Next else branch will be taken and thus: last = 0; mid = 0;
Given first == last another iteration happens and else branch is taken once again, and thus last is assigned mid - 1, i.e. 0 - 1 unsigned. Which leads to UB.

X86_REG_INVALID has id == 0, and insn_regs_intel has no entry with such an id, so a non zero identifier is the smallest. For this reason whenever the previous implementation of X86_insn_reg_intel got called with X86_REG_INVALID could go nuts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.