search

SafeExamBrowser / seb-win

Safe Exam Browser 2.x for Windows. IMPORTANT: This is the legacy version which isn't actively developed anymore.
https://safeexambrowser.org/windows/win_usermanual_en.html
69 stars 52 forks source link

Include all valid certificates in config tool #74

Closed rdbahm closed 3 years ago

rdbahm commented 5 years ago

In the previous version, the GetSSLCertificatesAndNames method uses the FindByKeyUsage FindType to locate valid certs for SSL trust. This does not adequately allow all possible certificates which a system might use for SSL to be added to the configuration file. By changing to "FindByTimeValid," we instead list all currently-valid certificates on the system. This does put additional onus on the administrator to know which certificate is the appropriate certificate - but should reduce head-scratching when their certificate isn't listed.

See SourceForge support request https://sourceforge.net/p/seb/support-requests/55/ for background on the issue in my environment.

rdbahm commented 5 years ago

Hi there, I thought I should check in on this issue. Do I need to include any more information for the pull request to be accepted?

danschlet commented 5 years ago

I don't think it's a good idea to just check for the date validity of SSL certificates and just show all. X.509 certificates have properties to identify for what they are intended to be used, and if these properties are not set correctly, the certificate is wrong and has to be replaced.

I can analyze the issue further if you send me the certificate which doesn't show up in the Config Tool. Maybe another certificate property has to be included in our criteria which certificates to show. Just showing all with a valid date seems like a very ugly workaround and not really a real solution for the problem. Therefore we won't accept the pull request.

rdbahm commented 5 years ago

Yes, I looked at all responses to my query and tested each. In inspecting the code, the code looks in CertificateAuthority and AddressBook. (SEBProtectionController.cs, lines 151-154). I tested Darren's suggested change with no impact.

Our certificate has the usages CrlSign, KeyCertSign, and DigitalSignature. It's excluded from the filter because it lacks KeyEncipherment. As it stands in the code today, both DigitalSignature and KeyEncipherment are required for it to appear in the list. KeyEncipherment isn't a requirement to include in XulRunner's certificate list (I can confirm this, since our certificates are being used in production with this configuration), so one possible alternative to my initial solution would be to remove KeyEncipherment as part of the find bitmask. This would still require that DigitalSignature be included in the usages, which may help allay your concerns about only allowing certificates which have the appopriate usages.

I've uploaded an updated version of the patch to the pull request for review.

danschlet commented 5 years ago

Thank you for looking further into this. I think this is a working solution. I will test the patch tomorrow.

danschlet commented 5 years ago

I don't see the update for the patch, are you sure you pushed the update?

rdbahm commented 5 years ago

You're correct - I made and tested the changes, but failed to press "push" before making my comment on GitHub. I'm sure the changes are there now.