Closed Bug 1269253 Opened 4 years ago Closed 3 years ago

Blank error page in iframe with insecure connection

Categories

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

48 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla51
Iteration:
51.2 - Aug 29
Tracking Status
firefox47 --- unaffected
firefox48 + wontfix
firefox49 + wontfix
firefox50 --- verified
firefox51 --- verified

People

(Reporter: arni2033, Assigned: dimi)

References

Details

(Keywords: regression, Whiteboard: [fxprivacy] [domsecurity-active])

Attachments

(2 files, 2 obsolete files)

>>>   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
[Tracking Requested - why for this release]:
Functional regression in 48.
Nihath, can you take a look?
Flags: needinfo?(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.
Flags: needinfo?(nhnt11) → needinfo?(gijskruitbosch+bugs)
(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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(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?
Flags: needinfo?(ckerschb) → needinfo?(bobowen.code)
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.
Flags: needinfo?(bobowen.code)
Whiteboard: [fxprivacy]
(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.
Flags: needinfo?(ckerschb)
Whiteboard: [fxprivacy] → [fxprivacy] [triage]
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
Flags: needinfo?(ckerschb) → needinfo?(bobbyholley)
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?
Priority: -- → P3
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
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.
Flags: needinfo?(bobbyholley)
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.
(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...
(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?
(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.
Sorry, I meant iframe sandbox without allow-scripts. Presumably that doesn't operate at the network level, because of inline <script> tags.
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.
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... :-)
Flags: needinfo?(ckerschb)
See Also: → 1222804
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.
Flags: needinfo?(sledru)
Flags: needinfo?(rkothari)
Flags: needinfo?(ckerschb)
(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.
Flags: needinfo?(rkothari)
OK, let's wontfix it in 48, don't hesitate to propose an uplift for 49.
Flags: needinfo?(sledru)
Version: unspecified → 48 Branch
Hi :ckerschb,
May I know is there any findings for this bug?
Flags: needinfo?(ckerschb)
Component: General → DOM: Security
Product: Firefox → Core
Priority: P3 → P2
Whiteboard: [fxprivacy] → [fxprivacy] [domsecurity-active]
Since Christoph is busy, I will work on this bug.
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Flags: needinfo?(ckerschb)
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>
Flags: needinfo?(ckerschb)
(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).
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?
Flags: needinfo?(nhnt11)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(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 :-)
Flags: needinfo?(nhnt11)
Flags: needinfo?(gijskruitbosch+bugs)
(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!
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!
Attachment #8780521 - Flags: review?(bugs)
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...
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).
Attachment #8780521 - Flags: review?(bugs) → review-
bz, smaug,
Thanks for suggestion! Upload another patch for review according to the comments
Attachment #8780521 - Attachment is obsolete: true
Attachment #8780972 - Flags: review?(bugs)
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.
Attachment #8780972 - Flags: review?(bugs) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34766b5100b9
Blank error page in iframe with insecure connection. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/34766b5100b9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
I'm not able to:
1. accept the security exception in the iframe
2. see the error code expanded
Thoughts?
Flags: needinfo?(dlee)
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!
Flags: needinfo?(dlee) → needinfo?(paul.silaghi)
Attached image ErrorPage.png
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.
Flags: needinfo?(paul.silaghi) → needinfo?(dlee)
(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
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
Flags: needinfo?(paul.silaghi)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(dlee)
(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
Flags: needinfo?(paul.silaghi)
Verified fixed FX 51.0a1 (2016-08-23) Win 7, Ubuntu 14.04, OS X 10.12.
Status: RESOLVED → VERIFIED
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?
Flags: needinfo?(dlee)
Flags: needinfo?(gijskruitbosch+bugs)
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
Flags: needinfo?(dlee)
Attachment #8781329 - Flags: approval-mozilla-aurora?
Comment on attachment 8781329 [details] [diff] [review]
(r+) Do not apply docshell's sandboxing flags to error pages.

see Comment 48
Attachment #8781329 - Flags: approval-mozilla-beta?
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 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.
Attachment #8781329 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
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+
Attachment #8781329 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Iteration: --- → 51.2 - Aug 29
Paul, could you please check this fix on Fx50 as well?
Flags: needinfo?(paul.silaghi)
Verified fixed FX 50b1, Win 7.
Flags: needinfo?(paul.silaghi)
You need to log in before you can comment on or make changes to this bug.