How well protected are your passwords? Checking out the Bitwarden project

Bitwarden is an open source password manager. This software helps you generate and manage unique passwords. Will the PVS-Studio analyzer be able to find errors in such a project?

Introduction

Password Manager is a solution for storing and generating passwords. Any person using such software wants to be sure that their data is safe. The quality of the code of such a tool is of great importance.

That is why I decided to take the Bitwarden source code from 03/15/2022 from repository and check it with the PVS-Studio static analyzer. The analyzer generated 247 warnings for the project code. Among them, I managed to find something interesting.

Extra assignment

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 warning: V3008 The ‘Amount’ variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 148, 142. BillingInfo.cs 148

Pay attention to initialization Amount. This property is assigned the expression inv.AmountDue / 100M. It looks unusual that literally after five lines of code, a similar operation is performed, but already with assignment inv.Total / 100M.

It is difficult to say which value the developer wanted to use. If the last assignment is true, then the first is redundant. This does not add beauty to the code, but still does not affect the logic of program execution. Otherwise, this section of code will not work correctly.

Logic errors

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 warning: V3123 Perhaps the ‘??’ operator works in a different way than it was expected. Its priority is lower than the priority of other operators in its left part. AppleIapService.cs 96

It looks like the developer was expecting either the property value to be added to the message Status, or the string “null”. After that, the result will be added to the line “Failed verifying Apple IAP after too many attempts. Last attempt status: “. Unfortunately, the behavior of the program will be different.

In order to understand the essence of this operation, it is worth remembering the priorities of the operators. Operator ‘??’ has a lower precedence than the ‘+’ operator. Therefore, the string will be added first with the property value Statusand after that the null-coalescing operator will work.

If lastReceiptStatus not null and Status not nullthis method works correctly.

If lastReceiptStatus or Status after all null, the following message is displayed: “Failed verifying Apple IAP after too many attempts. Last attempt status: “. It is obviously incorrect. The expected message is: “Failed verifying Apple IAP after too many attempts. Last attempt status: null”.

To fix the error, you need to take part of the expression in brackets:

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

Here PVS-Studio issues two warnings at once:

  • 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

Part of the boolean expression will always be false. To verify this, consider the possible combinations of values ​​in the condition:

  • if License not equal null, then the left operand of the operator ‘||’ – true. The right operand will not be evaluated;

  • if globalSettings.SelfHosted will true, then the left operand of the operator ‘||’ – true. The right operand will not be evaluated;

  • if License equals null, then the right operand of the operator ‘||’ – false;

  • if globalSettings.SelfHosted will false, then the right operand of the operator ‘||’ – false;

It turns out that the second operand of the operator ‘||’ either not checked at all, or will be equal to false. Therefore, it does not affect the truth of the entire condition. Part of the condition after ‘||’ is redundant.

Most likely, a similar recording format was chosen for some reasons of readability, but it turned out a little strange. Perhaps something else should be checked here.

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 warning: V3063 A part of conditional expression is always false if it is evaluated: sponsorship.SponsorshipLapsedDate.HasValue. OrganizationSponsorshipService.cs 308

The analyzer message says that part of the logical condition is always false. Pay attention to initialization sponsorship.SponsorshipLapsedDate. The developer assigns to this property nullafter which it checks the value in the condition HasValue he has. It is strange that the check is performed immediately after initialization. It might make sense if the property sponsorship.CloudSponsor changed the meaning sponsorship.SponsorshipLapsedDatebut it’s not. sponsorship.CloudSponsor – the usual auto-property:

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

Perhaps the check is implemented for the future, but now it looks strange.

Problems with 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 warning: V3095 The ‘userId’ object was used before it was verified against null. Check lines: 640, 683. CipherService.cs 640

For an introduction to the essence of triggering, it is worth noting that the variable userId is an object of nullable type.

Take a look at this code snippet:

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

Before contacting userId.Value. developer checks userId.HasValue. Most likely, he assumed that the value being checked could be equal to false.

Before the above message, there was one more:

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

It also refers to userId.Valuebut checks userId.HasValue somehow not. Or the developer forgot to check HasValue the first time, or made an extra check in the second. Let’s find out which option is correct. To do this, consider the initialization userId:

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

The code shows that both operands of the operator ‘??’ can take a value of a nullable type, which has a property HasValue will be equal to false. Hence, userId.HasValue may matter false.

It turns out that on the first call to userId.Value still worth checking out userId.HasValue. After all, if the property value HasValue equals falseappeal to value the same variable will result in throwing an exception of the type 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 warning: V3095 The ‘organization’ object was used before it was verified against null. Check lines: 1085, 1086. OrganizationService.cs 1085

In the condition check organization for equality null. It turns out that the developer assumed that this variable could be equal to null. Also before the condition there is a call to the property Seats variable organization without any check for null. If a organizationnullthis call will result in throwing an exception of type 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 warning: 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 by a null reference. Before passing a variable sub to the constructor SubscriptionInfo.BillingSubscriptionthe developer checks it for null. It is strange that immediately after this, without any check, the property is accessed CanceledAt this variable. Such a call may result in throwing an exception of type 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 warning: V3105 The ‘_billingSettings’ variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. FreshdeskController.cs 47

Pay attention to field initialization _billingSettings. You can notice that it is assigned a property value value, obtained using the null-conditional operator. It is most likely expected that billingSettings may matter null. So in the field _billingSettings can also be assigned null.

After initialization _billingSettings property is accessed FreshdeskApiKey:

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

This call may result in throwing an exception of type 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 warning: V3105 The ‘bSettings’ variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. PayPalIpnClient.cs 22

An entry similar to the previous one occurs in the implementation of the method PayPalIpnClient. Here the variable bSettings the value obtained using the null-conditional operator is assigned. Next, the property is accessed. PayPal the same variable. Such handling may result in throwing an exception of type 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 warning: V3095 The ‘pageOptions’ object was used before it was verified against null. Check lines: 135, 137. EventRepository.cs 135

Another oddity associated with the lack of verification for null. Accessing a variable pageOptions produced twice. On the second call, the null-conditional operator is used, but on the first call, for some reason, it is not.

Or the developer performed an unnecessary check on null in the second case, or forgot to check pageOptions In the first. If the second assumption is true, then access by a null reference is possible, which will lead to an exception of type 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 warning: V3125 The ‘taxInfo’ object was used after it was verified against null. Check lines: 135, 99. StripePaymentService.cs 135

And again, the analyzer found a place where a null reference could be dereferenced. Indeed, it looks strange that a variable is checked in the condition taxInfo on the nullbut with a number of calls to the same variable, there is no check.

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 warning: V3095 The ‘xu’ object was used before it was verified against null. Check lines: 24, 32. OrganizationUserUserViewQuery.cs 24

Unusually, the variable xi compared to null, because before that, its properties were accessed (and more than once!). It is possible that this is just an extra check. It is also possible that the developer forgot to check for null before assignment.

Wrong postfix

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 warning: V3159 Modified value of the ‘retriedCount’ operand is not used after the postfix increment operation. FreshdeskController.cs 167

Pay attention to the variable increment retriedCount. It is rather strange that postfix notation is used in this case. After all, the current value of the variable is returned first, and only then this value is increased. Perhaps you should change the postfix notation to prefix:

return await CallFreshdeskApiAsync(request, ++retriedCount)

For greater clarity, you can use the following notation:

return await CallFreshdeskApiAsync(request, retriedCount + 1)

Conclusion

Perhaps, among the described defects, there is nothing that poses a threat in terms of security. Most of the errors boil down to the possibility of throwing exceptions when dealing with null references. However, these places should be corrected.

As it was said at the beginning of the article, even from a relatively small number of analyzer responses, interesting moments can be distinguished. Perhaps some shortcomings do not affect the operation of the program, but they should also be avoided. At least so that other developers do not have unnecessary questions.

I find it very convenient to have a tool that allows you to quickly find defects in the code. As you can see, a static analyzer can become such a tool :). I offer you for free try PVS-Studioto see what errors are lurking in the project you are interested in.

If you want to share this article with an English-speaking audience, please use the translation link: Nikita Panevin. Are you sure your passwords are protected? The Bitwarden project check.

Similar Posts

Leave a Reply