Adsk Contrib - Improve LUT loading

[doug-walker]

No description provided.

[KevinJW]

Just to note OpenColorIO/issues/2257 so as an extended solution we could consider the larger issue ideally I'd like to see the removal of the macro.

[KevinJW]

I think there maybe another case to fix

OpenColorIO/src/OpenColorIO/fileformats/FileFormatIridasCube.cpp

Line 327 in aa842db

if (sscanf(line.c_str(), "%s %s %s %c", valR, valG, valB, &endTok) != 3)

[doug-walker]

@KevinJW , I had seen that as well, but my understanding is that "%c" will only read a singe character, it's sort of like "%1s" (although not null-terminated). So it is already bounded and there is no way it could be used in a buffer-overrun attack. Am I missing something?

[KevinJW]

I was thinking if the first %s was fed a few MB of none space characters, then valR would be overwritten and other things.

Basically if we allow any length of "line" which I think we do then any un constrained %s can overflow. basically we should limit each and every %s with a maximum field width matching it's destination buffer's capacity - 1.

See https://en.cppreference.com/cpp/io/c/fscanf

Matches a sequence of non-whitespace characters (a string). If width specifier is used, matches up to width or until the first whitespace character, whichever appears first. Always stores a null character in addition to the characters matched (so the argument array must have room for at least width+1 characters).

and

optional integer number (greater than zero) that specifies maximum field width, that is, the maximum number of characters that the function is allowed to consume when doing the conversion specified by the current conversion specification. Note that %s and %[ may lead to buffer overflow if the width is not provided

[KevinJW]

Actually by that statement there may be several other cases we need to fix up based on a simple scan of the code using scanf.*%s

[KevinJW]

Basically if we allow any length of "line" which I think we do

I guess in some case we limit it to 4096, but that is bigger than 64, in others where we use std::string it is effectively unbounded

I asked Gemini so find the easiest to mitigate issues and it found the same things but it identified a different approach to fixing them using some of our other utility code (sample from FileFormatSpi3D.cpp) it assumed we used std::sting for our buffers as it also recommended that rather than the fixed buffer size, I'm not convinced that is optimal from a performance standpoint, but that is aside the scanf issues.

        char greenValueS[64] = "";
        char blueValueS[64] = "";

        const StringUtils::StringVec lineParts
            = StringUtils::SplitByWhiteSpaces(StringUtils::Trim(lineBuffer));

        if (lineParts.size() == 6 &&
            StringToInt(&rIndex, lineParts[0].c_str()) &&
            StringToInt(&gIndex, lineParts[1].c_str()) &&
            StringToInt(&bIndex, lineParts[2].c_str()))
        {
            strncpy(redValueS, lineParts[3].c_str(), 63);
            strncpy(greenValueS, lineParts[4].c_str(), 63);
            strncpy(blueValueS, lineParts[5].c_str(), 63);
            const auto redValueAnswer = NumberUtils::from_chars(redValueS, redValueS + 64, redValue);
            const auto greenValueAnswer = NumberUtils::from_chars(greenValueS, greenValueS + 64, greenValue);
            const auto blueValueAnswer = NumberUtils::from_chars(blueValueS, blueValueS + 64, blueValue);

[doug-walker]

Ah, ok, I thought you were commenting about the %c. As discussed in the TSC, this PR is only to apply the fixes that the external contributor requested. This PR will be separate so we are able to credit this other person. I'm not making any other fixes in this one.

If you have any other suggestions, please make them in this other PR.

In that PR, the lines you reference already have width limits on the %s, as follows:

#ifdef _WIN32
            if (sscanf_s(line.c_str(), "%63s %63s %63s %c", valR, 64, valG, 64, valB, 64, &endTok, 1) != 3)
#else
            if (sscanf(line.c_str(), "%63s %63s %63s %c", valR, valG, valB, &endTok) != 3)
#endif

Regarding your Gemini code, I don't see any upside to making that change for 2.5.2. If you feel it is dramatically better, please propose it in a separate PR.

[KevinJW]

Fair enough, I was thinking from the security point of view it is usually better to group all faults of a similar type in a single update, I had not cross referenced the two PRs in that way.

[doug-walker]

Thanks for your understanding, Kevin. Sorry I was not clearer about the requirements for this PR.