423247 - Certificate error for iframe is shown as a dialog instead of an error page, making it impossible to add an exception

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P2)

Description

[Pedro Soria-Rodriguez]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4) Gecko/2008030318 Firefox/3.0b4
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4) Gecko/2008030318 Firefox/3.0b4

Using Ffox 3, beta4 on Linux

Ffox 3 normally displays a warning when the SSL cert of the server cannot be verified or trusted.  This warning includes the option to add an exception.

The bug I am reporting is that the option to add an exception is not shown in a particular case.

Reproducible: Always

Steps to Reproduce:
1. Go to www.renfe.es (spanish ralways)
2. There's a pull down menu with the text "Principales Estaciones Origen" (departure train stations).   In that menu, select any entry (for instance, "Madrid")
3. Then a warning dialog window comes up, with the following message:
     w1.renfe.es:443 uses an invalid security certificate.
     The certificate is not trusted because the issuer certificate is not trusted.
     (Error code: sec_error_untrusted_issuer)

Actual Results:  
A warning about sec_error_untrusted_issuer is shown, without option to add an exception.  User is locked out from using the website (destination train stations do not get to load up)   

Expected Results:  
Expected: a warning about sec_error_untrusted_issuer, with an option to add an exception.   The destination train station list should load after selecting the departure train station.  User should be able add exception, and continue to use the website.


I will attach two screen shows:  one with expected results (from another webpage) and one showing the bug.

I believe the problem may be at the website, rather than Firefox.  On this webpage (www.renfe.es), the website seems to load a page through SSL on the background, while the main page is not SSL-protected.   I wonder if this case is not treated by FFox3 as expected.   On other websites where sec_error_untrusted_issuer  happens, the "Secure Connection Failed" message comes with the option to add an exception.

Comment 1

[Pedro Soria-Rodriguez]

Attachment: Expected error message related to sec_error_untrusted_issuer

Comment 2

[Pedro Soria-Rodriguez]

Attachment: Pop-up dialog with sec_error_untrusted_issuer, without "add exception"

Comment 3

[Jesse Ruderman]

Here's a reduced teestcase:

data:text/html,<iframe src="https://w1.renfe.es/"></iframe>

I think Firefox should show an error page inside the iframe, since that's what happens if the server is simply unreachable.  Is there a reason it shows a dialog, or is that just a bug?

Comment 4

[Jo Hermans]

dupe of bug 399876 (where Kai is already working on) ?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Yes, though that bug's summary is less specific than this one. I think we should probably separate the "warn for blocked loads due to invalid cert errors" and "display error pages in subframes" issues that have been conflated in that bug. Perhaps this bug can cover landing attachment 284998 [details] [diff] [review].

Comment 6

[Boris Zbarsky [:bzbarsky]]

That attachment should just get checked in, imo.  I'm not sure why it hasn't been, exactly...

Comment 7

[Kai Engert (:KaiE:)]

(In reply to comment #6)
> That attachment should just get checked in, imo.  I'm not sure why it hasn't
> been, exactly...


Because we had agreed that complete silent is bad, because it makes it imppossible to diagnose what's failing.

It has been proposed to log an error to the console, but until now we have no patch to do that.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

I think not being able to see what's wrong on pages that use hidden iframes is better than getting an alert and not being able to add exceptions for pages that use visible frames, so preventing that patch from landing because we haven't figured out how to do error console reporting seems like a net loss.

Comment 9

[Johnathan Nightingale [:johnath]]

Attachment: Kai's patch from 399876, applies cleanly

(In reply to comment #7)
> (In reply to comment #6)
> > That attachment should just get checked in, imo.  I'm not sure why it hasn't
> > been, exactly...
> 
> Because we had agreed that complete silent is bad, because it makes it
> imppossible to diagnose what's failing.
> 
> It has been proposed to log an error to the console, but until now we have no
> patch to do that.

Yeah, but I wouldn't want the user experience to suffer for that, if we expect that to be difficult, and bug 399876 comment 19 makes it sound like logging in a consistently helpful way might be.

(In reply to comment #8)
> I think not being able to see what's wrong on pages that use hidden iframes is
> better than getting an alert and not being able to add exceptions for pages
> that use visible frames, so preventing that patch from landing because we
> haven't figured out how to do error console reporting seems like a net loss.

And I think there's no reason not to keep that bug open to solve the more general problem (including logging!) while at the same time using this bug to fix the specific problem of frames.

To that end, I've attached your patch from 399876 over here, and will give bz a review request, since the other one had no official review.

Kai - you indicated in bug 399876 comment 20 that you felt basically okay with landing this patch, though you pointed out that the work was not all done.  Are you okay with this approach that has us landing the patch here, and keeping that bug alive to track the question of other resource types, and logging?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Because we had agreed that complete silent is bad

For subresource errors?  We had?  I must have missed it....  It's not ideal, but it's better than the status quo, in my opinion.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 309989 [details] [diff] [review]
Kai's patch from 399876, applies cleanly

>Index: security/manager/ssl/src/nsNSSIOLayer.cpp
>+  nsCOMPtr<nsIDocShellTreeItem> rootItem;

That doesn't need to be declared in this outermost scope, right?  Please push the decl down to where it's used.

>+    if (rootItem) 

It won't be null.  I wouldn't bother checking.  Especially since do_QI is null-safe, but even if it weren't, a treeitem always has a root item of same type.

With those two nits fixed, r=bzbarsky.

Comment 12

[Kai Engert (:KaiE:)]

(In reply to comment #9)
> Kai - you indicated in bug 399876 comment 20 that you felt basically okay with
> landing this patch, though you pointed out that the work was not all done.  Are
> you okay with this approach that has us landing the patch here, and keeping
> that bug alive to track the question of other resource types, and logging?

Ok, thanks.

Comment 13

[Kai Engert (:KaiE:)]

(In reply to comment #10)
> > Because we had agreed that complete silent is bad
> 
> For subresource errors?  We had?  I must have missed it....  It's not ideal,
> but it's better than the status quo, in my opinion.


bug 399876 comment 8, your second comment. you propose to log to the error console.  And in bug 399876 comment 21 and 23 you tried to assist me to get the logging done.

But let's not nit pick on who said what. I hope we can agree that logging detected errors for diagnosing is a good thing, and that we should do it if we can do it. And I'm fine to postpone it to a later time, as Gavin and Johnathan proposed.

Comment 14

[Johnathan Nightingale [:johnath]]

Attachment: Address review comments

(In reply to comment #11)
> (From update of attachment 309989 [details] [diff] [review])
> >Index: security/manager/ssl/src/nsNSSIOLayer.cpp
> >+  nsCOMPtr<nsIDocShellTreeItem> rootItem;
> 
> That doesn't need to be declared in this outermost scope, right?  Please push
> the decl down to where it's used.

Moved inside the |if (item)| block.

> >+    if (rootItem) 
> 
> It won't be null.  I wouldn't bother checking.  Especially since do_QI is
> null-safe, but even if it weren't, a treeitem always has a root item of same
> type.

Removed the check, and changed the assert text to be slightly more meaningful.

Comment 15

[Johnathan Nightingale [:johnath]]

Comment on attachment 310018 [details] [diff] [review]
Address review comments

In principle, I need review from a PSM peer, and Kai's out since this is really his patch.  Tagging Bob, feel free to punt to WTC if that's a better play.

Comment 16

[Robert Relyea]

Comment on attachment 310018 [details] [diff] [review]
Address review comments

r+ Verified this patch incorporates the review comments from bz.

Comment 17

[Johnathan Nightingale [:johnath]]

Checking in security/manager/ssl/src/nsNSSIOLayer.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp,v  <--  nsNSSIOLayer.cpp
new revision: 1.153; previous revision: 1.152
done

Comment 18

[Jo Hermans]

I tried to verify this bug, but I stumbled on another problem : bug 423833 

Comment 19

[Johnathan Nightingale [:johnath]]

(In reply to comment #18)
> I tried to verify this bug, but I stumbled on another problem : bug 423833 

Yep, that's a bug alright!  :)

In the meantime, we can verify this fix using a url like:

data:text/html,<html><frameset cols="500,500"><frame src="http://google.com"><frame src="https://kuix.de:8443"></frameset></html>