839235 - shouldLoad called twice for Content Policies

Status: NEW

(Core :: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

Might be one for the pre-flight request and one for the actual request.  Sid says CSP had the same issue.  Sid, can you provide the bug number for that?

One side affect for this is that the error console shows the blocked content twice in bug 837351.

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

It's not limited to CSP, and it looks like the CSP bug isn't fixed.  See bug 766536 and bug 612921 and bug 614718.

bz suggests in bug 612921 comment 11 that we might want to add to the API whether a request is a speculative load or not.  I think that would fix this issue.

Is this double-calling happening for all loads or just some?

Comment 2

[Tanvi Vyas[:tanvi]]

This happens for all calls to content policy.  This is because the first call is for the preload and the second call is at actual execution time.  There could be a case where the first preload call to shouldLoad results in content being blocked and the second call results in the content being allowed (ex: for script-nonce where we don't know if the nonce matches until we parse the html).

What about the reverse case?  Is there a case today where a preload is allowed but a subsequent call to shouldLoad at rendering time is blocked?

I can think of one case where this might happen in the future.  With CSP meta policies, the meta policy can further restrict the CSP policy sent in the header.  We may not detect the meta policy until after preload time.  Hence a preload that succeeded could subsequently be blocked by the stricter policy from the meta tag.  (In this case, the network request for the content has already gone out with cookies, etc.)  Firefox hasn't implemented meta policy support yet, AFAIK.

Comment 3

[Tanvi Vyas[:tanvi]]

Correction from Garrett - the duplicate calls are only for images, stylesheets, and scripts.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I guess this belongs more in this bug:

So that I understand this stuff better, is the following correct

* We only get two calls into ShouldLoad if the first load is denied.
* If the prescanner-load is allowed to go through, then when we go do the "real" load we simply grab the
  existing load and don't call any content policies.
* The reason the second load happens is because if the prescan security checks fail, no channel is
  created and no preload data is set up. We simply act like if the prescanner hadn't found anything. So
  when the real load is about to happen, we don't find a failed load, we find nothing, and so a new load
  is started, which of course will go through security checks.
* If a preload happens due to a server error (like a 500), we don't drop that preload on the floor.
  Instead when the real load is about to happen it simply grabs the existing, failed, preload and we act
  like the real load failed.

If *all* of the above is correct, then I could think of two possible solutions here:

A) If a preload fails the security checks, create a fake channel which simply acts like a network error,
   then start a preload from that channel. When the real load is about to start, it fails with a network
   error.
B) Wait for the "new necko security hooks" architecture since it will solve this problem.

Comment 5

[Tanvi Vyas[:tanvi]]

(In reply to Jonas Sicking (:sicking) from comment #4)
> * We only get two calls into ShouldLoad if the first load is denied.
> * If the prescanner-load is allowed to go through, then when we go do the
> "real" load we simply grab the
>   existing load and don't call any content policies.
The above two statements are incorrect.  If the first load (the preload) is allowed, we still do a second call to the content policies.

For a script, we do the first preload call from nsScriptLoader.cpp with this set of calls
PreloadURI->StartLoad->ShouldLoadScript->CheckContentPolicy

If the script is allowed at preload time, we still check content policies again with this set of calls:
AttemptToExecute->MaybeProcessScirpt->ProcessScriptElement->CheckContentPolicy

If the script is blocked at preload time, we check content policies again with this set of calls:
AttemptToExecute->MaybeProcessScript->ProcessScriptElement->StartLoad->ShouldLoadScript->CheckContentPolicy

> * The reason the second load happens is because if the prescan security
> checks fail, no channel is
>   created and no preload data is set up. We simply act like if the
> prescanner hadn't found anything. So
>   when the real load is about to happen, we don't find a failed load, we
> find nothing, and so a new load
>   is started, which of course will go through security checks.
This sounds right - http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsScriptLoader.cpp#624

> * If a preload happens due to a server error (like a 500), we don't drop
> that preload on the floor.
>   Instead when the real load is about to happen it simply grabs the
> existing, failed, preload and we act
>   like the real load failed.
I don't know about this one.

> If *all* of the above is correct, then I could think of two possible
> solutions here:
> 
> A) If a preload fails the security checks, create a fake channel which
> simply acts like a network error,
>    then start a preload from that channel. When the real load is about to
> start, it fails with a network
>    error.
> B) Wait for the "new necko security hooks" architecture since it will solve
> this problem.
Playing with this in the patch to bug 1006881 for scripts, the preload generates a call to AsyncOpen2, and hence to the content policies.  But ProcessScriptElement() does not end up calling AsyncOpen2 if the preload is allowed (we lose the second call to content policies in the case where the preload goes through).  We need to figure out if we need to maintain the current behavior and call the content policies a second time, or if we are happy with the decision that occurs during preload time.