search

cmu-sei / pharos

Automated static analysis tools for binary programs
Other
1.56k stars 192 forks source link

DKII consistency failures #232

Closed sei-eschwartz closed 1 year ago

sei-eschwartz commented 2 years ago

Not sure if this is still related to the original problem but it still doesn't run through:

Contradictory information about constructor: factConstructor(0x5de050) but reasonNOTConstructor(0x5de050)
Constraint checks failed, retracting guess!
failed.
Consistency checks failed.
Contradictory information about constructor: factConstructor(0x600400) but reasonNOTConstructor(0x600400)
Constraint checks failed, retracting guess!
tryBinarySearch completely failed on [0x5de050] and will now backtrack to fix an upstream problem.
Refusing to backtrack into reasoningLoop to fix an upstream problem because backtrackForUpstream/0 is not set.
This likely indicates that there is a problem with the OO rules.
Please report this failure to the Pharos developers!
 [150] prolog_stack:get_prolog_backtrace(100,[frame(150,clause(<clause>(0x5ce281eb4d00),6),_5626440)|_5626428],[goal_term_depth(100)]) at /usr/local/lib/swipl/library/prolog_stack.pl:137
 [149] throw_with_backtrace(error(system_error(upstreamProblem))) at /usr/local/share/pharos/prolog/oorules/util.pl:185
  [26] solve_internal at /usr/local/share/pharos/prolog/oorules/setup.pl:681
  [25] catch(user:solve_internal,_5626664,user:((_5626732=error(resource_error(private_table_space),_5626746)->complain_table_space(ooscript);_5626796=error(resource_error(stack),_5626810)->complain_stack_size(ooscript);true),throw(_5626842))) at /usr/local/lib/swipl/boot/init.pl:562
  [24] solve(ooscript) at /usr/local/share/pharos/prolog/oorules/setup.pl:617

Originally posted by @Trass3r in https://github.com/cmu-sei/pharos/issues/209#issuecomment-1179610775

sei-eschwartz commented 2 years ago

I might see what is going wrong...

reasonMergeVFTables_A(constructor, 0x672900, 0x6728f8, 0x672900, 0, factVFTableWrite(0x5dd7cf, 0x5dd760, 0, 0x672900)).
Concluding mergeVFTables(0x672900, 0x6728f8).

So we decide that this vftable belongs to the current constructor based on the vftable write. Here is the vftable write:

.text:005DD7C0 call ??2@YAPAXI@Z ; operator new(uint)
.text:005DD7C5 add esp, 4
.text:005DD7C8 cmp eax, edi
.text:005DD7CA jz short loc_5DD7DB
.text:005DD7CC mov [eax+8], edi
.text:005DD7CF mov dword ptr [eax], offset some_vftable_0

In other words, we install this vftable into a new heap-allocated object, not the "this" object this constructor is constructing.

I think this is a side-effect of a change we made recently that exported possibleVFTableFacts for non-this objects. The solution is to validate that the vftable write is in "this" object. I am trying this now.

sei-eschwartz commented 2 years ago

I think this should fix it: https://github.com/cmu-sei/pharos/commit/cf77c937c7ef092af349151629fb79e5b64f2178

It certainly doesn't fail as quickly, but my run of DKII hasn't completed yet.

sei-eschwartz commented 2 years ago

This causes a regression in our unit tests:

117: FAILED: ooex_vs2010/Lite/oo ooanalyzer results +finalClass(0x0040247E void __thiscall std::basic_char_ostream::`vbase destructor(void), 0, 0xc, 0xc, 0x0040247E void __thiscall std::basic_char_ostream::`vbase destructor(void
), [0x0040247E void __thiscall std::basic_char_ostream::`vbase destructor(void), 0x00402FCC __thiscall std::basic_char_ostream::basic_char_ostream(class std::basic_char_streambuf *, bool)]).+
117: FAILED: ooex_vs2010/Lite/oo ooanalyzer results +finalClass(0x0041238C const std::basic_char_ostream::`vftable', 0x0041238C const std::basic_char_ostream::`vftable', 0, 0, 0, [0x00402766 virtual void * __thiscall std::basic_
char_ostream::`vector deleting destructor'(unsigned int)]).+
117: FAILED: ooex_vs2010/Lite/oo ooanalyzer results -finalClass(0x0041238C const std::basic_char_ostream::`vftable', 0x0041238C const std::basic_char_ostream::`vftable', 0xc, 0xc, 0x0040247E void __thiscall std::basic_char_ostre
am::`vbase destructor(void), [0x0040247E void __thiscall std::basic_char_ostream::`vbase destructor(void), 0x00402766 virtual void * __thiscall std::basic_char_ostream::`vector deleting destructor'(unsigned int), 0x00402FCC __th
iscall std::basic_char_ostream::basic_char_ostream(class std::basic_char_streambuf *, bool)]).-

We are no longer able to associate the vbase destructor with the correct class.

Binary ninja says the function simplifies to:

sub_40247e:    
0 @ 00402487  *(*(*arg1 + 4) + arg1 + 8 - 8) = 0x41238c    
1 @ 00402490  *(arg1 + 8) = 0x412384    
2 @ 0040249c  return sub_403d09(arg1 + 8)

Statement 0 installs std::ostream::vftable. This is obviously accessing a virtual base offset, which is why the change breaks things. This is unavoidable I think until we improve our support for virtual bases.

sei-eschwartz commented 2 years ago

@Trass3r Take a look at this? DKII.EXE.json.gz

Trass3r commented 1 year ago

The patch in https://github.com/cmu-sei/pharos/compare/master...sei-eschwartz:pharos:master (also mentioned in https://github.com/cmu-sei/pharos/issues/227#issuecomment-1179278436) indeed made it run to the end.