Undefined error handling logic when getting the first block of process memory and checking "iterator->last_error"

[1ndahous3]

We start by calling yr_rules_scan_proc(). It opens the iterator via yr_process_open_iterator(), which allocates and fills the members of the struct YR_MEMORY_BLOCK_ITERATOR iterator, except for last_error:

yara/libyara/proc.c

Lines 53 to 60 in e1360f6

	iterator->context = context;
	iterator->first = yr_process_get_first_memory_block;
	iterator->next = yr_process_get_next_memory_block;
	// In a process scan file size is undefined, when the file_size function is
	// set to NULL the value returned by the filesize keyword is YR_UNDEFINED.
	iterator->file_size = NULL;

Then it calls yr_rules_scan_mem_blocks() -> yr_scanner_scan_mem_blocks(), and the first block is returned via callback:

yara/libyara/scanner.c

Line 463 in a36b497

block = iterator->first(iterator);

In fact, the platform-specific yr_process_get_first_memory_block() is called, which almost directly calls yr_process_get_next_memory_block():

yara/libyara/proc/windows.c

Lines 178 to 189 in 9ab96d1

	YR_API YR_MEMORY_BLOCK* yr_process_get_first_memory_block(
	YR_MEMORY_BLOCK_ITERATOR* iterator)
	{
	YR_PROC_ITERATOR_CTX* context = (YR_PROC_ITERATOR_CTX*) iterator->context;
	YR_PROC_INFO* proc_info = (YR_PROC_INFO*) context->proc_info;
	context->current_block.size = 0;
	context->current_block.base = (size_t)
	proc_info->si.lpMinimumApplicationAddress;
	return yr_process_get_next_memory_block(iterator);
	}

The last one can return real block or NULL.
But depending on the platform, the function sets iterator->last_error = ERROR_SUCCESS; at the beginning of the function (Windows):

yara/libyara/proc/windows.c

Line 145 in 9ab96d1

iterator->last_error = ERROR_SUCCESS;

or only in case of real success (Linux):

yara/libyara/proc/linux.c

Line 402 in 9ab96d1

iterator->last_error = ERROR_SUCCESS;

Those we got a situation where "iterator->last_error" is ERROR_SUCCESS even if return value is NULL (Windows):

yara/libyara/proc/windows.c

Line 175 in 9ab96d1

return NULL;

but also "iterator->last_error" is an unassigned variable (almost always not ERROR_SUCCESS) if the return value is NULL (Linux):

yara/libyara/proc/linux.c

Line 392 in 9ab96d1

return NULL;

This is the first interesting point.

Let's assume we got NULL when trying to get the first block, so we skip the while (block != NULL) loop and now check for errors:

yara/libyara/scanner.c

Lines 503 to 506 in a36b497

	result = iterator->last_error;
	if (result != ERROR_SUCCESS)
	goto _exit;

Depending on the correct logic, we can exit here or continue execution here, this is the second point.

I got this error in the wild and caught it in the debugger:

[1ndahous3]

These 2 uses of unassigned "last_error" are effectively detected by valgrind:

> sudo valgrind --track-origins=yes --read-var-info=yes ./test
==463939== Memcheck, a memory error detector
==463939== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==463939== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==463939== Command: ./test
==463939== 
==463939== Conditional jump or move depends on uninitialised value(s)
==463939==    at 0x52D41B5: yr_scanner_scan_mem_blocks (scanner.c:437)
==463939==    by 0x52D3179: yr_rules_scan_mem_blocks (rules.c:189)
==463939==    by 0x52D3385: yr_rules_scan_proc (rules.c:304)
==463939==    by 0x52B3B76: yara_scanner::scan_proc(_yara_scan_proc_params_t const&, unsigned int, void (*)(_yara_error_t const*, _yara_result_process_t const*))::$_5::operator()(unsigned int) const (scanner.cpp:554)
==463939==    by 0x52B3A09: yara_scanner::scan_proc(_yara_scan_proc_params_t const&, unsigned int, void (*)(_yara_error_t const*, _yara_result_process_t const*)) (scanner.cpp:579)
==463939==    by 0x52B13BE: yara_scan_proc (yara_public_api.cpp:75)
==463939==    by 0x109E5B: main (test.cpp:262)
==463939==  Uninitialised value was created by a stack allocation
==463939==    at 0x52ABED0: ??? (in /home/kali/git/yara/libyara_scanner.so)
==463939== 
==463939== Conditional jump or move depends on uninitialised value(s)
==463939==    at 0x52D4589: yr_scanner_scan_mem_blocks (scanner.c:505)
==463939==    by 0x52D3179: yr_rules_scan_mem_blocks (rules.c:189)
==463939==    by 0x52D3385: yr_rules_scan_proc (rules.c:304)
==463939==    by 0x52B3B76: yara_scanner::scan_proc(_yara_scan_proc_params_t const&, unsigned int, void (*)(_yara_error_t const*, _yara_result_process_t const*))::$_5::operator()(unsigned int) const (scanner.cpp:554)
==463939==    by 0x52B3A09: yara_scanner::scan_proc(_yara_scan_proc_params_t const&, unsigned int, void (*)(_yara_error_t const*, _yara_result_process_t const*)) (scanner.cpp:579)
==463939==    by 0x52B13BE: yara_scan_proc (yara_public_api.cpp:75)
==463939==    by 0x109E5B: main (test.cpp:262)
==463939==  Uninitialised value was created by a stack allocation
==463939==    at 0x52ABED0: ??? (in /home/kali/git/yara/libyara_scanner.so)
==463939== 
==463939== Conditional jump or move depends on uninitialised value(s)
==463939==    at 0x52D489E: yr_scanner_scan_mem_blocks (scanner.c:562)
==463939==    by 0x52D3179: yr_rules_scan_mem_blocks (rules.c:189)
==463939==    by 0x52D3385: yr_rules_scan_proc (rules.c:304)
==463939==    by 0x52B3B76: yara_scanner::scan_proc(_yara_scan_proc_params_t const&, unsigned int, void (*)(_yara_error_t const*, _yara_result_process_t const*))::$_5::operator()(unsigned int) const (scanner.cpp:554)
==463939==    by 0x52B3A09: yara_scanner::scan_proc(_yara_scan_proc_params_t const&, unsigned int, void (*)(_yara_error_t const*, _yara_result_process_t const*)) (scanner.cpp:579)
==463939==    by 0x52B13BE: yara_scan_proc (yara_public_api.cpp:75)
==463939==    by 0x109E5B: main (test.cpp:262)
==463939==  Uninitialised value was created by a stack allocation
==463939==    at 0x52ABED0: ??? (in /home/kali/git/yara/libyara_scanner.so)
==463939== 
...

Adding 2 code changes makes valgring happy:

1. "iterator->last_error = ERROR_SUCCESS;" in yr_process_open_iterator():

  iterator->context = context;
  iterator->first = yr_process_get_first_memory_block;
  iterator->next = yr_process_get_next_memory_block;
  // In a process scan file size is undefined, when the file_size function is
  // set to NULL the value returned by the filesize keyword is YR_UNDEFINED.
  iterator->file_size = NULL;
  iterator->last_error = ERROR_SUCCESS; // <--

Such an assignment already exists in yr_scanner_scan_mem():

yara/libyara/scanner.c

Lines 653 to 657 in a36b497

	iterator.context = █
	iterator.first = _yr_get_first_block;
	iterator.next = _yr_get_next_block;
	iterator.file_size = _yr_get_file_size;
	iterator.last_error = ERROR_SUCCESS;

2. additional check in yr_process_get_first_memory_block() in case of a NULL block:

yara/libyara/proc/linux.c

Lines 435 to 439 in e26c15e

	_exit:
	YR_DEBUG_FPRINTF(2, stderr, "} = %p // %s()\n", result, __FUNCTION__);
	return result;

Add:

YR_API YR_MEMORY_BLOCK* yr_process_get_first_memory_block(
    YR_MEMORY_BLOCK_ITERATOR* iterator)
{
  YR_DEBUG_FPRINTF(2, stderr, "+ %s() {\n", __FUNCTION__);

  YR_MEMORY_BLOCK* result = NULL;
 ...
  result = yr_process_get_next_memory_block(iterator);

_exit:

  if (result == NULL) // <--
  {
    iterator->last_error = ERROR_COULD_NOT_READ_PROCESS_MEMORY; // <--
  }

  YR_DEBUG_FPRINTF(2, stderr, "} = %p // %s()\n", result, __FUNCTION__);

  return result;
}

If I got the point right :)