87209 - Unrecognized certificate authority dialog presented twice upon cancel

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P3)

Description

[Stephane Saux]

Visit above url. Presented with the dialog, click cancel. The dialog is
presented again. Clicking cancel again sets the url box to above URL but results
in blank page.

Choosing "do not accept this cert... do not connect..." radio button then "ok"
exhibits exactly the same behavior.

Expected behavior in both these instances is to stay on the current page.

Same faulty behavior with expired server certs:
https://bolohead.mcom.com:23168/ciphers.html

May warrant two bugs, but the behavior is so strickingly similar, I prefer to
lump them together.

Comment 1

[Stephane Saux]

P2
target 2.1

Comment 2

[John Unruh]

*** Bug 75594 has been marked as a duplicate of this bug. ***

Comment 3

[Boris Zbarsky [:bzbarsky]]

setting a URL that's accessible outside the firewall.  Old url:
https://junruh.mcom.com:6050/ciphers.html

Comment 4

[Stephane Saux]

P1

Comment 5

[Bob Lord]

Why is there a cancel button here at all?  Isn't the third option ("Do not
accept this certificate and do not connect to this web site") the same thing? 
If it's the same thing (which I'll bet it is since selecting this third option
makes you go through that window two times, just like "cancel") we should delete it.

Also, I don't know if I'd make this a P1 bug.  This is an exception case (where
the CA isn't known), and it's the least chosen of the three selections.  I'd bet
most of the time people click the button to accept the cert just for this session.
I'd probably put this at a P3 (or later it) myself unless people know of real
deployments where this bug would produce problems.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I would say that cancel is there so people who just want to abort the action
they just took that caused the dialog to appear don't have to read the whole
thing and then pick the right option just to get the thing to go away...

Comment 7

[Håkan Waara]

Seeing how you are arguing about the UI, I thought it might be good to know 
that I am redesigning this dialog according to a mpt-spec: bug 91466

Comment 8

[John Unruh]

Mass assigning QA to ckritzer.

Comment 9

[Boris Zbarsky [:bzbarsky]]

*** Bug 94389 has been marked as a duplicate of this bug. ***

Comment 10

[Boris Zbarsky [:bzbarsky]]

moving keyword from bug 94389

Comment 11

[SudheerAgrawal]

This problem occurs only if user selects both "Enable SSL Version 3"  
and "Enable TLS" options under preferences->privacy and security>SSL. If you 
select either "Enable SSL Version 3" or "Enable TLS" option this problem goes 
away. 

Comment 12

[SudheerAgrawal]

Attachment: Handling "Cancel" option

Comment 13

[SudheerAgrawal]

Problem is when user selects the cancel button mozilla thinks that server does 
not conform to SSL specs and it will try TLS. I attached a fix for this. This 
patch will also fix problem when user selects "Do not accept this cert" option 
(bugscape bug: 8209)

Comment 14

[Conrad Carlen (not reading bugmail)]

Attachment: The above whole file as a patch

Comment 15

[Conrad Carlen (not reading bugmail)]

Sudheer, I assume the file you posted was from MOZILLA_0_9_2_BRANCH? Here it is
in patch form.

Comment 16

[Conrad Carlen (not reading bugmail)]

I tested the patch here against both 0.9.2 and the current (as of this morning)
trunk. It works. I think some more work should be done here though. The problem
is a method which just returns a boolean for success or failure. If the user
cancels, all it can do is return PR_FALSE and the caller has no way of knowing
what happened. What we should do is make the method return an nsresult, return
NS_ERROR_USER_CANCELED (which may not exist but needs to), and then the caling
code would know not to try again.

Comment 17

[Stephane Saux]

reassigning to kai to follow through.
->kai

Comment 18

[Glen]

looks like we have the patch, can it get on the 9_2 branch ?

Comment 19

[(Don't use, old account) - Kai Engert]

Although this patch seems to work, it is not yet perfect, but introduces other
regressions.

For example, if you are connecting to an unknown CA, the user will never get
informed about any SSL errors that might appear during the session.

In addition, if a site is indeed not TLS tolerant, this patch will stop the
Browser from automatically retrying with older SSL.

I vote against taking this patch. I agree with solution suggested by Conrad,
hence, this patch has not yet been written.

I can't decide whether this patch should go into the 0.9.2 branch or not, but
for the trunk, I'd prefer Conrad's suggestion.

Removing patch and review keywords.

Comment 20

[SudheerAgrawal]

The code in this patch runs only when user is presented a dialog box. This 
patch keep track of the user's action. If user selects "Cancel" option it would 
not let mozilla try on TLS/SSL even if there are errors ( this dialog box 
should be shown only if SSL/TLS  connection is OK) because user does want to 
continue. SSL/TLS errors will occurs before presenting this dialog box. If 
server is not SSL compliant you won't the see the dialog box anyway and than 
mozilla will try the TLS( or will follow the same logic whatever it does now). 
In my opinion if user selects cancel you do not need to try TLS or old SSL. 
However if you are presenting a dialog box while there are known SSL/TLS 
problem that would be another mozilla bug. This patch seems to work fine for me.

Comment 21

[(Don't use, old account) - Kai Engert]

1) You are right regarding TLS/SSL. The error message is not displayed until
both sides have established a working connection, therefore your bug doesn't
prevent fallback to the old protocol, I was wrong.

2) I want to talk about the data transfer that happens after the handshake is
finished.

If the handshake is finished, but later during the transfer an error occurs, the
browser should show an error message.

With your patch you completely disable error message reporting to the user, once
you have displayed the unknown CA message.

3) I found a different problem. Go to https://kuix.de/ca . This has two
problems. First, you will normally not have the CA for the used certificate.
Second, the certificate doesn't match the host name.

While this page loads fine without your patch, it doesn't work with your patch!
I have not yet understood why this happens.

Comment 22

[Marek Z. Jeziorek]

what's the current status of this work? will it land anytime soon?

Comment 23

[(Don't use, old account) - Kai Engert]

I made a quick peek into this. As I added some state about "the last reported
error to the user" in a patch to bug 96024, I thought I could add some few lines
that would fix this bug.

However, it seems that some parts of Mozilla initiate a complete retry of the
transaction. And in that case I don't have an obvious instance avaialble, where
the state "already warned" could be added.

It seems that the http protocol layer it initiating the retry. This is the real
error we should prevent, maybe by returning a better return code.

I'm attaching a log.

There are open PSM bugs with higher priority, not sure when this will land.

Comment 24

[(Don't use, old account) - Kai Engert]

Attachment: Logfile

Comment 25

[(Don't use, old account) - Kai Engert]

CC'ing Darin. Could this be a http protocol bug, or should PSM fix this?

Comment 26

[Darin Fisher]

looks like the first attempt to read from the socket results in an immediate
EOF.  this tells HTTP that the server closed the connection, and the request is
redone on a new connection.  perhaps PSM needs to cache the fact that it has
already posted the CA dialog b/c there is no way to avoid the possibility of a
premature EOF.

Comment 27

[Stephane Saux]

Move to future. Won't have time to fix these for 2.1

Comment 28

[Michael Dunn]

Removing Topembed Keyword

Comment 29

[(Don't use, old account) - Kai Engert]

Changing my prefered e-mail address.

Comment 30

[John Unruh]

The behavior is still present in the 10/5 branch build.

Comment 31

[John Unruh]

*** Bug 114721 has been marked as a duplicate of this bug. ***

Comment 32

[Jeremy M. Dolan]

Queried for 'certificate cancel' before I filed a dupe. Fixing summary.

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

This is a consequence of the problem reported in bug 149910.
When a TLS handshake attempt fails, in almost all cases, PSM treats it
as a TLS intolerant server, and forces necko to retry using SSL3 
instead of TLS by returning an EOF to the first read.  

The proper solution to this problem is NOT (IMO) to "remember" that
the user elected to not accept this certificate when PSM retries 
with SSL3.  Rather, the proper solution is to NOT retry with SSL3
when the error is one that we know cannot be fixed by switching 
from TLS to ssl3.  

I think once bug 149910 is fixed, this bug 87209 will simply go away.
There are other, similar, bugs that will go away then, too.

Comment 34

[Kai Engert [:KaiE:]]

I want to fix this together with the patch in bug 87902 (do not confuse numbers,
this bug has a very similar number :-)

Comment 35

[Kai Engert [:KaiE:]]

Fixed on trunk with patch for bug 87902.

Comment 36

[John Unruh]

Verified.