Security considerations behind this code snippet in C

Question

I found this code on git and wanted to use it, but someone made a comment about a security bug in it. I don't seem to identify it:

int32_t read_arrbuff(FILE *f, uint32_t *arrmap) {
  int32_t i = 0, n_map;
  fread(&n_map, sizeof(n_map), 1, f);
  if (n_map > 256)
    return -1;
  while (n_map--) {
    fread(&arrmap[i++], sizeof(uint32_t), 1, f);
  }
  return n_map;
}

Answer 1

Taken in isolation, the problems include:

• You don't check that f is not NULL.
• You don't check that arrmap is not NULL.
• You don't check that the first fread() was successful.
• You don't fully validate the value that is read; negative or zero values will throw things off badly.
• You don't check that the second fread() was successful.
• You always return -1, regardless of whether anything worked or failed.

Which of those are security problems? At some levels, all of them. In some contexts, you could assume that f and arrmap are legitimate without checking. Not checking that the reads are successful, especially the first, is a serious problem. The negative values for n_map would be a serious issue. Claiming success when every read failed would be a problem.

When the loop completes, n_map is set to -1 (it was zero before the post-decrement). So, you return -1 on failure or success. That's not helpful. It should almost certainly return the value of n_map that was read from file, so the caller can tell how many values are in the array.

It is usually best not to hard code a size limit like 256 into the program. The interface should include the array size and you should check against the passed array size.

Working to the original interface, you could use:

#include "stderr.h"
#include <stdio.h>
#include <inttypes.h>

extern int32_t read_arrbuff(FILE *f, uint32_t *arrmap);

int32_t read_arrbuff(FILE *f, uint32_t *arrmap)
{
    int32_t n_map;
    if (fread(&n_map, sizeof(n_map), 1, f) != 1)
        err_syserr("failed to read data (count)\n");
    if (n_map > 256 || n_map <= 0)
        return -1;
    for (int32_t i = 0; i < n_map; i++)
    {
        if (fread(&arrmap[i], sizeof(uint32_t), 1, f) != 1)
            err_syserr("failed to read data (value %" PRId32 ")\n", i);
    }
    return n_map;
}

int main(int argc, char **argv)
{
    err_setarg0(argv[0]);
    if (argc != 1)
        err_usage("");

    char file[] = "data";

    /* Create data file */
    FILE *fp = fopen(file, "wb");
    if (fp == NULL)
        err_syserr("failed to open file '%s' for writing\n", file);
    int32_t nmap = 32;
    if (fwrite(&nmap, sizeof(nmap), 1, fp) != 1)
        err_syserr("failed to write to file '%s'\n", file);
    for (int32_t i = 0; i < nmap; i++)
    {
        if (fwrite(&i, sizeof(i), 1, fp) != 1)
            err_syserr("failed to write to file '%s'\n", file);
    }
    fclose(fp);

    /* Read data file */
    fp = fopen(file, "rb");
    if (fp == NULL)
        err_syserr("failed to open file '%s' for reading\n", file);

    uint32_t amap[256];
    int32_t rc = read_arrbuff(fp, amap);
    printf("rc = %" PRId32 "\n", rc);

    for (int32_t i = 0; i < rc; i++)
        printf("%3" PRId32 " = %3" PRId32 "\n", i, amap[i]);

    fclose(fp);

    return 0;
}

You can debate whether the unilateral exits imposed by err_syserr() are appropriate. (The declarations and source for the err_*() functions is in stderr.h and stderr.c, and is available from GitHub.)

An alternative version taking the maximum array size from a function parameter is:

#include "stderr.h"
#include <assert.h>
#include <stdio.h>
#include <inttypes.h>

extern int32_t read_arrbuff(FILE *f, int32_t a_size, uint32_t arrmap[a_size]);

int32_t read_arrbuff(FILE *f, int32_t a_size, uint32_t arrmap[a_size])
{
    int32_t n_map;
    assert(f != NULL && arrmap != NULL && a_size > 0 && a_size <= 256);
    if (fread(&n_map, sizeof(n_map), 1, f) != 1)
    {
        err_sysrem("failed to read data (count)\n");
        return -1;
    }
    if (n_map > a_size || n_map <= 0)
    {
        err_sysrem("count %" PRId32 " is out of range 1..%" PRId32 "\n",
                   n_map, a_size);
        return -1;
    }
    for (int32_t i = 0; i < n_map; i++)
    {
        if (fread(&arrmap[i], sizeof(uint32_t), 1, f) != 1)
        {
            err_syserr("failed to read data (value %" PRId32 " of %" PRId32 ")\n",
                       i, n_map);
        }
    }
    return n_map;
}

int main(int argc, char **argv)
{
    err_setarg0(argv[0]);
    if (argc != 1)
        err_usage("");

    char file[] = "data";

    /* Create data file */
    FILE *fp = fopen(file, "wb");
    if (fp == NULL)
        err_syserr("failed to open file '%s' for writing\n", file);
    int32_t nmap = 32;
    if (fwrite(&nmap, sizeof(nmap), 1, fp) != 1)
        err_syserr("failed to write to file '%s'\n", file);
    for (int32_t i = 0; i < nmap; i++)
    {
        if (fwrite(&i, sizeof(i), 1, fp) != 1)
            err_syserr("failed to write to file '%s'\n", file);
    }
    fclose(fp);

    /* Read data file */
    fp = fopen(file, "rb");
    if (fp == NULL)
        err_syserr("failed to open file '%s' for reading\n", file);

    enum { AMAP_SIZE = 256 };
    uint32_t amap[AMAP_SIZE];
    int32_t rc = read_arrbuff(fp, AMAP_SIZE, amap);
    printf("rc = %" PRId32 "\n", rc);

    for (int32_t i = 0; i < rc; i++)
        printf("%3" PRId32 " = %3" PRId32 "\n", i, amap[i]);

    fclose(fp);

    return 0;
}

This version reports a specific error and returns on error. It makes semi-appropriate assertions on entry (the 256 limit is not necessarily appropriate, but is consistent with the original code).

The output is unexciting (and the same from both):

rc = 32
  0 =   0
  1 =   1
  2 =   2
  3 =   3
  4 =   4
  5 =   5
  6 =   6
  7 =   7
  8 =   8
  9 =   9
 10 =  10
 11 =  11
 12 =  12
 13 =  13
 14 =  14
 15 =  15
 16 =  16
 17 =  17
 18 =  18
 19 =  19
 20 =  20
 21 =  21
 22 =  22
 23 =  23
 24 =  24
 25 =  25
 26 =  26
 27 =  27
 28 =  28
 29 =  29
 30 =  30
 31 =  31