Closed Bug 1496524 Opened 1 year ago Closed 1 year ago

Follow-up to bug 1495983 - add CSP to docshell/resorces/content/netError.xhtml

Categories

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

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla64

People

(Reporter: aceman, Assigned: aceman)

Details

(Whiteboard: [domsecurity-active])

Attachments

(3 files, 4 obsolete files)

On current TB trunk I get a crash just by starting it up.
In the log, it abort on this assertion:

Assertion failure: parsedPolicyStr.Find("default-src") >= 0 (about: page must contain a CSP including default-src), at mozilla/dom/base/nsDocument.cpp:5369

#0  0x00007f1812077f90 in nanosleep () at /lib64/libc.so.6
#1  0x00007f1812077eaa in sleep () at /lib64/libc.so.6
#2  0x00007f1806ff0115 in ah_crap_handler(int) (signum=11) at mozilla/toolkit/xre/nsSigHandlers.cpp:102
#3  0x00007f1806fdff2e in nsProfileLock::FatalSignalHandler(int, siginfo_t*, void*) (signo=11, info=0x7fff872db030, context=0x7fff872daf00) at mozilla/toolkit/profile/nsProfileLock.cpp:187
#4  0x00007f180782c1fd in WasmTrapHandler(int, siginfo_t*, void*) (signum=11, info=0x7fff872db030, context=0x7fff872daf00) at mozilla/js/src/wasm/WasmSignalHandlers.cpp:780
#5  0x00007f18124c9c00 in <signal handler called> () at /lib64/libpthread.so.0
#6  0x00007f1804f55fba in mozilla::StaticAutoPtr<nsTArray<nsTString<char> > >::operator->() const (this=<optimized out>) at mozilla/obj-x86_64-pc-linux-gnu/dist/include/mozilla/StaticPtr.h:70
#7  0x00007f1804f55fba in AssertAboutPageHasCSP(nsIURI*, nsIPrincipal*) (aDocumentURI=<optimized out>, aPrincipal=0x7f17eb948430) at mozilla/dom/base/nsDocument.cpp:5336
#8  0x00007f1804f63999 in nsDocument::EndLoad() (this=this@entry=0x7f17ebaf9000) at mozilla/obj-x86_64-pc-linux-gnu/dist/include/nsCOMPtr.h:842
#9  0x00007f1805a375af in nsHTMLDocument::EndLoad() (this=0x7f17ebaf9000) at mozilla/dom/html/nsHTMLDocument.cpp:844
#10 0x00007f1805f3134e in nsXMLContentSink::DidBuildModel(bool) (this=0x7f17eb93b000, aTerminated=<optimized out>) at mozilla/dom/xml/nsXMLContentSink.cpp:338
#11 0x00007f1804ab561c in nsParser::DidBuildModel(nsresult) (this=0x7f17ebdf6ac0, anErrorCode=<optimized out>) at mozilla/parser/htmlparser/nsParser.cpp:491
#12 0x00007f1804ab5f82 in nsParser::ResumeParse(bool, bool, bool) (this=0x7f17ebdf6ac0, allowIteration=true, aIsFinalChunk=<optimized out>, aCanInterrupt=<optimized out>) at mozilla/parser/htmlparser/nsParser.cpp:1102
#13 0x00007f1804ab60fa in nsParser::ContinueInterruptedParsing() (this=0x7f17ebdf6ac0) at mozilla/parser/htmlparser/nsParser.cpp:647
#14 0x00007f1804ab9606 in nsParserContinueEvent::Run() (this=<optimized out>) at mozilla/obj-x86_64-pc-linux-gnu/dist/include/mozilla/RefPtr.h:335
#15 0x00007f18041963c2 in nsThread::ProcessNextEvent(bool, bool*) (this=0x7f18011175b0, aMayWait=<optimized out>, aResult=0x7fff872dbdaf) at mozilla/xpcom/threads/nsThread.cpp:1231
#16 0x00007f18041904b7 in NS_ProcessNextEvent(nsIThread*, bool) (aThread=aThread@entry=0x7f18011175b0, aMayWait=aMayWait@entry=false) at mozilla/xpcom/threads/nsThreadUtils.cpp:530
#17 0x00007f18046251da in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (this=0x7f1801145740, aDelegate=0x7f180114b040) at mozilla/ipc/glue/MessagePump.cpp:97
#18 0x00007f18045d7373 in MessageLoop::RunInternal() (this=this@entry=0x7f180114b040) at mozilla/obj-x86_64-pc-linux-gnu/dist/include/mozilla/RefPtr.h:335
#19 0x00007f18045d750d in MessageLoop::RunHandler() (this=0x7f180114b040) at mozilla/ipc/chromium/src/base/message_loop.cc:318
#20 0x00007f18045d750d in MessageLoop::Run() (this=0x7f180114b040) at mozilla/ipc/chromium/src/base/message_loop.cc:298
#21 0x00007f18060a916d in nsBaseAppShell::Run() (this=0x7f1800f736d0) at mozilla/widget/nsBaseAppShell.cpp:158
#22 0x00007f1806f58020 in nsAppStartup::Run() (this=0x7f1800f69060) at mozilla/obj-x86_64-pc-linux-gnu/dist/include/nsCOMPtr.h:852
#23 0x00007f1806fed3cd in XREMain::XRE_mainRun() (this=this@entry=0x7fff872dc128) at mozilla/obj-x86_64-pc-linux-gnu/dist/include/nsCOMPtr.h:852
#24 0x00007f1806fedbab in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) (this=this@entry=0x7fff872dc128, argc=argc@entry=5, argv=argv@entry=0x7fff872dd438, aConfig=...)
    at mozilla/toolkit/xre/nsAppRunner.cpp:4924
#25 0x00007f1806fedef1 in XRE_main(int, char**, mozilla::BootstrapConfig const&) (argc=5, argv=0x7fff872dd438, aConfig=...) at mozilla/toolkit/xre/nsAppRunner.cpp:5016
#26 0x0000558be59c5186 in do_main(int, char**, char**) (argc=5, argv=0x7fff872dd438, envp=<optimized out>) at mozilla/comm/mail/app/nsMailApp.cpp:232
#27 0x0000558be59c4d45 in main(int, char**, char**) (argc=5, argv=0x7fff872dd438, envp=0x7fff872dd468) at mozilla/comm/mail/app/nsMailApp.cpp:311
Keywords: crash
Clearly caused by 
37fcdbb6756c Christoph Kerschbaumer — Bug 1495983: Assert system privileged about: pages have CSP. r=smaug

Also seen on the tree. Can you take a look?
Flags: needinfo?(acelists)
We need to define CSP in about: pages, something like:
<meta http-equiv="Content-Security-Policy" content="default-src 'none'; style-src chrome:" />

But how do we find out which pages are "about:"?
Flags: needinfo?(acelists)
OK, outputing the uri->Spec() in AssertAboutPageHasCSP() may work.
For me it asserts on about:neterror?e=netoffline
Summary: TB crashing on startup → Port bug 1495983 - TB crashing on startup due to CSP missing in about: pages
Attached patch 1496524.patch neterror (obsolete) — Splinter Review
It looks like we use docshell/resources/content/netError.xhtml for the TB error page (there are also others in m-c), which is missing CSP. This patch adds it at allows me to start TB in offline mode.
Assignee: nobody → acelists
Attachment #9014590 - Flags: review?(bugs)
Attached patch 1496524.patch c-c (obsolete) — Splinter Review
I added the CSP to some about: pages in TB (their URLs appear as about:* in the dom/base/nsDocument.cpp::AssertAboutPageHasCSP()).

about:preferences still asserts (crashes). I'm not sure how to add the CSP tag as our prefs page is a XUL file...
Attachment #9014594 - Flags: feedback?(richard.marti)
Attachment #9014594 - Flags: feedback?(jorgk)
Comment on attachment 9014590 [details] [diff] [review]
1496524.patch neterror

Maybe adding Christoph will speed up things ;-)
Attachment #9014590 - Flags: review?(ckerschb)
Comment on attachment 9014594 [details] [diff] [review]
1496524.patch c-c

Looks OK, but let's ask Christoph.
Attachment #9014594 - Flags: feedback?(richard.marti)
Attachment #9014594 - Flags: feedback?(jorgk)
Attachment #9014594 - Flags: feedback?(ckerschb)
Attachment #9014594 - Flags: feedback+
(In reply to :aceman from comment #6)
> about:preferences still asserts (crashes). I'm not sure how to add the CSP
> tag as our prefs page is a XUL file...

Did you try <xhtml:meta http-equiv="....... ?
Comment on attachment 9014590 [details] [diff] [review]
1496524.patch neterror

Review of attachment 9014590 [details] [diff] [review]:
-----------------------------------------------------------------

What about page is failing? Is that about:neterror or what is it? Alternatively to fixing this right away would be to update the whitelist in all.js to also include the specific about page for now. Which would at least make TB work again, then we file a follow up to remove the about page from the whitelist again and do the correct fix.

::: docshell/resources/content/netError.xhtml
@@ +21,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <html xmlns="http://www.w3.org/1999/xhtml">
>    <head>
> +    <meta http-equiv="Content-Security-Policy" content="default-src chrome:; script-src 'unsafe-inline' resource: chrome:; style-src 'unsafe-inline' resource: chrome:" />

From a CSP point of view it would be better to load styles and script from external resources only which would allow to remove the unsafe-inline values in script-src and style-src. Using unsafe-inline basically whitelists all inline scripts and hence does *not* provide any XSS protection.

From an assertion point of view that will do it of course, but I would rather have as do the right thing.
Attachment #9014590 - Flags: review?(ckerschb)
Comment on attachment 9014594 [details] [diff] [review]
1496524.patch c-c

Review of attachment 9014594 [details] [diff] [review]:
-----------------------------------------------------------------

Same here, we should try to remove unsafe-inline. Although here I am less worried since it's not code in mc.
Attachment #9014594 - Flags: feedback?(ckerschb)
What about pages are failing? You can find out by adding a print like
> fprintf(stderr, "\n\n ABOUTPAGE: %s\n", aboutSpec.get());
right here before the assertion
https://searchfox.org/mozilla-central/source/dom/base/nsDocument.cpp#5386
Flags: needinfo?(jorgk)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #13)
> What about pages are failing? You can find out by adding a print like
> > fprintf(stderr, "\n\n ABOUTPAGE: %s\n", aboutSpec.get());
> right here before the assertion
> https://searchfox.org/mozilla-central/source/dom/base/nsDocument.cpp#5386

As I already commented, I have done this and can see which pages assert.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> What about page is failing? Is that about:neterror or what is it?

Yes, it is the neterror page in the patch. 

> ::: docshell/resources/content/netError.xhtml
> @@ +21,5 @@
> >  <html xmlns="http://www.w3.org/1999/xhtml">
> >    <head>
> > +    <meta http-equiv="Content-Security-Policy" content="default-src chrome:; script-src 'unsafe-inline' resource: chrome:; style-src 'unsafe-inline' resource: chrome:" />
> 
> From a CSP point of view it would be better to load styles and script from
> external resources only which would allow to remove the unsafe-inline values
> in script-src and style-src. Using unsafe-inline basically whitelists all
> inline scripts and hence does *not* provide any XSS protection.

I have put the unsafe-inline there as there are inline scripts in that file. If you can remove that declaration and fix the file properly, please do.

I'm not sure why m-c could enable the assert when not all m-c pages are converted yet.

So what do you propose?
Will you accept the patch?
Will you make a proper fix soon?
Or will it take longer and we should add about:neterror to the whitelist?
(In reply to Magnus Melin [:mkmelin] from comment #10)
> > about:preferences still asserts (crashes). I'm not sure how to add the CSP
> > tag as our prefs page is a XUL file...
> 
> Did you try <xhtml:meta http-equiv="....... ?

Good idea, thanks. May it work if there is no <head> block? Or can we add that too as <html:head> ?
(In reply to :aceman from comment #14)
> So what do you propose?

Ok, so it seems that about:neterror resolves to docshell/resources/content/netError.xhtml for TB. Within Bug 1449872, and in particular within Bug 1453560 we applied a CSP to about:neterror and removed neterror from the whitelist within all.js. We can and should not land a CSP which contains script-src unsafe-inline since it doesn't provide any XSS Security and renders to be completely useless.

We should rewrite docshell/resources/content/netError.xhtml to something like we did within Bug 1453560, namely removing all inline usage of script which allows to remove unsafe-inline from the CSP.

Any chance you could provide that fix?
I think the questions have been answered. For the moment I landing
  pref("csp.skip_about_page_has_csp_assert", true);
so we can work again.
Flags: needinfo?(jorgk)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8a607b0960af
temporarily set csp.skip_about_page_has_csp_assert. rs=bustage-fix
Attachment #9014590 - Flags: review?(bugs) → review?(ckerschb)
Comment on attachment 9014590 [details] [diff] [review]
1496524.patch neterror

Christoph already cancelled the review, so let's not try again ;-)
Attachment #9014590 - Flags: review?(ckerschb)
Did you mean this?

Also, do you know how to add the CSP to a xul file? Or do we have to add those pages to the whitelist for now?
Attachment #9014590 - Attachment is obsolete: true
Attachment #9014953 - Flags: review?(ckerschb)
Attached patch 1496524.patch c-c v2 (obsolete) — Splinter Review
This makes preferences work now because I added them to the whitelist. But then we can remove the wholesale csp.skip_about_page_has_csp_assert = true to tighten the security.

I removed about:support from the patch as m-c also has that one whitelisted as being without the CSP. Once they add it, we will sync it with our file.
Attachment #9014594 - Attachment is obsolete: true
Attachment #9014955 - Flags: feedback?(jorgk)
Comment on attachment 9014953 [details] [diff] [review]
1496524.patch neterror v2 - please change to r=ckerschb

Review of attachment 9014953 [details] [diff] [review]:
-----------------------------------------------------------------

Paenglab, please check if I have distributes the style changes to the proper files so they work in TB.
Attachment #9014953 - Flags: ui-review?(richard.marti)
Comment on attachment 9014955 [details] [diff] [review]
1496524.patch c-c v2

Review of attachment 9014955 [details] [diff] [review]:
-----------------------------------------------------------------

Paenglab, please check if I have distributed the style changes to the proper files so they work in TB. (The css changes are dependent on the m-c netError patch.)
Attachment #9014955 - Flags: ui-review?(richard.marti)
Comment on attachment 9014953 [details] [diff] [review]
1496524.patch neterror v2 - please change to r=ckerschb

I wasn't able to force a neterror, especially to test #securityOverrideContent, but the removed display: none; is re-added on all stylesheets. -> ui-r+
Attachment #9014953 - Flags: ui-review?(richard.marti) → ui-review+
Comment on attachment 9014955 [details] [diff] [review]
1496524.patch c-c v2

The newsError works and the #securityOverrideContent has it's display: none; in our stylesheets. I saw no other style="" except the one style="display: none;" that needs a move to stylesheets.
Attachment #9014955 - Flags: ui-review?(richard.marti) → ui-review+
Comment on attachment 9014955 [details] [diff] [review]
1496524.patch c-c v2

Questions:
1) Any try run?
2) Christoph suggested to remove 'unsafe-inline'.
   That's not feasible?
3) Why the |display: none;|. What are we hiding and why?
Comment on attachment 9014953 [details] [diff] [review]
1496524.patch neterror v2 - please change to r=ckerschb

Review of attachment 9014953 [details] [diff] [review]:
-----------------------------------------------------------------

Yep, I think that looks good.
Regarding the XUL page, we can not add CSP to those until Bug 965637 and friends have landed, so you have to add those to the whitelist for now.
Attachment #9014953 - Flags: review?(ckerschb) → review+
Aceman, can you get the M-C part landed. Then perhaps revise the C-C part and answer my questions from comment #26.
(In reply to Jorg K (GMT+2) from comment #26)
> Questions:
> 1) Any try run?

Not yet.

> 2) Christoph suggested to remove 'unsafe-inline'.
>    That's not feasible?

Yes we can do that if we want to make the perfect version (we have time to do it now), and it will be something similar to the m-c patch.

> 3) Why the |display: none;|. What are we hiding and why?

The page is for multiple purposes/errors and depending on that it hides/shows some parts.

At least we know, that we need to have the whitelist for the XUL files for now so the c-c patch is basically OK, we just try to make it better for point 2).
I'm moving this to DOM::Security so the sheriffs can land the M-C part. Here's an M-C try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9e0ca711abbdf80677176b0d7de9f8955886eb9

Aceman, please inspect the result, and if good, set checkin-needed.
Severity: blocker → normal
Component: General → DOM: Security
Product: Thunderbird → Core
Summary: Port bug 1495983 - TB crashing on startup due to CSP missing in about: pages → Follow-up to bug 1495983 - add CSP to docshell/resorces/content/netError.xhtml
Comment on attachment 9014955 [details] [diff] [review]
1496524.patch c-c v2

This is going in the right direction: Removes the hack, adds some stuff to the csp.about_uris_without_csp pref, adds CSP to some pages. Maybe some details still need to be ironed out.
Attachment #9014955 - Flags: feedback?(jorgk) → feedback+
Please land the first patch "1496524.patch neterror v2" and change the commit message to:

Bug 1496524 - add CSP to docshell/resorces/content/netError.xhtml. r=ckerschb
                                                                    ^-- equal sign here.
Keywords: crashcheckin-needed
Attachment #9014953 - Attachment description: 1496524.patch neterror v2 → 1496524.patch neterror v2 - please change to r=ckerschb
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63a27bd95ecf
add CSP to docshell/resorces/content/netError.xhtml. r=ckerschb
Keywords: checkin-needed
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Attached patch 1496524.patch c-c v3 (obsolete) — Splinter Review
This would be the improved version that converts also the c-c files to have JS separated and thus do not need the insecure-inline clauses. However, for some reason the newsError page still asserts for me. Somehow the nsDocument.cpp can't see the CSP meta tag inside it (csp->GetPolicyString(0, parsedPolicyStr); returns empty). Christoph, may you see why?
Attachment #9014955 - Attachment is obsolete: true
Attachment #9015780 - Flags: feedback?(ckerschb)
Comment on attachment 9015780 [details] [diff] [review]
1496524.patch c-c v3

Review of attachment 9015780 [details] [diff] [review]:
-----------------------------------------------------------------

That seems correct to me. If you do a view-source on the about:neterror page, is it showing the right CSP? Don't know what might be wrong here.
Attachment #9015780 - Flags: feedback?(ckerschb) → feedback+
Attachment #9014605 - Attachment description: 1496524-temp-fix.patch → 1496524-temp-fix.patch [landed in comment 18]
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #36)
> That seems correct to me. If you do a view-source on the about:neterror
> page, is it showing the right CSP? Don't know what might be wrong here.

View Source does not seem to work on these special error pages. It tried to display the news article source and that is empty.
So we could land at least the pages that are working, add newserror to the whitelist for now and wonder about it in a new bug.
Attachment #9015780 - Attachment is obsolete: true
Attachment #9019547 - Flags: review?(jorgk)
Comment on attachment 9019547 [details] [diff] [review]
1496524.patch c-c v3

Review of attachment 9019547 [details] [diff] [review]:
-----------------------------------------------------------------

I assume you tested the "news error" page which you get when clicking onto an expired article. That page was introduced in bug 492329 (where I saved a rotting patch). Do the expired articles get removed?

::: mail/base/content/aboutRights.xhtml
@@ -30,5 @@
>  # Point 2 discusses Mozilla trademarks, and isn't needed when the build is unbranded.
>  # Point 3 discusses privacy policy, unbranded builds get a placeholder (for the vendor to replace)
>    <li>&rights.intro-point2a;<a href="https://www.mozilla.org/foundation/trademarks/policy.html">&rights.intro-point2b;</a>&rights.intro-point2c;</li>
>    <li>&rights.intro-point3a;<a href="https://www.mozilla.org/legal/privacy/">&rights.intro-point3b;</a>&rights.intro-point3c;</li>
> -  <li>&rights.intro-point4a;<a href="about:rights#webservices" onclick="showServices(event);">&rights.intro-point4b;</a>&rights.intro-point4c;</li>

What happened to onclick? Simply removed?

@@ -35,3 @@
>  #else
>    <li>&rights.intro-point3-unbranded;</li>
> -  <li>&rights.intro-point4a-unbranded;<a href="about:rights#webservices" onclick="showServices(event);">&rights.intro-point4b-unbranded;</a>&rights.intro-point4c-unbranded;</li>

And here.
(In reply to Jorg K (GMT+2) from comment #40)
> I assume you tested the "news error" page which you get when clicking onto
> an expired article. That page was introduced in bug 492329 (where I saved a
> rotting patch). Do the expired articles get removed?

Yes I tested it and therefore I know the page still asserts if not on the whitelist.
I don't know if expired articles are to be removed. Mine stay in the folder so far and that is good so that I have something to test on :)
 
> ::: mail/base/content/aboutRights.xhtml
> > -  <li>&rights.intro-point4a;<a href="about:rights#webservices" onclick="showServices(event);">&rights.intro-point4b;</a>&rights.intro-point4c;</li>
> 
> What happened to onclick? Simply removed?

Defined in the linked in chrome://global/content/aboutRights.js, https://dxr.mozilla.org/comm-central/source/toolkit/content/aboutRights.js .
Comment on attachment 9019547 [details] [diff] [review]
1496524.patch c-c v3

news.mozilla.org, mozilla.test.multimedia. Clicking on the link *did* remove the expired articles. Of course I could only test this once :-(
Attachment #9019547 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e55a387029ad
Port bug 1495983: add CSP to some about: pages. r=jorgk
The M-C part landed on mozilla64, the C-C part during the 65 cycle. A bit unfortunate.
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.