1031259 - CSS layout broken on Disqus (at e.g. cnn.com & wishtv.com), due to CSP blocking a CSS file

Status: VERIFIED FIXED

(Core :: DOM: Security, defect)

Description

[Jim Jeffery not reading bug-mail 1/2/11]

Visit CNN or any site that uses Disqus commenting.
http://www.cnn.com/2014/06/26/tech/innovation/nasa-orion-spacecraft/index.html?hpt=hp_c2

The layout is broken and shows CSS tree and comments may or not display.

Regression range on m-c builds:
K:\MCJun2014\20140626135646 398a5f3142c1 ok
K:\MCJun2014\20140626141348 3519e987aa3b Bad

Need help with m-i range

Comment 1

[Alice0775 White]

A lot of error in Error Console

Timestamp: 2014/06/27 23:04:26
Error: Content Security Policy: The page's settings blocked the loading of a resource at http://a.disquscdn.com/next/styles/lounge.0ba88a7f9c9b078d4ac100dcb6be5435.css ("").


Timestamp: 2014/06/27 23:04:27
Error: Content Security Policy: The page's settings blocked the loading of a resource at http://a.disquscdn.com/next/styles/discovery.2e8fcbb9a5f15e554ff5def263c47481.css ("").

Timestamp: 2014/06/27 23:04:28
Error: Content Security Policy: The page's settings blocked the loading of a resource at http://www.google-analytics.com/__utm.gif?utmwv=5.5.3&utms=13&utmn=1223138741&utmhn=disqus.com&utme=8(component*package*forum*version*userType)9(embed*vip*cnn*next*not_logged_in)&utmcs=UTF-8&utmsr=1280x1024&utmvp=591x1071&utmsc=24-bit&utmul=en-us&utmje=0&utmfl=14.0%20r0&utmdt=Disqus%20Comments&utmhid=1367820391&utmr=http%3A%2F%2Fedition.cnn.com%2F2014%2F06%2F26%2Ftech%2Finnovation%2Fnasa-orion-spacecraft%2Findex.html%3Fhpt%3Dhp_c2&utmp=%2Fembed%2Fcomments%2F%3Fbase%3Ddefault%26disqus_version%3D277a43fe%26f%3Dcnn%26t_i%3D%25252F2014%25252F06%25252F26%25252Ftech%25252Finnovation%25252Fnasa-orion-spacecraft%25252Findex.html%26t_u%3Dhttp%25253A%25252F%25252Fwww.cnn.com%25252F2014%25252F06%25252F26%25252Ftech%25252Finnovation%25252Fnasa-orion-spacecraft%25252Findex.html%26t_e%3DNASA%252527s%252520deep-space%252520craft%252520readying%252520for%252520launch%26t_d%3DOrion%25252C%252520NASA%252527s%252520deep-space%252520craft%25252C%252520readying%252520for%252520launch%252520-%252520CNN.com%26t_t%3DNASA%252527s%252520deep-space%252520craft%252520readying%252520for%252520launch%26t_c%3D207582%26s_o%3Ddefault&utmht=1403877868067&utmac=UA-1410476-6&utmcc=__utma%3D40641725.2086229710.1403873463.1403873463.1403877400.2%3B%2B__utmz%3D40641725.1403877400.2.2.utmcsr%3Dedition.cnn.com%7Cutmccn%3D(referral)%7Cutmcmd%3Dreferral%7Cutmcct%3D%2F2014%2F06%2F26%2Ftech%2Finnovation%2Fnasa-orion-spacecraft%2Findex.html%3B&utmu=qR~ ("").



Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc67c4e78d8d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 ID:20140626092949
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8f96ce0c95e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 ID:20140626093219
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=cc67c4e78d8d&tochange=a8f96ce0c95e

Regressed by:
a8f96ce0c95e	Christoph Kerschbaumer — Bug 925004 - CSP in CPP: flip pref to enable new CSP backend (r=sstamm)

Comment 2

[Daniel Holbert [:dholbert]]

So, the site's CSP is blocking loading of a CSS file.

Someone who knows CSP should take a look at this and determine whether this blocking is actually supposed to happen, per the site's CSP. (If that's the case, this would just be a Tech Evang bug, and we should get CNN or Disqus or whoever to fix their CSP)

Comment 3

[Jim Jeffery not reading bug-mail 1/2/11]

toggling off pref: security.csp.newbackend.enable restores Disqus to display again.

Comment 4

[Jim Jeffery not reading bug-mail 1/2/11]

Its not just CNN, this page uses Disqus and its also not working properly:

http://wishtv.com/2014/06/27/14-people-displaced-after-columbus-apartment-fire/

Comment 5

[Christoph Kerschbaumer [:ckerschb, back on Aug 10th}]

Incorrectly cleared the needinfo flag when adding the dependency, but I am working on the problem, thanks Daniel for bringing it to my attention.

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

Looks like this is the CSP that gets violated:

"script-src https://*.twitter.com:* disqus.com *.google-analytics.com http://*.twitter.com:* api.taboola.com a.disquscdn.com referrer.disqus.com *.services.disqus.com:* *.scorecardresearch.com 'unsafe-eval'"

No default-src should imply default-src * as per the spec.  Chris: what did you intend the new parser do?

Comment 7

[Christoph Kerschbaumer [:ckerschb, back on Aug 10th}]

Interesting case:
for *external loads*, we deny if no default-src and no specific directive is defined:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPUtils.cpp#774

for *inline loads*, we allow the load in such a case:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPUtils.cpp#812

Probably you are right, and we should allow the load in both cases.

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Yeah, I double-checked.  The spec doesn't say explicitly, but if there's not a relevant directive for stylesheets (no default-src or style-src), CSP MUST not enforce either default-src or style-src.  This means no restrictions on styles unless default-src and/or style-src is present.

See also bug 764937.

Comment 9

[Christoph Kerschbaumer [:ckerschb, back on Aug 10th}]

Attachment: bug_1031259.patch

As described in https://bugzilla.mozilla.org/show_bug.cgi?id=764937#c0, CSP should allow the load if no matching directive and also no default-src is defined. Discussed with Sid in person and on IRC - we both think it's not the safest thing to do, but according to the spec this is the right thing to do.

At least bug 1031417 revealed a minor mapping issue which also gets fixed with this bug (connect-src - websocket).

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8447395 [details] [diff] [review]
bug_1031259.patch

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

looks good to me.  File a bug for the mapping correctness, but lets land this as-is first to fix the regression.

::: content/base/src/nsCSPUtils.cpp
@@ +658,5 @@
> +    case CSP_MEDIA_SRC:   return nsIContentPolicy::TYPE_MEDIA;
> +    case CSP_OBJECT_SRC:  return nsIContentPolicy::TYPE_OBJECT;
> +    case CSP_FRAME_SRC:   return nsIContentPolicy::TYPE_SUBDOCUMENT;
> +    case CSP_REPORT_URI:  return nsIContentPolicy::TYPE_CSP_REPORT;
> +    case CSP_CONNECT_SRC: return nsIContentPolicy::TYPE_WEBSOCKET;

Looks like we are missing a few:

XMLHTTPREQUEST
BEACON
OBJECT_SUBREQUEST

See: http://mxr.mozilla.org/mozilla-central/source/content/base/src/contentSecurityPolicy.js#83

Note that the mapping from directive->content type is not 1:1, it's 1:many.  Do we need a bug to fix this up?  Seems to me what we should do is have a map from content types to directive names for the lookups that are wondering which directive to fix.  

Maybe remove this change from this patch and file a follow-up bug to make this correctness update.

Comment 11

[Christoph Kerschbaumer [:ckerschb, back on Aug 10th}]

Attachment: bug_1031259_v2.patch

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #10)
> Comment on attachment 8447395 [details] [diff] [review]
> bug_1031259.patch
> 
> Review of attachment 8447395 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> looks good to me.  File a bug for the mapping correctness, but lets land
> this as-is first to fix the regression.
> 
> ::: content/base/src/nsCSPUtils.cpp
> @@ +658,5 @@
> > +    case CSP_MEDIA_SRC:   return nsIContentPolicy::TYPE_MEDIA;
> > +    case CSP_OBJECT_SRC:  return nsIContentPolicy::TYPE_OBJECT;
> > +    case CSP_FRAME_SRC:   return nsIContentPolicy::TYPE_SUBDOCUMENT;
> > +    case CSP_REPORT_URI:  return nsIContentPolicy::TYPE_CSP_REPORT;
> > +    case CSP_CONNECT_SRC: return nsIContentPolicy::TYPE_WEBSOCKET;
> 
> Looks like we are missing a few:
> 
> XMLHTTPREQUEST
> BEACON
> OBJECT_SUBREQUEST
> 
> See:
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/
> contentSecurityPolicy.js#83
> 
> Note that the mapping from directive->content type is not 1:1, it's 1:many. 
> Do we need a bug to fix this up?  Seems to me what we should do is have a
> map from content types to directive names for the lookups that are wondering
> which directive to fix.  
> 
> Maybe remove this change from this patch and file a follow-up bug to make
> this correctness update.

I also think it should be handled in a separate bug, I filed 1031530 and removed that mapping from the current patch.

Carrying over r+ from sstamm.

Comment 12

[Christoph Kerschbaumer [:ckerschb, back on Aug 10th}]

This patch can land together with bug 1031372.
Try: https://tbpl.mozilla.org/?tree=Try&rev=a57cfa94499e

Comment 13

[Jim Jeffery not reading bug-mail 1/2/11]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> This patch can land together with bug 1031372.
> Try: https://tbpl.mozilla.org/?tree=Try&rev=a57cfa94499e

Just tested this 'try run', and seems to have fixed the issue.  Thanks.

Still getting this in error console however:
Fri Jun 27 2014 19:14:23
Error: Content Security Policy: The page's settings blocked the loading of a resource at self ("script-src https://*.twitter.com:* http://disqus.com http://*.google-analytics.com http://*.twitter.com:* http://api.taboola.com http://a.disquscdn.com http://referrer.disqus.com http://*.services.disqus.com:* http://*.scorecardresearch.com 'unsafe-eval'").
Source file: http://disqus.com/embed/comments/?base=default&disqus_version=8b7b7152&f=cnn&t_i=%2F2014%2F06%2F27%2Fpolitics%2Fmississippi-tea-party-leader-dies%2Findex.html&t_u=http%3A%2F%2Fwww.cnn.com%2F2014%2F06%2F27%2Fpolitics%2Fmississippi-tea-party-leader-dies%2Findex.html&t_e=Mississippi%20tea%20party%20leader%20tied%20to%20campaign%20smear%20dead%20in%20apparent%20suicide&t_d=Suicide%20probed%20in%20Mississippi%20tea%20party%20leader%27s%20death%20-%20CNN.com&t_t=Mississippi%20tea%20party%20leader%20tied%20to%20campaign%20smear%20dead%20in%20apparent%20suicide&t_c=207582&s_o=default#2
Source code:
onsubmit attribute on DIV element

I assume that error is https://bugzilla.mozilla.org/show_bug.cgi?id=1031417 ?

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c6625799585

Comment 15

[Christoph Kerschbaumer [:ckerschb, back on Aug 10th}]

(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #13)
> Still getting this in error console however:
> Fri Jun 27 2014 19:14:23
> Error: Content Security Policy: The page's settings blocked the loading of a
> resource at self ("script-src https://*.twitter.com:* http://disqus.com
> http://*.google-analytics.com http://*.twitter.com:* http://api.taboola.com
> http://a.disquscdn.com http://referrer.disqus.com
> http://*.services.disqus.com:* http://*.scorecardresearch.com
> 'unsafe-eval'").
> Source file:
> http://disqus.com/embed/comments/
> ?base=default&disqus_version=8b7b7152&f=cnn&t_i=%2F2014%2F06%2F27%2Fpolitics%
> 2Fmississippi-tea-party-leader-dies%2Findex.html&t_u=http%3A%2F%2Fwww.cnn.
> com%2F2014%2F06%2F27%2Fpolitics%2Fmississippi-tea-party-leader-dies%2Findex.
> html&t_e=Mississippi%20tea%20party%20leader%20tied%20to%20campaign%20smear%20
> dead%20in%20apparent%20suicide&t_d=Suicide%20probed%20in%20Mississippi%20tea%
> 20party%20leader%27s%20death%20-%20CNN.
> com&t_t=Mississippi%20tea%20party%20leader%20tied%20to%20campaign%20smear%20d
> ead%20in%20apparent%20suicide&t_c=207582&s_o=default#2
> Source code:
> onsubmit attribute on DIV element
> 
> I assume that error is https://bugzilla.mozilla.org/show_bug.cgi?id=1031417 ?

I am not completely sure what's causing this other CSP error - I am still investigating it - probably it goes away once bug 1031530 lands.

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5c6625799585

Comment 17

[Cristina Madaras, QA [:CristinaM]]

[testday-20140912]

Reproduced the issue on Nightly under Windows 7 x64bit.
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:33.0) Gecko/20100101 Firefox/33.0
BuilId: 20140627030212

Verified as fixed on Beta 33.0b3
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Build Id: 20140911191954

Comment 18

[Camelia Badau [:cbadau], Desktop Test Engineering]

Marking verified fixed based on comment #17.