Issue 29590611: Issue 5953 - Bypass site CSP for script injection in Firefox (Closed)
|
Created:
Oct. 27, 2017, 3:53 p.m. by tschuster Modified:
Feb. 21, 2018, 6:27 p.m. Visibility:
Public. |
DescriptionIssue 5953 - Bypass site CSP for script injection in Firefox
Patch Set 1 #Patch Set 2 : Actually inject the right code #
Total comments: 1
Patch Set 3 : #
Total comments: 2
Patch Set 4 : Add comment #MessagesTotal messages: 18
Unfortunately this doesn't seem to work.
You can put an alert in the injected function to check if it executes.
function injected(eventName, injectedIntoContentWindow)
{
alert("Hello");
...
It works on eyeo.com, not on github.com
I'm seeing this error in the console:
Loading failed for the <script> with source
“blob:https://github.com/8a1849a5-6f62-df4c-a870-19b92e1ad807”.
Content Security Policy: The page’s settings blocked the loading of a resource
at blob:https://github.com/8a1849a5-6f62-df4c-a870-19b92e1ad807 (“script-src
https://assets-cdn.github.com”).
Did it work for you?
This doesn't work because of "script-src assets-cdn.github.com" in the header. It can only load scripts from that domain.
On 2017/10/27 18:08:53, Manish Jethani wrote: > This doesn't work because of "script-src assets-cdn.github.com" in the header. > It can only load scripts from that domain. I should have clarified before: you need to Firefox 58 (Nightly). With those patches https://bugzilla.mozilla.org/show_bug.cgi?id=1406278 and https://bugzilla.mozilla.org/show_bug.cgi?id=1407056
On 2017/10/27 19:11:53, tschuster wrote: > On 2017/10/27 18:08:53, Manish Jethani wrote: > > This doesn't work because of "script-src assets-cdn.github.com" in the header. > > It can only load scripts from that domain. > > I should have clarified before: you need to Firefox 58 (Nightly). With those > patches https://bugzilla.mozilla.org/show_bug.cgi?id=1406278 and > https://bugzilla.mozilla.org/show_bug.cgi?id=1407056 Sorry, I did not realize this is based on a change in 58. My comments inline. https://codereview.adblockplus.org/29590611/diff/29590614/inject.preload.js File inject.preload.js (right): https://codereview.adblockplus.org/29590611/diff/29590614/inject.preload.js#n... inject.preload.js:402: let blob = new Blob([code], { type: "text/javascript" }); As per the style guide [1] we don't add spaces immediately after opening or before closing an object literal. [1]: https://adblockplus.org/coding-style#javascript Also the official MIME type for JavaScript as per RFC 4329 is "application/javascript" (which we are using above). But I think we should leave the MIME type out here, it's not required because there's already a directive on the script tag that tells the browser how to interpret the content.
By the way, since this is a change in adblockpluschrome, you should include both Sebastian and Dave, one of them in the reviewers list.
Interesting, I never realized that application/javascript is the official MIME type. Added the reviewers.
What if the CSP doesn't allow blob: URLs?
On 2017/11/03 20:56:55, Sebastian Noack wrote: > What if the CSP doesn't allow blob: URLs? The CSP doesn't matter anymore, because assigning to src from a WebExtension is now privileged.
On 2017/11/03 20:58:12, tschuster wrote: > On 2017/11/03 20:56:55, Sebastian Noack wrote: > > What if the CSP doesn't allow blob: URLs? > > The CSP doesn't matter anymore, because assigning to src from a WebExtension is > now privileged. I see. Looks good to me then (besides a small bit). However, I wonder whether this is the only place where we inject a dynamically created <script> element? Also is the stylesheet which we inject on.older Firefox versions effected as well?
https://codereview.adblockplus.org/29590611/diff/29596693/inject.preload.js File inject.preload.js (right): https://codereview.adblockplus.org/29590611/diff/29596693/inject.preload.js#n... inject.preload.js:402: let blob = new Blob([code]); Nit: At least this temporary variable can be avoided.
https://codereview.adblockplus.org/29590611/diff/29596693/inject.preload.js File inject.preload.js (right): https://codereview.adblockplus.org/29590611/diff/29596693/inject.preload.js#n... inject.preload.js:402: let blob = new Blob([code]); On 2017/11/03 21:12:15, Sebastian Noack wrote: > Nit: At least this temporary variable can be avoided. Yea, how about this? let url = URL.createObjectURL(new Blob([ "(" + injected + ")('" + randomEventName + "');" ])); Also would you mind adding a comment to explain why we're using this approach to inject the code?
On 2017/11/06 11:32:51, kzar wrote: > https://codereview.adblockplus.org/29590611/diff/29596693/inject.preload.js > File inject.preload.js (right): > > https://codereview.adblockplus.org/29590611/diff/29596693/inject.preload.js#n... > inject.preload.js:402: let blob = new Blob([code]); > On 2017/11/03 21:12:15, Sebastian Noack wrote: > > Nit: At least this temporary variable can be avoided. > > Yea, how about this? > > let url = URL.createObjectURL(new Blob([ > "(" + injected + ")('" + randomEventName + "');" > ])); > > Also would you mind adding a comment to explain why we're using this approach to > inject the code? Patch Set 4 implements all of these suggestions.
LGTM
LGTM
This issue can be closed now.
On 2018/02/01 11:17:02, Manish Jethani wrote: > This issue can be closed now. This issue still appears in my list of incoming reviews. Tom, can you close this please? I can't edit it.
Message was sent while issue was closed.
On 2018/02/21 14:52:59, Manish Jethani wrote: > On 2018/02/01 11:17:02, Manish Jethani wrote: > > This issue can be closed now. > > This issue still appears in my list of incoming reviews. Tom, can you close this > please? I can't edit it. Sorry, closed it. 
|
