There are flawed devices/routers out there, that use duplicate issuer-serial-numbers. I discovered a minor bug in the way we handle temporary exceptions, and fixing it can ease the pain for users who use temporary overrides with such servers/devices. When we add a temporary exception to the override list (in psm), we still import the related cert into the nss db (permanently). As a consequence, after Mozilla session restart, this cert is still around, and causes pain for people with conflicting certs. As keeping this cert is unnecessary, I propose we do not keep it around (for temporary exceptions). I did a bit of testing, by using such conflicting on 2 test servers. Using temporary exceptions, not storing certs permanently, and using "clear private data / authenticated sessions" was sufficient to connect to the second, conflicting server, without the need to restart.
I further propose, that on "clear private data / authenticated sessions" that we clear all temporary overrides. This way we could give out a recommended workaround for the people suffering from those devices: - use temporary overrides - clear authenticated session
Congrats for finding this bug!
> When we add a temporary exception to the override list (in psm), we still > import the related cert into the nss db (permanently). Oops
the imported cert for the temporary exception does not have any trust associated, but it's still around
Yup, and that's all it takes to have duplicate issuer and serial. No trust required.
Created attachment 343995 [details] [diff] [review] Patch A1, don't import cert for temporary overrides One line patch, 25 lines context.
Kai, in the temp case, do we still keep a reference to the certificate? bob
(In reply to comment #7) > Kai, in the temp case, do we still keep a reference to the certificate? No. With this patch, cert manager will display the override, but not the cert. There is currently no way to keep a reference, and make the reference will get cleared. Doing this smarter approach would require me to find a place to keep the reference, and remove it when the temporary override goes away. Maybe we should do that.
Sorry, it's not that simple to get ability to connect to a conflicting cert in the same session. Even with these simple steps to reproduce I get the duplicate-issuer-serial error code from NSS: - visit site A (invalid cert) - do NOT add an exception - leave page (go to a plain http page) - clear private data / auth sessions - visit site B (conflicting cert) The cert from site A is apparently still in memory. I remember, in each browser window, we cache the previously seen cert. This was done in order to reduce the number of times we verify certs for EV... Getting this capability (allow conflicting certs in same session when using only temporary overrides) would require more cleanup at the PSM level (iterate through all open windows and clean up all cached references to certs).
Created attachment 344008 [details] [diff] [review] Patch v2 Despite my previous comment, it makese sense to add this enhanced patch. - it avoids permanent import for temporary overrides - it keeps the override certificate in memory (only) by incrementing the reference count - it releases the reference when the override goes away (when the user explicitly deletes the temp override, or uses clear private data / auth sessions => logout, which I have changed to clear all temporary overrides, too) The benefit of this patch: At least restarting Firefox helps to get rid of the old cert.
Do you agree or disagree that user action "clear private data / authenticated sessions" should also remove all temporary overrides?
Changing subject from "Improve usability with duplicate-issuer-serial certs and temporary exceptions" to "Temporary SSL server exceptions must not use permanent cert import"
In reply to comment 11, I agree. With respect to patch v2, my primary concern is that we be sure we're not creating a cert reference leaks.
Comment on attachment 344008 [details] [diff] [review] Patch v2 r+ assuming the COMptr automatically decrements the pointer when assigned the value NULL. (the problem with having me review XPCOM code..)
Why hasn't this landed? It received r+ a year ago.
> Why hasn't this landed? It received r+ a year ago. I'd guess MoCo needs to increase the PSM staffing.
(In reply to comment #16) > > Why hasn't this landed? It received r+ a year ago. > I'd guess MoCo needs to increase the PSM staffing. I dunno - I hear your advocacy here, Nelson, but I think it's safe to assume that sometimes bugs fall through the cracks (particularly in light of the fact that MoCo has increased PSM staffing in the past year - HonzaB is a PSM peer).
Comment on attachment 344008 [details] [diff] [review] Patch v2 It'd be nice to get this in 3.6, and possibly a 3.5.x
Apparently, this landed a year ago. http://hg.mozilla.org/mozilla-central/rev/d390f74453ad http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d390f74453ad
Comment on attachment 344008 [details] [diff] [review] Patch v2 This probably applies fine to 1.9.1, considering how old the patch is... Requesting approval based on dveditz's wish.
Comment on attachment 344008 [details] [diff] [review] Patch v2 Actually, this is already on 1.9.1, too. http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d390f74453ad
This caused bug 564584.