bug#20264: [PATCH] fix: w32_executable_type() causes a segmentation fault

bug#20264: [PATCH] fix: w32_executable_type() causes a segmentation fault

[Koichi Arakawa]

Hi folks,

On Windows platform, w32_executable_type() in src/w32proc.c scans
'dllname' in an EXE file. But there are some strange EXE files that
'dllname' points to an illegal address, for example, Microsoft's Excel
(excel.exe) and PowerPoint (POWEPNT.EXE). w32_executable_type() causes
a segmentation fault for those files.

objdump in binutils seems to know those illegal pointers and discard
them (pe_print_idata() in bfd/peXXigen.c).

In the following patch, 'dllname' is checked whether it points to the
valid section's address space and discarded when it's invalid.

Regards,
Koichi Arakawa

diff --git a/src/ChangeLog b/src/ChangeLog
index 1c3f933..a49fdf4 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,8 @@
+2015-04-06  Koichi Arakawa  <arakawa@pp.iij4u.or.jp>
+
+	* w32proc.c (w32_executable_type): Check whether 'dllname' points
+	to the section's address space.
+
 2015-04-04  Jan Djärv  <jan.h.d@swipnet.se>
 
 	* xselect.c (x_reply_selection_request)
diff --git a/src/w32proc.c b/src/w32proc.c
index 7d982f8..d3d9405 100644
--- a/src/w32proc.c
+++ b/src/w32proc.c
@@ -1618,16 +1618,23 @@ w32_executable_type (char * filename,
                 data_dir[IMAGE_DIRECTORY_ENTRY_IMPORT];
               IMAGE_IMPORT_DESCRIPTOR * imports;
               IMAGE_SECTION_HEADER * section;
+              char * base;
+              DWORD_PTR real_size;
 
               section = rva_to_section (import_dir.VirtualAddress, nt_header);
               imports = RVA_TO_PTR (import_dir.VirtualAddress, section,
                                     executable);
+              base = RVA_TO_PTR (section->VirtualAddress, section, executable);
+              real_size = max (section->SizeOfRawData, section->Misc.VirtualSize);
 
               for ( ; imports->Name; imports++)
                 {
                   char * dllname = RVA_TO_PTR (imports->Name, section,
                                                executable);
 
+                  if (imports->Name < base || dllname >= base + real_size)
+                    break;
+
                   /* The exact name of the cygwin dll has changed with
                      various releases, but hopefully this will be reasonably
                      future proof.  */

bug#20264: [PATCH] fix: w32_executable_type() causes a segmentation fault

[Eli Zaretskii]

> Date: Mon, 06 Apr 2015 12:23:23 +0900 (東京
> 	(標準時))
> From: Koichi Arakawa <arakawa@pp.iij4u.or.jp>
> 
> On Windows platform, w32_executable_type() in src/w32proc.c scans
> 'dllname' in an EXE file. But there are some strange EXE files that
> 'dllname' points to an illegal address, for example, Microsoft's Excel
> (excel.exe) and PowerPoint (POWEPNT.EXE). w32_executable_type() causes
> a segmentation fault for those files.
> 
> objdump in binutils seems to know those illegal pointers and discard
> them (pe_print_idata() in bfd/peXXigen.c).
> 
> In the following patch, 'dllname' is checked whether it points to the
> valid section's address space and discarded when it's invalid.

Thanks.

>                for ( ; imports->Name; imports++)
>                  {
>                    char * dllname = RVA_TO_PTR (imports->Name, section,
>                                                 executable);
>  
> +                  if (imports->Name < base || dllname >= base + real_size)
> +                    break;
> +

Shouldn't that "break" be "continue" instead?  IOW, shouldn't we try
all the other entries in the DLL import list?

bug#20264: [PATCH] fix: w32_executable_type() causes a segmentation fault

[Koichi Arakawa]

Eli Zaretskii <eliz@gnu.org> wrote:
>>                for ( ; imports->Name; imports++)
>>                  {
>>                    char * dllname = RVA_TO_PTR (imports->Name, section,
>>                                                 executable);
>>  
>> +                  if (imports->Name < base || dllname >= base + real_size)
>> +                    break;
>> +
> 
> Shouldn't that "break" be "continue" instead?  IOW, shouldn't we try
> all the other entries in the DLL import list?

I apologize insufficient research. The *illegal* dllname actually
points to another section. So the previous patch is wrong and it
should be as follows.

diff --git a/src/w32proc.c b/src/w32proc.c
index 7d982f8..5ae55ff 100644
--- a/src/w32proc.c
+++ b/src/w32proc.c
@@ -1625,6 +1625,7 @@ w32_executable_type (char * filename,
 
               for ( ; imports->Name; imports++)
                 {
+                  section = rva_to_section (imports->Name, nt_header);
                   char * dllname = RVA_TO_PTR (imports->Name, section,
                                                executable);
 
-- 
Koichi Arakawa

bug#20264: [PATCH] fix: w32_executable_type() causes a segmentation fault

[Eli Zaretskii]

> Date: Mon, 06 Apr 2015 18:48:11 +0900
>  (東京 (標準時))
> Cc: 20264@debbugs.gnu.org
> From: Koichi Arakawa <arakawa@pp.iij4u.or.jp>
> 
> I apologize insufficient research. The *illegal* dllname actually
> points to another section. So the previous patch is wrong and it
> should be as follows.

Thanks, I pushed it.

bug#20264: [PATCH] fix: w32_executable_type() causes a segmentation fault

[Koichi Arakawa]

Eli Zaretskii <eliz@gnu.org> wrote:
>> I apologize insufficient research. The *illegal* dllname actually
>> points to another section. So the previous patch is wrong and it
>> should be as follows.
> 
> Thanks, I pushed it.

Thank you. I think it works fine.
-- 
Koichi Arakawa