Using fseek and ftell to determine the size of a file has a vulnerability?

Question

I've read posts that show how to use fseek and ftell to determine the size of a file.

FILE *fp;
long file_size;
char *buffer;

fp = fopen("foo.bin", "r");
if (NULL == fp) {
 /* Handle Error */
}

if (fseek(fp, 0 , SEEK_END) != 0) {
  /* Handle Error */
}

file_size = ftell(fp);
buffer = (char*)malloc(file_size);
if (NULL == buffer){
  /* handle error */
}

I was about to use this technique but then I ran into this link that describes a potential vulnerability.

The link recommends using fstat instead. Can anyone comment on this?

Embankment

Answer 1

The link is one of the many nonsensical pieces of C coding advice from CERT. Their justification is based on liberties the C standard allows an implementation to take, but which are not allowed by POSIX and thus irrelevant in all cases where you have fstat as an alternative.

POSIX requires:

1. that the "b" modifier for fopen have no effect, i.e. that text and binary mode behave identically. This means their concern about invoking UB on text files is nonsense.

2. that files have a byte-resolution size set by write operations and truncate operations. This means their concern about random numbers of null bytes at the end of the file is nonsense.

Sadly with all the nonsense like this they publish, it's hard to know which CERT publications to take seriously. Which is a shame, because lots of them are serious.

Cruces

I agree with this w.r.t. the idea that the advice against using fseek to figure out the size of a file improves security. I still would use fstat if I wanted to know the size of a file, but because I think it's clean and clear, not because I think it will avoid security problems by itself. – Prothalamion

Using fseek has the advantage that it will work on block device files – Berman

I would avoid fstat because it precludes operating on device nodes where your application might otherwise work fine. – Cruces

Why would they want text and binary to be read the same way?! that's stupid as hell! – Diffractive

@MarcusJ: I'm not sure what you're asking or what your comment is supposed to contribute to this question or this answer. – Cruces

@Diffractive Basically POSIX is saying that text and binary files are treated the same so an end of line character on POSIX systems will always be one character. But on Windows it isn't one character and hence the C standard needs both b and r file modes. – Ternan

Answer 2

If your goal is to find the size of a file, definitely you should use fstat() or its friends. It's a much more direct and expressive method--you are literally asking the system to tell you the file's statistics, rather than the more roundabout fseek/ftell method.

A bonus tip: if you only want to know if the file is available, use access() rather than opening the file or even stat'ing it. This is an even simpler operation which many programmers aren't aware of.

Prothalamion

This relies on POSIX functionality (fstat) rather than plain C, and it's less reliable. For instance, if the "file" is /dev/sda1, fstat will show 0 size, but the fseek/ftell method will get the size of the partition. (Actually you should use fseeko/ftello which use off_t, since long is likely too short to store the size of a partition...) – Cruces

What if the goal is to determine the size of the file in order to allocate a buffer big enough. Does it make a difference? – Embankment

@Frank: If you need to read the entire file into a buffer at once, you should think hard about why that is. I can think of virtually no cases where it wouldn't be better to read the file in chunks, which anyway is less likely to fail on large files. – Prothalamion

There are plenty of cases where you need to read the whole file. sort is the first that comes to mind. – Cruces

Do you think sort allocates one humongous buffer the size of the input file? I don't know for sure, but I would be a little surprised. – Prothalamion

@John: well, it won't allocate one big buffer if you call it with -m, but otherwise I'd expect it to do it. – Barbie

I just tried it. It will allocate a buffer up to the size of the longest line, but after that, it uses a temporary file to store lines while sorting. It does not seem to allocate one giant region for the entire input file. – Prothalamion

In any case you need to read the whole file and store the contents somewhere to implement sort. Whether you use memory or a temp file is a small detail for handling extremely large files. There are plenty of cases like this, in any case, where the max file size would be orders of magnitude smaller than virtual address space and thus where loading the whole file may be the simplest approach. – Cruces

Answer 3

I'd tend to agree with their basic conclusion that you generally shouldn't use the fseek/ftell code directly in the mainstream of your code -- but you probably shouldn't use fstat either. If you want the size of a file, most of your code should use something with a clear, direct name like filesize.

Now, it probably is better to implement that using fstat where available, and (for example) FindFirstFile on Windows (the most obvious platform where fstat usually won't be available).

The other side of the story is that many (most?) of the limitations on fseek with respect to binary files actually originated with CP/M, which didn't explicitly store the size of a file anywhere. The end of a text file was signaled by a control-Z. For a binary file, however, all you really knew was what sectors were used to store the file. In the last sector, you had some amount of unused data that was often (but not always) zero-filled. Unfortunately, there might be zeros that were significant, and/or non-zero values that weren't significant.

If the entire C standard had been written just before being approved (e.g., if it had been started in 1988 and finished in 1989) they'd probably have ignored CP/M completely. For better or worse, however, they started work on the C standard in something like 1982 or so, when CP/M was still in wide enough use that it couldn't be ignored. By the time CP/M was gone, many of the decisions had already been made and I doubt anybody wanted to revisit them.

For most people today, however, there's just no point -- most code won't port to CP/M without massive work; this is one of the relatively minor problems to deal with. Making a modern program run in only 48K (or so) of memory for both the code and data is a much more serious problem (having a maximum of a megabyte or so for mass storage would be another serious problem).

CERT does have one good point though: you probably should not (as is often done) find the size of a file, allocate that much space, and then assume the contents of the file will fit there. Even though the fseek/ftell will give you the correct size with modern systems, that data could be stale by the time you actually read the data, so you could overrun your buffer anyway.

Volcano

Regarding your last paragraph, there's no overrun danger as long as you use the same length you computed for allocating the buffer and reading into it. And there's no way to read a file atomically, so the possibility of the length being stale, while real, is inevitable. – Cruces

@R: Right -- the problem arises if you compute a length, and then just read the whole file, assuming it'll fit. To avoid the length being stale, you just read to the end of file, and expand the buffer if necessary (or use something like mmap...). – Volcano

Your workaround only works if the only change being made to the file is appending. And mmap is even more dangerous. If the file size goes down after mmap, you'll SIGBUS on read and you cannot safely recover. – Cruces

@R: yes, if truncation is a possibility, you have to take rather different steps. I'd dispute SIGBUS being more dangerous -- it certainly shuts down the program, but doesn't threaten the rest of the system (at worst enables a DoS attack, which is minor compared to many buffer overrun-based attacks). – Volcano

I can't imagine how you would make a buffer overrun here. p=malloc(len); if (!p) goto error; fread(p,len,1,f); – Cruces

@R: You obviously lack imagination! p=malloc(len); while ((ch=getc())!=EOF) *p++=ch; Of course, I have a little advantage: I don't have to imagine -- I've fixed code at least that bad. – Volcano

Windows has GetFileSizeEx, no need for the FindFirstFile nonsense. In general, I am amazed by how limited Linux / POSIX API is compared to WinAPI. For example, you can't get the size of a file in one syscall and without any extra junk that fstat returns. – Eliath

@VioletGiraffe: Depends a bit on the situation. In some cases you don't even want to try to read the file if it's too large, but GetFileSizeEx doesn't support that--it requires that you open the file before you can get the size (at which point you've already changed the file's last access time, even though you end up not reading the file). So if you're recommending one rather blindly, without knowing all the details of how it's being used, FindFirstFile is the less intrusive (and therefore more widely applicable). – Volcano

Answer 4

The reason to not use fstat is that fstat is POSIX, but fopen, ftell and fseek are part of the C Standard.

There may be a system that implements the C Standard but not POSIX. On such a system fstat would not work at all.

Jeaz

The point of the CERT advisory is that on such a system, the seek/tell approach might have implementation-defined or undefined behavior. However on such a system you don't have fstat. Any system that does have fstat certainly has sane, well-defined idea of file offsets and lengths. – Cruces

C is used on some systems where there isn't even an operating system. So you can't really assume POSIX unless you know who is going to run your code. Even Windows isn't POSIX although it does have a fstat function (but the function is called _fstat because Microsoft wants you to know it isn't a standard function.) – Ternan

Answer 5

According to C standard, §7.21.3:

Setting the file position indicator to end-of-file, as with fseek(file, 0, SEEK_END), has undefined behavior for a binary stream (because of possible trailing null characters) or for any stream with state-dependent encoding that does not assuredly end in the initial shift state.

A letter-of-the-law kind of guy might think this UB can be avoided by calculating file size with:

fseek(file, -1, SEEK_END);
size = ftell(file) + 1;

But the C standard also says this:

A binary stream need not meaningfully support fseek calls with a whence value of SEEK_END.

As a result, there is nothing we can do to fix this with regard to fseek / SEEK_END. Still, I would prefer fseek / ftell instead of OS-specific API calls.

Suicide