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

Bitwarden – менеджер паролей с открытым исходным кодом\. Это программное обеспечение помогает генерировать уникальные пароли и управлять ими\. Получится ли у анализатора PVS\-Studio отыскать ошибки в таком проекте?

![0947_Bitwarden_ru/image1.png](https://import.viva64.com/docx/blog/0947_Bitwarden_ru/image1.png)

## Введение

Менеджер паролей – решение для хранения и генерации паролей\. Любой человек, использующий подобный софт, хочет быть уверен, что его данные находятся в безопасности\. Качество кода такого инструмента имеет большое значение\.

Именно поэтому я решил взять исходный код Bitwarden от 15\.03\.2022 из [репозитория](https://github.com/bitwarden/server) и проверить его с помощью статического анализатора PVS\-Studio\. Анализатор выдал на код проекта 247 предупреждений\. Среди них мне удалось найти кое\-что интересное\.

## Лишнее присваивание

**Issue 1**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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"\.

Чтобы исправить ошибку, нужно взять часть выражения в скобки:

```cpp
throw new Exception("Failed verifying Apple IAP " +
                    "after too many attempts. " +
                    "Last attempt status: " +
                    (lastReceiptStatus?.Status ?? "null"));
```

**Issue 3, 4**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3063/) A part of conditional expression is always false if it is evaluated: globalSettings\.SelfHosted\. PremiumRequestModel\.cs 23
* [V3063](https://pvs-studio.ru/ru/docs/warnings/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**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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_ — обычное автосвойство:

```cpp
public class OrganizationSponsorship : ITableObject<Guid>
{
  ....
  public bool CloudSponsor { get; set; }
  ....
}
```

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

## Проблемы с null

**Issue 6**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3095/) The 'userId' object was used before it was verified against null\. Check lines: 640, 683\. CipherService\.cs 640

Для введения в суть срабатывания стоит отметить, что переменная _userId_ является объектом nullable\-типа\.

Обратите внимание на данный фрагмент кода:

```cpp
if (userId.HasValue)
{
  await _pushService.PushSyncVaultAsync(userId.Value);
}
```

Перед обращением к _userId\.Value\. _разработчик_ _проверяет _userId\.HasValue_\. Скорее всего, он предполагал, что проверяемое значение может быть равно _false\._

Перед вышеописанным обращением было еще одно:

```cpp
_policyRepository.GetCountByTypeApplicableToUserIdAsync(userId.Value, ....);
```

Здесь также производится обращение к _userId\.Value_, но проверки _userId\.HasValue _почему\-то нет\. Либо разработчик забыл проверить _HasValue_ в первый раз, либо произвёл лишнюю проверку во второй\. Выясним, какой из вариантов верный\. Для этого рассмотрим инициализацию _userId_:

```cpp
var userId = folders.FirstOrDefault()?.UserId ??
             ciphers.FirstOrDefault()?.UserId;
```

По коду видно, что оба операнда оператора '??' могут принять значение nullable\-типа, у которого свойство_ HasValue_ будет равно _false_\. Следовательно, _userId\.HasValue_ может иметь значение _false_\.

Получается, что при первом обращении к _userId\.Value_ всё\-таки стоит проверить_ userId\.HasValue_\. Ведь если значение свойства _HasValue_ равно_ false,_ обращение к _Value_ этой же переменной приведёт к выбрасыванию исключения типа _InvalidOperationException\._

**Issue 7**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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_:

```cpp
_freshdeskAuthkey = Convert.ToBase64String(
                Encoding.UTF8
                .GetBytes($"{_billingSettings.FreshdeskApiKey}:X"));
```

Данное обращение может привести к выбрасыванию исключения типа _NullReferenceException\._

**Issue 10**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3125/) The 'taxInfo' object was used after it was verified against null\. Check lines: 135, 99\. StripePaymentService\.cs 135

И снова анализатор обнаружил место, в котором может произойти разыменование нулевой ссылки\. Действительно, выглядит странно, что в условии происходит проверка переменной _taxInfo_ на _null_, а вот при ряде обращений к этой же переменной проверки нет\.

**Issue 13**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3159/) Modified value of the 'retriedCount' operand is not used after the postfix increment operation\. FreshdeskController\.cs 167

Обратите внимание на инкрементацию переменной _retriedCount_\. Весьма странно, что в данном случае использована постфиксная запись\. Ведь сначала возвращается текущее значение переменной, а уже потом производится увеличение этого значения\. Возможно, стоит изменить постфиксную запись на префиксную:

```cpp
return await CallFreshdeskApiAsync(request, ++retriedCount)
```

Для большей наглядности можно использовать следующую запись: 

```cpp
return await CallFreshdeskApiAsync(request, retriedCount + 1)
```

## Заключение 

Пожалуй, среди описанных дефектов нет чего\-то, что представляет угрозу в плане безопасности\. Большинство ошибок сводится к возможности возникновения исключений при работе с нулевыми ссылками\. Тем не менее, эти места стоит поправить\.

Как было сказано в начале статьи, даже из относительно небольшого количества срабатываний анализатора можно выделить интересные моменты\. Возможно, некоторые недочёты и не влияют на работу программы, однако их также стоит избегать\. Хотя бы для того, чтобы у других разработчиков не возникало лишних вопросов\.

Мне кажется, весьма удобно иметь средство, позволяющее быстро найти дефекты в коде\. Как видите, таким средством может стать статический анализатор :\)\. Предлагаю вам бесплатно [попробовать PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/try-free/), чтобы посмотреть, какие ошибки таятся в интересующем вас проекте\.