460829 - Temporary SSL server exceptions must not use permanent cert import

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kai Engert (:KaiE:)]

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.

Comment 1

[Kai Engert (:KaiE:)]

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

[Eddy Nigg (StartCom)]

Congrats for finding this bug!

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

> When we add a temporary exception to the override list (in psm), we still
> import the related cert into the nss db (permanently).

Oops

Comment 4

[Kai Engert (:KaiE:)]

the imported cert for the temporary exception does not have any trust associated, but it's still around

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Yup, and that's all it takes to have duplicate issuer and serial.
No trust required.

Comment 6

[Kai Engert (:KaiE:)]

Attachment: Patch A1, don't import cert for temporary overrides

One line patch, 25 lines context.

Comment 7

[Robert Relyea]

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

bob

Comment 8

[Kai Engert (:KaiE:)]

(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.

Comment 9

[Kai Engert (:KaiE:)]

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).

Comment 10

[Kai Engert (:KaiE:)]

Attachment: 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.

Comment 11

[Kai Engert (:KaiE:)]

Do you agree or disagree that user action
  "clear private data / authenticated sessions"
should also remove all temporary overrides?

Comment 12

[Kai Engert (:KaiE:)]

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

to
  "Temporary SSL server exceptions must not use permanent cert import"

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

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

[Robert Relyea]

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..)

Comment 15

[Reed Loden [:reed]]

Why hasn't this landed? It received r+ a year ago.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

> Why hasn't this landed? It received r+ a year ago.
I'd guess MoCo needs to increase the PSM staffing.

Comment 17

[Johnathan Nightingale [:johnath]]

(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 18

[Daniel Veditz [:dveditz]]

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

Comment 19

[Reed Loden [:reed]]

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 20

[Reed Loden [:reed]]

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 21

[Reed Loden [:reed]]

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

This caused bug 564584.