Fix Null Reference Exceptions on non-configured certificate stores #3023
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3023 +/- ##
==========================================
- Coverage 56.70% 56.63% -0.07%
==========================================
Files 356 356
Lines 68110 68445 +335
Branches 13950 14082 +132
==========================================
+ Hits 38619 38764 +145
- Misses 25339 25499 +160
- Partials 4152 4182 +30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -578,7 +578,7 @@ private ServiceResult UpdateCertificate( | |||
| try | |||
| { | |||
| Utils.LogCertificate(Utils.TraceMasks.Security, "Add new issuer certificate: ", issuer); | |||
| issuerStore.Add(issuer).Wait(); | |||
| issuerStore?.Add(issuer).Wait(); | |||
There was a problem hiding this comment.
The ? will prevent the NullReferenceException to be thrown, which then leads to an inconsitency with the log message logged just before and gives no indication that this add actually failed. Wouldn't it be better to have an explicit null check with a clear failing excection.
This method is actually really huge, maybe such test should be part of some preliminary conditions before appling changes to stores.
There was a problem hiding this comment.
NonConfigured Issuer store should not cause any error messages.
Good Point with the inconsistent logging, cahanged to explicit null in newest push.
| @@ -144,20 +144,23 @@ private ServiceResult Open( | |||
| ICertificateStore store = m_trustedStore.OpenStore(); | |||
| try | |||
| { | |||
| if ((masks & TrustListMasks.TrustedCertificates) != 0) | |||
| if (store != null) | |||
There was a problem hiding this comment.
Similar question here and below in this method about the case of store being null and the return status code Good. When one of the store is null, shall the result still be good? And wouldn't it be good to test all stores prior to any modification to one of them?
There was a problem hiding this comment.
Imho a store with just issuer or just trusted certs should be okay. E.g. for user certs you only want to allow specific certs and not trust all certs from a ca, then UserIssuerStore would be unconfigured, but still a valid trust list.
| var certCollection = store.FindByThumbprint(thumbprint).GetAwaiter().GetResult(); | ||
|
|
||
| if (certCollection.Count == 0) | ||
| if (store != null) |
There was a problem hiding this comment.
Same question here, result is considered good if the store is null, is this what we want?
There was a problem hiding this comment.
Imho a store with just issuer or just trusted certs should be okay. E.g. for user certs you only want to allow specific certs and not trust all certs from a ca, then UserIssuerStore would be unconfigured, but still a valid trust list.
|
There are some other places where a store is check to be not null, but what is the result status code supposed to be in case the store is actually null. It is a general comment to other places where this has been modified. |
Proposed changes
If issuer store or rejected store are not configured null reference exceptions could occur.
Add Null checks to avoid these exceptions
Related Issues
Types of changes
Checklist
Further comments