CODEWITHSUNDEEP
csecuritymallocmemcpy
Dimen
Dimen

Reputation: 391

Vulnerability of malloc and memcpy

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!

Upvotes: 2

Views: 1668

Answers (2)

chqrlie
chqrlie

Reputation: 144780

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);   
}

Upvotes: 1

Che Huu
Che Huu

Reputation: 163

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)

Upvotes: 0

Related Questions