Bug 69359 - CSP connect-src directive should block redirects
: CSP connect-src directive should block redirects
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Page Loading
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Daniel Bates
: BlinkMergeCandidate, InRadar, WebExposed
Depends on: 153622
Blocks: 85558
  Show dependency treegraph
 
Reported: 2011-10-04 11:34 PDT by Sam Weinig
Modified: 2016-02-08 17:27 PST (History)
11 users (show)

See Also:


Attachments
Patch and Layout Tests (84.79 KB, patch)
2016-02-05 13:17 PST, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch and Layout Tests (84.73 KB, patch)
2016-02-07 14:33 PST, Daniel Bates
bfulgham: review+
bfulgham: commit‑queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2011-10-04 11:34:48 PDT
As pointed out by Adam in bug 69353, we should be blocking redirect of XHR, EventSource and WebSockets if there is a connect-src directive.
Comment 1 Sam Weinig 2011-10-04 13:05:20 PDT
Actually, I don't think WebSockets support redirects, but the other two do.
Comment 2 Sam Weinig 2011-10-04 13:09:42 PDT
This is made slightly complicated by the fact that we don't get a chance to stop redirects in the same class that started the load.  I am told this is because we don't want to block an XHR on a worker from making progress, so all policy information has to be on the ThreadableLoader itself. 

One way we could do this is to make ThreadableLoader aware of either the whole ContentSecurityPolicy object (making access to it safe from multiple threads). Another is to just have a way to copy the relevant part of the policy into the loader, and have a mechanism for it provide reports.  There may be other ways, but I am currently leaning toward making the ContentSecurityPolicy thread safe.

Adam, your comments are appreciated.
Comment 3 Adam Barth 2011-10-04 15:35:44 PDT
I would just teach DocumentThreadableLoader about ContentSecurityPolicy.  It might be worth keeping the check you have in XMLHttpRequest.cpp so we can throw an exception in the non-redirect case.  In the redirect case, we can just treat it as a network error.
Comment 4 Adam Barth 2012-05-03 17:33:39 PDT
This isn't likely to be fixed for CSP 1.0.
Comment 5 Daniel Bates 2016-01-15 14:38:56 PST
*** Bug 116067 has been marked as a duplicate of this bug. ***
Comment 6 Daniel Bates 2016-01-15 14:40:20 PST
As mentioned by Ryosuke Niwa in bug 116067, comment 0, we should merge <https://chromium.googlesource.com/chromium/blink/+/2853f594838e8bf24813482ad02f87853cae4366>.
Comment 7 Radar WebKit Bug Importer 2016-01-27 20:12:59 PST
<rdar://problem/24383025>
Comment 8 Daniel Bates 2016-02-05 13:17:51 PST
Created attachment 270764 [details]
Patch and Layout Tests
Comment 9 Daniel Bates 2016-02-07 14:33:29 PST
Created attachment 270831 [details]
Patch and Layout Tests

Rebased patch following <https://bugs.webkit.org/show_bug.cgi?id=153622>.
Comment 10 WebKit Commit Bot 2016-02-07 14:36:12 PST
Attachment 270831 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/WorkerThreadableLoader.h:46:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Brent Fulgham 2016-02-08 10:05:19 PST
Looks like an EFL build error:

Last 500 characters of output:
/loader/DocumentThreadableLoader.cpp.o -c ../../Source/WebCore/loader/DocumentThreadableLoader.cpp
../../Source/WebCore/loader/DocumentThreadableLoader.cpp: In member function 'bool WebCore::DocumentThreadableLoader::isAllowedByContentSecurityPolicy(const WebCore::URL&)':
../../Source/WebCore/loader/DocumentThreadableLoader.cpp:451:1: error: control reaches end of non-void function [-Werror=return-type]
 }
 ^
cc1plus: all warnings being treated as errors
ninja: build stopped: subcommand failed.
Comment 12 Brent Fulgham 2016-02-08 10:18:43 PST
Comment on attachment 270831 [details]
Patch and Layout Tests

View in context: https://bugs.webkit.org/attachment.cgi?id=270831&action=review

r=me, thought you might ping an EFL person to see if they can help figure out why the build is failing.

Overall this looks good, but I had a couple of ideas/suggestions as I looked thing over, which you can choose to ignore if they don't seem useful.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:450
> +    }

For some reason, EFL thinks these new enums do not exist!

> Source/WebCore/loader/DocumentThreadableLoader.h:-42
> -#include <wtf/text/WTFString.h>

Thank you for getting rid of these! :-)

> Source/WebCore/loader/WorkerThreadableLoader.cpp:99
> +    ASSERT(contentSecurityPolicy);

Should we change the API to make these references, if they are never supposed to be nullptr?

> Source/WebCore/workers/AbstractWorker.cpp:46
> +URL AbstractWorker::resolveURL(const String& url, bool shouldBypassMainWorldContentSecurityPolicy, ExceptionCode& ec)

Would it make sense to pass the ContentSecurityPolicyEnforcement enum instead of a boolean for these uses? Maybe we only care about block/dont-block, so we don't need to worry about script versus other blocking scenarios.

> Source/WebCore/workers/Worker.cpp:97
> +    // FIXME: Enforce Content Security Policy child-src directive when shouldBypassMainWorldContentSecurityPolicy is false. See <https://bugs.webkit.org/show_bug.cgi?id=153562>.

Don't we use FIXME(153562) for this kind of thing?

> Source/WebCore/workers/WorkerGlobalScope.cpp:186
> +        // FIXME: Convert this to check the isolated world's Content Security Policy once webkit.org/b/104520 is solved.

FIXME(104520)
Comment 13 Daniel Bates 2016-02-08 17:23:09 PST
(In reply to comment #12)
> Comment on attachment 270831 [details]
> Patch and Layout Tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270831&action=review
> 
> r=me, thought you might ping an EFL person to see if they can help figure
> out why the build is failing.
> 
> Overall this looks good, but I had a couple of ideas/suggestions as I looked
> thing over, which you can choose to ignore if they don't seem useful.
> 
> > Source/WebCore/loader/DocumentThreadableLoader.cpp:450
> > +    }
> 
> For some reason, EFL thinks these new enums do not exist!
> 

[[
Last 500 characters of output:
/loader/DocumentThreadableLoader.cpp.o -c ../../Source/WebCore/loader/DocumentThreadableLoader.cpp
../../Source/WebCore/loader/DocumentThreadableLoader.cpp: In member function 'bool WebCore::DocumentThreadableLoader::isAllowedByContentSecurityPolicy(const WebCore::URL&)':
../../Source/WebCore/loader/DocumentThreadableLoader.cpp:451:1: error: control reaches end of non-void function [-Werror=return-type]
 }
]]
<https://webkit-queues.webkit.org/results/797253>

EFL is using cc1plus and it does not seem to be able to reason that the switch statement handles all cases.

Will add the following code after the switch statement DocumentThreadableLoader::isAllowedByContentSecurityPolicy() a==.

ASSERT_NOT_REACHED();
return false;


> [...]
> > Source/WebCore/loader/WorkerThreadableLoader.cpp:99
> > +    ASSERT(contentSecurityPolicy);
> 
> Should we change the API to make these references, if they are never
> supposed to be nullptr?
> 

We should look to pass this by references. Ideally, we should do this once we fix up SecurityContent to ensure it returns a reference. I suggest that we defer this task to separate bug/patch since we will need to change many places throughout WebCore.

> > Source/WebCore/workers/AbstractWorker.cpp:46
> > +URL AbstractWorker::resolveURL(const String& url, bool shouldBypassMainWorldContentSecurityPolicy, ExceptionCode& ec)
> 
> Would it make sense to pass the ContentSecurityPolicyEnforcement enum
> instead of a boolean for these uses? Maybe we only care about
> block/dont-block, so we don't need to worry about script versus other
> blocking scenarios.
> 

We only care about block/don't block. So, it is sufficient to use a boolean. The purpose of the enum class ContentSecurityPolicyEnforcement is to specify a particular CSP directive to enforce. It seems weird to make use of ContentSecurityPolicyEnforcement for this block/don't block decision.

> > Source/WebCore/workers/Worker.cpp:97
> > +    // FIXME: Enforce Content Security Policy child-src directive when shouldBypassMainWorldContentSecurityPolicy is false. See <https://bugs.webkit.org/show_bug.cgi?id=153562>.
> 
> Don't we use FIXME(153562) for this kind of thing?
> 

As far as I know we prefer to only prefix with "FIXME:" from <https://webkit.org/code-style-guidelines/#comments-fixme>. On another note, Xcode only recognizes FIXME comments of this form when showing them in the list of functions in a file.

> > Source/WebCore/workers/WorkerGlobalScope.cpp:186
> > +        // FIXME: Convert this to check the isolated world's Content Security Policy once webkit.org/b/104520 is solved.
> 
> FIXME(104520)

Ditto.
Comment 14 Daniel Bates 2016-02-08 17:27:01 PST
Committed r196283: <http://trac.webkit.org/changeset/196283>