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?
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 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.
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
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.
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 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:
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:
A part of the logical expression is always false. Look at possible combinations of values in the condition:
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
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 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:
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.
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
To understand the warning, note that the userld variable is a nullable type object.
Look at the following code fragment:
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:
_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:
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
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
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
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 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
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
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:
_freshdeskAuthkey = Convert.ToBase64String(
Encoding.UTF8
.GetBytes($"{_billingSettings.FreshdeskApiKey}:X"));Such accessing can result in 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 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
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 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
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
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
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 '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.
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
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:
return await CallFreshdeskApiAsync(request, ++retriedCount)For more clarity, you can use the following notation:
return await CallFreshdeskApiAsync(request, retriedCount + 1)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 for free and see what errors are lurking in the project interesting for you.