OAuth implementation for Puredata

Question

I am managing an OAuth implementation for Puredata (Pd), which is written in C. OAuth can accept RSA keys, but Pd cannot send messages with newlines, so placing private keys will come as a list of strings with spaces acting as separators.

Basically this function looks into the different parts of the block and adds either a newline or a space at the end, such that -----BEGIN PRIVATE KEY----- will end up in one line, while A7qVvdqxevEuUkW4K+2KdMXmnQbG9Aa7k7eBjK1S+0LYmVjPKlJGNXHDGuy5Fw/d 7rjVJ0BLB+ubPK8iA/Tw3hLQgXMRRGRXXCn8ikfuQfjUS1uZSatdLB81mydBETlJ will end up in separate ones.

Some functions, types and constants, that are used here: MAXPDSTRING: the maximum length of a string, t_atom: message part, atom_string(*t_atom src, *char dest, size_t max_length): convert src to dest with a max_length.

static char *string_create(size_t *const newl, const size_t strl) {
    char *gen;

    /* newl is not the length of the string, but the memory size */
    (*newl) = 1 + strl;
    gen = getbytes((*newl) * sizeof(char));
    if (gen == NULL) {
        pd_error(0, "not enough memory.");
        return gen;
    }
    return memset(gen, 0x00, (*newl));
}

static void oauth_set_rsa_key(t_oauth *const oauth, const int argc, t_atom *const argv) {
    char temp[MAXPDSTRING];
    size_t rsa_key_len = 1; /* Accomodate NULL terminator */
    short use_newline = 0;

    for (int i = 1; i < argc; i++) {
        atom_string(argv + i, temp, MAXPDSTRING);
        rsa_key_len += strlen(temp) + 1; /* Each section + 1 for space or newline */
    }
    oauth->oauth.rsa_key = string_create(&oauth->oauth.rsa_key_len, rsa_key_len);
    for (int i = 1; i < argc; i++) {
        atom_string(argv + i, temp, MAXPDSTRING);
        if (strncmp(temp, "-----", 5) == 0 && strlen(oauth->oauth.rsa_key) > 1)  {
            /* Cutoff trailing space and add newline */
            memset(oauth->oauth.rsa_key + strlen(oauth->oauth.rsa_key) - 1, 0x00, 1);
            strcat(oauth->oauth.rsa_key, "\n");
            use_newline = 0;
        }
        if (strlen(temp) >= 5 && strncmp(temp + strlen(temp) - 5, "-----", 5) == 0) {
            use_newline = 1;
        }
        strcat(oauth->oauth.rsa_key, temp);
        if (i < argc - 1) {
            if (use_newline == 1)  {
                /* This gets flagged as possible buffer overflow */
                strcat(oauth->oauth.rsa_key, "\n");
            } else {
                /* This also */
                strcat(oauth->oauth.rsa_key, " ");
            }
        }
    }
}

Static analysis in Coverity does flag strcat(oauth->oauth.rsa_key, "\n"); as possible buffer overflow, but I cannot see the possible overflow here. What am I missing?

I could not provoke a buffer overflow by arbitrary data containing any combinations of -, spaces or anything.

Answer 1

Almost certainly, Coverity is simply looking for all uses of strcat, strcpy, sprintf, etc., and flagging them as "potential buffer overflow" no matter what the context. You can check that by adding some code to your project that does something simple and obviously correct like

char *p = malloc(10);
strcpy(p, "12345");
strcat(p, "6789");
free(p);

or

char *mystrdupdot(const char *s) {
    char *p = malloc(strlen(s) + 2);
    strcpy(p, s);
    return strcat(p, ".");
}

If Coverity flags those strcpys or strcats as buffer overflows, then you've got your answer.

---

memset(oauth->oauth.rsa_key + strlen(oauth->oauth.rsa_key) - 1, 0x00, 1);

That is a very weird way of saying

oauth->oauth.rsa_key[strlen(oauth->oauth.rsa_key)-1] = '\0';

Also, you should probably look for a way to stop recomputing strlen every time through this loop.