Vulnerability of malloc and memcpy

user_8076266 0
Viewed 1028
void *copy_elements(void *ele_src[], int ele_cnt, size_t ele_size) 
{ 
    /*
     * Allocate buffer for ele_cnt objects, each of ele_size bytes 
     * and copy from locations designated by ele_src
     */

    void *result = malloc(ele_cnt * ele_size); 

    if (result == NULL)
        /* malloc failed */
        return NULL; 
   
    void *next = result;
    int i;

    for (i = 0; i < ele_cnt; i++) {
        /* Copy object i to destination */ 
        memcpy(next, ele_src[i], ele_size);
        /* Move pointer to next memory region */ 
        next += ele_size;
    }

    return result; 
}

The above code has a vulnerability issue. Under some ele_cnt and ele_size values, the code might crash. I suspect it has to do with unsigned - signed conversion that happens with malloc and memcpy but I'm not sure how to exploit this properly. Any help would be thankful!

memcpy malloc c security
2 Answers

There are multiple issues with the posted code:

  • since ele_cnt is defined with a signed type, one should test if the value passed is negative or zero and return NULL without attempting to allocate a huge amount of memory, which may cause a runtime error.
  • for defensive coding, ele_size should not be zero either.
  • next += ele_size; is invalid as next has void * type. Some compilers allow it as an extension, but the code is not portable.
  • copying the elements one at a time is inefficient, a single call to memcpy suffices.

Here is a modified verion:

#include <stdlib.h>
#include <string.h>

void *copy_elements(const void *ele_src[], int ele_cnt, size_t ele_size) 
{ 
    /*
     * Allocate buffer for ele_cnt objects, each of ele_size bytes 
     * and copy from locations designated by ele_src
     */
    void *result;

    if (ele_cnt <= 0 || ele_size == 0) {
        /* invalid arguments */
        return NULL;
    }

    if ((result = malloc(ele_cnt * ele_size)) == NULL) {
        /* malloc failure */
        return NULL;
    }

    /* copy all elements with a single call to memcpy */
    return memcpy(result, ele_src, ele_cnt * ele_size);   
}
user_45932670

I think the problems is next += ele_size is not as you suppose. When you adding pointer, it automatically multiply with offset. You can print next value in for loop so you can check if it true using printf("%p", next)

user_45323940
Related