Никита Паневин

13 Май 2022

Теги:

#CSharp

Насколько хорошо защищены ваши пароли? Проверка проекта Bitwarden

13 Май 2022

Автор: Никита Паневин

Введение
Лишнее присваивание
Логические ошибки
Проблемы с null
Ошибочный постфикс
Заключение

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 выдаёт сразу два предупреждения:

V3063 A part of conditional expression is always false if it is evaluated: globalSettings.SelfHosted. PremiumRequestModel.cs 23
V3063 A part of conditional expression is always false if it is evaluated: License != null. PremiumRequestModel.cs 23

Часть логического выражения всегда будет ложной. Чтобы в этом убедиться, следует рассмотреть возможные комбинации значений в условии:

если License не равно null, то левый операнд оператора '||' – true. Правый операнд вычисляться не будет;
если globalSettings.SelfHosted будет true, то левый операнд оператора '||' – true. Правый операнд вычисляться не будет;
если License равно null, то правый операнд оператора '||' – false;
если globalSettings.SelfHosted будет false, то правый операнд оператора '||' – false;

Получается, что второй операнд оператора '||' либо вообще не проверяется, либо будет равен 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; }
  ....
}

Возможно, проверка реализована на будущее, но сейчас она выглядит странно.

Проблемы с null

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, чтобы посмотреть, какие ошибки таятся в интересующем вас проекте.

#CSharp