Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 434528 - media-sound/dagrab fails with buffer overflow detected when using cddb
Summary: media-sound/dagrab fails with buffer overflow detected when using cddb
Alias: None
Product: Gentoo Linux
Classification: Unclassified
Component: Current packages (show other bugs)
Hardware: AMD64 Linux
: Normal normal (vote)
Assignee: Gentoo Sound Team
Keywords: PATCH
Depends on:
Reported: 2012-09-10 03:22 UTC by Gil Kloepfer
Modified: 2012-09-10 11:44 UTC (History)
0 users

See Also:
Package list:
Runtime testing required: ---

patch to cddb.c with proposed fix for error leading to buffer overflow (dagrab-cddb-patch,342 bytes, patch)
2012-09-10 03:22 UTC, Gil Kloepfer
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gil Kloepfer 2012-09-10 03:22:25 UTC
Created attachment 323362 [details, diff]
patch to cddb.c with proposed fix for error leading to buffer overflow

Fortify detects a buffer overflow on runtime when using dagrab with cddb lookup, namely using the following command line:

dagrab -d /dev/sr0 -i -C -S -D ./localdb

The buffer overflow has been traced to line 320 of cddb.c, namely:
sprintf(id, "%lx", cddb_discid(tl));

where id is declared as a 12-byte char.  The problem is actually due to an error in cddb_discid() as it is returning an invalid discid which should only be a maxmium of 32 bits, but because of an error mixing signed ints and unsigned longs in an expression, the upper 16 bits of the return value become 0xffffffff instead of zero (entire function included since it is small):

unsigned long cddb_discid(cd_trk_list * tl)
        int i, t, n = 0;

        for (i = tl->min; i <= tl->max; i++)
                n += cddb_sum(tl->starts[i - tl->min] + CD_MSF_OFFSET);
        t = (tl->starts[tl->max - tl->min + 1] - tl->starts[0]) / 75;

        return (n % 0xff) << 24 | t << 8 | (tl->max - tl->min + 1);

The expression (n % 0xff) << 24 will become negative for a 32 bit signed int if the value of "n" is >127, and this will result in the upper 32 bits of the 64 bit unsigned long being set to 1.

The fix can be to either (preferably) declare i, t, and n as unsigned ints, since they are not going to be negative, or to cast (n % 0xff) as an unsigned long.  I am going to provide a patch for the former, as it tends to be the safest fix.

The code is actually badly written in general since if the discid can potentially be 64 bits long (an unsigned long), the variables i, t, and n in cddb_discid() should all be unsigned longs, and the buffer in the later code that uses the sprintf should be able to hold a string representing 64 bits rather than 32.  However, looking at how this works, I don't see that this needs to be immediately addressed and there is not any danger in leaving the function declaration as it is.
Comment 1 Gil Kloepfer 2012-09-10 03:26:39 UTC
(correction to my explanation)

I said, "upper 16 bits of the return value become 0xffffffff" when I should have said "upper 32 bits of the return value become 0xffffffff".

Either way, the explanation is correct.
Comment 2 Gil Kloepfer 2012-09-10 03:34:52 UTC
I should also add this is ebuild media-sound/dagrab

(sorry for the continued addition of comments)