За 2022 год разработчики PVS-Studio написали много статей, в которых рассказали об ошибках, найденных в различных Open Source проектах. Пришло время подвести итоги и представить десяток самых интересных срабатываний.
Как составлялся топ?
Я взял все наши статьи за год и отсмотрел разобранные в них баги. Чтобы топ вышел разнообразнее, я следовал следующим принципам:
не более одной ошибки из проекта;
ошибки не должны быть похожи друг на друга.
Перейдём непосредственно к разбору.
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
@foto_shooter, кстати, открыл 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 нет кнопки, которая позволяла бы показывать именно те фрагменты кода, которые связаны с ошибками. Хотя постойте, вот же она:
Удивительным образом нажатие этой чудо-кнопки позволило обнаружить проблему:
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,
}
Как и в прошлый раз, я буду издеваться предложу вам сначала поискать ошибку самостоятельно :). Ну, или можете нажать волшебную кнопку:
PVS-Studio -> Check Current File
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, чтобы попробовать его в деле. А у меня на этом всё :). Счастливого вам Нового Года!
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Nikita Lipilin. Top 10 bugs found in C# projects in 2022.
