Opened 3 years ago

Closed 3 years ago

Last modified 5 weeks ago

#4770 closed defect (rejected)

Deprecated child-src content security policy used

Reported by: kzar Assignee:
Priority: P3 Milestone:
Module: Platform Keywords: externaldependency
Cc: sebastian, trev, mapx Blocked By:
Blocking: Platform: Chrome
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29372401/

Description (last modified by kzar)

Environment

Adblock Plus 1.12.4, Chrome 55, Debian stretch

How to reproduce

  1. Add the filter *$websocket,domain=kzar.co.uk
  2. Browse to http://csp.kzar.co.uk/

Observed behaviour

The content security policy connect-src http:; child-src http:; object-src http: is added.

Expected behaviour

A content security without the deprecated child-src directive should be added instead.

Notes

From https://w3c.github.io/webappsec-csp/#directive-child-src

The child-src directive is deprecated. Authors who wish to regulate nested browsing contexts and
workers SHOULD use the frame-src and worker-src directives, respectively.

Hints for testers

  • Ensure that no WebSocket connections are opened on my test page for Chrome 49 or 55 when the filter is added. (This is to check that we didn't break something.)
  • I'm not sure how you can test the exact CSP that's injected, frame-src vs child-src.

Change History (14)

comment:1 Changed 3 years ago by kzar

  • Owner set to kzar
  • Priority changed from Unknown to P3
  • Ready set

According to the docs child-src is now equivalent to frame-src + worker-src. I just tested in Chrome 49 and 55 however and neither recognise worker-src. Furthermore I can't figure out how to create a Worker using a data URI in Chrome without using a Blob and connect-src prevents that from working anyway.

So unless I missed something I think it's safe to just replace child-src with frame-src. (I've tested that change on Chrome 49 and 55 with my test page and all the connections are blocked.)

comment:2 Changed 3 years ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:3 Changed 3 years ago by abpbot

A commit referencing this issue has landed:
Issue 4770 - Replace deprecated child-src CSP directive

comment:4 Changed 3 years ago by kzar

  • Description modified (diff)
  • Milestone set to Adblock-Plus-for-Chrome-Opera-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:5 Changed 3 years ago by kzar

  • Keywords externaldependency added
  • Milestone Adblock-Plus-1.13-for-Chrome-Opera deleted
  • Ready unset
  • Resolution fixed deleted
  • Status changed from closed to reopened

frame-src is not equivalent to child-src and current versions of Chrome do not yet support worker-src. We're going to revert this change for #4866, we can fix this when Chrome supports worker-src.

comment:6 Changed 3 years ago by kzar

  • Owner kzar deleted

comment:7 Changed 3 years ago by sebastian

I suppose this is issue is no longer relevant, if with #5027 we no longer inject CSP headers on Chrome >=58?

Last edited 3 years ago by sebastian (previous) (diff)

comment:8 Changed 3 years ago by kzar

Assuming Chrome < 58 all support the child-src directive and the new API in Chrome 58 really does allow us to block all WebSocket connections without resorting to Content Security Policies then I agree.

I think we should confirm that first before closing this issue, like I mentioned in the other issue I fear the WebSocket opened from the SharedWorker on my test page won't be blocked with the new API and so we'll need to keep the CSP code for the time being to avoid regressing there.

comment:9 Changed 3 years ago by sebastian

Now, I understand what you were talking about. It seems that SharedWorkers don't get associated with the context in which they got created. That kinda makes sense as SharedWorkers can be requested and shared by multiple documents. This limitation, however, isn't specific to WebSockets (what if somebody just uses XHR/fetch in the SharedWorker?). So I don't think we should keep performing this workaround, holding back #5027, only to bypass this limitation specifically for WebSockets.

comment:10 Changed 3 years ago by kzar

Well you're right, but in practice this will mean a regression in practice. Filter list authors will no longer have a tool to stop these circumvention methods. I'd like to avoid that. Perhaps we need to consider a new filter type that explicitly injects these CSP directives, rather than shoehorning it into the $websocket type?

comment:11 Changed 3 years ago by sebastian

Yes, if this kind of filtering is important enough to keep supporting it, it should be separated from WebSocket blocking, adding a new generic filter option.

But perhaps we should first file a Chrome bug, and figure out whether the Chromium team would consider setting the tabId/frameId for SharedWorkers to correspond to the document that caused it to be created, even though there might be multiple documents using it. This would make such an option redundant, and also makes the webRequest API consistent with the CSP mechanism and developer tools.

Also, if necessary, filter list authors can still resort to element hiding. If element hiding shouldn't be powerful enough, however, this would be a much larger problem than requests in SharedWorkers, since websites that deploy such elaborate circumvention attempts, could as well bundle their ads with the actual content which renders request blocking useless anyway.

Last edited 3 years ago by sebastian (previous) (diff)

comment:12 Changed 3 years ago by sebastian

  • Resolution set to rejected
  • Status changed from reopened to closed

Closing this issue, as we established in #5027, to no longer inject this CSP (using deprecated features) on Chrome >=58, but to rely on the webRequest API instead. Eventually, we will get rid of this code altogether (or reimplement it in a different way).

comment:13 Changed 7 months ago by Sunny999

spam

Last edited 5 weeks ago by kzar (previous) (diff)

comment:14 Changed 8 weeks ago by rockson

spam

Last edited 5 weeks ago by kzar (previous) (diff)
Note: See TracTickets for help on using tickets.