1269253 - Blank error page in iframe with insecure connection

Status: VERIFIED FIXED

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

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160429030215
STR:
1. Open   data:text/html,<iframe sandbox src="https://wrong.host.badssl.com/" height="500"></iframe>

AR:  Blank error page in iframe
ER:  Error page should contain sensible information

This is regression from bug 1240594. Regression range:
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=04e4fbe0584e4c8d4cfb773875d60b74eeec7543&tochange=6c75ca57566bae6b9586fd843e90616f919c666f

Comment 1

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Functional regression in 48.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Nihath, can you take a look?

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

I investigated this for a while, here are my findings:

- Only iframes that have the sandbox attribute are affected
- This blocks scripts in the page from running, which means that initPage is never called, and the AboutNetErrorLoad event is never fired (I confirmed this with some good old printf debugging).
- about:neterror is affected by this in current release (46) - it's blank
- about:certerror was *not* affected for some reason. I suspect it's whitelisted somewhere in platform.

I don't know why about:certerror worked correctly before the merge landed. Gijs, can you recommend someone from platform to ping about this? Or if there's some flaw in my understanding, please let me know.

Comment 4

[:Gijs (he/him)]

(In reply to Nihanth Subramanya [:nhnt11] from comment #3)
> I investigated this for a while, here are my findings:
> 
> - Only iframes that have the sandbox attribute are affected
> - This blocks scripts in the page from running, which means that initPage is
> never called, and the AboutNetErrorLoad event is never fired (I confirmed
> this with some good old printf debugging).
> - about:neterror is affected by this in current release (46) - it's blank
> - about:certerror was *not* affected for some reason. I suspect it's
> whitelisted somewhere in platform.
> 
> I don't know why about:certerror worked correctly before the merge landed.
> Gijs, can you recommend someone from platform to ping about this? Or if
> there's some flaw in my understanding, please let me know.

For sandboxing and CSP stuff, Christoph is my go-to person... can you take a quick look?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch from comment #4)
> For sandboxing and CSP stuff, Christoph is my go-to person... can you take a
> quick look?

Actually Bob used to work on iframe sandboxing, he is probably the better person to ask for this. Bob, any ideas?

Comment 6

[Bob Owen (:bobowen)]

I'm afraid I'm not so familiar with the script blocking part of the implementation and I don't know why about:certerror used to load differently from about:neterror.

The sandbox flags are generally set at (or soon after) document (and docshell) creation, so I can only suggest backing out the changes locally and debugging those parts.

It will probably be a call to SetSandboxFlags in either case.

Comment 7

[:Gijs (he/him)]

(In reply to Bob Owen (:bobowen) from comment #6)
> The sandbox flags are generally set at (or soon after) document (and
> docshell) creation, so I can only suggest backing out the changes locally
> and debugging those parts.
> 
> It will probably be a call to SetSandboxFlags in either case.

I looked at this a bit in DXR and couldn't work out where any special flags get set for either page. The about: pages themselves have the same about: URI flags, so not sure how to proceed here. Christoph said he could take another look if nobody at frontend has time/knowledge to dive in, which I think is the case.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

I am happy to debug more, but I suppose I need a little guidance from someone who knows this code. The results from the 3 testcases we created underneath are [0=works, 1=fails, 2=works]. Setting |sandbox="allow-scripts"| within testcase [2] makes the test work again which indicates that the sandbox flags get applied incorrectly when loading "chrome://browser/content/aboutNetError.xhtml".

I am confused, because within AboutRedirector [a] we are setting 'nsIAboutModule::ALLOW_SCRIPT' which should (in my opinion) allow the script to run. So somewhere, the sandbox flags and nsIAboutModule::ALLOW_SCRIPT collide.

Maybe Bobby knows, because he is familiar with the AboutRedirector code. Bobby, any ideas?

[0] data:text/html,<iframe src="https://wrong.host.badssl.com/" height="500"></iframe>
[1] data:text/html,<iframe sandbox src="https://wrong.host.badssl.com/" height="500"></iframe>
[2] data:text/html,<iframe sandbox="allow-scripts" src="https://wrong.host.badssl.com/" height="500"></iframe>

[a] http://mxr.mozilla.org/mozilla-central/source/browser/components/about/AboutRedirector.cpp#50

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Big question is: How come this ever worked before? Could our packaging mechanism be responsible for it? In my opinion testcase [1] from comment 8 should never have worked - why did it?

Comment 10

[Bobby Holley (:bholley)]

I'm not really sure what the specific issue is here, but nsIAboutModule::ALLOW_SCRIPT is used to determine whether the principal is immune to the script policy here:

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSRuntime.cpp#368

This is relevant because nsDocShell::RecomputeCanExecuteScripts() calls  scriptability.SetDocShellAllowsScript(mCanExecuteScripts), which is how it propagates the script policy to XPConnect. If the principal is immune,  SetDocShellAllowsScript has no effect.

I'm not sure if our CSP script execution stuff goes through that docshell path or not, but if it doesn't it probably should.

That's about all I can say off the top of my head without digging deeper.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Thanks Bobby! Your assumption seems to be correct and I can confirm the following for the two testcases:

> data:text/html,<iframe src="https://wrong.host.badssl.com/" height="500"></iframe>
PrincipalURI within PrincipalImmuneToScriptPolicy is:
about:certerror?e=nssBadCert&u=https%3A//wrong.host.badssl.com

> data:text/html,<iframe sandbox src="https://wrong.host.badssl.com/" height="500"></iframe>
PrincipalURI within PrincipalImmuneToScriptPolicy is:
moz-nullprincipal:{56e20116-a4ba-418a-9e1c-041caef5276f}


which seems correct, because we load using a NullPrincipal when the load is sandboxed. However, that doesn't still provide any information why the 'sandboxed' test used to work before we merged certerror and neterror. Again, that shouldn't have worked before.

Not sure where to go from here in terms of debugging this problem.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Potentially certerror was in the list of exceptions here:
[1] http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#1721

Comment 13

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> Potentially certerror was in the list of exceptions here:
> [1]
> http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.
> cpp#1721

I don't see us adding things to that list though... maybe I'm missing it? Bobby seems to have written the API there, so maybe he knows? :-)

Either way, I think the confusion is also about whether we want this to work.

There is currently no reasonable way to make the network and cert error pages work without any scripting. So IMO we should be changing both error pages to work in sandboxed iframes. I don't know how hard that is, though...

Comment 14

[Bobby Holley (:bholley)]

(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> > Potentially certerror was in the list of exceptions here:
> > [1]
> > http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.
> > cpp#1721
> 
> I don't see us adding things to that list though... maybe I'm missing it?
> Bobby seems to have written the API there, so maybe he knows? :-)

The stuff in nsScriptSecurityManager is about nsIDomainPolicy, which is an API to allow addons like NoScript to configure whitelists/blacklists of domains. We shouldn't be using it directly from Gecko to set script policy, and I highly doubt we are.

There are basically 3 ways that you can disable JS execution in a global:
(1) Use nsIDomainPolicy
(2) Disable it on the docshell
(3) Call BlockScriptForGlobal

Which of these mechanisms is CSP using to block script?

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Bobby Holley (busy) from comment #14)
> There are basically 3 ways that you can disable JS execution in a global:
> (1) Use nsIDomainPolicy
> (2) Disable it on the docshell
> (3) Call BlockScriptForGlobal
> 
> Which of these mechanisms is CSP using to block script?

Just for clarification, there is no CSP involved within this bug. But in case you are curious, CSP implements nsIContentPolicy to block any kind of network loads including JS.

Comment 16

[Bobby Holley (:bholley)]

Sorry, I meant iframe sandbox without allow-scripts. Presumably that doesn't operate at the network level, because of inline <script> tags.

Comment 17

[:Gijs (he/him)]

Looks like script disabling for sandboxing happens here:

https://dxr.mozilla.org/mozilla-central/rev/c4449eab07d39e20ea315603f1b1863eeed7dcfe/dom/base/nsDocument.cpp#8466-8473

Comment 18

[Bobby Holley (:bholley)]

Oh ok - I guess that's an entirely separate mechanism. Kinda redundant and fragile, but I guess that's the price of following the spec to the letter.

Comment 19

[:Gijs (he/him)]

Christoph, any chance you can take this further? Over in bug 1222804, :bz said it should be easy to just add an exception for the sandboxing to error page documents... :-)

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Sylveste, Ritu, this bug is marked tracking for 48 and 49. It's also marked as a P3. I just chatted with Gijs and we both agree that this bug can wait a little longer to be fixed. Mostly because parts of what the bug describes has been a blank error page for quite some time anyway, so we suppose it could wait a little longer. Are you fine with updating the flags to wontfixes for 48 and 49.

I'll have a look after the allhands.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #20)
> Sylveste, Ritu, this bug is marked tracking for 48 and 49. It's also marked
> as a P3. I just chatted with Gijs and we both agree that this bug can wait a
> little longer to be fixed. Mostly because parts of what the bug describes
> has been a blank error page for quite some time anyway, so we suppose it
> could wait a little longer. Are you fine with updating the flags to
> wontfixes for 48 and 49.
> 
> I'll have a look after the allhands.

Hi Christoph, I'll let Sylvestre decide on whether that is OK or not. We might be ok wontfixing assuming some of this was already broken for a while and this is not a new regression. However, we still have 5 weeks of Beta48 cycle before RC48 week so we should try fixing it in 48 if possible.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

OK, let's wontfix it in 48, don't hesitate to propose an uplift for 49.

Comment 23

[Gerry Chang [:gchang]]

Hi :ckerschb,
May I know is there any findings for this bug?

Comment 24

[Dimi Lee [:dimi]]

Since Christoph is busy, I will work on this bug.

Comment 25

[Dimi Lee [:dimi]]

Hi Christoph,
I found the reason why 'certerror' works before is related to how aboutCertError.xhtml is implemented.
Variables(title, descriptions ...etc) in aboutCertError have initial values, so even we remove all the scripts from aboutCertError.xhtml, the page can still show some information.

However after we merge certerror and neterror, aboutNetError.xhtml requires script to run to
show information, for example,

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/aboutNetError.xhtml#497
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/aboutNetError.xhtml#576

So it will show blank error page when we include the sandbox attribute.
If we use [1] in the old build, it will also show blank page because it is aboutNetError.xhtml

Does this sound reasonable to you?
Sorry I am not familiar with frontend, so not sure what is the best way to fix this, could you provide some suggestion ?
Thanks!

[1]data:text/html,<iframe sandbox src="http://www.mozillamozilla.com" width="500" height="500"></iframe>

Comment 26

[:Gijs (he/him)]

(In reply to Dimi Lee[:dimi][:dlee] from comment #25)
> Hi Christoph,
> I found the reason why 'certerror' works before is related to how
> aboutCertError.xhtml is implemented.
> Variables(title, descriptions ...etc) in aboutCertError have initial values,
> so even we remove all the scripts from aboutCertError.xhtml, the page can
> still show some information.
> 
> However after we merge certerror and neterror, aboutNetError.xhtml requires
> script to run to
> show information, for example,
> 
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/
> aboutNetError.xhtml#497
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/
> aboutNetError.xhtml#576
> 
> So it will show blank error page when we include the sandbox attribute.
> If we use [1] in the old build, it will also show blank page because it is
> aboutNetError.xhtml
> 
> Does this sound reasonable to you?

This makes sense.

> Sorry I am not familiar with frontend, so not sure what is the best way to
> fix this, could you provide some suggestion ?

See comment #19. I think we need to add an exception to the sandboxing so we run code for these error documents (about:neterror, about:certerror, about:blocked) irrespective of whether the iframe is sandboxed).

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

Hey Dimi, unfortunately I am not the right person to ask because I simply don't know.

Gijs, Nihanth, can you maybe provide some pointers for Dimi's questions within Comment 25?

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #27)
> Hey Dimi, unfortunately I am not the right person to ask because I simply
> don't know.
> 
> Gijs, Nihanth, can you maybe provide some pointers for Dimi's questions
> within Comment 25?

I suppose Gijs was faster than me :-)

Comment 29

[Dimi Lee [:dimi]]

(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #26)
> 
> See comment #19. I think we need to add an exception to the sandboxing so we
> run code for these error documents (about:neterror, about:certerror,
> about:blocked) irrespective of whether the iframe is sandboxed).

Oh, I missed that comment,
I will work on it!

Comment 30

[Dimi Lee [:dimi]]

Attachment: Do not apply docshell's sandboxing flags to error pages

Hi smaug,
Can you help review this patch ?
The sandbox attribute of iframe make error page(about:neterror, about:certerror ...etc ) becomes blank.

I am trying to fix this according to bz's suggestion in bug 1222804 comment 6
Thanks!

Comment 31

[Boris Zbarsky [:bzbarsky]]

Are we sure we can't race and get a StartDocumentLoad at a point when the docshell has started another load with a different loadtype?  I would much rather we had an "error page load" flag on nsILoadInfo or something...

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

yeah, it is rather scary to have nsDocument::IsErrorPage which returns whatever value based on docshell's state. 
Adding flag to LoadInfo sounds reasonable(, even though I'd prefer to keep that interface simple and not add more and more flags).

Comment 33

[Dimi Lee [:dimi]]

Attachment: Do not apply docshell's sandboxing flags to error pages. v2

bz, smaug,
Thanks for suggestion! Upload another patch for review according to the comments

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8780972 [details] [diff] [review]
Do not apply docshell's sandboxing flags to error pages. v2

>+++ b/docshell/base/nsDocShell.cpp
>@@ -10801,16 +10801,20 @@ nsDocShell::DoURILoad(nsIURI* aURI,
>   if (isSandBoxed) {
>     securityFlags |= nsILoadInfo::SEC_SANDBOXED;
>   }
> 
>   if (UsePrivateBrowsing()) {
>     securityFlags |= nsILoadInfo::SEC_FORCE_PRIVATE_BROWSING;
>   }
> 
>+  if (mLoadType == LOAD_ERROR_PAGE) {
>+    securityFlags |= nsILoadInfo::SEC_LOAD_ERROR_PAGE;
>+  }
Could you move nsLoadFlags loadFlags = mDefaultLoadFlags; and the two 'if's after that (currently higher up in this same method) somewhere here, and then have just one 
if (mLoadType == LOAD_ERROR_PAGE) {, in which loadFlags and securityFlags are both set.

Comment 35

[Dimi Lee [:dimi]]

Attachment: (r+) Do not apply docshell's sandboxing flags to error pages.

Address comment 34

Comment 36

[Dimi Lee [:dimi]]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6c80da849b4

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34766b5100b9
Blank error page in iframe with insecure connection. r=smaug

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/34766b5100b9

Comment 39

[Paul Silaghi, QA [:pauly]]

I'm not able to:
1. accept the security exception in the iframe
2. see the error code expanded
Thoughts?

Comment 40

[Dimi Lee [:dimi]]

Hi Paul,
I can see the error code by using the following link,(see attachment for screenshot)
data:text/html,<iframe sandbox src="https://wrong.host.badssl.com/" height="500"></iframe>

and also "Go Back" buttons works(Is this what you mean accept the security exception?).

Could you let me know what is your reproduce step ? thanks!

Comment 41

[Dimi Lee [:dimi]]

Attachment: ErrorPage.png

Comment 42

[Paul Silaghi, QA [:pauly]]

Hi Dimi,
If I open https://wrong.host.badssl.com/ in a normal page, I see an "Add exception" button which allows me to accept the security exception. Moreover, clicking on the blue cert error code expands below the cert error details.
Opening data:text/html,<iframe sandbox src="https://wrong.host.badssl.com/" height="500"></iframe> I see there is no "Add exception" button, also clicking on the cert error code displays 2 buttons with "copy text to clipboard" instead of expanding the cert error details - see http://imgur.com/a/p5BRq.

Comment 43

[Dimi Lee [:dimi]]

(In reply to Paul Silaghi, QA [:pauly] from comment #42)

Thanks for the information!
Just a quick update that this issue isn't related to sandbox flag of iframe, i can reproduce with
<iframe src="https://wrong.host.badssl.com/" height="500"></iframe>

I will check more detail tomorrow

Comment 44

[Dimi Lee [:dimi]]

The problem is when we load the url by 
data:text/html,<iframe src="https://wrong.host.badssl.com/" height="500"></iframe>

the document.content in content.js isn't aboutNetError.xhtml, so things like content.document.documentURI[1], content.document.getElementById("certificateErrorText");[2] won't work.

Hi Paul,
I think we should create another bug for this because this is not what we intend to fix in this bug.

Hi Gijs,
Do you know anyone familiar with front-end code could help check this ? Thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#309
[2] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#326

Comment 45

[Paul Silaghi, QA [:pauly]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #44)
> Hi Paul,
> I think we should create another bug for this because this is not what we
> intend to fix in this bug.
Filed bug 1297630

Comment 46

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 51.0a1 (2016-08-23) Win 7, Ubuntu 14.04, OS X 10.12.

Comment 47

[Gerry Chang [:gchang]]

Hi :dimi,
Since this bug is a regression and also affects 49/50, are you also considering to uplift this patch to 49/50 if this patch is not too risky?

Comment 48

[Dimi Lee [:dimi]]

Comment on attachment 8781329 [details] [diff] [review]
(r+) Do not apply docshell's sandboxing flags to error pages.

Approval Request Comment
[Feature/regressing bug #]: 1240594
[User impact if declined]: when insecure connection occurs in iframe, user will see blank error page.
[Describe test coverage new/current, TreeHerder]: tests in central
[Risks and why]: Rish should be low, there is two changes in this patch:
1. Move reading default load flags earlier, this is code refactor and it doesn't change anything
2. Make error page don't inherit sandbox flags, so normal web pages won't be affected by this patch
[String/UUID change made/needed]: none

Comment 49

[Dimi Lee [:dimi]]

Comment on attachment 8781329 [details] [diff] [review]
(r+) Do not apply docshell's sandboxing flags to error pages.

see Comment 48

Comment 50

[Liz Henry (:lizzard) (relman/hg->git project)]

It is too late for 49 at this point for me to feel comfortable, with docshell changes, unless it is critical, I would rather see this have some bake time. But I am super happy to see this fixed and verified in 51.  Ritu I leave  the aurora uplift decision to you. There is a follow up bug as well!

Comment 51

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8781329 [details] [diff] [review]
(r+) Do not apply docshell's sandboxing flags to error pages.

We are heading into beta 8 now with the RC build in 1 week. Too late for a possibly risky change in not well understood part of the code base.

Comment 52

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8781329 [details] [diff] [review]
(r+) Do not apply docshell's sandboxing flags to error pages.

The fix was verified on Nightly, recent regression since 48, Aurora50+

Comment 53

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c7673e35bbec

Comment 54

[Andrei Vaida [:avaida]]

Paul, could you please check this fix on Fx50 as well?

Comment 55

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 50b1, Win 7.