Изучаем подозрительные места в коде AWS SDK для .NET

• Немного деталей об анализе
• Разбор подозрительных фрагментов кода
• Заключение

Сегодня под нашим скальпелем оказался проект AWS SDK для .NET. Мы посмотрим на подозрительные места из исходного кода, разберёмся, что в них происходит, и попробуем воспроизвести некоторые проблемы. Заваривайте кофе и устраивайтесь поудобнее.

Немного деталей об анализе

Что за проект?

AWS.SDK для .NET — проект, который помогает работать с Amazon Web Services, Amazon S3, Amazon DynamoDB и т. п. Исходники взял со страницы проекта на GitHub. Если нужна точная версия, вот SHA коммита: 93a94821dc8ff7a0073b74def6549728da3b51c7.

Чем проверяли?

Код проверял анализатором PVS-Studio через плагин для Visual Studio.

Что-то ещё?

Некоторые предупреждения могут показаться вам знакомыми — местами код не поменялся со времени прошлой проверки. Если предупреждения казались мне достаточно интересными, я дублировал их в эту статью.

Но хватит о деталях проверки, переходим непосредственно к разбору подозрительных мест в коде.

Разбор подозрительных фрагментов кода

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: #1, #2.

Похоже, разработчик забыл интерполировать сообщения исключений. Из-за этого вместо фактического значения переменной part будет использован строковый литерал {part}.

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

Ссылка на GitHub.

Большой фрагмент кода нужен для лучшего понимания контекста. Сама ошибка спряталась здесь:

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

Похоже, в условии оператора if на null проверяется значение не той переменной — нужно было проверять samlCoreSTSClient.

Обратите внимание на следующие элементы:

• название результирующей переменной — samlCoreSTSClient;
• тип интерфейса, к которому выполняется приведение — ICoreAmazonSTS_SAML;
• текст сообщения исключения — "... doesn't support SAML authentication".

SAML упоминается везде, кроме имени проверяемой переменной, — coreSTSClient. :)

Интересно, как из-за проверки разных переменных меняется логика, если приведение выполнить не удаётся.

При проверке samlCoreSTSClient:

• -> приведение с помощью оператора as
• -> проверка samlCoreSTSClient на равенство null
• -> выброс исключения NotImplementedException

При проверке coreSTSClient:

• -> приведение с помощью оператора as
• -> проверка coreSTSClient на неравенство null
• -> попытка вызвать метод CredentialsFromSAMLAuthentication
• -> выброс исключения NullReferenceException
• -> перехват исключения в catch
• -> логгирование проблемы
• -> выброс исключения AmazonClientException.

То есть как минимум во внешний код прилетит исключение другого типа и с другим сообщением.

А вообще говоря, проверка не той переменной после использования оператора as — достаточно распространённый паттерн ошибки в C#. Посмотрите на другие примеры.

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

Ссылка на GitHub.

Обратите внимание на порядок объявления и инициализации полей.

Сначала объявляются поля EC2_APITOKEN_URL, EC2_DYNAMICDATA_ROOT, EC2_USERDATA_ROOT, EC2_METADATA_ROOT. Каждое из них использует в инициализаторе поле LATEST. Однако само поле на момент использования ещё не инициализировано, так как оно объявляется ниже по коду. Как результат, при вычислении значений для полей EC2_* будет использоваться не строка "/latest", а значение default(string) — null.

В описанном выше легко убедиться, обратившись к соответствующему 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);

Результат выполнения кода:

Как видите, ни в одной строке нет литерала "/latest".

А вот ошибка это или нет — вопрос открытый. Порядок инициализации полей поменяли отдельным коммитом. В этом же коммите поля декорировали атрибутом Obsolete. Хотя если не предполагается использование фактического значения LATEST, лучше просто его не использовать. Так код не будет никого смущать.

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

Ссылка на GitHub.

Первые два оператора if полностью дублируют друг друга как условиями, так и телами. Или один из них лишний и его нужно убрать, или в одном из операторов должно быть другое условие и другие действия.

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

Ссылка на GitHub.

Интересный код. С одной стороны, в нём есть ошибка. С другой — при работе исключительно со свойством Region она себя не проявит.

Сама ошибка затаилась в методе доступа set. В свойство this.linker.s3.region всегда будет записано значение value: проверка String.IsNullOrEmpty(value) ни на что не влияет. При этом в методе доступа get также есть проверка: если linker.s3.region — null или пустая строка, свойство вернёт значение "us-east-1".

Получается вот что. Если пользователь работает только со свойством Region, для него нет разницы, есть ошибка или нет. Но её в любом случае лучше исправить.

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

Ссылка на GitHub.

Странный порядок работы с потенциальными null-значениями притягивает баги. Бывает, что сначала значение используется, потом проверяется на null. И здесь начинаются головоломки: то ли это ошибка и возможно исключение, то ли просто проверка избыточная, а null-значения в переменной быть не может, то ли ещё что...

Здесь аналогичная ситуация. Сначала к переменной endpoint обращаются безусловно (endpoint.GetEndpointForService), а ниже по коду используют оператор условного доступа (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: #1, #2.

Предупреждаю: от похожих названий сейчас начнёт рябить в глазах. Думаю, из-за этого ошибка и возникла.

В типе GetObjectMetadataResponse определены свойства ServerSideEncryptionMethod и ServerSideEncryptionCustomerMethod. Они используют соответствующие backing поля —serverSideEncryption и serverSideEncryptionCustomerMethod:

• ServerSideEncryptionMethod -> serverSideEncryption;
• ServerSideEncryptionCustomerMethod -> serverSideEncryptionCustomerMethod.

А ещё есть методы IsSetServerSideEncryptionMethod и IsSetServerSideEncryptionCustomerMethod. Как можно предположить, они тоже используют backing-поля serverSideEncryption и serverSideEncryptionCustomerMethod соответственно... но нет. Из-за ошибки оба метода проверяют одно и то же поле — serverSideEncryptionCustomerMethod.

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

Метод IsSetServerSideEncryptionMethod должен проверять поле serverSideEncryption.

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

Ссылка на GitHub.

Тип RSACryptoServiceProvider реализует интерфейс IDisposable. Однако в этом коде метод Dispose не вызывается ни явно, ни косвенно (через использование rsa в выражении using).

Я не могу сказать, насколько это критично в данном случае. Но в целом кажется, что Dispose для очистки данных лучше всё-таки вызывать, особенно когда код работает с паролями и т. п.

Issue #9

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

Ссылка на GitHub.

Из-за опечатки в get-accessor'е свойства OnFailure используется не backing-поле onFailure, а само свойство — OnFailure. При попытке получить значение свойства возникает бесконечная рекурсия, которая приводит к исключению StackOverflowException.

Ошибку легко воспроизвести, воспользовавшись соответствующим API:

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

Компилируем код, запускаем на исполнение, получаем ожидаемый результат:

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

Ссылка на GitHub.

Код выглядит подозрительно: сначала идёт разыменование ссылки из переменной conditionValues, а затем её проверка на null. При этом значение переменной не изменяется. Соответственно, если ссылка была нулевой, уже при первом обращении — conditionValues.Count == 0 — возникнет исключение NullReferenceException.

Этот код может содержать как ошибку, так и избыточную проверку на неравенство null.

Отмечу одну вещь. У меня сложилось впечатление, что в проекте любят добавлять проверки на равенство null на всякий случай. :) Ниже перечислю несколько таких примеров.

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

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

Ссылка на GitHub.

Метод String.Split не возвращает null. Похожая проверка есть здесь.

Другой пример похожей проверки:

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

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

Ссылка на GitHub.

Метод Enumerable.ToList не возвращает null, так что значение переменной constructors никогда не будет равно null.

А пример ниже ближе к изначальному — тоже сначала разыменовали ссылку, а затем проверяют её значение на null:

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

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

Ссылка на GitHub.

Хотя случаев, когда свойство Listeners может иметь значение null, я не нашёл. В .NET возвращаемое значение свойства так и вовсе размечено null-forgiving оператором (ссылка на 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");
}

Ссылка на GitHub.

Последняя строка метода выглядит очень странно. С помощью String.Format в шаблон "Xamarin_{0}" подставляют строковый литерал "Xamarin". При этом значение переменной platform, которое может хранить необходимую информацию, игнорируется. Выглядит странно.

Предположу, что выражение return должно выглядеть так:

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

Кстати, рядом есть похожий метод с получением информации о Unity. Он написан по схожему шаблону, но возвращаемое значение уже формируется нормально:

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

Заключение

Обо всех найденных проблемах я уведомил разработчиков ещё до выхода статьи — вот ссылка на баг-репорт.

Хотите проверить, нет ли в вашем проекте похожих проблем? Проанализируйте код с помощью PVS-Studio.