399696 - allow temporary certificate exceptions

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Jo Hermans]

I was trying to view this link :

https://www.nissan-global.com/EN/TECHNOLOGY/INTRODUCTION/DETAILS/AVM/index.html

But this uses a certificate for secure.nissan.co.jp, which throws us a ssl_error_bad_cert_domain error obviously. In the latest trunk we can't blindly accept that certificate anymore (I'm not complaining, I'm all in favor), so I went to the View Certificates dialog box, and added the exception, and then I could see the page.

What I really wanted to do is to add only a temporary certificate, I never wanted to store it permanently. But that's not possible anymore, you have to go back to the View Certificates dialog box, and remove the certificate manually.

Can't we add a 'temporary exception' (delete after exit) as a workaround ? Maybe we can ask in the Exception dialog box if we want to store it permanently or temporary. Then store it with a flag in the cert-database (not in memory like the old implementation), and then delete them at shutdown.

I repeat: I'm in favor of the current change. This is not one of those 'revert back to the old insecure model' bugs.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

If there's enough demand for this feature, I have no immediate objections.

Comment 2

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Comment 3

[Kai Engert (:KaiE:)]

Comment on attachment 286878 [details] [diff] [review]
Patch v1

Bob, can you please review?

Comment 4

[Robert Relyea]

Comment on attachment 286878 [details] [diff] [review]
Patch v1

r-

the patch looks mostly correct, exception one minor detail. I fail to see where the temporary entries are not written out when we do a 'write' after we add and exception. (maybe it's there, but it's not clear from this patch).

bob

Comment 5

[Kai Engert (:KaiE:)]

Comment on attachment 286878 [details] [diff] [review]
Patch v1

It's there, look here, in the write callback (that is reached for each entry), if we detect the entry is a temporary one, we immediately skip processing of this entry by returning "hash_next", and therefore not writing it out.

Re-requesting review.

 PR_STATIC_CALLBACK(PLDHashOperator)
 WriteEntryCallback(nsCertOverrideEntry *aEntry,
                    void *aArg)
 {
   static const char kNew[] = "\n";
   static const char kTab[] = "\t";
 
   nsIOutputStream *rawStreamPtr = (nsIOutputStream *)aArg;
 
   nsresult rv;
 
   if (rawStreamPtr && aEntry)
   {
     const nsCertOverride &settings = aEntry->mSettings;
+    if (settings.mIsTemporary)
+      return PL_DHASH_NEXT;

Comment 6

[Robert Relyea]

Comment on attachment 286878 [details] [diff] [review]
Patch v1

r+ so it is... I thought I looked for the callback. Must of missed it.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 286878 [details] [diff] [review]
Patch v1

a=1.9 for drivers

though one question -- doesn't the uuid on nsICertOverrideService.idl need to be revved, with the interface change?  I guess it's a new interface so it doesn't really matter, but it probably should be for completeness.

Comment 8

[Kai Engert (:KaiE:)]

Attachment: Patch as checked in

Vlad, thanks a lot for your proposal, I agree it's a good idea to use a new uuid.

In addition, the context around my changes has changed a lot in the meantime, so I'm attaching the new patch that is merged to the trunk.

Comment 9

[Kai Engert (:KaiE:)]

checked in

Comment 10

[Jo Hermans]

This now works for me in
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112005 Minefield/3.0b2pre

I noticed that the 'permanent' state is the default, so you have to click the toggle button. Previously, the 'temporary' state was the default in the dialog box. I think it's dangerous, although it now takes more steps to accidentally add an exception.

Comment 11

[Kai Engert (:KaiE:)]

I'm open to change the default to "temporary".

In another bug, we said
  "maybe the add exception dialog should always use the same default
   for temporary/permanent, independent of context"

And I think Johnathan agreed to that idea.

Should I attach a patch to change the default?

Comment 12

[Johnathan Nightingale [:johnath]]

I would agree that temporary feels like a safer default, yeah, and I agree that this seems like a sensible default across the board for this dialog.  If a specific application wants the ability to control it later, we'll deal with it then.  

Comment 13

[Kai Engert (:KaiE:)]

Bob Relyea raised a concern about using "temporary" as a default: "The default shouldn't be temporary. The issue is, if you make the default temporary, you will continue to generate warnings. The more often the error comes up, the more you train the user to ignore it."

Comment 14

[AndrewM]

With regards to whether the default should be 'temporary' or 'permanent', how about adding a pref to control this behavior (with the default set to 'temporary', which I agree is the better choice)?

Given the pushback from some users that occurred after the SSL error page was originally introduced (albeit before bug 401575 was checked in), I'm thinking that anything that can be done, which has no impact on security or performance, to ease the transition to Fx 3 for those users who are used to clicking through the Fx 2 warning dialogs, is worth considering.

A pref which makes all future exceptions permanent by default would save one extra click each time you need an exception to permanent, and if you find yourself needing to do this frequently, it could save quite a bit of time.

Comment 15

[Johnathan Nightingale [:johnath]]

Okay, I'm coming around to Bob's way of thinking, I think.

If the attack that we're worried about with bad SSL certs is a man in the middle attack - that the cert is deceptively attempting to reroute an SSL connection from the legitimate site to an attacker - then it hardly matters what default we choose.  That kind of attack has a very short lifetime anyhow, and whatever convinced the user to add the exception once, it's hard to believe that they would do something different the second or third or fifth time (indeed, they'd just get frustrated and make it permanent, most likely).

Exceptions are bound to host+port, so the other obvious attack that going temporary would mitigate - getting users to trust a cert for a benign site that also allows spoofing of other sites - is a non-issue.

On the other hand, Bob's not wrong when he says that there's a real possibility for dialog/error page fatigue.  Ideally a user with regular, habitual browsing behaviour will go months without seeing one of these pages, because the pages they trust either have legitimate, signed certs, or because they added one exception long ago.  It won't help that person be secure to have these pages showing up more often. 

Supporting temp exceptions is important, and if we wanted to pref-control it, so be it, but I'd be okay with leaving it as-is, on further consideration.