A bunch of memory allocation bugs in CGD

[ALeine]

I took a quick look at the latest NetBSD CGD code and found
out that out of 19 memory allocation operations 11 (almost 60%)
are done in a way that could lead to a segmentation violation
which would leave behind a core dump full of sensitive
information that could be used to compromise a CGD encrypted
disk. While this attack is not very practical since it requires
the attacker to be able to cause resource starvation at a
specific time when cgdconfig is used, it is still possible.
Here are the details...

Memory allocation operations in files in src/sbin/cgdconfig:

params.c        1 wrong, 1 correct
pkcs5_pbkdf2.c  1 wrong, 1 correct
utils.c         9 wrong, 6 correct

params.c:90:    p = malloc(sizeof(*p));
params.c-91-    params_init(p);
params.c-92-    return p;
params.c-93-}
params.c-94-
params.c-95-static void
params.c-96-params_init(struct params *p)
params.c-97-{
params.c-98-
params.c-99-    p->algorithm = NULL;
--
pkcs5_pbkdf2.c:103:     data = malloc(Slen + 4);
pkcs5_pbkdf2.c-104-     memcpy(data, S, Slen);
--
utils.c:94:     ret = malloc((nwords+1) * sizeof(char *));
utils.c-95-     tmp1 = tmp = strdup(line);
utils.c-96-     while ((cur = strsep_getnext(&tmp, " \t")) != NULL)
utils.c-97-             ret[i++] = strdup(cur);
--
utils.c:150:    out = malloc(sizeof(*out));
utils.c-151-    out->length = inlength;
utils.c:152:    out->text = malloc(out->length + 1);
utils.c-153-    memcpy(out->text, intext, out->length);
utils.c-154-    out->text[out->length] = '\0';
--
utils.c:188:    sum = malloc(sizeof(*sum));
utils.c-189-    sum->length = a1->length + a2->length;
utils.c:190:    sum->text = malloc(sum->length + 1);
utils.c-191-    memcpy(sum->text, a1->text, a1->length);
--
utils.c-255-    /* XXX do some level of error checking here */
utils.c:256:    b = malloc(sizeof(*b));
utils.c-257-    b->length = len;
utils.c:258:    b->text = malloc(BITS2BYTES(b->length));
utils.c-259-    memcpy(b->text, buf, BITS2BYTES(b->length));
--
utils.c-323-    /* XXX do some level of error checking here */
utils.c:324:    b = malloc(sizeof(*b));
utils.c-325-    b->length = MAX(x1->length, x2->length);
utils.c:326:    b->text = calloc(1, BITS2BYTES(b->length));
utils.c-327-    for (i=0; i < BITS2BYTES(MIN(x1->length, x2->length)); i++)
utils.c-328-            b->text[i] = x1->text[i] ^ x2->text[i];

Also, using free_notnull() is pointless, that function should be removed
and all calls to that function should be replaced with calls to free(3),
since free(3) takes no action if the pointer is NULL.

utils.c:171:    free_notnull(s->text);
utils.c:276:    free_notnull(b->text);
utils.c:411:            free_notnull(tmp);
utils.c:412:            free_notnull(out);
--
utils.c-495-void
utils.c:496:free_notnull(void *b)
utils.c-497-{
utils.c-498-
utils.c-499-    if (b)
utils.c-500-            free(b);
utils.c-501-}

I find it alarming that this kind of sloppy programming can be found in
a piece of software that is supposedly designed to be secure and provide
security. I believe the CGD code should be seriously audited before
anyone considers using it in a production environment.

ALeine
___________________________________________________________________
WebMail FREE http://mail.austrosearch.net