Undefined behavior and heap-buffer-overflow in StringHelpers::GroupStrings(...)

[Arpan3323]

Describe the bug

Hi, fuzz testing the function below has resulted in two distinct errors:

1. runtime error: negation of -2147483648 cannot be represented in type 'int'; cast to an unsigned type to negate this value to itself
2. heap-buffer-overflow
   

   visit/src/common/utility/StringHelpers.C

   Lines 138 to 142 in 3021deb

   	StringHelpers::GroupStrings(vector<string> stringList,
   	vector<vector<string> > &stringGroups,
   	vector<string> &groupNames,
   	int numLeadingVals,
   	string nonRelevantChars)

Error 1 occurs at the line linked below due to negating INT_MIN:

visit/src/common/utility/StringHelpers.C

Line 168 in 3021deb

if (-numLeadingVals > len)

Error 2 occurs at the line linked below because of attempting to access the string pointed to by stringPtrs[i] using j:

visit/src/common/utility/StringHelpers.C

Line 222 in 3021deb

thisVal += stringPtrs[i][j];

Fuzz test source

#include <fuzzer/FuzzedDataProvider.h>

extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
{
    FuzzedDataProvider provider(data, size);

    int numStrings = provider.ConsumeIntegralInRange<int>(1, 100);
    std::vector<std::string> stringList;
    for (int i = 0; i < numStrings; ++i)
    {
        stringList.push_back(provider.ConsumeRandomLengthString(100));
    }

    std::vector<std::vector<std::string>> stringGroups;
    std::vector<std::string> groupNames;
    int numLeadingVals = provider.ConsumeIntegral<int>();
    std::string nonRelevantChars = provider.ConsumeRandomLengthString(10);

    GroupStrings(stringList, stringGroups, groupNames, numLeadingVals, nonRelevantChars);

    return 0;
}

Potential fix

For error 1: As the error suggests, updating the function StringHelpers::GroupStrings to use unsigned int numLeadingVals instead of int numLeadingVals fixes the bug.

For error 2: Wrapping thisVal += stringPtrs[i][j]; in if (j < sizeof(stringPtrs[i])) fixes the bug.

Adding these two fixes makes the function withstand random inputs supplied by the fuzz test while retaining the original functionality. Please let me know if you have any questions or if you would like to see the crash reports generated by the fuzz test :)

[Arpan3323]

@JustinPrivitera sorry for nudging you again but whenever you could get a chance, can you please let me know whether the 2 listed bugs are indeed considered as bugs? Please take your time.

[JustinPrivitera]

Thanks for this report. These look like bugs. However, I don't think StringHelpers::GroupStrings() is used anywhere in the VisIt code base. It might make more sense to just remove the function entirely. Additionally, while it is best to be robust to strange values passed as numLeadingVals, I think it is quite unlikely a developer would pass -2147483648 as the number of leading characters. I appreciate you looking into these kinds of issues; it is very helpful. But we are a small team and can't tackle everything we'd like to tackle; we have to pick our battles. We will discuss and categorize this ticket at a later date as we continue to progress through our unreviewed tickets.

[Arpan3323]

Thank you for your prompt and thorough investigation. I appreciate the VisIt development team's efforts in maintaining and continuously improving the tool.

I also couldn't confirm whether this function is used anywhere in the code base but since it existed, I thought I should fuzz it anyways. I agree, it might be a good idea to just remove the function entirely.