Temporary SSL server exceptions must not use permanent cert import

RESOLVED FIXED in mozilla1.9.1b2

Status

()

Core
Security: PSM
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

1.9.0 Branch
mozilla1.9.1b2
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
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

Comment 2

9 years ago
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
(Assignee)

Comment 4

9 years ago
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.
(Assignee)

Comment 6

9 years ago
Created attachment 343995 [details] [diff] [review]
Patch A1, don't import cert for temporary overrides


One line patch, 25 lines context.
Attachment #343995 - Flags: review?(rrelyea)

Comment 7

9 years ago
Kai, in the temp case, do we still keep a reference to the certificate?

bob
(Assignee)

Comment 8

9 years ago
(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.
(Assignee)

Comment 9

9 years ago
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).
(Assignee)

Comment 10

9 years ago
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.
(Assignee)

Comment 11

9 years ago
Do you agree or disagree that user action
  "clear private data / authenticated sessions"
should also remove all temporary overrides?
(Assignee)

Updated

9 years ago
Attachment #343995 - Attachment is obsolete: true
Attachment #343995 - Flags: review?(rrelyea)
(Assignee)

Updated

9 years ago
Attachment #344008 - Flags: review?(rrelyea)
(Assignee)

Comment 12

9 years ago
Changing subject from
  "Improve usability with duplicate-issuer-serial certs and temporary exceptions"

to
  "Temporary SSL server exceptions must not use permanent cert import"
Summary: Improve usability with duplicate-issuer-serial certs and temporary exceptions → 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 14

9 years ago
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..)
Attachment #344008 - Flags: review?(rrelyea) → review+
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).
Keywords: checkin-needed
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
Attachment #344008 - Flags: approval1.9.2?
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
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2b1
status1.9.2: --- → beta1-fixed
Attachment #344008 - Flags: approval1.9.2?
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.
Attachment #344008 - Flags: approval1.9.1.5?
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
Attachment #344008 - Flags: approval1.9.1.5?
status1.9.2: beta1-fixed → ---
Target Milestone: mozilla1.9.2b1 → mozilla1.9.1b2
Depends on: 564584
This caused bug 564584.
You need to log in before you can comment on or make changes to this bug.