Fix OOB in the Gameboy instruction decoder ##anal #17088

condret · 2020-06-16T15:49:20Z

Your checklist for this pull request

I've read the guidelines for contributing to this repository
I made sure to follow the project's coding style
I've added tests that prove my fix is effective or that my feature works (if possible)
I've updated the documentation and the radare2 book with the relevant information (if needed)

Detailed description

...

Test plan

...

Closing issues

...

ret2libc · 2020-06-16T16:12:07Z

Filling this template sucks -->

The only thing that sucks at the moment is that you didn't fill it with, at least, the output of ASAN I provided. When in 3 months from now someone will look at this PR, they will have no idea why this change was done. There are reasons why there is this template to file.

thestr4ng3r

Please add a description of what this is even for.

trufae · 2020-06-17T09:31:17Z

libr/anal/p/anal_gb.c

@@ -714,7 +714,10 @@ static bool gb_custom_daa (RAnalEsil *esil) {
 static int gb_anop(RAnal *anal, RAnalOp *op, ut64 addr, const ut8 *data, int len, RAnalOpMask mask) {
 	int ilen = gbOpLength (gb_op[data[0]].type);
 	if (ilen > len) {
-		ilen = 0;
+		op->addr = addr;
+		op->type = R_ANAL_OP_TYPE_UNK;


i would say that if the instruction trying to be disassembled is bigger than the provided buffer the type should be INV (as for invalid) and about op->size. i would keep the original opsize, and just return 0 or -1. because this way the user knows how many bytes must be provided to get the disasm of that instruction.

also i think op->addr is filled by the caller of this callback. so no need to set it in here.

sorry for the delay, my laptop broke and i had to wait for spare parts to repair

"also i think op->addr is filled by the caller of this callback. so no need to set it in here." - why do you pass addr to plugin->op then, if you already pass op to it?

trufae · 2020-06-17T09:33:57Z

Can you provide a test for this bug? any reproducer? stuff like this must be properly defined in the documentation and stated in the unit test for the RAsm api. because invalid,unaligned,truncated are valid events in r2 disasm but it's probably not clear enough how to handle this without reading other plugin's code

trufae · 2020-06-20T10:35:13Z

ping @condret

trufae

You can add a test like this:

$ rasm2 -a gb -d 30
invalid

$ r2 -qc 'pad 30@a:gb' -
invalid

But i think there was a reproducer somewhere, anyway

trufae · 2020-06-27T22:24:35Z

libr/anal/p/anal_gb.c

+		op->addr = addr;
+		op->type = R_ANAL_OP_TYPE_UNK;
+		op->size = 0;
+		return 0;
 	} else if (mask & R_ANAL_OP_MASK_DISASM) {


don't else after return

trufae · 2020-06-27T22:28:18Z

libr/anal/p/anal_gb.c

@@ -714,7 +714,10 @@ static bool gb_custom_daa (RAnalEsil *esil) {
 static int gb_anop(RAnal *anal, RAnalOp *op, ut64 addr, const ut8 *data, int len, RAnalOpMask mask) {
 	int ilen = gbOpLength (gb_op[data[0]].type);
 	if (ilen > len) {
-		ilen = 0;
+		op->addr = addr;


i think this field is set by the caller, so you dont need to define it here

I'm not sure about that. Also if that is the case, why do we pass addr to plugin->op anyway?

Good question, we can probably get rid of this argument too

But i think it is better to focus on rarch refactoring to do that

so far this plugin has set the addr on it's own, and I believe that other plugins do that as well. so not sure if removing that line would be good

trufae · 2020-06-27T22:29:06Z

libr/anal/p/anal_gb.c

-		ilen = 0;
+		op->addr = addr;
+		op->type = R_ANAL_OP_TYPE_UNK;
+		op->size = 0;


you may set op->size = ilen; this way the caller knows the size of the instruction that was not possible to disassebmle because of lack of data provided

well, I can do that, but that requires the caller to check the return value and the value in op. ... Maybe one should consider making plugin->op return a bool instead of length?

Yeah seems like an option here. Enforcing opsize only in one place and return bool to specify success or not. The reason can be found in the ranalop struct if needed. But that should be done in another pr. Kinda big change i guess

yes, so for now, I'd like to keep this as it is

condret · 2020-06-30T22:56:42Z

@thestr4ng3r I don't see what is so hard to understand about "fix oob heap access".

thestr4ng3r · 2020-07-01T09:31:26Z

It's unclear from the diff where the oob comes from without a reproducer.

trufae · 2020-07-01T17:12:04Z

op.addr is overwritten after calling the plugin callback

condret · 2020-07-01T17:57:30Z

Ok, I will change that and also check other plugins and open issues for them

condret · 2020-07-01T22:19:06Z

here some copypasta showing the problem.

[=================================================================                                                                                            
==1320272==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6250002a00a0 at pc 0x7fdbeace9c93 bp 0x7ffeea7d4db0 sp 0x7ffeea7d4da0                                                                                                                                                                                  
READ of size 1 at 0x6250002a00a0 thread T0                                     
    #0 0x7fdbeace9c92 in gb_anop ../libr/anal/p/anal_gb.c:821                                                                                                 
    #1 0x7fdbeac50dbb in r_anal_op ../libr/anal/op.c:107                                                                                                      
    #2 0x7fdbebabed43 in r_core_anal_search_xrefs ../libr/core/canal.c:3967                                                                                   
    #3 0x7fdbebbd46fe in r_core_anal_refs ../libr/core/cmd_anal.c:8617                                                                                        
    #4 0x7fdbebbd8bc0 in cmd_anal_all ../libr/core/cmd_anal.c:9194                                                                                            
    #5 0x7fdbebbde8a1 in cmd_anal ../libr/core/cmd_anal.c:10102                                                                                               
    #6 0x7fdbebcacd15 in r_cmd_call_parsed_args ../libr/core/cmd_api.c:347                                                                                    
    #7 0x7fdbebc8e93c in handle_ts_arged_command_internal ../libr/core/cmd.c:5000                                                                             
    #8 0x7fdbebc8e0c8 in handle_ts_arged_command ../libr/core/cmd.c:4959                                                                                      
    #9 0x7fdbebca3d69 in handle_ts_command ../libr/core/cmd.c:6496                                                                                            
    #10 0x7fdbebca47ec in handle_ts_commands_internal ../libr/core/cmd.c:6551                                                                                 
    #11 0x7fdbebca40b3 in handle_ts_commands ../libr/core/cmd.c:6516                                                                                          
    #12 0x7fdbebca5317 in core_cmd_tsr2cmd ../libr/core/cmd.c:6642                                                                                            
    #13 0x7fdbebca57da in r_core_cmd ../libr/core/cmd.c:6685                                                                                                  
    #14 0x7fdbebca6731 in r_core_cmd0 ../libr/core/cmd.c:6892                                                                                                 
    #15 0x7fdbede75f55 in r_main_radare2 ../libr/main/radare2.c:1297                                                                                          
    #16 0x401522 in main ../binr/radare2/radare2.c:96                                                                                                         
    #17 0x7fdbedcaa1a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)                                                                                        
    #18 0x40115d in _start (/home/rschiron/.local/bin/radare2+0x40115d)                                                                                       

0x6250002a00a0 is located 0 bytes to the right of 8096-byte region [0x62500029e100,0x6250002a00a0)                                                                                                                                                                                                                           
allocated by thread T0 here:                                                   
    #0 0x7fdbee2c3d18 in __interceptor_malloc (/lib64/libasan.so.5+0x10dd18)                                                                                  
    #1 0x7fdbebabea03 in r_core_anal_search_xrefs ../libr/core/canal.c:3934                                                                                   
    #2 0x7fdbebbd46fe in r_core_anal_refs ../libr/core/cmd_anal.c:8617                                                                                        
    #3 0x7fdbebbd8bc0 in cmd_anal_all ../libr/core/cmd_anal.c:9194                                                                                            
    #4 0x7fdbebbde8a1 in cmd_anal ../libr/core/cmd_anal.c:10102                                                                                               
    #5 0x7fdbebcacd15 in r_cmd_call_parsed_args ../libr/core/cmd_api.c:347                                                                                    
    #6 0x7fdbebc8e93c in handle_ts_arged_command_internal ../libr/core/cmd.c:5000                                                                             
    #7 0x7fdbebc8e0c8 in handle_ts_arged_command ../libr/core/cmd.c:4959                                                                                      
    #8 0x7fdbebca3d69 in handle_ts_command ../libr/core/cmd.c:6496                                                                                            
    #9 0x7fdbebca47ec in handle_ts_commands_internal ../libr/core/cmd.c:6551                                                                                  
    #10 0x7fdbebca40b3 in handle_ts_commands ../libr/core/cmd.c:6516                                                                                          
    #11 0x7fdbebca5317 in core_cmd_tsr2cmd ../libr/core/cmd.c:6642                                                                                            
    #12 0x7fdbebca57da in r_core_cmd ../libr/core/cmd.c:6685                                                                                                  
    #13 0x7fdbebca6731 in r_core_cmd0 ../libr/core/cmd.c:6892                                                                                                 
    #14 0x7fdbede75f55 in r_main_radare2 ../libr/main/radare2.c:1297                                                                                          
    #15 0x401522 in main ../binr/radare2/radare2.c:96                                                                                                         
    #16 0x7fdbedcaa1a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)

thestr4ng3r

Thanks, that clears it up for me.

trufae

LGTM!

condret requested a review from thestr4ng3r as a code owner June 16, 2020 15:49

github-actions bot added the RAnal label Jun 16, 2020

thestr4ng3r suggested changes Jun 16, 2020

View reviewed changes

trufae requested changes Jun 17, 2020

View reviewed changes

trufae changed the title ~~fix oob heap access~~ Fix oob in the Gameboy disassembler ##asm Jun 17, 2020

trufae changed the title ~~Fix oob in the Gameboy disassembler ##asm~~ Fix OOB in the Gameboy disassembler ##asm Jun 17, 2020

trufae requested changes Jun 27, 2020

View reviewed changes

radare changed the title ~~Fix OOB in the Gameboy disassembler ##asm~~ Fix OOB in the Gameboy disassembler ##anal Jul 1, 2020

trufae changed the title ~~Fix OOB in the Gameboy disassembler ##anal~~ Fix OOB in the Gameboy instruction decoder ##anal Jul 1, 2020

condret force-pushed the master branch from 97fce22 to f3742d9 Compare July 1, 2020 22:14

fix oob heap access

e1fc4d5

condret force-pushed the master branch from f3742d9 to e1fc4d5 Compare July 1, 2020 22:21

thestr4ng3r approved these changes Jul 2, 2020

View reviewed changes

trufae approved these changes Jul 2, 2020

View reviewed changes

thestr4ng3r merged commit 4f108ad into radareorg:master Jul 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix OOB in the Gameboy instruction decoder ##anal #17088

Fix OOB in the Gameboy instruction decoder ##anal #17088

condret commented Jun 16, 2020

ret2libc commented Jun 16, 2020 •

edited by thestr4ng3r

Loading

thestr4ng3r left a comment

trufae Jun 17, 2020

trufae Jun 27, 2020

condret Jun 30, 2020

condret Jun 30, 2020

trufae commented Jun 17, 2020

trufae commented Jun 20, 2020

trufae left a comment

trufae Jun 27, 2020

trufae Jun 27, 2020

condret Jun 30, 2020

radare Jul 1, 2020

radare Jul 1, 2020

condret Jul 1, 2020

trufae Jun 27, 2020

condret Jun 30, 2020

radare Jul 1, 2020

condret Jul 1, 2020

condret commented Jun 30, 2020

thestr4ng3r commented Jul 1, 2020

trufae commented Jul 1, 2020

condret commented Jul 1, 2020

condret commented Jul 1, 2020

thestr4ng3r left a comment

trufae left a comment

Fix OOB in the Gameboy instruction decoder ##anal #17088

Fix OOB in the Gameboy instruction decoder ##anal #17088

Conversation

condret commented Jun 16, 2020

ret2libc commented Jun 16, 2020 • edited by thestr4ng3r Loading

thestr4ng3r left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trufae commented Jun 17, 2020

trufae commented Jun 20, 2020

trufae left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

condret commented Jun 30, 2020

thestr4ng3r commented Jul 1, 2020

trufae commented Jul 1, 2020

condret commented Jul 1, 2020

condret commented Jul 1, 2020

thestr4ng3r left a comment

Choose a reason for hiding this comment

trufae left a comment

Choose a reason for hiding this comment

ret2libc commented Jun 16, 2020 •

edited by thestr4ng3r

Loading