kern/83477: NDIS compat code makes incorrect assumptions about PE-COFF format header lengths

Andrew R. Reiter arr at watson.org
Thu Jul 14 19:30:12 GMT 2005


>Number:         83477
>Category:       kern
>Synopsis:       NDIS compat code makes incorrect assumptions about PE-COFF format header lengths
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Thu Jul 14 19:30:10 GMT 2005
>Closed-Date:
>Last-Modified:
>Originator:     Andrew R. Reiter
>Release:        CURRENT
>Organization:
Websense, Inc
>Environment:
FreeBSD d400.funfun.com 6.0-CURRENT FreeBSD 6.0-CURRENT #3: Sun Jun 19 19:19:53 PDT 2005     root at d400.funfun.com:/usr/obj/usr/src/sys/HI_MOM  i386

>Description:
The issue is in the code that deals with the optional header and where the section headers are.  The basic issue is that the size of the optional header is variable due to the nature of how the DataDirectory's are used.  In about every calculation in this code base, the assumption is made that there are always the defined maximum number of data directories in the binary; since this is not always the case, there is a possibility for there to be incorrect calculations to get to the beginning of the section header table... amongst other things.
>How-To-Repeat:
Noticed via code review; to reproduce one would have to modify a driver, possibly, to remove some data directories.  This is not hard as they are only 16 bytes in length; all one has to do is adjust optional header length in the file header, adjust number of rva and sizes in the optional header, shift up the start of the section headers to be at the end of the last data directory entry, and then recalculate and set the checksum value since that is validated for drivers (and DLLs).  Running this would probably cause a system crash.  I've tested similar things in userland for work related code.
>Fix:
A fix is located at:
  http://www.watson.org/~arr/ndis_opthdrsz.diff

Or:
Index: pe_var.h
===================================================================
RCS file: /home/ncvs/src/sys/compat/ndis/pe_var.h,v
retrieving revision 1.13
diff -u -u -r1.13 pe_var.h
--- pe_var.h    11 Apr 2005 02:02:34 -0000      1.13
+++ pe_var.h    29 Jun 2005 18:47:58 -0000
@@ -214,6 +214,10 @@
 
 typedef struct image_nt_header image_nt_header;
 
+#define        IMAGE_SIZEOF_NT_HEADER(nthdr)                                   
\
+       (offsetof(image_nt_header, inh_optionalhdr) +                   \
+         ((image_nt_header *)(nthdr))->inh_filehdr.ifh_optionalhdrlen)
+
 /* Directory Entries */
 
 #define IMAGE_DIRECTORY_ENTRY_EXPORT         0   /* Export Directory */
@@ -281,6 +285,12 @@
 
 #define IMAGE_SIZEOF_SECTION_HEADER          40
 
+#define IMAGE_FIRST_SECTION(nthdr)                                     \
+       ((image_section_header *)((vm_offset_t)(nthdr) +                \
+         offsetof(image_nt_header, inh_optionalhdr) +                  \
+         ((image_nt_header *)(nthdr))->inh_filehdr.ifh_optionalhdrlen))
+
+
 /*
  * Import format
  */
Index: subr_pe.c
===================================================================
RCS file: /home/ncvs/src/sys/compat/ndis/subr_pe.c,v
retrieving revision 1.11
diff -u -u -r1.11 subr_pe.c
--- subr_pe.c   24 Feb 2005 17:58:27 -0000      1.11
+++ subr_pe.c   29 Jun 2005 18:47:58 -0000
@@ -142,7 +142,7 @@
        nt_hdr = (image_nt_header *)(imgbase + dos_hdr->idh_lfanew);
 
        bcopy ((char *)&nt_hdr->inh_optionalhdr, (char *)hdr,
-           sizeof(image_optional_header));
+           nt_hdr->inh_filehdr.ifh_optionalhdrlen);
 
        return(0);
 }
@@ -170,8 +170,7 @@
        nt_hdr = (image_nt_header *)(imgbase + dos_hdr->idh_lfanew);
 
        bcopy ((char *)&nt_hdr->inh_filehdr, (char *)hdr,
-           sizeof(image_file_header));
-
+           IMAGE_SIZEOF_NT_HEADER(nt_hdr));
        return(0);
 }
 
@@ -197,8 +196,7 @@
 
        dos_hdr = (image_dos_header *)imgbase;
        nt_hdr = (image_nt_header *)(imgbase + dos_hdr->idh_lfanew);
-       sect_hdr = (image_section_header *)((vm_offset_t)nt_hdr +
-           sizeof(image_nt_header));
+       sect_hdr = IMAGE_FIRST_SECTION(nt_hdr);
 
        bcopy ((char *)sect_hdr, (char *)hdr, sizeof(image_section_header));
 
@@ -280,8 +278,7 @@
 
        dos_hdr = (image_dos_header *)imgbase;
        nt_hdr = (image_nt_header *)(imgbase + dos_hdr->idh_lfanew);
-       sect_hdr = (image_section_header *)((vm_offset_t)nt_hdr +
-           sizeof(image_nt_header));
+       sect_hdr = IMAGE_FIRST_SECTION(nt_hdr);
 
        /*
         * The test here is to see if the RVA falls somewhere
@@ -339,8 +336,7 @@
 
        dos_hdr = (image_dos_header *)imgbase;
        nt_hdr = (image_nt_header *)(imgbase + dos_hdr->idh_lfanew);
-       sect_hdr = (image_section_header *)((vm_offset_t)nt_hdr +
-           sizeof(image_nt_header));
+       sect_hdr = IMAGE_FIRST_SECTION(nt_hdr);
 
        for (i = 0; i < sections; i++) {
                if (!strcmp ((char *)&sect_hdr->ish_name, name)) {


>Release-Note:
>Audit-Trail:
>Unformatted:


More information about the freebsd-bugs mailing list