Browse code

Add option to not remove missing sections (PE)

This addresses a regression with sample 848092559:

LDB sig (Win.Virus.Virut-5898123-1) that uses 'NumberOfSections:3-3'
started matching on a PE that has 4 sections, but one is totally outside
of the file and gets removed. Previously, two of the ClamAV PE header
parsing implementations handled this case differently, and the NDB/LDB
matching code would be told there were 4 sections while the bytecode
and unpacking code would only see 3 sections. When consolidating the
PE header parsing code, I made it so that the section always gets
removed.

For now we just replicate the original behavior by providing a new
flag to the PE header parsing code. We should re-evaluate the effects
that this has later, once we have better tests for the bytecode API
and we have test samples for each of the hardcoded detection cases in
cli_scanpe.

Also, fixes some memory leaks based on the changes in my last commit x_x

Andrew authored on 2019/03/13 05:57:05
Showing 2 changed files
... ...
@@ -2781,8 +2781,7 @@ int cli_scanpe(cli_ctx *ctx)
2781 2781
     struct cli_exe_info _peinfo;
2782 2782
     struct cli_exe_info *peinfo = &_peinfo;
2783 2783
 
2784
-    uint32_t opts = CLI_PEHEADER_OPT_DBG_PRINT_INFO;
2785
-    ;
2784
+    uint32_t opts = CLI_PEHEADER_OPT_DBG_PRINT_INFO | CLI_PEHEADER_OPT_REMOVE_MISSING_SECTIONS;
2786 2785
 
2787 2786
 #if HAVE_JSON
2788 2787
     if (SCAN_COLLECT_METADATA) {
... ...
@@ -2808,16 +2807,20 @@ int cli_scanpe(cli_ctx *ctx)
2808 2808
         if (DETECT_BROKEN_PE) {
2809 2809
             // TODO Handle allmatch
2810 2810
             ret = cli_append_virus(ctx, "Heuristics.Broken.Executable");
2811
+            cli_exe_info_destroy(peinfo);
2811 2812
             return ret;
2812 2813
         }
2813 2814
         cli_dbgmsg("cli_scanpe: PE header appears broken - " PE_HDR_PARSE_FAIL_CONSEQUENCE);
2815
+        cli_exe_info_destroy(peinfo);
2814 2816
         return CL_CLEAN;
2815 2817
 
2816 2818
     } else if (CLI_PEHEADER_RET_JSON_TIMEOUT == ret) {
2817 2819
         cli_dbgmsg("cli_scanpe: JSON creation timed out - " PE_HDR_PARSE_FAIL_CONSEQUENCE);
2820
+        cli_exe_info_destroy(peinfo);
2818 2821
         return CL_ETIMEOUT;
2819 2822
     } else if (CLI_PEHEADER_RET_GENERIC_ERROR == ret) {
2820 2823
         cli_dbgmsg("cli_scanpe: An error occurred when parsing the PE header - " PE_HDR_PARSE_FAIL_CONSEQUENCE);
2824
+        cli_exe_info_destroy(peinfo);
2821 2825
         return CL_CLEAN;
2822 2826
     }
2823 2827
 
... ...
@@ -4460,7 +4463,7 @@ int cli_pe_targetinfo(fmap_t *map, struct cli_exe_info *peinfo)
4460 4460
  *             parsing.  The options are (prefixed with CLI_PEHEADER_OPT_):
4461 4461
  *              - NONE - Do default parsing
4462 4462
  *              - COLLECT_JSON - Populate ctx's json obj with PE header
4463
- *                                   info
4463
+ *                               info
4464 4464
  *              - DBG_PRINT_INFO - Print debug information about the
4465 4465
  *                                 PE file. Right now, cli_peheader is
4466 4466
  *                                 called multiple times for a given PE,
... ...
@@ -4473,6 +4476,10 @@ int cli_pe_targetinfo(fmap_t *map, struct cli_exe_info *peinfo)
4473 4473
  *                                      executable cause RET_BROKEN_PE
4474 4474
  *                                      to be returned, but otherwise
4475 4475
  *                                      these will be tolerated.
4476
+ *              - REMOVE_MISSING_SECTIONS - If a section exists outside of the
4477
+ *                                          file, remove it from
4478
+ *                                          peinfo->sections. Otherwise, the
4479
+ *                                          rsz is just set to 0 for it.
4476 4480
  * @param ctx The overaching cli_ctx.  This is required with certain opts, but
4477 4481
  *            optional otherwise.
4478 4482
  * @return If the PE header is parsed successfully, CLI_PEHEADER_RET_SUCCESS
... ...
@@ -4490,6 +4497,13 @@ int cli_pe_targetinfo(fmap_t *map, struct cli_exe_info *peinfo)
4490 4490
  * TODO Simplify and get rid of CLI_PEHEADER_OPT_STRICT_ON_PE_ERRORS if
4491 4491
  * possible.  We should either fail always or ignore always, IMO.
4492 4492
  *
4493
+ * TODO Simplify and get rid of CLI_PEHEADER_OPT_REMOVE_MISSING_SECTIONS if
4494
+ * possible.  I don't think it makes sense to have pieces of the code work
4495
+ * off of incomplete representations of the sections (for instance, I wonder
4496
+ * if this makes any of the bytecode APIs return unexpected values).  This
4497
+ * appears to have been implemented to prevent ClamAV from crashing, though,
4498
+ * (bb11155) so we need to ensure the underlying issues are addressed.
4499
+ *
4493 4500
  * TODO Consolidate when information about the PE is printed (after successful
4494 4501
  * PE parsing).  This will allow us to simplify the code.  Some fail cases,
4495 4502
  * then, will cause PE info to not be printed at all, but I think this is
... ...
@@ -5126,39 +5140,46 @@ int cli_peheader(fmap_t *map, struct cli_exe_info *peinfo, uint32_t opts, cli_ct
5126 5126
         section->uraw = EC32(section_hdr->PointerToRawData);
5127 5127
         section->ursz = EC32(section_hdr->SizeOfRawData);
5128 5128
 
5129
-        // First, if a section exists totally outside of a file, remove the
5130
-        // section from the list.
5131
-        // TODO Document that this happens in the function documentation
5129
+        /* First, if a section exists totally outside of a file, remove the
5130
+         * section from the list or zero out it's size. */
5132 5131
         if (section->rsz) { /* Don't bother with virtual only sections */
5133 5132
             if (section->raw >= fsize || section->uraw >= fsize) {
5134 5133
                 cli_dbgmsg("cli_peheader: Broken PE file - Section %d starts or exists beyond the end of file (Offset@ %lu, Total filesize %lu)\n", section_pe_idx, (unsigned long)section->raw, (unsigned long)fsize);
5135
-                if (peinfo->nsections == 1) {
5136
-                    ret = CLI_PEHEADER_RET_BROKEN_PE;
5137
-                    goto done;
5138
-                }
5139 5134
 
5140
-                for (j = i; j < peinfo->nsections - 1; j++)
5141
-                    memcpy(&(peinfo->sections[j]), &(peinfo->sections[j + 1]), sizeof(struct cli_exe_section));
5135
+                if (opts & CLI_PEHEADER_OPT_REMOVE_MISSING_SECTIONS) {
5136
+                    if (peinfo->nsections == 1) {
5137
+                        ret = CLI_PEHEADER_RET_BROKEN_PE;
5138
+                        goto done;
5139
+                    }
5142 5140
 
5143
-                for (j = i; j < peinfo->nsections - 1; j++)
5144
-                    memcpy(&section_hdrs[j], &section_hdrs[j + 1], sizeof(struct pe_image_section_hdr));
5141
+                    for (j = i; j < peinfo->nsections - 1; j++)
5142
+                        memcpy(&(peinfo->sections[j]), &(peinfo->sections[j + 1]), sizeof(struct cli_exe_section));
5145 5143
 
5146
-                peinfo->nsections--;
5144
+                    for (j = i; j < peinfo->nsections - 1; j++)
5145
+                        memcpy(&section_hdrs[j], &section_hdrs[j + 1], sizeof(struct pe_image_section_hdr));
5147 5146
 
5148
-                // Adjust i since we removed a section and continue on
5149
-                i--;
5150
-                continue;
5151
-            }
5147
+                    peinfo->nsections--;
5152 5148
 
5153
-            // Verify that the section is fully contained within the file
5154
-            if (!CLI_ISCONTAINED(0, fsize, section->raw, section->rsz)) {
5155
-                cli_dbgmsg("cli_peheader: PE Section %d raw+rsz extends past the end of the file by %lu bytes\n", section_pe_idx, (section->raw + section->rsz) - fsize);
5156
-                section->rsz = fsize - section->raw;
5157
-            }
5149
+                    // Adjust i since we removed a section and continue on
5150
+                    i--;
5151
+                    continue;
5152
+
5153
+                } else {
5154
+                    section->rsz  = 0;
5155
+                    section->ursz = 0;
5156
+                }
5157
+            } else {
5158 5158
 
5159
-            if (!CLI_ISCONTAINED(0, fsize, section->uraw, section->ursz)) {
5160
-                cli_dbgmsg("cli_peheader: PE Section %d uraw+ursz extends past the end of the file by %lu bytes\n", section_pe_idx, (section->uraw + section->ursz) - fsize);
5161
-                section->ursz = fsize - section->uraw;
5159
+                /* If a section is truncated, adjust it's size value */
5160
+                if (!CLI_ISCONTAINED(0, fsize, section->raw, section->rsz)) {
5161
+                    cli_dbgmsg("cli_peheader: PE Section %d raw+rsz extends past the end of the file by %lu bytes\n", section_pe_idx, (section->raw + section->rsz) - fsize);
5162
+                    section->rsz = fsize - section->raw;
5163
+                }
5164
+
5165
+                if (!CLI_ISCONTAINED(0, fsize, section->uraw, section->ursz)) {
5166
+                    cli_dbgmsg("cli_peheader: PE Section %d uraw+ursz extends past the end of the file by %lu bytes\n", section_pe_idx, (section->uraw + section->ursz) - fsize);
5167
+                    section->ursz = fsize - section->uraw;
5168
+                }
5162 5169
             }
5163 5170
         }
5164 5171
 
... ...
@@ -5548,6 +5569,7 @@ cl_error_t cli_check_auth_header(cli_ctx *ctx, struct cli_exe_info *peinfo)
5548 5548
         cli_exe_info_init(peinfo, 0);
5549 5549
 
5550 5550
         if (cli_peheader(*ctx->fmap, peinfo, CLI_PEHEADER_OPT_NONE, NULL) != CLI_PEHEADER_RET_SUCCESS) {
5551
+            cli_exe_info_destroy(peinfo);
5551 5552
             return CL_EFORMAT;
5552 5553
         }
5553 5554
     }
... ...
@@ -5786,6 +5808,7 @@ int cli_genhash_pe(cli_ctx *ctx, unsigned int class, int type, stats_section_t *
5786 5786
     cli_exe_info_init(peinfo, 0);
5787 5787
 
5788 5788
     if (cli_peheader(*ctx->fmap, peinfo, CLI_PEHEADER_OPT_NONE, NULL) != CLI_PEHEADER_RET_SUCCESS) {
5789
+        cli_exe_info_destroy(peinfo);
5789 5790
         return CL_EFORMAT;
5790 5791
     }
5791 5792
 
... ...
@@ -72,10 +72,6 @@ struct cli_pe_hook_data {
72 72
 
73 73
 int cli_scanpe(cli_ctx *ctx);
74 74
 
75
-#define CL_CHECKFP_PE_FLAG_NONE 0x00000000
76
-#define CL_CHECKFP_PE_FLAG_STATS 0x00000001
77
-#define CL_CHECKFP_PE_FLAG_AUTHENTICODE 0x00000002
78
-
79 75
 enum {
80 76
     CL_GENHASH_PE_CLASS_SECTION,
81 77
     CL_GENHASH_PE_CLASS_IMPTBL,
... ...
@@ -89,6 +85,7 @@ enum {
89 89
 #define CLI_PEHEADER_OPT_DBG_PRINT_INFO 0x2
90 90
 #define CLI_PEHEADER_OPT_EXTRACT_VINFO 0x4
91 91
 #define CLI_PEHEADER_OPT_STRICT_ON_PE_ERRORS 0x8
92
+#define CLI_PEHEADER_OPT_REMOVE_MISSING_SECTIONS 0x10
92 93
 
93 94
 #define CLI_PEHEADER_RET_SUCCESS 0
94 95
 #define CLI_PEHEADER_RET_GENERIC_ERROR -1