CVE-2017-8929: Use After Free (CWE-416)
value: The sized_string_cmp function in libyara/sizedstr.c in YARA 3.5.0 allows remote attackers to cause a denial of service (use-after-free and application crash) via a crafted rule.
CWE description: The product reuses or references memory after it has been freed. At some point afterward, the memory may be allocated again and saved in another pointer, while the original pointer references a location somewhere within the new allocation. Any operations using the original pointer are no longer valid because the memory "belongs" to the code that operates on the new pointer.
CVSS v3 base score: 7.5
Published: 2017-05-14T22:29Z
Repository: https://github.com/VirusTotal/yara
Language: C
Files changed: libyara/arena.c, libyara/exec.c, libyara/include/yara/object.h, libyara/include/yara/sizedstr.h, libyara/modules/tests.c, libyara/object.c, libyara/sizedstr.c, tests/test-rules.c
Commit date: 2017-05-14 10:27:39 +0200
Commit message: Fix issue #658


Diff: diff --git a/libyara/arena.c b/libyara/arena.c
index 48faa425..f53775da 100644
--- a/libyara/arena.c
+++ b/libyara/arena.c
@@ -314,11 +314,11 @@ void yr_arena_destroy(
 //
 // Args:
 //    YR_ARENA* arena  - Pointer to the arena.
 //
 // Returns:
-//    A pointer to the arena's data. NULL if the no data has been written to
+//    A pointer to the arena's data. NULL if no data has been written to
 //    the arena yet.
 //
 
 void* yr_arena_base_address(
   YR_ARENA* arena)
diff --git a/libyara/exec.c b/libyara/exec.c
index d42c1b03..80379911 100644
--- a/libyara/exec.c
+++ b/libyara/exec.c
@@ -32,10 +32,11 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #include <string.h>
 #include <assert.h>
 #include <time.h>
 #include <math.h>
 
+#include <yara/arena.h>
 #include <yara/endian.h>
 #include <yara/exec.h>
 #include <yara/limits.h>
 #include <yara/error.h>
 #include <yara/object.h>
@@ -175,10 +176,12 @@ int yr_execute_code(
   #endif
 
   YR_RULE* rule;
   YR_MATCH* match;
   YR_OBJECT_FUNCTION* function;
+  YR_OBJECT** obj_ptr;
+  YR_ARENA* obj_arena;
 
   char* identifier;
   char* args_fmt;
 
   int i;
@@ -199,10 +202,14 @@ int yr_execute_code(
   stack = (YR_VALUE*) yr_malloc(stack_size * sizeof(YR_VALUE));
 
   if (stack == NULL)
     return ERROR_INSUFFICIENT_MEMORY;
 
+  FAIL_ON_ERROR_WITH_CLEANUP(
+      yr_arena_create(1024, 0, &obj_arena),
+      yr_free(stack));
+
   while(!stop)
   {
     switch(*ip)
     {
       case OP_NOP:
@@ -435,10 +442,12 @@ int yr_execute_code(
 
         #ifdef PROFILING_ENABLED
         rule->clock_ticks += clock() - start;
         start = clock();
         #endif
+
+        assert(sp == 0);
         break;
 
       case OP_OBJ_LOAD:
         identifier = *(char**)(ip + 1);
         ip += sizeof(uint64_t);
@@ -575,22 +584,30 @@ int yr_execute_code(
             result = function->prototypes[i].code(args, context, function);
             break;
           }
         }
 
+        // if i == MAX_OVERLOADED_FUNCTIONS at this point no matching
+        // prototype was found, but this shouldn't happen.
+
         assert(i < MAX_OVERLOADED_FUNCTIONS);
 
+        // make a copy of the returned object and push the copy into the stack
+        // function->return_obj can't be pushed because it can change in
+        // subsequent calls to the same function.
+
         if (result == ERROR_SUCCESS)
-        {
-          r1.o = function->return_obj;
-          push(r1);
-        }
-        else
-        {
-          stop = TRUE;
-        }
+          result = yr_object_copy(function->return_obj, &r1.o);
+
+        // a pointer to the copied object is stored in a arena in order to
+        // free the object before exiting yr_execute_code
 
+        if (result == ERROR_SUCCESS)
+          result = yr_arena_write_data(obj_arena, &r1.o, sizeof(r1.o), NULL);
+
+        stop = (result != ERROR_SUCCESS);
+        push(r1);
         break;
 
       case OP_FOUND:
         pop(r1);
         r1.i = r1.s->matches[tidx].tail != NULL ? 1 : 0;
@@ -1144,10 +1161,21 @@ int yr_execute_code(
     }
 
     ip++;
   }
 
+  obj_ptr = (YR_OBJECT**) yr_arena_base_address(obj_arena);
+
+  while (obj_ptr != NULL)
+  {
+    yr_object_destroy(*obj_ptr);
+
+    obj_ptr = (YR_OBJECT**) yr_arena_next_address(
+        obj_arena, obj_ptr, sizeof(YR_OBJECT*));
+  }
+
+  yr_arena_destroy(obj_arena);
   yr_modules_unload_all(context);
   yr_free(stack);
 
   return result;
 }
diff --git a/libyara/include/yara/object.h b/libyara/include/yara/object.h
index 0fbf311b..5f412931 100644
--- a/libyara/include/yara/object.h
+++ b/libyara/include/yara/object.h
@@ -84,10 +84,15 @@ int yr_object_from_external_variable(
 
 void yr_object_destroy(
     YR_OBJECT* object);
 
 
+int yr_object_copy(
+    YR_OBJECT* object,
+    YR_OBJECT** object_copy);
+
+
 YR_OBJECT* yr_object_lookup_field(
     YR_OBJECT* object,
     const char* field_name);
 
 
diff --git a/libyara/include/yara/sizedstr.h b/libyara/include/yara/sizedstr.h
index 9e29ec6a..da4b98d3 100644
--- a/libyara/include/yara/sizedstr.h
+++ b/libyara/include/yara/sizedstr.h
@@ -49,18 +49,22 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 typedef struct _SIZED_STRING
 {
   uint32_t length;
   uint32_t flags;
-  
+
   char c_string[1];
 
 } SIZED_STRING;
 
 #pragma pack(pop)
 
 
 int sized_string_cmp(
-  SIZED_STRING* s1,
-  SIZED_STRING* s2);
+    SIZED_STRING* s1,
+    SIZED_STRING* s2);
+
+
+SIZED_STRING* sized_string_dup(
+    SIZED_STRING* s);
 
 #endif
diff --git a/libyara/modules/tests.c b/libyara/modules/tests.c
index 55fd2f30..a1526383 100644
--- a/libyara/modules/tests.c
+++ b/libyara/modules/tests.c
@@ -86,10 +86,27 @@ define_function(match)
 {
   return_integer(yr_re_match(regexp_argument(1), string_argument(2)));
 }
 
 
+define_function(foobar)
+{
+  int64_t arg = integer_argument(1);
+
+  switch (arg)
+  {
+    case 1:
+      return_string("foo");
+      break;
+    case 2:
+      return_string("bar");
+      break;
+  }
+
+  return_string("oops")
+}
+
 begin_declarations;
 
   begin_struct("constants");
     declare_integer("one");
     declare_integer("two");
@@ -123,10 +140,11 @@ begin_declarations;
   declare_function("isum", "iii", "i", isum_3);
   declare_function("fsum", "ff", "f", fsum_2);
   declare_function("fsum", "fff", "f", fsum_3);
   declare_function("length", "s", "i", length);
   declare_function("empty", "", "s", empty);
+  declare_function("foobar", "i", "s", foobar);
 
 end_declarations;
 
 
 int module_initialize(
diff --git a/libyara/object.c b/libyara/object.c
index d2d84f58..5b5b2655 100644
--- a/libyara/object.c
+++ b/libyara/object.c
@@ -571,15 +571,27 @@ int yr_object_copy(
       &copy));
 
   switch(object->type)
   {
     case OBJECT_TYPE_INTEGER:
-      ((YR_OBJECT_INTEGER*) copy)->value = UNDEFINED;
+      ((YR_OBJECT_INTEGER*) copy)->value = ((YR_OBJECT_INTEGER*) object)->value;
       break;
 
     case OBJECT_TYPE_STRING:
-      ((YR_OBJECT_STRING*) copy)->value = NULL;
+      if (((YR_OBJECT_STRING*) object)->value != NULL)
+      {
+        ((YR_OBJECT_STRING*) copy)->value = sized_string_dup(
+            ((YR_OBJECT_STRING*) object)->value);
+      }
+      else
+      {
+        ((YR_OBJECT_STRING*) copy)->value = NULL;
+      }
+      break;
+
+    case OBJECT_TYPE_FLOAT:
+      ((YR_OBJECT_DOUBLE*) copy)->value = ((YR_OBJECT_DOUBLE*) object)->value;
       break;
 
     case OBJECT_TYPE_FUNCTION:
 
       func = (YR_OBJECT_FUNCTION*) object;
diff --git a/libyara/sizedstr.c b/libyara/sizedstr.c
index 767aa7e0..3bf058ac 100644
--- a/libyara/sizedstr.c
+++ b/libyara/sizedstr.c
@@ -25,10 +25,12 @@ LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
 ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
 SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 */
 
+#include <string.h>
+#include <yara/mem.h>
 #include <yara/sizedstr.h>
 
 
 int sized_string_cmp(
   SIZED_STRING* s1,
@@ -52,5 +54,23 @@ int sized_string_cmp(
   else if (s1->c_string[i] < s2->c_string[i])
     return -1;
   else
     return 1;
 }
+
+
+SIZED_STRING* sized_string_dup(
+    SIZED_STRING* s)
+{
+  SIZED_STRING* result = (SIZED_STRING*) yr_malloc(
+      sizeof(SIZED_STRING) + s->length);
+
+  if (result == NULL)
+    return NULL;
+
+  result->length = s->length;
+  result->flags = s->flags;
+
+  strncpy(result->c_string, s->c_string, s->length + 1);
+
+  return result;
+}
diff --git a/tests/test-rules.c b/tests/test-rules.c
index 63433e08..2a28c005 100644
--- a/tests/test-rules.c
+++ b/tests/test-rules.c
@@ -1437,10 +1437,24 @@ static void test_modules()
        rule test { \
         condition: tests.fsum(1.0,2.0,3.0) == 6.0 \
       }",
       NULL);
 
+  assert_true_rule(
+      "import \"tests\" \
+       rule test { \
+        condition: tests.foobar(1) == tests.foobar(1) \
+      }",
+      NULL);
+
+  assert_true_rule(
+      "import \"tests\" \
+       rule test { \
+        condition: tests.foobar(1) != tests.foobar(2) \
+      }",
+      NULL);
+
   assert_true_rule(
       "import \"tests\" \
        rule test { \
         condition: tests.length(\"dummy\") == 5 \
       }",

Vulnerable code: //    A pointer to the arena's data. NULL if the no data has been written to
        {
          r1.o = function->return_obj;
          push(r1);
        }
        else
        {
          stop = TRUE;
        }
  
  SIZED_STRING* s1,
  SIZED_STRING* s2);
      ((YR_OBJECT_INTEGER*) copy)->value = UNDEFINED;
      ((YR_OBJECT_STRING*) copy)->value = NULL;
Fixed code: //    A pointer to the arena's data. NULL if no data has been written to
#include <yara/arena.h>
  YR_OBJECT** obj_ptr;
  YR_ARENA* obj_arena;
  FAIL_ON_ERROR_WITH_CLEANUP(
      yr_arena_create(1024, 0, &obj_arena),
      yr_free(stack));


        assert(sp == 0);
        // if i == MAX_OVERLOADED_FUNCTIONS at this point no matching
        // prototype was found, but this shouldn't happen.

        // make a copy of the returned object and push the copy into the stack
        // function->return_obj can't be pushed because it can change in
        // subsequent calls to the same function.

          result = yr_object_copy(function->return_obj, &r1.o);

        // a pointer to the copied object is stored in a arena in order to
        // free the object before exiting yr_execute_code
        if (result == ERROR_SUCCESS)
          result = yr_arena_write_data(obj_arena, &r1.o, sizeof(r1.o), NULL);

        stop = (result != ERROR_SUCCESS);
        push(r1);
  obj_ptr = (YR_OBJECT**) yr_arena_base_address(obj_arena);

  while (obj_ptr != NULL)
  {
    yr_object_destroy(*obj_ptr);

    obj_ptr = (YR_OBJECT**) yr_arena_next_address(
        obj_arena, obj_ptr, sizeof(YR_OBJECT*));
  }

  yr_arena_destroy(obj_arena);
int yr_object_copy(
    YR_OBJECT* object,
    YR_OBJECT** object_copy);



    SIZED_STRING* s1,
    SIZED_STRING* s2);


SIZED_STRING* sized_string_dup(
    SIZED_STRING* s);
define_function(foobar)
{
  int64_t arg = integer_argument(1);

  switch (arg)
  {
    case 1:
      return_string("foo");
      break;
    case 2:
      return_string("bar");
      break;
  }

  return_string("oops")
}

  declare_function("foobar", "i", "s", foobar);
      ((YR_OBJECT_INTEGER*) copy)->value = ((YR_OBJECT_INTEGER*) object)->value;
      if (((YR_OBJECT_STRING*) object)->value != NULL)
      {
        ((YR_OBJECT_STRING*) copy)->value = sized_string_dup(
            ((YR_OBJECT_STRING*) object)->value);
      }
      else
      {
        ((YR_OBJECT_STRING*) copy)->value = NULL;
      }
      break;

    case OBJECT_TYPE_FLOAT:
      ((YR_OBJECT_DOUBLE*) copy)->value = ((YR_OBJECT_DOUBLE*) object)->value;
#include <string.h>
#include <yara/mem.h>


SIZED_STRING* sized_string_dup(
    SIZED_STRING* s)
{
  SIZED_STRING* result = (SIZED_STRING*) yr_malloc(
      sizeof(SIZED_STRING) + s->length);

  if (result == NULL)
    return NULL;

  result->length = s->length;
  result->flags = s->flags;

  strncpy(result->c_string, s->c_string, s->length + 1);

  return result;
}
  assert_true_rule(
      "import \"tests\" \
       rule test { \
        condition: tests.foobar(1) == tests.foobar(1) \
      }",
      NULL);

  assert_true_rule(
      "import \"tests\" \
       rule test { \
        condition: tests.foobar(1) != tests.foobar(2) \
      }",
      NULL);