Security vulnerabilities in fairly simple c code

Question

I'm studying for my last ever exam (yey!), and have run into a problem I have a hard time figuring out. It's an old exam question where you are supposed to find at least two vulnerabilities that can be exploited in a function which reads a ppm image file. The only issue I can identify is if cols and/or rows are given unexpected values, either too large (causing integer overflow) or negative, which leads to img->raster having an incorrect size, opening up the possibility of a heap-based buffer-overflow attack.

As far as I can reason, the unchecked malloc should not be exploitable.

struct image *read_ppm(FILE *fp)
{
    int version;
    int rows, cols, maxval;
    int pixBytes=0, rowBytes=0, rasterBytes;
    uint8_t *p;
    struct image *img;
    /* Read the magic number from the file */
    if ((fscanf(fp, " P%d ", &version) < 1) || (version != 6)) {
        return NULL;
    }
    /* Read the image dimensions and color depth from the file */
    if (fscanf(fp, " %d %d %d ", &cols, &rows, &maxval) < 3) {
        return NULL;
    }
    /* Calculate some sizes */
    pixBytes = (maxval > 255) ? 6 : 3; // Bytes per pixel
    rowBytes = pixBytes * cols; // Bytes per row
    rasterBytes = rowBytes * rows; // Bytes for the whole image
    /* Allocate the image structure and initialize its fields */
    img = malloc(sizeof(*img));
    if (img == NULL) return NULL;
    img->rows = rows;
    img->cols = cols; 
    img->depth = (maxval > 255) ? 2 : 1;
    img->raster = (void*)malloc(rasterBytes);
    /* Get a pointer to the first pixel in the raster data. */
    /* It is to this pointer that all image data will be written. */
    p = img->raster;
    /* Iterate over the rows in the file */
    while (rows--) {
        /* Iterate over the columns in the file */
        cols = img->cols;
        while (cols--) {
            /* Try to read a single pixel from the file */
            if (fread(p, pixBytes, 1, fp) < 1) {
                /* If the read fails, free memory and return */
                free(img->raster);
                free(img);
                return NULL;
            }
            /* Advance the pointer to the next location to which we
            should read a single pixel. */
            p += pixBytes;
        }
    }
    /* Return the image */
    return img;
}

Original (the last question): http://www.ida.liu.se/~TDDC90/exam/old/TDDC90%20TEN1%202009-12-22.pdf

Thanks for any help.

Answer 1

Create a large file such that reading row and cols are both negatives. rasterBytes = pixBytes * rows * cols is positive so everything will be fine till p = img->raster;. But at this point you have two infinite loops, and the program may overwrite the heap.

Another attack is to set up row and cols such that they have different sign. You can choose either value to be -1, while the other is large enough to read the data you want. The allocation

  img->raster = (void*)malloc(rasterBytes);

will fail, which lead img->raster to point to NULL. Which means

 fread(p, pixBytes, 1, fp) < 1

will try to read the content of the file to kernel memory. If this code is executed in kernel mode, depending of the system (let say old unix which doesn use memory segment), then you will overwrite the content of the kernel memory with the content of the file. A kernel which doesn use memory segment rely not on segmentation faults but on page faults (a virtual address which doesnt have any real page assigned to it). The issue is that there are virtual memory designs such that the first real pages are directly assigned to the kernel pages. Ie kernel virtual address 0x0 is correspond to the real memory at 0x0 and is perfectly valid (inside the kernel).

EDIT: In both of those cases, the goal of the attacker is to inject the content of the input file (which is totally under his control) in a region of memory he should not have access to, while not being able to modify the function read_ppm().