| Summary: | media-sound/dagrab fails with buffer overflow detected when using cddb | ||
|---|---|---|---|
| Product: | Gentoo Linux | Reporter: | Gil Kloepfer <gbz> |
| Component: | Current packages | Assignee: | Gentoo Sound Team <sound> |
| Status: | UNCONFIRMED --- | ||
| Severity: | normal | Keywords: | PATCH |
| Priority: | Normal | ||
| Version: | 10.0 | ||
| Hardware: | AMD64 | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Package list: | Runtime testing required: | --- | |
| Attachments: | patch to cddb.c with proposed fix for error leading to buffer overflow | ||
(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. I should also add this is ebuild media-sound/dagrab (sorry for the continued addition of comments) |
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.