[Forensics-changes] [yara] 198/368: Add error handling for block iteration on Windows

Hilko Bengen bengen at moszumanska.debian.org
Sat Jul 1 10:30:39 UTC 2017


This is an automated email from the git hooks/post-receive script.

bengen pushed a commit to annotated tag v3.5.0
in repository yara.

commit cd06890cc2afff7086e507851efff2292825e1b2
Author: Kyle Reed <kallanreed at outlook.com>
Date:   Sat Feb 27 21:00:04 2016 -0800

    Add error handling for block iteration on Windows
    
    Signed-off-by: Kyle Reed <kallanreed at outlook.com>
---
 .gitignore      |   1 +
 libyara/proc.c  | 185 +++++++++-----------------------------------------------
 libyara/rules.c |   5 +-
 3 files changed, 34 insertions(+), 157 deletions(-)

diff --git a/.gitignore b/.gitignore
index 25c5fd2..7b08c2a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -59,6 +59,7 @@ x64/
 *.sdf
 *.opendb
 *.opensdf
+*.user
 
 # NuGet
 windows/*/packages/
\ No newline at end of file
diff --git a/libyara/proc.c b/libyara/proc.c
index 9b831fb..08796f2 100644
--- a/libyara/proc.c
+++ b/libyara/proc.c
@@ -22,7 +22,6 @@ limitations under the License.
 #include <yara/error.h>
 #include <yara/proc.h>
 
-
 int _yr_attach_process(
     int pid,
     void** hProcess)
@@ -47,6 +46,8 @@ int _yr_attach_process(
         NULL);
   }
 
+  // TODO: should this be COULD NOT ATTACH?
+
   if (hToken != NULL)
     CloseHandle(hToken);
 
@@ -75,17 +76,12 @@ int _yr_get_process_blocks(
     YR_MEMORY_BLOCK** head)
 {
   PVOID address;
-  int result = ERROR_SUCCESS;
-  int sections = 0;
-
   YR_MEMORY_BLOCK* new_block;
   YR_MEMORY_BLOCK* current = NULL;
-
   SYSTEM_INFO si;
   MEMORY_BASIC_INFORMATION mbi;
 
   GetSystemInfo(&si);
-
   address = si.lpMinimumApplicationAddress;
 
   while (address < si.lpMaximumApplicationAddress &&
@@ -93,8 +89,13 @@ int _yr_get_process_blocks(
   {
     if (mbi.State == MEM_COMMIT && ((mbi.Protect & PAGE_NOACCESS) == 0)) // TODO: check for read permission?
     {
+      // TODO: test read so we don't return blocks that can't be read
+
       new_block = (YR_MEMORY_BLOCK*)yr_malloc(sizeof(YR_MEMORY_BLOCK));
 
+      if (new_block == NULL)
+        return ERROR_INSUFICIENT_MEMORY;
+
       new_block->base = (size_t)mbi.BaseAddress;
       new_block->size = mbi.RegionSize;
 
@@ -105,16 +106,12 @@ int _yr_get_process_blocks(
         current->next = new_block;
 
       current = new_block;
-
-      ++sections;
     }
 
     address = (uint8_t*)address + mbi.RegionSize;
   }
 
-  printf("%lu sections\n", sections);
-
-  return result;
+  return ERROR_SUCCESS;
 }
 
 int _yr_read_process_block(
@@ -149,135 +146,12 @@ int _yr_read_process_block(
   }
 
   // TODO: compare read with block size
-
+  // it would be bad to assume block size bytes were read
   *data = buffer;
 
   return result;
 }
 
-
-
-//int yr_process_get_memory(
-//    int pid,
-//    YR_MEMORY_BLOCK** first_block)
-//{
-//  PVOID address;
-//  SIZE_T read;
-//
-//  unsigned char* data;
-//  int result = ERROR_SUCCESS;
-//
-//  SYSTEM_INFO si;
-//  MEMORY_BASIC_INFORMATION mbi;
-//
-//  YR_MEMORY_BLOCK* new_block;
-//  YR_MEMORY_BLOCK* current_block = NULL;
-//
-//  TOKEN_PRIVILEGES tokenPriv;
-//  LUID luidDebug;
-//  HANDLE hProcess = NULL;
-//  HANDLE hToken = NULL;
-//
-//  if (OpenProcessToken(GetCurrentProcess(), TOKEN_ADJUST_PRIVILEGES, &hToken) &&
-//      LookupPrivilegeValue(NULL, SE_DEBUG_NAME, &luidDebug))
-//  {
-//    tokenPriv.PrivilegeCount = 1;
-//    tokenPriv.Privileges[0].Luid = luidDebug;
-//    tokenPriv.Privileges[0].Attributes = SE_PRIVILEGE_ENABLED;
-//
-//    AdjustTokenPrivileges(
-//        hToken,
-//        FALSE,
-//        &tokenPriv,
-//        sizeof(tokenPriv),
-//        NULL,
-//        NULL);
-//  }
-//
-//  hProcess = OpenProcess(
-//      PROCESS_VM_READ | PROCESS_QUERY_INFORMATION,
-//      FALSE,
-//      pid);
-//
-//  *first_block = NULL;
-//
-//  if (hProcess == NULL)
-//  {
-//    if (hToken != NULL)
-//      CloseHandle(hToken);
-//
-//    return ERROR_COULD_NOT_ATTACH_TO_PROCESS;
-//  }
-//
-//  GetSystemInfo(&si);
-//
-//  address = si.lpMinimumApplicationAddress;
-//  size_t allocated = 0;
-//
-//  while (address < si.lpMaximumApplicationAddress &&
-//         VirtualQueryEx(hProcess, address, &mbi, sizeof(mbi)) != 0)
-//  {
-//    if (mbi.State == MEM_COMMIT && ((mbi.Protect & PAGE_NOACCESS) == 0))
-//    {
-//      data = (unsigned char*) yr_malloc(mbi.RegionSize);
-//
-//      if (data == NULL)
-//      {
-//        result = ERROR_INSUFICIENT_MEMORY;
-//        break;
-//      }
-//
-//      allocated += mbi.RegionSize;
-//
-//      if (ReadProcessMemory(
-//              hProcess,
-//              mbi.BaseAddress,
-//              data,
-//              mbi.RegionSize,
-//              &read))
-//      {
-//        new_block = (YR_MEMORY_BLOCK*) yr_malloc(sizeof(YR_MEMORY_BLOCK));
-//
-//        if (new_block == NULL)
-//        {
-//          yr_free(data);
-//          result = ERROR_INSUFICIENT_MEMORY;
-//          break;
-//        }
-//
-//        if (*first_block == NULL)
-//          *first_block = new_block;
-//
-//        new_block->base = (size_t) mbi.BaseAddress;
-//        new_block->size = mbi.RegionSize;
-//        new_block->data = data;
-//        new_block->next = NULL;
-//
-//        if (current_block != NULL)
-//          current_block->next = new_block;
-//
-//        current_block = new_block;
-//      }
-//      else
-//      {
-//        yr_free(data);
-//      }
-//    }
-//
-//    address = (PVOID)((ULONG_PTR) mbi.BaseAddress + mbi.RegionSize);
-//  }
-//
-//  printf("Allocated %lu bytes\n", allocated);
-//
-//  if (hToken != NULL)
-//    CloseHandle(hToken);
-//
-//  if (hProcess != NULL)
-//    CloseHandle(hProcess);
-//
-//  return result;
-//}
-
 #else
 
 #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__) || \
@@ -521,7 +395,7 @@ _exit:
 
 // process iterator abstraction
 
-static int _yr_free_block_data(
+static void _yr_free_context_data(
     YR_PROCESS_CONTEXT* context)
 {
   if (context->data != NULL)
@@ -529,58 +403,58 @@ static int _yr_free_block_data(
     yr_free(context->data);
     context->data = NULL;
   }
-
-  return ERROR_SUCCESS;
 }
 
 static YR_MEMORY_BLOCK* _yr_get_first_block(
     YR_BLOCK_ITERATOR* iterator)
 {
-  printf("!!! first block\n");
-
   YR_PROCESS_CONTEXT* ctx = (YR_PROCESS_CONTEXT*)iterator->context;
 
+  _yr_free_context_data(ctx);
   ctx->current = ctx->blocks;
 
-  _yr_free_block_data(ctx);
-
   return ctx->current;
 }
 
 static YR_MEMORY_BLOCK* _yr_get_next_block(
     YR_BLOCK_ITERATOR* iterator)
 {
-  printf("next block\n");
-
   YR_PROCESS_CONTEXT* ctx = (YR_PROCESS_CONTEXT*)iterator->context;
 
+  _yr_free_context_data(ctx);
+
   if (ctx->current == NULL)
     return NULL;
 
   ctx->current = ctx->current->next;
 
-  _yr_free_block_data(ctx);
-
   return ctx->current;
 }
 
 static uint8_t* _yr_fetch_block_data(
     YR_BLOCK_ITERATOR* iterator)
 {
-  printf("fetching block\n");
-
   YR_PROCESS_CONTEXT* ctx = (YR_PROCESS_CONTEXT*)iterator->context;
 
   if (ctx->current == NULL)
     return NULL;
 
-  _yr_free_block_data(ctx);
+  // reuse cached data if available
+  if (ctx->data != NULL)
+    return ctx->data;
+
+  _yr_free_context_data(ctx);
 
-  _yr_read_process_block(
+  int result = _yr_read_process_block(
       ctx->process_context,
       ctx->current,
       &ctx->data);
 
+  // TODO should this return error code?
+  // On one hand it's useful, on the other failure
+  // is expected in cases when the section isn't
+  // readable and that's not a reason to exit
+
   return ctx->data;
 }
 
@@ -607,9 +481,10 @@ int yr_open_process_iterator(
       pid,
       &context->process_context);
 
-  result = _yr_get_process_blocks(
-      context->process_context,
-      &context->blocks);
+  if(result == ERROR_SUCCESS)
+    result = _yr_get_process_blocks(
+        context->process_context,
+        &context->blocks);
 
   return result;
 }
@@ -622,10 +497,10 @@ int yr_close_process_iterator(
   if (ctx == NULL)
     return ERROR_SUCCESS;
 
-  // NOTE: detach is responsible for freeing allocated process context
+  // NOTE: detach must free allocated process context
   _yr_detach_process(ctx->process_context);
 
-  _yr_free_block_data(ctx);
+  _yr_free_context_data(ctx);
 
   YR_MEMORY_BLOCK* current = ctx->blocks;
   YR_MEMORY_BLOCK* next;
diff --git a/libyara/rules.c b/libyara/rules.c
index f7947e7..f8bfb43 100644
--- a/libyara/rules.c
+++ b/libyara/rules.c
@@ -309,7 +309,8 @@ int _yr_rules_scan_mem_block(
   return ERROR_SUCCESS;
 }
 
-// Single block iterator impl TODO: belongs in this file?
+// single block iterator impl
+// TODO: belongs in this file?
 static YR_MEMORY_BLOCK* _yr_get_first_block(
     YR_BLOCK_ITERATOR* iterator)
 {
@@ -387,7 +388,7 @@ YR_API int yr_rules_scan_mem_blocks(
   context.flags = flags;
   context.callback = callback;
   context.user_data = user_data;
-  context.file_size = block->size;
+  context.file_size = block->size; // TODO: does this make sense for processes?
   context.iterator = iterator;
   context.entry_point = UNDEFINED;
   context.objects_table = NULL;

-- 
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/forensics/yara.git



More information about the forensics-changes mailing list