fileinfo crashes when initializing Asn1Sequence during digital-signature verification

[s3rvac]

fileinfo crashes when initializing Asn1Sequence during digital-signature verification of the PE file below.

Input

Run

$ retdec-fileinfo FILE

where FILE is:

• 21617B9A2D143CBFBE7B84DB794763415E1F3FA391483F805FE5B13D255B0B80

Output

Segmentation fault

Expected output

fileinfo does not crash when analyzing the file.

Output from valgrind

Invalid read of size 1
   at 0x4C33B58: memmove (vg_replace_strmem.c:1258)
   by 0x243593: unsigned char* std::__copy_move<...>::__copy_m<unsigned char>(...) (stl_algobase.h:368)
   by 0x243515: unsigned char* std::__copy_move_a<...>(...) (stl_algobase.h:386)
   by 0x322DB8: __gnu_cxx::__normal_iterator<...> std::__copy_move_a2<...>(...) (stl_algobase.h:422)
   by 0x3225B7: __gnu_cxx::__normal_iterator<...> std::move<...>(...) (stl_algobase.h:488)
   by 0x322037: std::vector<...>::_M_erase(...) (vector.tcc:171)
   by 0x3217A3: std::vector<...>::erase(...) (stl_vector.h:1210)
   by 0x320B04: retdec::fileformat::Asn1Sequence::init() (asn1.cpp:248)
   by 0x320947: retdec::fileformat::Asn1Sequence::Asn1Sequence(...) (asn1.cpp:226)
   by 0x325951: void __gnu_cxx::new_allocator<...>::construct<...>(...) (new_allocator.h:136)
   by 0x32516F: void std::allocator_traits<...>::construct<...>(...) (alloc_traits.h:475)
   by 0x3248B3: std::_Sp_counted_ptr_inplace<...>::_Sp_counted_ptr_inplace<...>(...) (shared_ptr_base.h:526)
 Address 0x74b6f87 is 20 bytes after a block of size 19 alloc'd
   at 0x4C2D54F: operator new(unsigned long) (vg_replace_malloc.c:334)
   by 0x242C85: __gnu_cxx::new_allocator<unsigned char>::allocate(unsigned long, void const*) (new_allocator.h:111)
   by 0x242821: std::allocator_traits<...>::allocate(std::allocator<unsigned char>&, unsigned long) (alloc_traits.h:436)
   by 0x2422BD: std::_Vector_base<...>::_M_allocate(unsigned long) (stl_vector.h:172)
   by 0x322469: void std::vector<...>::_M_range_initialize<...>(...) (stl_vector.h:1323)
   by 0x321ED3: void std::vector<...>::_M_initialize_dispatch<...>(...) (stl_vector.h:1299)
   by 0x32161C: std::vector<...>::vector<__gnu_cxx::__normal_iterator<...>, void>(...) (stl_vector.h:414)
   by 0x31FF32: retdec::fileformat::Asn1Item::getContentData() const (asn1.cpp:111)
   by 0x320A34: retdec::fileformat::Asn1Sequence::init() (asn1.cpp:241)
   by 0x320947: retdec::fileformat::Asn1Sequence::Asn1Sequence(...) (asn1.cpp:226)
   by 0x325951: void __gnu_cxx::new_allocator<...>::construct<...>(...) (new_allocator.h:136)
   by 0x32516F: void std::allocator_traits<...>::construct<...>(...) (alloc_traits.h:475)

Analysis

PeFormat::verifySignature() in src/fileformat/file_format/pe/pe_format.cpp contains the following code:

1226   std::vector<std::uint8_t> contentInfoData(contentInfoPtr, contentInfoPtr + contentInfoLen);
1227   auto contentInfo = Asn1Item::parse(contentInfoData);

On line 1227, Asn1Item::parse() gets called, which, in the end, constructs Asn1Sequence:

60     case Asn1Tag_Sequence:
61         return std::make_shared<Asn1Sequence>(data);

The constructor of Asn1Sequence calls Asn1Sequence::init(), which contains the following code:

240 void Asn1Sequence::init()
241 {
242     auto contentData = getContentData();
243     while (!contentData.empty())
244     {
245         auto element = Asn1Item::parse(contentData);
246         if (element == nullptr)
247             return;
248
249         contentData.erase(contentData.begin(), contentData.begin() + element->getLength());
250         _elements.push_back(std::move(element));
251     }
252 }

On line 249, attempts to erase elements from contentData are made. However, in one of the attempts, contentData.size() is 7 but element->getLength() returns 39, which is more than contentData.size(). This access results in a segmentation fault.

Configuration

• Commit: 4b94da8 (current master)
• 64b Arch Linux, GCC 7.3.0, Debug build of RetDec

[s3rvac]

I have added an assertion into Asn1Sequence::init() until this issue is fixed (08953bb). In this way, we will know when we stumble upon this problem in the future.

[PeterMatula]

I added a check that prevents moving vector iterator beyond vector end. Fixing the problem this way was not that complicated. I also attempted to find out why this situation happened in the first place. The problem is that Asn1Item element created from data has greater size than the data itself. I was not able to find any obvious bug here, it looks like the algorithm to create Asn1Sequence allows this - it somehow computes the size of element and this computation gives this greater value.