stat(), fstat(), lstat(), and fopen(); how to write TOCTOU protected system independent code

Question

I've been dealing with a problem for a few weeks now updating 20 year code that needs to be system independent (work on both Linux and Windows). It involves Time-of-Check, Time-of-Use (TOCTOU) issues. I made a thread here, but it didn't go very far, and after ruminating on it for a while and searching deeper into the problem, I think I understand my question a bit better. Maybe I can ask it a bit better too...

From what I've read, the code needs to check if the file exists, if it is accessible, open the file, do some operations and finally close the file. It seems the best way to do this is a call to lstat(), a call to fopen(), a call to fstat() (to rule out the TOCTOU), and then the operations and closing the file.

However, I've been lead to believe that lstat() and fstat() are POSIX defined, not C Standard defined, ruling out their use for a system agnostic program, much in the same way open() shouldn't be used for cross-compatibility. How would you implement this?

If you look at my first post, you can see the developer from 20 years ago used the C preprocessor to cut the code into cross-compatible parts, but even if I did that, I wouldn't know what to replace lstat() or fstat() with (their windows counterparts).

Edit: Added abreviated code to this post; if something is unclear please go to the original post

#ifdef WIN32
    struct _stat buf;
#else
    struct stat buf;
#endif //WIN32

    FILE *fp;
    char data[2560];

    // Make sure file exists and is readable
#ifdef WIN32
    if (_access(file.c_str(), R_OK) == -1) {
#else
    if (access(file.c_str(), R_OK) == -1) {
#endif //WIN32

        char message[2560];
        sprintf(message, "File '%s' Not Found or Not Readable", file.c_str());
        throw message;
        }

    // Get the file status information
#ifdef WIN32
    if (_stat(file.c_str(), &buf) != 0) {
#else
    if (stat(file.c_str(), &buf) != 0) {
#endif //WIN32

        char message[2560];
        sprintf(message, "File '%s' No Status Available", file.c_str());
        throw message;
        }

    // Open the file for reading
    fp = fopen(file.c_str(), "r");
    if (fp == NULL) {
        char message[2560];
        sprintf(message, "File '%s' Cound Not be Opened", file.c_str());
        throw message;
    }

    // Read the file
    MvString s, ss;
    while (fgets(data, sizeof(data), fp) != (char *)0) {
        s = data;
        s.trimBoth();
        if (s.compare( 0, 5, "GROUP" ) == 0) {
            //size_t t = s.find_last_of( ":" );
            size_t t = s.find( ":" );
            if (t != string::npos) {
                ss = s.substr( t+1 ).c_str();
                ss.trimBoth();
                ss = ss.substr( 1, ss.length() - 3 ).c_str();
                group_list.push_back( ss );
            }
        }
    }
    // Close the file
    fclose(fp);
}

I think there are specific windows api functions that could help you write the windows part separately. — Iharob Al Asimi, Jun 02 '15 at 17:15
You could have a wrapper module for each OS and use conditional build. — too honest for this site, Jun 02 '15 at 17:20
@iharob Any idea what they might be called? I'm fresh out of school, and we never used any of the windows api, exclusively gnu; maybe a good resource to start learning? — , Jun 02 '15 at 17:21
@Olaf This system is very fragile. Any edits are supposed to be minimal, so adding new modules is not really a viable solution. I need to modify the source files in the least intrusive way. — , Jun 02 '15 at 17:23
I am sorry I don't work with that either, if you can use c++ then I recommend Qt, you will write everything just once and it will work everywhere, but if you need just c, then you need to dive into the MSDN sea. — Iharob Al Asimi, Jun 02 '15 at 17:23
I see from your last comment that then switching to c++ is not an option. — Iharob Al Asimi, Jun 02 '15 at 17:24
Checking file permissions with `access` before opening is necessary for setuid programs in unix because of the difference between real uid and effective uid. And the `fstat` after open prevents the race. But what is the equivalent attack on Windows, where setuid doesn't exist? What are you actually trying to prevent? — , Jun 02 '15 at 17:27
@WumpusQ.Wumbley I've been tasked with fixing security flaws in old code. One of the issues is a TOCTOU error. They use a call to `access()` too, (which I didn't understand the purpose for until your comment). I ommited the code in this post because it cluttered everything too much, but it is at my [older post](http://stackoverflow.com/q/29526377/3633538) if you'd like to see it. Should I move some of it here? — , Jun 02 '15 at 17:31
File attributes/permissions are different in Ux/Win worlds. You can check [GetFileAttributes](https://msdn.microsoft.com/en-us/library/windows/desktop/aa364944(v=vs.85).aspx) or [GetFileTime](https://msdn.microsoft.com/en-us/library/windows/desktop/ms724320(v=vs.85).aspx) for Win `stat` corresponding data. — CristiFati, Jun 02 '15 at 17:33
@CristiFati The previous developer used something like `_stat()`. Is this depricated? or just wrong? — , Jun 02 '15 at 17:34
@Makenbaccon Can you explain how the TOCTOU vulnerability is exploited on Windows? i.e. why can't you just open the file and let the OS do all the permission checking? Is the program being run by one user on behalf of another user? (is it a server?) — , Jun 02 '15 at 17:36
@WumpusQ.Wumbley It is a server. Despite the amount of time I've been working on this project, I don't have a full grasp on what everything actually does. Many of the function calls in the original code to `access()` or `stat()` appear incomplete to me. I have no idea how it would be exploited on Windows; I have no experience coding for windows. I'm looking at the code and trying my best to emulate the control flow of the previous developer, while eliminating any security issues he might have overlooked. — , Jun 02 '15 at 17:40
If you don't have at least a rough idea of how the problem could be exploited, how do you know there is a problem? — , Jun 02 '15 at 17:41
@WumpusQ.Wumbley There is a company that does static analysis of code; they scanned our code, and found an issue here. Actually, they found 1080 issues throughout the code base, but this one has been quite troubling to me. The static analysis says that there is a TOCTOU issue here, and after reading around, I agree. The static analysis only scanned the Linux version of this code, however. I see similar control flow between the C-preprocessor statements, so I'm trying to translate what POSIX code might write into Windows code as well. — , Jun 02 '15 at 17:45
@Makenbaccon - no it is not deprecated but these are native windows functions. But again nothing wrong with `_stat`. — CristiFati, Jun 02 '15 at 17:46
@CristiFati Wumpus seems to think I shouldn't even be worried about TOCTOU on Windows, and you seem to understand Windows a bit; what do you think? What was the developer from years ago trying to do [here](http://stackoverflow.com/q/29526377/3633538)? — , Jun 02 '15 at 17:49
@WumpusQ.Wumbley So basically, I was handed code marked in red pen with general guidelines on their improvement. This one though is less transparent than others for me. — , Jun 02 '15 at 17:52
My personal opinion is that was trying to writhe code as generel/portable as possible (with least of platform dependent `#ifdef`s). — CristiFati, Jun 02 '15 at 17:53
@CristiFati I guess I was looking for an analagous way of dealing with TOCTOU issues to the POSIX `lstat(),fopen(),fstat()` solution. Have you come across something like this in your time working on windows? If you have, I can just interleave them with the `#ifdef`s. — , Jun 02 '15 at 17:56
@CristiFati Could you give me a quick comment on why someone would use `GetFileAttributes` or `GetFileTime` over `_stat()`? — , Jun 02 '15 at 18:07
@WumpusQ.Wumbley Are you still there? You seemed like someone wise in matters that concern this issue. — , Jun 02 '15 at 18:10
Because (i said it earlier) those are Windows native functions. Probably `_stat` calls internally `GetFileTime` in order to get the file's timestamps. — CristiFati, Jun 02 '15 at 18:16
@Makenbaccon I'm not very wise about Windows, but after looking at the code, it looks like it probably originated on unix, and the ifdef'd changes were minimal to get it to run. There's an `access` and a `stat` before an `fopen`, but their purpose is unclear since the resulting stat `buf` is never used. If that's supposed to be some kind of security check it won't work. But it also might not be *necessary* to do that kind of check, since the program is apparently not setuid anyway. — , Jun 02 '15 at 18:21
I note that `throw message;` is not C code (or requires serious abuse of the C preprocessor to make it into valid C code). — Jonathan Leffler, Jun 02 '15 at 18:24
The closest winapi equivalent for `fstat` afaik is `GetFileInformationByHandle` — loreb, Jun 02 '15 at 19:35

Jonathan Leffler · Accepted Answer · 2015-06-02T22:50:41.207

The reliable way to check whether the file exists and can be opened is to try opening it. If it was opened, all was OK. If it was not opened, you can think about spending time to analyze what went wrong.

The access() function formally asks a different question from what you think; it asks 'can the real user ID or the real group ID access the file', but the program will use the effective user ID or the effective group ID to access the file. If your program is not running SUID or SGID, and was not launched from a program running SUID or SGID — and that's the normal case — then there's no difference. But the question is different.

The use of stat() or lstat() doesn't seem helpful. In particular, lstat() only tells you whether you start at a symlink, but the code doesn't care about that.

Both the access() and the stat() calls provide you with TOCTOU windows of vulnerability; the file could be removed after they reported it was present, or created after they reported it was absent.

You should simply call fopen() and see whether it works; the code will be simpler and more resistant to TOCTOU problems. You might need to consider whether to use open() with all its extra controls (O_EXCL, etc), and then convert the file descriptor to a file pointer (fdopen()).

All of this applies to the Unix side.

The details will be different, but on the Windows side, you will still be best off trying to open the file and reacting appropriately to failure.

In both systems, make sure the options provided to the open function are appropriate.

I'll have to look closer into using `open()` with controls. I was hoping not to use POSIX calls, but you are right, it may be the way to go. Thank you for spelling things out for me. =D — , Jun 02 '15 at 22:46
Often, the "_analyze what went wrong_" step is simply `perror(filename)`, or some other logging of `strerror(errno)`, then a suitable `return`. — Toby Speight, Jul 29 '23 at 15:51

stat(), fstat(), lstat(), and fopen(); how to write TOCTOU protected system independent code

1 Answers1