DEV Community

Sergey Vasiliev
Sergey Vasiliev

Posted on • Updated on

Converting string to enum at the cost of 50 GB: CVE-2020-36620

In this article, we're going to discuss the CVE-2020-36620 vulnerability and see how a NuGet package for converting string to enum can make a C# application vulnerable to DoS attacks.

1038_CVE_EnumStringValues/image1.png

Imagine a server application that interacts with a user. In one of the scenarios, the application receives data from the user in a string representation and converts it into enumeration elements (string -> enum).

To convert a string into an enumeration element, we can use standard .NET tools:

String colorStr = GetColorFromUser();
if (Enum.TryParse(colorStr, out ConsoleColor parsedColor))
{
  // Process value...
}
Enter fullscreen mode Exit fullscreen mode

Or we can find some NuGet package and try to do the same with it. For example, EnumStringValues.

Since we have a sense of adventure (right?), let's take the EnumStringValues 4.0.0 package to convert strings. The exclamation point in front of the package version makes it even more intriguing...

Here's how the code using package's API might look like:

static void ChangeConsoleColor()
{
  String colorStr = GetColorFromUser();

  if (colorStr.TryParseStringValueToEnum<ConsoleColor>(
        out var parsedColor))
  {
    // Change console color...
  }
  else
  {
    // Error processing
  }
}
Enter fullscreen mode Exit fullscreen mode

Let's see what's going on here:

  • the data from the user is written to the colorStr variable;
  • with the help of the library's API, the EnumStringValues string is converted to an instance of the ConsoleColor enumeration;
  • if the conversion is successful (then-branch), the color of the console changes;
  • if otherwise (else-branch), an error warning is issued.

Strings are converted, the application is running. Everything seems good... but there's one thing. It turns out that the application can behave as follows:

1038_CVE_EnumStringValues/image2.png

Oh, wait, we have a package marked with an "exclamation point", right... Let's try to figure out why there's such a high memory consumption. The following code will help us:

while (true)
{
  String valueToParse = ....;
  _ = valueToParse.TryParseStringValueToEnum<ConsoleColor>(
        out var parsedValue);
}
Enter fullscreen mode Exit fullscreen mode

Using the library, the code infinitely parses strings — nothing unusual. If valueToParse takes values of string representations of ConsoleColor elements ("Black", "Red", etc.), the application will behave as expected:

1038_CVE_EnumStringValues/image3.png

The issues appear when we write unique strings to valueToParse. For example, as follows:

String valueToParse = Guid.NewGuid().ToString();
Enter fullscreen mode Exit fullscreen mode

In this case, the application starts to consume memory uncontrollably.

1038_CVE_EnumStringValues/image4.png

Let's try to figure out what's going on. Take a look at the TryParseStringValueToEnum<T> method:

public static bool 
TryParseStringValueToEnum<TEnumType>(
  this string stringValue, 
  out TEnumType parsedValue) where TEnumType : System.Enum
{
  if (stringValue == null)
  {
    throw new ArgumentNullException(nameof(stringValue), 
                                    "Input string may not be null.");
  }

  var lowerStringValue = stringValue.ToLower();
  if (!Behaviour.UseCaching)
  {
    return TryParseStringValueToEnum_Uncached(lowerStringValue, 
                                              out parsedValue);
  }

  return TryParseStringValueToEnum_ViaCache(lowerStringValue, 
                                            out parsedValue);
}
Enter fullscreen mode Exit fullscreen mode

Well, well, well, interesting. It turns out that there's a caching option under the hood — Behavior.UseCaching. Since we didn't explicitly use the caching option, let's look at the default value:

/// <summary>
/// Controls whether Caching should be used. Defaults to false.
/// </summary>
public static bool UseCaching
{
  get => useCaching;
  set { useCaching = value; if (value) { ResetCaches(); } }
}

private static bool useCaching = true;
Enter fullscreen mode Exit fullscreen mode

If the comment to the property is true, caches are disabled by default. Actually, they are enabled (useCachingtrue).

You can already guess what's the issue. However, to be sure, let's dive deeper into the code.

With the knowledge obtained, we return to the TryParseStringValueToEnum method. Depending on the caching option, one of two methods will be called — TryParseStringValueToEnum_Uncached or TryParseStringValueToEnum_ViaCache:

if (!Behaviour.UseCaching)
{
  return TryParseStringValueToEnum_Uncached(lowerStringValue, 
                                            out parsedValue);
}

return TryParseStringValueToEnum_ViaCache(lowerStringValue, 
                                          out parsedValue);
Enter fullscreen mode Exit fullscreen mode

In this case, the UseCaching property has the true value, the TryParseStringValueToEnum_ViaCache method gets control. You can view its code below, but you don't have to delve deeper into it — further on we're going to analyze the method step by step.

/// <remarks>
/// This is a little more complex than one might hope, 
/// because we also need to cache the knowledge 
/// of whether the parse succeeded or not.
/// We're doing that by storing `null`, 
/// if the answer is 'No'. And decoding that, specifically.
/// </remarks>
private static bool 
TryParseStringValueToEnum_ViaCache<TEnumType>(
  string lowerStringValue, out TEnumType parsedValue) where TEnumType 
                                                        : System.Enum
{
  var enumTypeObject = typeof(TEnumType);

  var typeAppropriateDictionary 
    = parsedEnumStringsDictionaryByType.GetOrAdd(
        enumTypeObject, 
        (x) => new ConcurrentDictionary<string, Enum>());

  var cachedValue 
    = typeAppropriateDictionary.GetOrAdd(
        lowerStringValue, 
        (str) =>
        {
          var parseSucceededForDictionary =       
                TryParseStringValueToEnum_Uncached<TEnumType>(
                  lowerStringValue, 
                  out var parsedValueForDictionary);

          return   parseSucceededForDictionary 
                 ? (Enum) parsedValueForDictionary 
                 : null;
        });

  if (cachedValue != null)
  {
    parsedValue = (TEnumType)cachedValue;
    return true;
  }
  else
  {
    parsedValue = default(TEnumType);
    return false;
  }
}
Enter fullscreen mode Exit fullscreen mode

Let's analyze what happens in the method.

var enumTypeObject = typeof(TEnumType);

var typeAppropriateDictionary 
  = parsedEnumStringsDictionaryByType.GetOrAdd(
      enumTypeObject, 
      (x) => new ConcurrentDictionary<string, Enum>());
Enter fullscreen mode Exit fullscreen mode

In the parsedEnumStringsDictionaryByType dictionary, the key is the type of enumeration that is used, and the value is the cache of strings and the results of their parsing.

So, we get the following cache scheme:

Cache <Enumeration type -> Cache <Source string -> Parsing result>>

parsedEnumStringsDictionaryByType is a static field:

private static 
ConcurrentDictionary<Type, ConcurrentDictionary<string, Enum>> 
parsedEnumStringsDictionaryByType;
Enter fullscreen mode Exit fullscreen mode

Thus, typeAppropriateDictionary stores a reference to the cache of values for the enumeration type that we are working with (enumTypeObject).

Then the code parses the input string and saves the result in typeAppropriateDictionary:

var cachedValue 
  = typeAppropriateDictionary.GetOrAdd(lowerStringValue, (str) =>
    {
      var parseSucceededForDictionary 
        = TryParseStringValueToEnum_Uncached<TEnumType>(
            lowerStringValue, 
            out var parsedValueForDictionary);

      return   parseSucceededForDictionary 
             ? (Enum) parsedValueForDictionary 
             : null;
    });
Enter fullscreen mode Exit fullscreen mode

In the end, the method returns the success flag and writes the resulting value to the out parameter:

if (cachedValue != null)
{
  parsedValue = (TEnumType)cachedValue;
  return true;
}
else
{
  parsedValue = default(TEnumType);
  return false;
}
Enter fullscreen mode Exit fullscreen mode

The key problem is described in the method's comment:

This is a little more complex than one might hope, because we also need to cache the knowledge of whether the parse succeeded or not. We're doing that by storing null, if the answer is 'No'. And decoding that, specifically.

Even if the input string could not be parsed, it will still be saved to the typeAppropriateDictionary cache: the null value will be written as the result of parsing. Since typeAppropriateDictionary is a reference from the parsedEnumStringsDictionaryByType dictionary stored statically, objects exist between method calls (this makes sense because they are caches).

So, here's what happens. If attackers can send unique strings (that are parsed with the help of the library's API) to the application, they have an opportunity to "spam" the cache with all consequences that it implies.

1038_CVE_EnumStringValues/image5.png

The unique string parsing makes the typeAppropriateDictionary dictionary bigger. The debugger confirms that the cache is "spammed":

1038_CVE_EnumStringValues/image6.png

So, we've just discussed the CVE-2020-36620 vulnerability. Here's some additional information:

The fix is simple — the parsing of input values was removed (the commit).

Previously, typeAppropriateDictionary was filled in as data was obtained:

  • if the input string is "Yellow", the { "yellow", ConsoleColor.Yellow } pair is written to the cache;
  • if the input string is "Unknown", the { "unknown", null } pair is written to the cache
  • and so on.

Now, typeAppropriateDictionary is filled in advance. The dictionary initially stores the relationships of string representations of enumeration elements to the actual values:

1038_CVE_EnumStringValues/image7.png

Input values are not written to the dictionary — there's only an attempt to extract them:

if (typeAppropriateDictionary.TryGetValue(lowerStringValue, 
                                          out var cachedValue))
  ....
Enter fullscreen mode Exit fullscreen mode

This fix made the cache no longer vulnerable to clogging with unique strings.

The library 4.0.1 already includes the fix, but the corresponding NuGet package is marked as vulnerable. Apparently, the information is taken from GitHub Advisory. It states that 4.0.2 is the secure version. However, the same entry contains links to data from NVD and vuldb. It indicates that the package has been secured since the 4.0.1 version, not 4.0.2. So, there's some confusion.

Here's another interesting thing: the vulnerability was closed at the end of May 2019, and information about it appeared in the databases 3.5 years later — at the end of December 2022.

As long as information about the vulnerability is not recorded in public databases, some tools will not be able to issue warnings about a security defect in the package.

On the one hand, such a delay is understandable — the project has 3 forks and 16 stars, it can be classified as "personal". On the other hand, the project has 200K package downloads in total — that's a significant number.

**

At that point we finish the review of the CVE-2020-36620 vulnerability. If you enjoyed it, I invite you to look through a couple of more similar notes:

P.S. I also post links to my publications on Twitter and LinkedIn — perhaps it will be convenient for someone to follow me there.

Top comments (6)

Collapse
 
hisuwh profile image
Henry Ing-Simmons

Is there a significant benefit to adding caching to enum parsing in the first place? Seems like premature optimisation and ironic that this resulted in a DOS vector

Collapse
 
_sergvasiliev_ profile image
Sergey Vasiliev

Is there a significant benefit to adding caching to enum parsing in the first place?

I don't think so.

Collapse
 
vinchenzo89 profile image
Vinchenzo89 • Edited

Is it not a concern that a task as simple as mapping a string to an enum results in your application secretly allocating and storing multiple concurrent data structures at runtime?

Collapse
 
_sergvasiliev_ profile image
Sergey Vasiliev

Unfortunately, it's what you can unexpectedly get when you use external libraries. You often don't know what's under the hood.

Collapse
 
theredpea profile image
Nate

"parsing of input values was removed" do you mean caching of input values?

"The debugger confirms the cash "spam":" do you mean cache spam?

Collapse
 
_sergvasiliev_ profile image
Sergey Vasiliev

"The debugger confirms the cash "spam":" do you mean cache spam?

Yes, there was a typo here. I've fixed it. Thanks!

"parsing of input values was removed" do you mean caching of input values?

Well, yes and no at the same time. First of all, exactly the parsing of input values was removed (the TryParseStringValueToEnum_Uncached method call). And as you said, caching of the input values was removed as well.