# Are you sure your passwords protected? The Bitwarden project check

Bitwarden is an open\-source password management service\. The software helps generate and manage unique passwords\. Will PVS\-Studio find errors in such a project?

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

## Introduction

Password management is a solution that generates and stores passwords\. Anyone who uses this service wants to be sure that their data is secure\. The code quality of such a tool should be high\.

That's why I decided to check the Bitwarden source code \([repository](https://github.com/bitwarden/server) from 15\.03\.2022\) with the PVS\-Studio static analyzer\. The analyzer issued 247 warnings on the project's code\. Let's look at the most interesting warnings there\.

## Redundant assignment

**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 warning: [V3008](https://pvs-studio.com/en/docs/warnings/v3008/) The 'Amount' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 148, 142\. BillingInfo\.cs 148

Look at the initialization of _Amount_\. The _inv\.AmountDue / 100M_ expression is assigned to this property\. What's strange — there's a similar operation five lines below this one\. But this time the _inv\.Total / 100M_ is assigned\.

Hard to say what value the developer wanted to use\. If the last assignment is true, then the first one is redundant\. Theis doesn't make code beautiful, but it doesn't affect the code logic either\. If the last assignment is false, then this fragment will work incorrectly\. 

## Logical errors

**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 warning: [V3123](https://pvs-studio.com/en/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

Seems like the developer expected the message to get either the _Status_ property value, or the null string\. Then, the value or null is supposed to be added to the "Failed verifying Apple IAP after too many attempts Last attempt status: "\. Unfortunately, the code's behavior is different\. 

To understand the problem here, let's remember the operators' priorities\. The '??' operator's priority is lower than the '\+' operator's priority\. Therefore, the value of the _Status_ property is added to the string first, and after that the null coalescing operator snaps into action\. 

If _lastReceiptStatus_ is not _null_, and _Status_ is not _null_, this method works correctly\.

If _lastReceiptStatus_ or _Status_ — _null_, we'll get the following message: "Failed verifying Apple IAP after too many attempts\. Last attempt status: "\. This is obviously incorrect\.  The message we expect to get looks like this: "Failed verifying Apple IAP after too many attempts\. Last attempt status: null"\.

To fix this, take part of the expression in brackets:

```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);
}
```

Here PVS\-Studio issues two warnings:

* [V3063](https://pvs-studio.com/en/docs/warnings/v3063/) A part of conditional expression is always false if it is evaluated: globalSettings\.SelfHosted\. PremiumRequestModel\.cs 23
* [V3063](https://pvs-studio.com/en/docs/warnings/v3063/) A part of conditional expression is always false if it is evaluated: License \!\= null\. PremiumRequestModel\.cs 23

A part of the logical expression is always false\. Look at possible combinations of values in the condition:

* if _License_ is not _null_ then the left operand of the '\|\|' operator is _true_\. The right operand is not evaluated\.
* if _globalSettings\.SelfHosted_ is _true_, then the left operand of the '\|\|' operator is _true_\. The right operand is not evaluated\.
* if _License_ is _null_, then the right operand of the '\|\|' operator is _false_;
* if _globalSettings\.SelfHosted_ is _false_, then the right operand of the '\|\|' operator is _false_;

So, the second operand of the '\|\|' operator is either not checked or _false_\. This operand does not affect the result of the condition\. A part of the condition after '\|\|' is redundant\.

Most likely, the developer chose such a notation because of readability, but the result is a little strange\. Perhaps something else should be checked here\.

**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 warning: [V3063](https://pvs-studio.com/en/docs/warnings/v3063/) A part of conditional expression is always false if it is evaluated: sponsorship\.SponsorshipLapsedDate\.HasValue\. OrganizationSponsorshipService\.cs 308

The analyzer message says that a part of the logical expression is always false\. Look at the initialization of _sponsorship\.SponsorshipLapsedDate_\. The developer assigns _null_ to this property and after that checks _HasValue_ of the same property\. It's strange that the check goes right after the initialization\. It might make sense if _sponsorship\.CloudSponsor_ changed the value of _sponsorship\.SponsorshipLapsedDate_, but it doesn't\. _sponsorship\.CloudSponsor_ is an auto\-property:

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

Maybe the check is implemented here for some further actions but now it looks weird\.

## Problems with 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 warning: [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'userId' object was used before it was verified against null\. Check lines: 640, 683\. CipherService\.cs 640

To understand the warning, note that the _userld_ variable is a nullable type object\.

Look at the following code fragment:

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

Before accessing _userId\.Value _the developer checks _userId\.HasValue_\. Most likely, they assumed that the value checked could be _false\._

There was another accessing just above the previous one:

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

Here the developer also accesses _userId\.Value_ but doesn't check _userId\.HasValue_\. They either forgot to check _HasValue_ the first time or extra checked it the second time\. Let's figure out which guess is correct\. To do this, we'll go find the _userId_ initialization:

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

The code shows that both operands of the '??' operator can take the nullable type value\. The _HasValue_ property of this value is _false_\. So, _userId\.HasValue_ can be _false_\.

When the developer first accesses _userId\.Value_, they should check _userId\.HasValue_\. If the _HasValue_ property's value is _false_, accessing _Value_ of this variable results in _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 warning: [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'organization' object was used before it was verified against null\. Check lines: 1085, 1086\. OrganizationService\.cs 1085

This condition checks whether _organization_ is _null_\. So, the developer supposed that this variable can be _null_\. Besides, above the condition, the _Seats_ property of the _organization_ variable is accessed without any _null_ check\. If _organization_ – _null_, accessing _Seats_ results in _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 warning: [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The 'sub' object was used after it was verified against null\. Check lines: 1554, 1549\. StripePaymentService\.cs 1554

The analyzer reports a possible access to a null reference\. Before passing the _sub_ variable to the _SubscriptionInfo\.BillingSubscription_ constructor, the developer checks it for _null_\. It is strange that immediately after this the _CanceledAt_ property of this variable is accessed without any check\. Such accessing can result in _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 warning: [V3105](https://pvs-studio.com/en/docs/warnings/v3105/) The '\_billingSettings' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. FreshdeskController\.cs 47

Look at the initialization of the _\_billingSettings_ field\. Here the field is assigned with the _Value_ property's value obtained with the null\-conditional operator\. The developer probably expects that _billingSettings_ can be _null_\. Which means _null_ can be assigned to the _\_billingSettings_ field\.

After initializing _\_billingSettings_, the _FreshdeskApiKey_ property is accessed:

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

Such accessing can result in _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 warning: [V3105](https://pvs-studio.com/en/docs/warnings/v3105/) The 'bSettings' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. PayPalIpnClient\.cs 22

An issue similar to the previous one is found in the implementation of the _PayPalIpnClient_ method\. Here, the _bSettings_ variable is assigned a value obtained with the null\-conditional operator\. Next, the _PayPal_ property of the same variable is accessed\. Such accessing can result in _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 warning: [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'pageOptions' object was used before it was verified against null\. Check lines: 135, 137\. EventRepository\.cs 135

Another weird fragment related to the absence of _null_ check\. The _pageOptions_ variable is accessed twice\. In the second access, the developer uses the null\-conditional operator\. For some reason, they don't use it the first time\. 

The developer either extra checked for _null_ in the second access or forgot to check _pageOptions _in the first one\. If the second assumption is correct, then it is possible to access the null reference\. This will lead to _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 warning: [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The 'taxInfo' object was used after it was verified against null\. Check lines: 135, 99\. StripePaymentService\.cs 135

The analyzer again found a fragment where a null reference can be dereferenced\. Indeed, it looks strange that the condition checks the _taxInfo_ variable for _null_, but there is no check in a number of accesses to this variable\.

**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 warning: [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'x\.u' object was used before it was verified against null\. Check lines: 24, 32\. OrganizationUserUserViewQuery\.cs 24

It's weird that the _x\.u_ variable is compared with _null_, because before that the developer accessed its properties \(and not even once\!\)\. Maybe it's an extra check\. There is also a possibility that the developer forgot to check for _null_ before assigning this variable to the initialization fields\.

## Erroneous postfix

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

Look at the incrementation of the _retriedCount_ variable\. Weird — the postfix notation is used here\. The current value of the variable is returned first, and only then this value is increased\. Maybe the developer should replace postfix notation with the prefix one:

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

For more clarity, you can use the following notation: 

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

## Conclusion 

Perhaps, none of the described issues here poses a security threat\. Most warnings are issued on the possibility of exceptions that can be thrown on the work with null references\. Nevertheless, these places should be corrected\.

We can find a lot of interesting moments even in a relatively small number of analyzer warnings\. It is possible that some of the issues do not affect the program's operation, but the developers still should avoid them\. At least so that other developers won't have unnecessary questions\.

I think it is cool to have a tool that quickly finds errors in code\. As you can see, a static analyzer can become such a tool :\)\. You can [try PVS\-Studio](https://pvs-studio.com/en/pvs-studio/try-free/) for free and see what errors are lurking in the project interesting for you\.