1
votes

I don't post here often, so bear with me while I try to decide how to solve this problem.

I'm updating a code base that hasn't been touched for between 10 - 20 years. The code was written without adherence to best practices, and by many authors with sometimes incomplete understandings of security conventions, or perhaps even before those conventions were common practice. The compiler used on this code is c++98 or c++03, but not more recent. The code is also cross platform between Linux and Windows.

All of that said, I need the help of some C++ veterans understanding the proper use of access(), stat() and open() and their perversions as it relates to TOCTOU issues.

Here is an example block of code illustrating the TOCTOU issue:

#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

        /* This is a fix from a previous 
           Stack-based Buffer Overflow
           issue.  I tried to keep the original
           code as close to possible while
           dealing with the potential security
           issue, as I can't be certain of how
           my change might effect the system. */

        std::string checkStr("File ");
        checkStr += file.c_str();
        checkStr += " Not Found or Not Readable";
        if(checkStr.length() >= 2560)
            throw checkStr.c_str();

        char message[2560];
        sprintf(message, "File '%s' Not Found or Not Readable", file.c_str());
        //DISPLAY_MSG_ERROR( this, message, "GetFileContents", "System" );
        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

        /* Same story here. */

        std::string checkStr("File ");
        checkStr += file.c_str();
        checkStr += " No Status Available";
        if(checkStr.length() >= 2560)
            throw checkStr.c_str();

        char message[2560];
        sprintf(message, "File '%s' No Status Available", file.c_str());
        //DISPLAY_MSG_ERROR( this, message, "GetFileContents", "System" );
        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());
        //DISPLAY_MSG_ERROR( this, message, "GetFileContents", "System" );
        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);
}

As you can see, the previous developer(s) wanted to make sure the user had access to "file", stat-ed "file", and then opened it without re-access()-ing and without re-stat()-ing "file" after fopen()-ing it. I understand why this is a TOCTOU problem, but I'm not sure of how to fix it.

From what I've researched, open() is preferred over fopen() and fstat() over stat(), but where does lstat() fit in? and if I do a stat, do I even need access() or faccessat()?

And to compound that, this needs to be compatable with a Windows compiler, and I've found articles saying that fopen() is better because it is cross platform while open() is not, making open() potentially unusable; this also seems to be the same for variations of stat() and access(), but I'm not certain.

If you've gotten this far, thank you for reading it all, and I welcome any criticism about the post and help.

I've found articles saying that fopen() is better because it is cross platform while open() is not That´s correct. For some use cases, platform-specific functions may be the only (possible/acceptable) solution, but if fopen can do the job, always prefer it.deviantfan
Personally, I'd get rid of the calls to access and stat, since they don't seem to be doing anything useful. The code doesn't even bother to use the data that stat is returning!Harry Johnston
Do those exceptions actually work? They seem to return a pointer do a deleted (due to stack unwinding) local buffer.MikeMB
I'm not sure if the exceptions actually work or not. I'm new to the system, and it seems they never made a way to unit test these routines.user3633538
The access and stat calls are only used in the if statements to check if the file is within the user's permissions, and that the file is actually there. If they return failure, there is a sort of exception thrown. That's why this all needs to be re written.user3633538