Examining suspicious code fragments in AWS SDK for .NET

• Some analysis details
• Examining suspicious code fragments
• Conclusion

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