Bitwarden – менеджер паролей с открытым исходным кодом. Это программное обеспечение помогает генерировать уникальные пароли и управлять ими. Получится ли у анализатора PVS-Studio отыскать ошибки в таком проекте?
Менеджер паролей – решение для хранения и генерации паролей. Любой человек, использующий подобный софт, хочет быть уверен, что его данные находятся в безопасности. Качество кода такого инструмента имеет большое значение.
Именно поэтому я решил взять исходный код Bitwarden от 15.03.2022 из репозитория и проверить его с помощью статического анализатора PVS-Studio. Анализатор выдал на код проекта 247 предупреждений. Среди них мне удалось найти кое-что интересное.
Issue 1
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; }
}
Предупреждение PVS-Studio: V3008 The 'Amount' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 148, 142. BillingInfo.cs 148
Обратите внимание на инициализацию Amount. Данному свойству присваивается выражение inv.AmountDue / 100M. Необычно выглядит то, что буквально через пять строчек кода производится аналогичная операция, но уже с присваиванием inv.Total / 100M.
Сложно сказать, какое именно значение хотел использовать разработчик. Если последнее присваивание верно, то первое является избыточным. Это не добавляет красоты коду, но всё же на логику выполнения программы не влияет. В противном же случае данный участок кода будет работать некорректно.
Issue 2
private async Task<AppleReceiptStatus> GetReceiptStatusAsync(
....,
AppleReceiptStatus lastReceiptStatus = null)
{
try
{
if (attempt > 4)
{
throw new Exception("Failed verifying Apple IAP " +
"after too many attempts. " +
"Last attempt status: " +
lastReceiptStatus?.Status ?? "null"); // <=
}
....
}
....
}
Предупреждение PVS-Studio: V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. AppleIapService.cs 96
Похоже, разработчик ожидал, что в сообщение будет добавлено либо значение свойства Status, либо строка "null". После чего полученный результат будет прибавлен к строке "Failed verifying Apple IAP after too many attempts. Last attempt status: ". К сожалению, поведение программы будет иным.
Для того чтобы разобраться в сути данного срабатывания, стоит вспомнить приоритеты операторов. Оператор '??' имеет более низкий приоритет по сравнению с оператором '+'. Следовательно, сначала произойдет сложение строки со значением свойства Status, а уже после сработает оператор null-coalescing.
В случае если lastReceiptStatus не null и Status не null, данный метод работает корректно.
Если же lastReceiptStatus или Status всё-таки null, выведется следующее сообщение: "Failed verifying Apple IAP after too many attempts. Last attempt status: ". Оно, очевидно, является некорректным. Ожидаемое сообщение выглядит следующим образом: "Failed verifying Apple IAP after too many attempts. Last attempt status: null".
Чтобы исправить ошибку, нужно взять часть выражения в скобки:
throw new Exception("Failed verifying Apple IAP " +
"after too many attempts. " +
"Last attempt status: " +
(lastReceiptStatus?.Status ?? "null"));
Issue 3, 4
public bool Validate(GlobalSettings globalSettings)
{
if(!(License == null && !globalSettings.SelfHosted) ||
(License != null && globalSettings.SelfHosted)) // <=
{
return false;
}
return globalSettings.SelfHosted || !string.IsNullOrWhiteSpace(Country);
}
Здесь PVS-Studio выдаёт сразу два предупреждения:
Часть логического выражения всегда будет ложной. Чтобы в этом убедиться, следует рассмотреть возможные комбинации значений в условии:
Получается, что второй операнд оператора '||' либо вообще не проверяется, либо будет равен false. Следовательно, он не влияет на истинность всего условия. Часть условия после '||' является избыточной.
Скорее всего, подобный формат записи был выбран из некоторых соображений читаемости, но получилось немного странно. Возможно, тут должно проверяться что-то другое.
Issue 5
internal async Task DoRemoveSponsorshipAsync(
Organization sponsoredOrganization,
OrganizationSponsorship sponsorship = null)
{
....
sponsorship.SponsoredOrganizationId = null;
sponsorship.FriendlyName = null;
sponsorship.OfferedToEmail = null;
sponsorship.PlanSponsorshipType = null;
sponsorship.TimesRenewedWithoutValidation = 0;
sponsorship.SponsorshipLapsedDate = null; // <=
if (sponsorship.CloudSponsor || sponsorship.SponsorshipLapsedDate.HasValue)
{
await _organizationSponsorshipRepository.DeleteAsync(sponsorship);
}
else
{
await _organizationSponsorshipRepository.UpsertAsync(sponsorship);
}
}
Предупреждение PVS-Studio: V3063 A part of conditional expression is always false if it is evaluated: sponsorship.SponsorshipLapsedDate.HasValue. OrganizationSponsorshipService.cs 308
Сообщение анализатора говорит о том, что часть логического условия всегда ложна. Обратите внимание на инициализацию sponsorship.SponsorshipLapsedDate. Разработчик присваивает данному свойству null, после чего в условии проверяет значение HasValue у него же. Странно, что проверка производится сразу после инициализации. Она могла бы иметь смысл, если бы свойство sponsorship.CloudSponsor изменяло значение sponsorship.SponsorshipLapsedDate, но это не так. sponsorship.CloudSponsor — обычное автосвойство:
public class OrganizationSponsorship : ITableObject<Guid>
{
....
public bool CloudSponsor { get; set; }
....
}
Возможно, проверка реализована на будущее, но сейчас она выглядит странно.
Issue 6
public async Task ImportCiphersAsync(
List<Folder> folders,
List<CipherDetails> ciphers,
IEnumerable<KeyValuePair<int, int>> folderRelationships)
{
var userId = folders.FirstOrDefault()?.UserId ??
ciphers.FirstOrDefault()?.UserId;
var personalOwnershipPolicyCount =
await _policyRepository
.GetCountByTypeApplicableToUserIdAsync(userId.Value, ....);
....
if (userId.HasValue)
{
await _pushService.PushSyncVaultAsync(userId.Value);
}
}
Предупреждение PVS-Studio: V3095 The 'userId' object was used before it was verified against null. Check lines: 640, 683. CipherService.cs 640
Для введения в суть срабатывания стоит отметить, что переменная userId является объектом nullable-типа.
Обратите внимание на данный фрагмент кода:
if (userId.HasValue)
{
await _pushService.PushSyncVaultAsync(userId.Value);
}
Перед обращением к userId.Value. разработчик проверяет userId.HasValue. Скорее всего, он предполагал, что проверяемое значение может быть равно false.
Перед вышеописанным обращением было еще одно:
_policyRepository.GetCountByTypeApplicableToUserIdAsync(userId.Value, ....);
Здесь также производится обращение к userId.Value, но проверки userId.HasValue почему-то нет. Либо разработчик забыл проверить HasValue в первый раз, либо произвёл лишнюю проверку во второй. Выясним, какой из вариантов верный. Для этого рассмотрим инициализацию userId:
var userId = folders.FirstOrDefault()?.UserId ??
ciphers.FirstOrDefault()?.UserId;
По коду видно, что оба операнда оператора '??' могут принять значение nullable-типа, у которого свойство HasValue будет равно false. Следовательно, userId.HasValue может иметь значение false.
Получается, что при первом обращении к userId.Value всё-таки стоит проверить userId.HasValue. Ведь если значение свойства HasValue равно false, обращение к Value этой же переменной приведёт к выбрасыванию исключения типа InvalidOperationException.
Issue 7
public async Task<List<OrganizationUser>> InviteUsersAsync(
Guid organizationId,
Guid? invitingUserId,
IEnumerable<(OrganizationUserInvite invite, string externalId)> invites)
{
var organization = await GetOrgById(organizationId);
var initialSeatCount = organization.Seats;
if (organization == null || invites.Any(i => i.invite.Emails == null))
{
throw new NotFoundException();
}
....
}
Предупреждение PVS-Studio: V3095 The 'organization' object was used before it was verified against null. Check lines: 1085, 1086. OrganizationService.cs 1085
В условии проверяют organization на равенство null. Получается, разработчик предполагал, что эта переменная может быть равна null. Также перед условием происходит обращение к свойству Seats переменной organization без какой-либо проверки на null. Если organization – null, данное обращение приведёт к выбросу исключения типа NullReferenceException.
Issue 8
public async Task<SubscriptionInfo> GetSubscriptionAsync(
ISubscriber subscriber)
{
....
if (!string.IsNullOrWhiteSpace(subscriber.GatewaySubscriptionId))
{
var sub = await _stripeAdapter.SubscriptionGetAsync(
subscriber.GatewaySubscriptionId);
if (sub != null)
{
subscriptionInfo.Subscription =
new SubscriptionInfo.BillingSubscription(sub);
}
if ( !sub.CanceledAt.HasValue
&& !string.IsNullOrWhiteSpace(subscriber.GatewayCustomerId))
{
....
}
}
return subscriptionInfo;
}
Предупреждение PVS-Studio: V3125 The 'sub' object was used after it was verified against null. Check lines: 1554, 1549. StripePaymentService.cs 1554
Анализатор сообщает о возможном обращении по нулевой ссылке. Перед тем как передать переменную sub в конструктор SubscriptionInfo.BillingSubscription, разработчик проверяет её на null. Странно, что сразу же после этого без какой-либо проверки происходит обращение к свойству CanceledAt этой переменной. Такое обращение может привести к выбрасыванию исключения типа NullReferenceException.
Issue 9
public class FreshdeskController : Controller
{
....
public FreshdeskController(
IUserRepository userRepository,
IOrganizationRepository organizationRepository,
IOrganizationUserRepository organizationUserRepository,
IOptions<BillingSettings> billingSettings,
ILogger<AppleController> logger,
GlobalSettings globalSettings)
{
_billingSettings = billingSettings?.Value; // <=
_userRepository = userRepository;
_organizationRepository = organizationRepository;
_organizationUserRepository = organizationUserRepository;
_logger = logger;
_globalSettings = globalSettings;
_freshdeskAuthkey = Convert.ToBase64String(
Encoding.UTF8
.GetBytes($"{_billingSettings.FreshdeskApiKey}:X")); // <=
}
....
}
Предупреждение PVS-Studio: V3105 The '_billingSettings' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. FreshdeskController.cs 47
Обратите внимание на инициализацию поля _billingSettings. Можно заметить, что ему присваивается значение свойства Value, полученное с применением оператора null-conditional. Скорее всего, ожидается, что billingSettings может иметь значение null. Значит, в поле _billingSettings также может быть присвоен null.
После инициализации _billingSettings происходит обращение к свойству FreshdeskApiKey:
_freshdeskAuthkey = Convert.ToBase64String(
Encoding.UTF8
.GetBytes($"{_billingSettings.FreshdeskApiKey}:X"));
Данное обращение может привести к выбрасыванию исключения типа NullReferenceException.
Issue 10
public PayPalIpnClient(IOptions<BillingSettings> billingSettings)
{
var bSettings = billingSettings?.Value;
_ipnUri = new Uri(bSettings.PayPal.Production ?
"https://www.paypal.com/cgi-bin/webscr" :
"https://www.sandbox.paypal.com/cgi-bin/webscr");
}
Предупреждение PVS-Studio: V3105 The 'bSettings' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. PayPalIpnClient.cs 22
Запись, аналогичная предыдущей, встречается в реализации метода PayPalIpnClient. Здесь переменной bSettings присваивается значение, полученное с помощью оператора null-conditional. Далее происходит обращение к свойству PayPal этой же переменной. Подобное обращение может привести к выбрасыванию исключения типа NullReferenceException.
Issue 11
public async Task<PagedResult<IEvent>> GetManyAsync(
....,
PageOptions pageOptions)
{
....
var query = new TableQuery<EventTableEntity>()
.Where(filter)
.Take(pageOptions.PageSize); // <=
var result = new PagedResult<IEvent>();
var continuationToken = DeserializeContinuationToken(
pageOptions?.ContinuationToken); // <=
....
}
Предупреждение PVS-Studio: V3095 The 'pageOptions' object was used before it was verified against null. Check lines: 135, 137. EventRepository.cs 135
Очередная странность, связанная с отсутствием проверки на null. Обращение к переменной pageOptions производится два раза. При втором обращении используется оператор null-conditional, а вот при первом – почему-то нет.
Либо разработчик выполнил излишнюю проверку на null во втором случае, либо забыл проверить pageOptions в первом. Если верно второе предположение, то возможно обращение по нулевой ссылке, что приведет к исключению типа NullReferenceException.
Issue 12
public async Task<string> PurchaseOrganizationAsync(...., TaxInfo taxInfo)
{
....
if (taxInfo != null && // <=
!string.IsNullOrWhiteSpace(taxInfo.BillingAddressCountry) &&
!string.IsNullOrWhiteSpace(taxInfo.BillingAddressPostalCode))
{
....
}
....
Address = new Stripe.AddressOptions
{
Country = taxInfo.BillingAddressCountry, // <=
PostalCode = taxInfo.BillingAddressPostalCode,
Line1 = taxInfo.BillingAddressLine1 ?? string.Empty,
Line2 = taxInfo.BillingAddressLine2,
City = taxInfo.BillingAddressCity,
State = taxInfo.BillingAddressState,
}
....
}
Предупреждение PVS-Studio: V3125 The 'taxInfo' object was used after it was verified against null. Check lines: 135, 99. StripePaymentService.cs 135
И снова анализатор обнаружил место, в котором может произойти разыменование нулевой ссылки. Действительно, выглядит странно, что в условии происходит проверка переменной taxInfo на null, а вот при ряде обращений к этой же переменной проверки нет.
Issue 13
public IQueryable<OrganizationUserUserDetails> Run(DatabaseContext dbContext)
{
....
return query.Select(x => new OrganizationUserUserDetails
{
Id = x.ou.Id,
OrganizationId = x.ou.OrganizationId,
UserId = x.ou.UserId,
Name = x.u.Name, // <=
Email = x.u.Email ?? x.ou.Email, // <=
TwoFactorProviders = x.u.TwoFactorProviders, // <=
Premium = x.u.Premium, // <=
Status = x.ou.Status,
Type = x.ou.Type,
AccessAll = x.ou.AccessAll,
ExternalId = x.ou.ExternalId,
SsoExternalId = x.su.ExternalId,
Permissions = x.ou.Permissions,
ResetPasswordKey = x.ou.ResetPasswordKey,
UsesKeyConnector = x.u != null && x.u.UsesKeyConnector, // <=
});
}
Предупреждение PVS-Studio: V3095 The 'x.u' object was used before it was verified against null. Check lines: 24, 32. OrganizationUserUserViewQuery.cs 24
Необычно, что переменная x.u сравнивается с null, ведь перед этим к её свойствам происходило обращение (и не один раз!). Возможно, что это просто лишняя проверка. Также есть вероятность, что разработчик забывал проверять на null перед присваиванием.
Issue 14
private async Task<HttpResponseMessage> CallFreshdeskApiAsync(
HttpRequestMessage request,
int retriedCount = 0)
{
try
{
request.Headers.Add("Authorization", _freshdeskAuthkey);
var response = await _httpClient.SendAsync(request);
if ( response.StatusCode != System.Net.HttpStatusCode.TooManyRequests
|| retriedCount > 3)
{
return response;
}
}
catch
{
if (retriedCount > 3)
{
throw;
}
}
await Task.Delay(30000 * (retriedCount + 1));
return await CallFreshdeskApiAsync(request, retriedCount++); // <=
}
Предупреждение PVS-Studio: V3159 Modified value of the 'retriedCount' operand is not used after the postfix increment operation. FreshdeskController.cs 167
Обратите внимание на инкрементацию переменной retriedCount. Весьма странно, что в данном случае использована постфиксная запись. Ведь сначала возвращается текущее значение переменной, а уже потом производится увеличение этого значения. Возможно, стоит изменить постфиксную запись на префиксную:
return await CallFreshdeskApiAsync(request, ++retriedCount)
Для большей наглядности можно использовать следующую запись:
return await CallFreshdeskApiAsync(request, retriedCount + 1)
Пожалуй, среди описанных дефектов нет чего-то, что представляет угрозу в плане безопасности. Большинство ошибок сводится к возможности возникновения исключений при работе с нулевыми ссылками. Тем не менее, эти места стоит поправить.
Как было сказано в начале статьи, даже из относительно небольшого количества срабатываний анализатора можно выделить интересные моменты. Возможно, некоторые недочёты и не влияют на работу программы, однако их также стоит избегать. Хотя бы для того, чтобы у других разработчиков не возникало лишних вопросов.
Мне кажется, весьма удобно иметь средство, позволяющее быстро найти дефекты в коде. Как видите, таким средством может стать статический анализатор :). Предлагаю вам бесплатно попробовать PVS-Studio, чтобы посмотреть, какие ошибки таятся в интересующем вас проекте.
0