PDA

View Full Version : Another link from Microsoft about bug in fread



vart
05-06-2008, 01:06 AM
04/15/2008: "A bug in fread() could lead to a buffer overflow vulnerability" (http://www.davidlitchfield.com/blog/archives/00000037.htm)
I see it as a bug in using fread, not fread itself...

And as result will say do not strcpy buffer read by fread - use the return value to determine how many chars should be copied...

CornedBee
05-06-2008, 04:20 AM
Nope, it's a bug in fread. A specification bug, maybe. It trashes parts of the buffer that lie beyond the index returned as the number of characters written. A programmer who expected the buffer beyond this index to be unmodified would be caught by this.
Also, this behaves differently from an fread that is implemented exactly as the standard says, so the as-if rule is violated, and the function is in violation of the specification.

For reference, the standard says that fread is implemented to have an effect equivalent to this function:

size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream)
{
int m, s, c;
for(m = 0; m < nmemb; ++m) {
unsigned char *p = (unsigned char*)ptr;
p += m * size;
for(s = 0; s < size; ++s) {
c = fgetc(stream);
if(c == EOF) {
return (m * size) + s;
}
p[s] = (unsigned char)c;
}
}
return nmemb * size;
}
Note that, since fgetc() does the line ending conversion, this cannot possibly write beyond the index it returns.


The conclusions the blogger draws are incorrect, however. It's not a buffer overflow bug, because the function won't write beyond the end of the buffer as you indicate it. Neither is there any danger of you overflowing the buffer if - as is necessary, because fread() never gives nul-termination - you passed a shorter length to fread than indicated and nulled the stuff beyond out. The danger there is is reading garbage.
Also, he says not to trust the return value. This is wrong. It's the return value you have to trust. What you can't trust is the state of the buffer beyond the index of this return value.

Another error of the blogger: CRLF pairs are converted to LFs, not CRs.

whiteflags
05-06-2008, 11:56 AM
I'm bemused how he attempts to prove a point about "buffer overruns" and yet he uses strcpy. Given the conclusion he drew, he wouldn't have considered copying just bytesread bytes, but it would have made the issue go away. Meh.