to the top
close form
Для получения триального ключа
заполните форму ниже
Team license
Enterprise license
** Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

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

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

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

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

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

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


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

>
>
>
Топ-10 ошибок, найденных в C#-проектах …

Топ-10 ошибок, найденных в C#-проектах за 2022 год

28 Дек 2022

За 2022 год разработчики PVS-Studio написали много статей, в которых рассказали об ошибках, найденных в различных Open Source проектах. Пришло время подвести итоги и представить десяток самых интересных срабатываний.

1020_CSharpTop_2022_ru/image1.png

Как составлялся топ?

Я взял все наши статьи за год и отсмотрел разобранные в них баги. Чтобы топ вышел разнообразнее, я следовал следующим принципам:

  • не более одной ошибки из проекта;
  • ошибки не должны быть похожи друг на друга.

Перейдём непосредственно к разбору.

10 место. Попытка отписаться в Stride

Открывает сегодняшний топ классическое срабатывание анализатора из статьи о проверке Stride Game Engine:

private void RemoveEditor(....)
{
  ....
  if (....)
  {
    multiEditor.OpenedAssets.CollectionChanged -= (_, e) => 
      MultiEditorOpenAssetsChanged(multiEditor, e);
    ....
  }
  ....
}

V3084. Anonymous function is used to unsubscribe from 'CollectionChanged' event. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration. AssetEditorsManager.cs 444

Из сообщения анализатора понятно, что никакой отписки от события данный код не производит. Приносит ли это проблемы? Вполне возможно!

9 место. Неоднозначное присваивание в Bitwarden

Следующую позицию занимает ошибка из весьма интересной статьи с ярким названием — "Насколько хорошо защищены ваши пароли? Проверка проекта Bitwarden". Давайте же взглянем на неё:

public class BillingInvoice
{
  public BillingInvoice(Invoice inv)
  {
    Amount = inv.AmountDue / 100M;      // <=
    Date = inv.Created;
    Url = inv.HostedInvoiceUrl;
    PdfUrl = inv.InvoicePdf;
    Number = inv.Number;
    Paid = inv.Paid;
    Amount = inv.Total / 100M;          // <=
  }
  public decimal Amount { get; set; }
  public DateTime? Date { get; set; }
  public string Url { get; set; }
  public string PdfUrl { get; set; }
  public string Number { get; set; }
  public bool Paid { get; set; }
}

V3008. The 'Amount' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 148, 142. BillingInfo.cs 148

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

8 место. Элемент из пустоты в Barotrauma

На 8 место списка я поставил ошибку из статьи о проверке отличной игры Barotrauma:

public ParticlePrefab(XElement element, ContentFile file)
{
  ....
  if (CollisionRadius <= 0.0f) 
    CollisionRadius = Sprites.Count > 0 ? 1 : 
                                       Sprites[0].SourceRect.Width / 2.0f;
}

V3106 Possibly index is out of bound. The '0' index is pointing beyond 'Sprites' bound. ParticlePrefab.cs 303

Обращение к первому элементу пустой коллекции — это явно что-то странное :). Остаётся только надеяться, что выбрасываемое исключение не сильно влияет на игровой процесс.

7 место. Инициализация полей в AvalonStudio

Седьмую позицию занимает практически незаметная ошибка из статьи о проверке AvalonStudio:

public class ColorScheme
{
  private static List<ColorScheme> s_colorSchemes =
    new List<ColorScheme>();
  private static Dictionary<string, ColorScheme> s_colorSchemeIDs =
    new Dictionary<string, ColorScheme>();
  private static readonly ColorScheme DefaultColorScheme = 
    ColorScheme.SolarizedLight;
  ....
  public static readonly ColorScheme SolarizedLight = new ColorScheme
  {
    ....
  };
}

О, кажется в этот раз я не покажу предупреждений анализатора сразу :). Просто ради того, чтобы вы почувствовали, каково искать подобные проблемы без статического анализа. Хотя ошибка в этот раз простая, поэтому я уверен, что вы справитесь.

На код выше анализатор выдаёт следующее предупреждение:

V3070. Uninitialized variable 'SolarizedLight' is used when initializing the 'DefaultColorScheme' variable. ColorScheme.cs 32

И правда, с порядком инициализации полей тут что-то явно не так.

6 место. Путаница в .NET 7

Да-да, это ошибка из статьи про тот самый .NET 7! Никто не идеален :).

Сперва взглянем на конструктор XmlConfigurationElementTextContent:

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

А теперь взгляните, где он используется:

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

Обратите внимание на порядок аргументов и параметров:

  • аргументы: ..., lineNumber, linePosition;
  • параметры: ..., linePosition, lineNumber.

Именно эту проблему и обнаружил анализатор:

  • V3066 Possible incorrect order of arguments passed to 'XmlConfigurationElementTextContent' constructor: 'lineNumber' and 'linePosition'. XmlStreamConfigurationProvider.cs 133
  • V3066 Possible incorrect order of arguments passed to 'XmlConfigurationElementTextContent' constructor: 'lineNumber' and 'linePosition'. XmlStreamConfigurationProvider.cs 148

Мой коллега, кстати, открыл issue на GitHub, и этот код поправили: аргументы поменяли местами и даже добавили тест.

5 место. Преждевременная очистка в Orleans

Вторую половину топа открывает ошибка из статьи о проверке Orleans:

private class BatchOperation
{
  private readonly List<TableTransactionAction> batchOperation;
  ....

  public async Task Flush()
  {
    if (batchOperation.Count > 0)
    {
      try
      {
        ....
        batchOperation.Clear();                              // <=
        keyIndex = -1;

        if (logger.IsEnabled(LogLevel.Trace))
        {
          for (int i = 0; i < batchOperation.Count; i++)     // <=
          {
            logger.LogTrace(....)
          }
        }
      }
      catch (Exception ex)
      {
        ....
      }
    }
  }
}

V3116. Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. AzureTableTransactionalStateStorage.cs 345

Что-то мне подсказывает, что очищать batchOperation стоило чуть позже. И, судя по исправлениям, разработчики Orleans со мной абсолютно согласны.

4 место. Теневые особенности Eto.Forms

С небольшим отставанием от трёх наиболее интересных срабатываний расположилась ошибка из статьи о проверке GUI-фреймворка Eto.Forms:

public NSShadow TextHighlightShadow
{
  get
  {
    if (textHighlightShadow == null)
    {
      textHighlightShadow = new NSShadow();
      textHighlightShadow.ShadowColor = NSColor.FromDeviceWhite(0F, 0.5F);
      textHighlightShadow.ShadowOffset = new CGSize(0F, -1.0F);
      textHighlightShadow.ShadowBlurRadius = 2F;
    }
    return textHighlightShadow;
  }
  set { textShadow = value; }         // <=
}

public NSShadow TextShadow
{
  get
  {
    if (textShadow == null)
    {
      textShadow = new NSShadow();
      textShadow.ShadowColor = NSColor.FromDeviceWhite(1F, 0.5F);
      textShadow.ShadowOffset = new CGSize(0F, -1.0F);
      textShadow.ShadowBlurRadius = 0F;
    }
    return textShadow;
  }
  set { textShadow = value; }
}

Предупреждение PVS-Studio: V3140 Property accessors use different backing fields. Eto.Mac64 MacImageAndTextCell.cs 162

Copy-paste передаёт привет :).

Возможно, присваивание значения свойству TextHighlightShadow и правда должно было как-то влиять на textShadow. Но даже в таком случае непонятно, почему оно никак не влияет на поле textHighlightShadow, использующееся в get-аксессоре.

3 место. Потерянная миграция в Squidex

Тройку финалистов открывает необычное срабатывание из статьи о проблемах в проектах на ASP.NET Core. Эта ошибка была найдена в проекте Squidex:

private IEnumerable<IMigration?> ResolveMigrators(int version)
{
  yield return serviceProvider.GetRequiredService<StopEventConsumers>();

  // Version 06: Convert Event store. Must always be executed first.
  if (version < 6)
  {
    yield return serviceProvider.GetRequiredService<ConvertEventStore>();
  }

  // Version 22: Integrate Domain Id.
  if (version < 22)
  {
    yield return serviceProvider
                   .GetRequiredService<AddAppIdToEventStream>();
  }

  // Version 07: Introduces AppId for backups.
  else if (version < 7) 
  {
    yield return serviceProvider
                   .GetRequiredService<ConvertEventStoreAppId>();
  }

  // Version 05: Fixes the broken command architecture and requires a
  // rebuild of all snapshots.
  if (version < 5)
  {
    yield return serviceProvider.GetRequiredService<RebuildSnapshots>();
  }
  else
  {
    // Version 09: Grain indexes.
    if (version < 9)
    {
      yield return serviceProvider.GetService<ConvertOldSnapshotStores>();
    }

    ....
  }

  // Version 13: Json refactoring
  if (version < 13)
  {
    yield return serviceProvider
                   .GetRequiredService<ConvertRuleEventsJson>();
  }

  yield return serviceProvider.GetRequiredService<StartEventConsumers>();
}

Желаю удачи в её поисках :).

Если же искать лень, то сразу взгляните на сокращённый фрагмент:

// Version 22: Integrate Domain Id.
if (version < 22)
{
  yield return serviceProvider.GetRequiredService<AddAppIdToEventStream>();
}

// Version 07: Introduces AppId for backups.
else if (version < 7) 
{
  yield return serviceProvider
                 .GetRequiredService<ConvertEventStoreAppId>();
}

Уверен, так гораздо легче. Даже жаль, что в IDE нет кнопки, которая позволяла бы показывать именно те фрагменты кода, которые связаны с ошибками. Хотя постойте, вот же она:

1020_CSharpTop_2022_ru/image2.png

Удивительным образом нажатие этой чудо-кнопки позволило обнаружить проблему:

V3022. Expression 'version < 7' is always false. MigrationPath.cs 55

И правда, условие version < 7 проверяется только в случае, если version >= 22. Странное дело!

Чтобы кнопка появилась, вам нужно установить расширение, загрузить которое можно здесь.

Если вдруг ваша IDE не поддерживается, то вы можете написать тут о своей заинтересованности в плагине для неё.

2 место. Весёлые сдвиги в Discord.NET

Серебро в этот раз забирает ошибка из статьи о проверке Discord.NET:

public enum GuildFeature : long
{
  None = 0,
  AnimatedBanner = 1 << 0,
  AnimatedIcon = 1 << 1,
  Banner = 1 << 2,
  ....
  TextInVoiceEnabled = 1 << 32,
  ThreadsEnabled = 1 << 33,
  ThreadsEnabledTesting = 1 << 34,
  ....
  VIPRegions = 1 << 40,
  WelcomeScreenEnabled = 1 << 41,
}

Как и в прошлый раз, я буду издеваться предложу вам сначала поискать ошибку самостоятельно :). Ну, или можете нажать волшебную кнопку:

V3134. Shift by 32 bits is greater than the size of 'Int32' type of expression '1'. GuildFeature.cs 147

Проблема в том, что типом операндов сдвига здесь выступает int, и такой же тип будет у результата. В этом случае значение вида 1 << 32 равно 1, значение 1 << 33 — то же самое, что 1 << 1, и так далее.

Получилась странная ситуация: элементы AnimatedBanner и TextInVoiceEnabled оказываются одним и тем же, равно как и пара AnimatedIcon-ThreadsEnabled и т. д. Если добавить к этому тот факт, что внутренним типом перечисления является long, то становится ясно — разработчик закладывал совсем не такое поведение.

1 место. Уязвимость из BlogEngine.NET

Вот мы и добрались до срабатывания, которое я посчитал наиболее интересным из всех, описанных нами за 2022 год. Встречайте — XXE-уязвимость из проекта BlogEngine.NET:

public XMLRPCRequest(HttpContext input)
{
  var inputXml = ParseRequest(input);

  // LogMetaWeblogCall(inputXml);
  this.LoadXmlRequest(inputXml); // Loads Method Call 
                                 // and Associated Variables
}

private static string ParseRequest(HttpContext context)
{
  var buffer = new byte[context.Request.InputStream.Length];

  context.Request.InputStream.Position = 0;
  context.Request.InputStream.Read(buffer, 0, buffer.Length);

  return Encoding.UTF8.GetString(buffer);
}

private void LoadXmlRequest(string xml)
{
  var request = new XmlDocument();
  try
  {
    if (!(xml.StartsWith("<?xml") || xml.StartsWith("<method")))
    {
      xml = xml.Substring(xml.IndexOf("<?xml"));
    }

    request.LoadXml(xml);              // <=
  }
  catch (Exception ex)
  {
    throw new MetaWeblogException("01", 
                     $"Invalid XMLRPC Request. ({ex.Message})");
  }
  ....
}

Что? Где? Какая ещё уязвимость?

Даже несмотря на то, что я показал только методы, непосредственно связанные с проблемой, увидеть тут какую-то уязвимость к XXE может быть непросто.

В статье представлено подробное описание подобных уязвимостей в целом и конкретной проблемы на BlogEngine.NET в частности. Здесь же я коротко пройдусь по основным моментам.

Для начала взглянем на метод ParseRequest:

private static string ParseRequest(HttpContext context)
{
  var buffer = new byte[context.Request.InputStream.Length];

  context.Request.InputStream.Position = 0;
  context.Request.InputStream.Read(buffer, 0, buffer.Length);

  return Encoding.UTF8.GetString(buffer);
}

context.Request.InputStream содержит некоторые данные, отправленные неким пользователем в приложение. Если ранее содержимое запроса не проверялось, то в InputStream может быть что угодно. Данные такого рода называют потенциально заражёнными (подробнее тут).

Метод ParseRequest считывает данные запроса в буфер, преобразовывает в строку, а затем возвращает их "как есть". Куда же они идут?

Ответ здесь:

public XMLRPCRequest(HttpContext input)
{
  var inputXml = ParseRequest(input);

  // LogMetaWeblogCall(inputXml);
  this.LoadXmlRequest(inputXml); // Loads Method Call 
                                 // and Associated Variables
}

Мы видим, что результат работы ParseRequest передаётся в метод LoadXmlRequest. Ещё раз напомню — в этих данных может быть что угодно, так как их содержимое полностью контролируется отправителем. Давайте же взглянем, как они обрабатываются в методе LoadXmlRequest:

private void LoadXmlRequest(string xml)
{
  var request = new XmlDocument();
  try
  {
    if (!(xml.StartsWith("<?xml") || xml.StartsWith("<method")))
    {
      xml = xml.Substring(xml.IndexOf("<?xml"));
    }

    request.LoadXml(xml);              // <=
  }
  catch (Exception ex)
  {
    throw new MetaWeblogException("01", 
                     $"Invalid XMLRPC Request. ({ex.Message})");
  }
  ....
}

Вот тут-то и стреляет уязвимость! Потенциально заражённые данные из запроса, переданные в параметре xml, передаются в метод LoadXml класса XmlDocument.

Ну и что, казалось бы? У нас ведь есть try-catch, и даже кое-какая проверка. Вот только от реальной угрозы это всё равно не спасает.

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

В статье показано, как в последствии это содержимое может вернуться отправителю запроса. Там продемонстрирована не просто "теоретическая опасность", а вполне рабочий сценарий эксплуатации уязвимости.

Как же так выходит, что обработка пользовательского xml приводит к считыванию файлов из системы? Краткое пояснение этому даёт анализатор:

V5614. Potential XXE vulnerability inside method. Insecure XML parser is used to process potentially tainted data from the first argument: 'inputXml'. BlogEngine.Core XMLRPCRequest.cs 41

Таким образом, PVS-Studio обнаруживает и указывает на 2 составляющие уязвимости XML External Entity:

  • небезопасный (или точнее, небезопасно сконфигурированный) парсер XML;
  • передаваемые ему заражённые данные.

Если заражённые данные мы уже упоминали, то вот понять, в чём состоит небезопасность парсера, может быть сложно. В текущем примере небезопасной является использованная конфигурация по умолчанию. Однако в общем случае всё сложнее. Подробнее обо всём этом, а также о способах защиты от подобных проблем, вы можете прочитать в статье "Уязвимости из-за обработки XML-файлов: XXE в C# приложениях в теории и на практике".

Также советую посмотреть посвящённый данной теме доклад с конференции DotNext:

Проблема в BlogEngine.NET стоит особняком от других разобранных в этой статье, потому что она является не просто "странностью в коде", а реальной уязвимостью, о которой есть запись в базе CVE.

Заключение

2022 выдался богатым на интересные статьи, благодаря чему мне удалось собрать разнообразный топ. Это также позволяет оценить многообразие реальных ошибок, которые может находить PVS-Studio. Какие-то из них связаны с невнимательностью, какие-то — с незнанием особенностей языка, а где-то проблему и вовсе не видно до тех пор, пока её не покажет анализатор.

Традиционно оставлю ссылку на страницу, с которой можно загрузить PVS-Studio, чтобы попробовать его в деле. А у меня на этом всё :). Счастливого вам Нового Года!

Популярные статьи по теме
Под капотом SAST: как инструменты анализа кода ищут дефекты безопасности

Дата: 26 Янв 2023

Автор: Сергей Васильев

Сегодня речь о том, как SAST-решения ищут дефекты безопасности. Расскажу, как разные подходы к поиску потенциальных уязвимостей дополняют друг друга, зачем нужен каждый из них и как теория ложится на…
Wave Function Collapse для процедурной генерации в Unity

Дата: 24 Янв 2023

Автор: Андрей Москалёв

Wave Function Collapse – это алгоритм, c помощью которого можно реализовать генерацию чего угодно, что можно было бы описать с помощью правил или конкретным примером. В этой статье мы рассмотрим, как…
PVS-Studio научился анализировать Blazor компоненты

Дата: 10 Янв 2023

Автор: Алексей Авдеев

Всем привет. Перед вами небольшая статья о добавлении анализа Blazor компонентов в PVS-Studio. По ходу рассказа постараемся предугадать ваши немые вопросы по теме и ответить на них. Приятного прочтен…
Создание .NET библиотеки от А до Я

Дата: 05 Янв 2023

Автор: Гость

Думаете о создании .NET библиотеки, но не знаете, в какую сторону двигаться? Уже разрабатываете нечто подобное, но хочется открыть для себя что-то новое? Ищете варианты расширить автоматизацию? Не зн…
PVS-Studio и protobuf-net: лучшие предупреждения в один клик

Дата: 21 Дек 2022

Автор: Артём Ровенский

Разбираем best practice знакомства с PVS-Studio. Покажем быстрый старт работы с анализатором на примере проекта protobuf-net.

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

Следующие комментарии next comments
close comment form
Unicorn with delicious cookie
Мы используем куки, чтобы пользоваться сайтом было удобно.
Хорошо