Мы используем куки, чтобы пользоваться сайтом было удобно.
Хорошо
to the top
close form

Заполните форму в два простых шага ниже:

Ваши контактные данные:

Шаг 1
Поздравляем! У вас есть промокод!

Тип желаемой лицензии:

Шаг 2
Team license
Enterprise license
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности
close form
Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
RUB
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Бесплатная лицензия PVS‑Studio для специалистов Microsoft MVP
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Для получения лицензии для вашего открытого
проекта заполните, пожалуйста, эту форму
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
Мне интересно попробовать плагин на:
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

close form
check circle
Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

Вебинар: Что скрывают Blender и YTsaurus? Вся правда о технологии статического анализа - 30.05

>
>
>
Изучаем подозрительные места в коде AWS…

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

04 Июл 2023

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

1057_AWS_SDK_NET_ru/image1.png

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

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

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.

String literal contains potential interpolated expression. Consider inspecting: part. Fn.cs 82

String literal contains potential interpolated expression. Consider inspecting: part. Fn.cs 93

Похоже, разработчик забыл интерполировать сообщения исключений. Из-за этого вместо фактического значения переменной 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.

Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'coreSTSClient', 'samlCoreSTSClient'. FederatedAWSCredentials.cs 219

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

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.

Uninitialized variable 'LATEST' is used when initializing the 'EC2_METADATA_ROOT' variable. EC2InstanceMetadata.cs 57

Uninitialized variable 'LATEST' is used when initializing the 'EC2_USERDATA_ROOT' variable. EC2InstanceMetadata.cs 60

Uninitialized variable 'LATEST' is used when initializing the 'EC2_DYNAMICDATA_ROOT' variable. EC2InstanceMetadata.cs 63

Uninitialized variable 'LATEST' is used when initializing the 'EC2_APITOKEN_URL' variable. EC2InstanceMetadata.cs 66

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

Сначала объявляются поля 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);

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

1057_AWS_SDK_NET_ru/image2.png

Как видите, ни в одной строке нет литерала "/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.

The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 41, 43. GetObjectTorrentRequestMarshaller.cs 41

Первые два оператора 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.

The 'this.linker.s3.region' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 116, 114. S3Link.cs 116

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

Сама ошибка затаилась в методе доступа set. В свойство this.linker.s3.region всегда будет записано значение value: проверка String.IsNullOrEmpty(value) ни на что не влияет. При этом в методе доступа get также есть проверка: если linker.s3.regionnull или пустая строка, свойство вернёт значение "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.

The 'endpoint' object was used before it was verified against null. Check lines: 111, 118. AmazonS3Client.Extensions.cs 111

Странный порядок работы с потенциальными 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.

It is odd that the body of 'IsSetServerSideEncryptionMethod' function is fully equivalent to the body of 'IsSetServerSideEncryptionCustomerMethod' function. GetObjectMetadataResponse.cs 311

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

В типе 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.

IDisposable object 'rsa' is not disposed before method returns. GetPasswordDataResponse.Extensions.cs 48

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

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

Issue #9

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

Ссылка на GitHub.

Possible infinite recursion inside 'OnFailure' property. ResizeJobFlowStep.cs 171

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

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

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

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

1057_AWS_SDK_NET_ru/image3.png

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.

The 'conditionValues' object was used before it was verified against null. Check lines: 233, 238. JsonPolicyWriter.cs 233

Код выглядит подозрительно: сначала идёт разыменование ссылки из переменной 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.

The 'platform' variable is assigned but is not used by the end of the function. InternalSDKUtils.netstandard.cs 70

Последняя строка метода выглядит очень странно. С помощью 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.

GetFreeTrialImage
Популярные статьи по теме


Комментарии (0)

Следующие комментарии next comments
close comment form