Opened 12 years ago

Closed 11 years ago

#5288 closed defect (fixed)

HTTPRequest::SendResponseFile file race and negative returns

Reported by: Erik Hovland <erik@…> Owned by: Isaac Richards
Priority: minor Milestone: 0.22
Component: mythtv Version: head
Severity: low Keywords:
Cc: Ticket locked: no

Description

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.

Attachments (4)

libs_libmythupnp-negative-returns.patch (3.2 KB) - added by Erik Hovland <erik@…> 12 years ago.
reworks the member function so that it does not have a file race nor a unchecked open() call
libmythupnp-negative-returns.patch (3.4 KB) - added by Erik Hovland <erik@…> 11 years ago.
Replaces previous patch as of svn revision #18033
5288-v3.patch (14.0 KB) - added by danielk 11 years ago.
5288-v4.patch (5.7 KB) - added by Nigel 11 years ago.
Files remaining after [18412]

Download all attachments as: .zip

Change History (14)

Changed 12 years ago by Erik Hovland <erik@…>

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

Changed 11 years ago by Erik Hovland <erik@…>

Replaces previous patch as of svn revision #18033

comment:1 Changed 11 years ago by 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.

comment:2 Changed 11 years ago by 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.

Changed 11 years ago by danielk

Attachment: 5288-v3.patch added

comment:3 Changed 11 years ago by danielk

Milestone: unknown0.22

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.

comment:4 Changed 11 years ago by 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!

comment:5 Changed 11 years ago by 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.

comment:6 Changed 11 years ago by Nigel

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

Changed 11 years ago by Nigel

Attachment: 5288-v4.patch added

Files remaining after [18412]

comment:7 Changed 11 years ago by Nigel

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

comment:8 Changed 11 years ago by Nigel

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

comment:9 Changed 11 years ago by danielk

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

comment:10 Changed 11 years ago by danielk

Resolution: fixed
Status: newclosed

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

Note: See TracTickets for help on using tickets.