HTTPRequest::SendResponseFile file race and negative returns

[Erik Hovland <erik@…>]

A file race exists in HTTPRequest::SendResponseFile? often termed as "time of check, time of use". The function has a string which should be a file name. That string is used to get the files information with a stat() call. Then (in this case) the file is opened using the same string. A nefarious user could exploit the time in between these two calls to get the function to open whatever file it wants as long as it is still called the same thing by the string since the stat information was retrieved. The second defect is that open() is called but the returned file descriptor is not checked to see if the open() worked. Then the function passes the file descriptor to close(), if it is negative - close()'es behavior is unspecified and might cause crashing.

The patches solution is to do a QFile::open() instead of a QFile::exists() on the file name. And error out if there was a problem. Then use the QFile::size() instead of stat to get the amount of bytes in the file. Finally, the open() call is removed and the file will close when the QFile object goes out of scope.

[Erik Hovland <erik@…>]

reworks the member function so that it does not have a file race nor a unchecked open() call

[Erik Hovland <erik@…>]

Replaces previous patch as of svn revision #18033

[danielk]

Note: We used stat throughout MythTV to check on file sizes since QFile::size() used to only return a proper file size for small files.

We should confirm that QFile::size() returns the proper size for large files now (post-Qt4 conversion, large being files >4GB); then, assuming it does, we should systematically replace stat() calls that are solely used for file size checks in MythTV.

[anonymous]

OK, I will attempt to see if I can test whether Qt4 does the right thing with big files. The main problem here is not the use of stat per-se but the use of stat before open. I am willing to rework the patch so that it is open then fstat if that is more agreeable.

Thanks for taking a look.

[danielk]

I checked, and QFileInfo & QFile both handle files larger than 2GB now. I've attached a patch that replaces all includes the upnp fix along with a number of other stat replacements. It needs testing though.

[Nigel]

I have systematically tested the UPnP and preview generator changes, and the other ones roughly (except for the job queue and transcode which I haven't had time for). They all look good to commit!

[Nigel]

(In [18356]) Use QFile::size() or QFileInfo::size() instead of stat(). See #5288. Daniel patched other occurences, but these are the only ones I have tested.

[Nigel]

(In [18377]) Re-add sys/stat.h after [18356]. See #5288. Thanks Jay. Closes #5735.

[Nigel]

Files remaining after [18412]

[Nigel]

(In [19307]) Replace stat() with QFile::size(). See #5288

[Nigel]

(In [19400]) One less invocation of stat() to determine file file. See #5288

[danielk]

(In [19447]) Refs #5288. Header cleanup extracted from stat->QFileInfo conversion patch.

[danielk]

(In [19448]) Fixes #5288. The main reported error was fixed long ago, this just applies the remaining stat()->QFile[Info] conversions.