User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.106 Safari/537.36 Steps to reproduce: Issue a CSP that bans inline script. Content-Security-Policy: default-src 'self' Include an inline script on a page to cause a CSP violation. <script>alert("Hello World!");</script> The CSP report that is sent contains a blocked-uri value. "blocked-uri":"self" According to the specification the blocked-uri should be an "empty string if the resource has no URL (inline script and inline style, for example)". http://www.w3.org/TR/CSP2/#violation-report-blocked-uri Actual results: The CSP report that is sent contains a blocked-uri value. "blocked-uri":"self" Expected results: The CSP report that is sent contains an empty blocked-uri. "blocked-uri":""

Hi! I would like to attempt to solve this bug. I need to confirm few things. I was initially planning to use regular expressions to find urls. What values can reportBlockedURI take? Is it a url, a url embedded in a string or an empty string?

Please check the patch and tell me if its good enough

Comment on attachment 8800745 [details] [diff] [review] Please check this patch Review of attachment 8800745 [details] [diff] [review]: ----------------------------------------------------------------- Ideally I suppose we could change | nsISupports* aBlockedContentSource | to |nsIURI* aBlockedURI|. I am not entirely certain what aBlockedContentSource might contain other than an nsIURI, potentially 'self' and some other strings. If it's only 'self' then my approach would work, but this needs verification of course. If am correct however, we could eliminate that whole else [1] branch; only strip the URI for reporting if there is a URI and leave the empty string in case aBlockedURI is null. [1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPContext.cpp?q=nsCOMPtr%3CnsISupportsCString%3E+cstr+%3D+do_QueryInterface%28aBlockedContentSource%29&redirect_type=single#812

Right, I have removed the else condition. However, I don't understand why checking if reportBlockedURI is a url is incorrect. It takes care of all cases except if its a url. I've attached the modified patch. Please review

I'm sorry. I just realised that I don't have to make a function to check for url. This patch is the one you need. Please review

Please Review

Comment on attachment 8801568 [details] [diff] [review] bug1236222.patch Review of attachment 8801568 [details] [diff] [review]: ----------------------------------------------------------------- Hey Tanuja, as suggested in comment 3 we should try to do the following: 1) Change the signature of the function from nsCSPContext::SendReports(nsISupports* aBlockedContentSource, to nsCSPContext::SendReports(nsIURI* aBlockedURI, 2) in the cases where we used to pass a string as aBlockedContentSource we should pass nullptr (if possible) 3) We should replace that block // blocked-uri if (aBlockedContentSource) { nsAutoCString reportBlockedURI; nsCOMPtr<nsIURI> uri = do_QueryInterface(aBlockedContentSource); // could be a string or URI if (uri) { StripURIForReporting(uri, mSelfURI, reportBlockedURI); } else { nsCOMPtr<nsISupportsCString> cstr = do_QueryInterface(aBlockedContentSource); if (cstr) { cstr->GetData(reportBlockedURI); } } if (reportBlockedURI.IsEmpty()) { // this can happen for frame-ancestors violation where the violating // ancestor is cross-origin. NS_WARNING("No blocked URI (null aBlockedContentSource) for CSP violation report."); } report.mCsp_report.mBlocked_uri = NS_ConvertUTF8toUTF16(reportBlockedURI); } with something like if (aBlockedURI) { nsCOMPtr<nsIURI> uri = aBlockedURI; // could be a string or URI StripURIForReporting(uri, mSelfURI, reportBlockedURI); report.mCsp_report.mBlocked_uri = NS_ConvertUTF8toUTF16(reportBlockedURI); }

Thanks Christoph, I did not Change the signature of the function from nsCSPContext::SendReports(nsISupports* aBlockedContentSource, to nsCSPContext::SendReports(nsIURI* aBlockedURI, earlier, as the function is being called elsewhere (line 1061 [1]) and it will no longer be a part of the nsCSPContext class, and thus throws this error /home/fay/mozilla-central/dom/security/nsCSPContext.cpp:783:1: error: prototype for ‘nsresult nsCSPContext::SendReports(nsIURI*, nsIURI*, nsAString_internal&, uint32_t, nsAString_internal&, nsAString_internal&, uint32_t)’ does not match any in class ‘nsCSPContext’ 0:05.14 nsCSPContext::SendReports(nsIURI* aBlockedURI, The patch I submitted earlier takes a different approach to reporting only the URL. But I'll try to implement what you are suggesting. I'll have to modify multiple functions that call the SendReports function. [1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPContext.cpp?#1061

This one is the one you were suggesting. Please review

Comment on attachment 8801726 [details] [diff] [review] bug1236222.patch Review of attachment 8801726 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/security/nsCSPContext.cpp @@ +1048,5 @@ > > // 2) send reports for the policy that was violated > + nsCOMPtr<nsIURI> uri = do_QueryInterface(mBlockedContentSource); > + // could be a string or URI > + if (uri) { What I meant in my earlier comment is that we should go further back than one level and fix all the entry points that end up here. E.g. can we also the signature of CSPReportSenderRunnable() as well as AsyncReportViolation() to use an nsIURI instead of an nsISupports?

By the way, does your patch cause any tests within dom/security/test/csp to fail?

Shall I check for the url in AsyncReportViolation(), cause its being called in three different places. Also, it didn't cause any failure

(In reply to Tanuja from comment #13) > Shall I check for the url in AsyncReportViolation(), cause its being called > in three different places. That would be the plan. > Also, it didn't cause any failure That's no good. We must have a test for that somewhere. I would imagine that test_redirects.html would fail once we update the right bits within our code.

I've fixed it many functions deep. Please review.

I tried the nightly on a site with blocked scripts. When 'self', its blank. Else it gives url. fay@ENVY:~/mozilla-central$ ./mach run 0:00.89 /home/fay/mozilla-central/obj-x86_64-pc-linux-gnu/dist/bin/firefox -no-remote -profile /home/fay/mozilla-central/obj-x86_64-pc-linux-gnu/tmp/scratch_user 1476730890981 addons.xpi-utils WARN Disabling foreign installed add-on ubufox@ubuntu.com in app-system-share ATTENTION: default value of option force_s3tc_enable overridden by environment. ATTENTION: default value of option force_s3tc_enable overridden by environment. ATTENTION: default value of option force_s3tc_enable overridden by environment. Blocked: Blocked:http://htmledit.squarefree.com/

This one is better. Please review.

Comment on attachment 8801874 [details] [diff] [review] bug1236222.patch Review of attachment 8801874 [details] [diff] [review]: ----------------------------------------------------------------- I think we need to go all the way to find the root cause and replace all nsISupports* with nsIURI* - thanks! ::: dom/security/nsCSPContext.cpp @@ +804,2 @@ > // blocked-uri > + nit: trailing whitespace. @@ +809,2 @@ > // could be a string or URI > + StripURIForReporting(uri, mSelfURI, reportBlockedURI); no need for the tmp variable uri, just use: StripURIForReporting(aBlockedURI, ...); Please also fix the trailing whitespace issue. @@ +811,2 @@ > report.mCsp_report.mBlocked_uri = NS_ConvertUTF8toUTF16(reportBlockedURI); > + nit: trailing whitespace. @@ +1083,5 @@ > return NS_OK; > } > > private: > + nsCOMPtr<nsIURI> mBlockedURI; nit, please align with other member variables. @@ +1132,5 @@ > uint32_t aLineNum) > { > NS_ENSURE_ARG_MAX(aViolatedPolicyIndex, mPolicies.Length() - 1); > > + nsCOMPtr<nsIURI> uri = do_QueryInterface(aBlockedContentSource); As suggested earlier, we also need to update the callsites of AsyncReportViolation to pass aURI instead of aBlockedContentSource @@ +1134,5 @@ > NS_ENSURE_ARG_MAX(aViolatedPolicyIndex, mPolicies.Length() - 1); > > + nsCOMPtr<nsIURI> uri = do_QueryInterface(aBlockedContentSource); > + > + if (uri) { why do we need to branch here and duplicate code? ::: dom/security/nsCSPContext.h @@ +56,5 @@ > uint32_t aLineNumber, > uint32_t aColumnNumber, > uint32_t aSeverityFlag); > > + nsresult SendReports(nsIURI* mBlockedURI, prefix 'm' denotes member variable, prefix of 'a' denotes 'argument', please update to aBlockedURI.

I've fixed the AsyncReportViolation part. Please review

Comment on attachment 8805803 [details] [diff] [review] bug1236222.patch Review of attachment 8805803 [details] [diff] [review]: ----------------------------------------------------------------- Hey Tanuja, I had a close look at your patch and also at our code. It took me a long time but I traced all the callsites for you to figure out whether we can update AsyncReportViolation as well as SendReports. Answer is, we can only update SendReports but not AsyncReportViolation. Reason is that AsyncReportViolation also logs information to the developer console [1] which would get lost if we only pass a nullptr instead of the actual string to be reported to the console. In other words that means, that whenver we call SendReports() we have to check if we can query the interface of nsISupports into an nsIURI. If that's not possible the result should be a nullptr anyway and we should be good to go. So you would have to do something like: nsCOMPtr<nsIURI> uri = do_QueryInterface(uriOrString); an then call SendReports(uri, ...) [1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPContext.cpp#1090 ::: dom/security/nsCSPContext.cpp @@ +807,2 @@ > nsAutoCString reportBlockedURI; > + StripURIForReporting(aBlockedURI, mSelfURI, reportBlockedURI); Keep those changes. ::: dom/security/nsCSPContext.h @@ +56,5 @@ > uint32_t aLineNumber, > uint32_t aColumnNumber, > uint32_t aSeverityFlag); > > + nsresult SendReports(nsIURI* aBlockedURI, Keep that. @@ +64,5 @@ > nsAString& aSourceFile, > nsAString& aScriptSample, > uint32_t aLineNum); > > + nsresult AsyncReportViolation(nsIURI* aBlockedContentSource, Please change that back.

I've added the changes you've asked for. Please review.

Comment on attachment 8806300 [details] [diff] [review] bug1236222.patch Review of attachment 8806300 [details] [diff] [review]: ----------------------------------------------------------------- This one is getting really close. Does it pass all csp tests in the tree? E.g. does it pass: ./mach mochitest dom/security/test/csp? ::: dom/security/nsCSPContext.cpp @@ +1043,5 @@ > mViolatedDirective.get()); > NS_ENSURE_SUCCESS(rv, rv); > > // 2) send reports for the policy that was violated > + nsCOMPtr<nsIURI> uri = do_QueryInterface(mBlockedContentSource); please name the variable blockedURI @@ +1050,5 @@ > mSourceFile, mScriptSample, mLineNum); > > // 3) log to console (one per policy violation) > // mBlockedContentSource could be a URI or a string. > + nsCOMPtr<nsIURI> blockedURI = uri; please remove that line and just reuse blockedURI from above. ::: dom/security/nsCSPContext.h @@ +159,5 @@ > private: > nsCOMPtr<nsINetworkInterceptController> mInterceptController; > }; > > +#endif /* nsCSPContext_h___ */ what did change here?

Fixed the problems. The tests gave the following results: 0 INFO TEST-START | Shutdown 1 INFO Passed: 660 2 INFO Failed: 1 3 INFO Todo: 2 4 INFO Mode: e10s 5 INFO SimpleTest FINISHED The following tests failed: 866 INFO TEST-UNEXPECTED-FAIL | dom/security/test/csp/test_report.html | Incorrect blocked-uri - got "", expected "self" SimpleTest.is@SimpleTest/SimpleTest.js:270:5 window.checkResults@dom/security/test/csp/test_report.html:51:3 ml@dom/security/test/csp/test_report.html:80:7 SUITE-END | took 32s Failed as expected. This was the bug

(In reply to Tanuja from comment #23) > 866 INFO TEST-UNEXPECTED-FAIL | dom/security/test/csp/test_report.html | > Incorrect blocked-uri - got "", expected "self" Alright, so before we can merge that change into the codebase we have to update that test.

Fixed the test case. Got this as output WARNING | leakcheck | refcount logging is off, so leaks can't be detected! runtests.py | Running tests: end. 0 INFO TEST-START | Shutdown 1 INFO Passed: 661 2 INFO Failed: 0 3 INFO Todo: 2 4 INFO Mode: e10s 5 INFO SimpleTest FINISHED SUITE-END | took 30s

Comment on attachment 8808167 [details] [diff] [review] bug1236222.patch Review of attachment 8808167 [details] [diff] [review]: ----------------------------------------------------------------- Code looks good and test update also looks good. Please update the commit message to something like: > Bug 126222 - CSP: Blocked URI should be empty for inline violations. r=ckerschb Btw, did you also run web platform tests? ./mach test testing/web-platform/tests/content-security-policy/

Got this as the result. Summary ======= Ran 492 tests (171 parents, 321 subtests) Expected results: 474 Unexpected results: 8 (FAIL: 4, PASS: 4) Skipped: 10 Unexpected Results ================== /content-security-policy/blink-contrib/object-src-applet-archive-codebase.sub.html ---------------------------------------------------------------------------------- FAIL expected NOTRUN Expecting logs: ["PASS"] PASS expected FAIL Violation report status OK. /content-security-policy/blink-contrib/object-src-applet-archive.sub.html ------------------------------------------------------------------------- FAIL expected NOTRUN Expecting logs: ["PASS"] PASS expected FAIL Violation report status OK. /content-security-policy/blink-contrib/object-src-applet-code-codebase.sub.html ------------------------------------------------------------------------------- FAIL expected NOTRUN Expecting logs: ["PASS"] PASS expected FAIL Violation report status OK. /content-security-policy/blink-contrib/object-src-applet-code.sub.html ---------------------------------------------------------------------- FAIL expected NOTRUN Expecting logs: ["PASS"] PASS expected FAIL Violation report status OK. 7:31.39 LOG: MainThread INFO Closing logging queue 7:31.39 LOG: MainThread INFO queue closed So I retried with the default code. Built it again and ran the test again Got this Summary ======= Ran 498 tests (174 parents, 324 subtests) Expected results: 481 Unexpected results: 8 (FAIL: 4, PASS: 4) Skipped: 9 Unexpected Results ================== /content-security-policy/blink-contrib/object-src-applet-archive-codebase.sub.html ---------------------------------------------------------------------------------- FAIL expected NOTRUN Expecting logs: ["PASS"] PASS expected FAIL Violation report status OK. /content-security-policy/blink-contrib/object-src-applet-archive.sub.html ------------------------------------------------------------------------- FAIL expected NOTRUN Expecting logs: ["PASS"] PASS expected FAIL Violation report status OK. /content-security-policy/blink-contrib/object-src-applet-code-codebase.sub.html ------------------------------------------------------------------------------- FAIL expected NOTRUN Expecting logs: ["PASS"] PASS expected FAIL Violation report status OK. /content-security-policy/blink-contrib/object-src-applet-code.sub.html ---------------------------------------------------------------------- FAIL expected NOTRUN Expecting logs: ["PASS"] PASS expected FAIL Violation report status OK. 7:17.46 LOG: MainThread INFO Closing logging queue 7:17.46 LOG: MainThread INFO queue closed So the problem is not the changes we made.

Thanks; please upload a new patch with an updated commit message, something like: > Bug 1236222: CSP: Blocked URI should be empty for inline violations. r=ckerschb and I am happy to r+ that changeset.

Hi, the latest patch I uploaded has that commit message.

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9c1069e2a42e CSP: Blocked URI should be empty for inline violations. r=ckerschb

Backout by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/mozilla-inbound/rev/eac5fd08280a Backed out changeset 9c1069e2a42e for failing xpcshell test test_csp_reports.js. r=backout

(In reply to Pulsebot from comment #31) > Backout by archaeopteryx@coole-files.de: > https://hg.mozilla.org/integration/mozilla-inbound/rev/eac5fd08280a > Backed out changeset 9c1069e2a42e for failing xpcshell test > test_csp_reports.js. r=backout Tanuja, can you take a look?

Backed this out for failing xpcshell test test_csp_reports.js: Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=38882473&repo=mozilla-inbound [task 2016-11-09T11:17:53.321401Z] 11:17:53 INFO - "CONSOLE_MESSAGE: (error) [JavaScript Error: "Content Security Policy: The page’s settings blocked the loading of a resource at data:image/png;base64,iVBORw0KGgoAAAANSU... (“default-src 'none'â€)."]" [task 2016-11-09T11:17:53.323815Z] 11:17:53 INFO - "CONSOLE_MESSAGE: (error) [JavaScript Error: "Content Security Policy: The page’s settings blocked the loading of a resource at intent://mymaps.com/maps?um=1&ie=UTF-8&fb=1&sll (“default-src 'none'â€)."]" [task 2016-11-09T11:17:53.326137Z] 11:17:53 INFO - "CONSOLE_MESSAGE: (error) [JavaScript Error: "Content Security Policy: The page’s settings blocked the loading of a resource at http://localhost:47040/foo/self/foo.js#bar (“default-src 'none'â€)."]" [task 2016-11-09T11:17:53.329703Z] 11:17:53 INFO - "CONSOLE_MESSAGE: (error) [JavaScript Error: "Content Security Policy: The page’s settings blocked the loading of a resource at ftp://blocked.test/profile.png (“default-src 'none'â€)."]" [task 2016-11-09T11:17:53.331619Z] 11:17:53 WARNING - TEST-UNEXPECTED-FAIL | dom/security/test/unit/test_csp_reports.js | makeReportHandler/< - [makeReportHandler/< : 55] "self" == "" [task 2016-11-09T11:17:53.333731Z] 11:17:53 INFO - /home/worker/workspace/build/tests/xpcshell/tests/dom/security/test/unit/test_csp_reports.js:makeReportHandler/<:55 [task 2016-11-09T11:17:53.337338Z] 11:17:53 INFO - resource://testing-common/httpd.js:ServerHandler.prototype.handleResponse:2374 [task 2016-11-09T11:17:53.339187Z] 11:17:53 INFO - resource://testing-common/httpd.js:Connection.prototype.process:1225 [task 2016-11-09T11:17:53.340839Z] 11:17:53 INFO - resource://testing-common/httpd.js:RequestReader.prototype._handleResponse:1679 [task 2016-11-09T11:17:53.343214Z] 11:17:53 INFO - resource://testing-common/httpd.js:RequestReader.prototype._processBody:1527 [task 2016-11-09T11:17:53.344893Z] 11:17:53 INFO - resource://testing-common/httpd.js:RequestReader.prototype.onInputStreamReady:1395 [task 2016-11-09T11:17:53.346950Z] 11:17:53 INFO - /home/worker/workspace/build/tests/xpcshell/head.js:_do_main:210 [task 2016-11-09T11:17:53.348927Z] 11:17:53 INFO - /home/worker/workspace/build/tests/xpcshell/head.js:_execute_test:545 [task 2016-11-09T11:17:53.350814Z] 11:17:53 INFO - -e:null:1 [task 2016-11-09T11:17:53.353285Z] 11:17:53 INFO - exiting test

So, shall I modify this test too? It seems to expect a 'self'.

Also, how do I run the unit test?

(In reply to Tanuja from comment #34) > So, shall I modify this test too? It seems to expect a 'self'. Yes, please test. (In reply to Tanuja from comment #35) > Also, how do I run the unit test? ./mach test dom/security/test/unit/test_csp_reports.js

I am finishing this for Tanuja Sawant. This is the follow-up with the tiny test changes. I've pushed this to our try server at https://treeherder.mozilla.org/#/jobs?repo=try&revision=de099c71e4021654aeb94b65c4fd49ea317b0681 Please review, when you get to it, ckerschb. (You don't officially accept review requests)

Comment on attachment 8879121 [details] [diff] [review] 0001-Bug-126222-CSP-Blocked-URI-should-be-empty-for-inlin.patch Review of attachment 8879121 [details] [diff] [review]: ----------------------------------------------------------------- thanks for pushing this one over the finishing line. r=me

Is this worth picking up? It looks like the patch won't remotely apply anymore.

I wonder why this never landed, but it got r+ed. :jkt, can you drag it over the finishing line? thanks.

Comment on attachment 8957384 [details] Bug 1236222 - CSP: Blocked URI should be empty for inline violations. https://reviewboard.mozilla.org/r/226312/#review232286 Yep, I think that looks good. Can you please mark the other patch as obsolete? Thanks!

Pushed by jkingston@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7d62c45d5973 CSP: Blocked URI should be empty for inline violations. r=ckerschb

Backed out changeset 7d62c45d5973 (bug 1236222) for X xpcshell failures in :toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7d62c45d5973cc60e8109cc8cfdfee1f9c81f199&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=success&selectedJob=167470802 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=167470802&repo=autoland&lineNumber=7734 Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6d9ad29e778423f2001f8c3298aa8d3b7b248bf4&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success

Pushed by jkingston@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0e90c9123426 CSP: Blocked URI should be empty for inline violations. r=ckerschb