Closed Bug 1195167 Opened 9 years ago Closed 9 years ago

Use channel->asyncOpen2 in FetchDriver.cpp

Categories

(Core :: DOM: Security, defect)

Product:
Core
Component:
DOM: Security
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: ckerschb, Assigned: sicking)

References

(
URL
)

Details

Attachments

(6 files, 2 obsolete files)

Part 1: Let necko handle all protocols
15.70 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
part 2: Remove redundant aCORSFlag argument and instead use mCORSFlagEverSet
6.32 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
part 3: Remove more scheme-specific handling from FetchDriver
1.94 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
part 4: Remove FetchDriver::BasicFetch since it is empty
5.82 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
part 6: Some code simplification since necko handles fetch recursion
6.69 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
part 5: Make FetchDriver use AsyncOpen2
31.37 KB, patch
bkelly
: review+
Details | Diff | Splinter Review 
      No description provided.
Assignee: nobody → mozilla
Blocks: 1182535
Please note that I've found a problem with FetchDriver's current use of nsCORSListenerProxy in bug 1184607.  Mainly, it should not use nsCORSListenerProxy if mRequest.Mode() != RequestMode::Cors.  This is because the proxy overwrites the CorsMode flag which prevents us from correctly doing fetch(url, {mode: 'same-origin'}).

If we end up setting REQUIRE_SAME_ORIGIN_* in the security flags in mRequest.Mode() is RequestMode::Same_origin, then it may be ok to still use the proxy.

Can you flag me for feedback on the patch when you have it ready?
URL: https://bugzilla.mozilla.org/show_bug...
(In reply to Ben Kelly [:bkelly] from comment #1)
> Please note that I've found a problem with FetchDriver's current use of
> nsCORSListenerProxy in bug 1184607.  Mainly, it should not use
> nsCORSListenerProxy if mRequest.Mode() != RequestMode::Cors.  This is
> because the proxy overwrites the CorsMode flag which prevents us from
> correctly doing fetch(url, {mode: 'same-origin'}).
> 
> If we end up setting REQUIRE_SAME_ORIGIN_* in the security flags in
> mRequest.Mode() is RequestMode::Same_origin, then it may be ok to still use
> the proxy.

Thanks for the update.
 
> Can you flag me for feedback on the patch when you have it ready?

Will do, but might take a few more days.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f7b6ec7bd4d

Passes the dom/tests/mochitest/fetch mochitests locally, but still needs some work.
Assignee: mozilla → jonas
Summary: Use channel->ascynOpen2 dom/fetch/FetchDriver.cpp → Use channel->asyncOpen2 in FetchDriver.cpp
Ben, Ehsan, feel free to divide up the reviews between you. I wasn't sure who to assign to, so I assigned to Ben since I've chatted more with him about this code lately.
Slightly updated version. Hopefully you haven't started reviewing yet?
Attachment #8675202 - Attachment is obsolete: true
Attachment #8675202 - Flags: review?(bkelly)
I'll review this on Monday.  I'd like to take it since I've been looking at the tainting stuff recently.  Thanks.
Comment on attachment 8675197 [details] [diff] [review]
Part 1: Let necko handle all protocols

Review of attachment 8675197 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

It seems all the effected protocols (about:, blob:, and data:) are tested in here:

  https://dxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/fetch/test_fetch_basic.js

But a test for about: protocol content-length is missing.  Can you add one?  It seems there should be no content-length header for about: pages.

Also, it seems that this change is going to enable file:, ftp:, and other schemes.  The fetch spec has some weasel words for file: and ftp:

  https://fetch.spec.whatwg.org/#concept-basic-fetch

But it says other protocols should fail.

Is it worth blocking these other protocol schemes?

::: dom/fetch/FetchDriver.cpp
@@ +209,5 @@
>  
>  nsresult
>  FetchDriver::BasicFetch()
>  {
> +  return HttpFetch();

Since all protocols are going through the channel AsyncOpen(), can you please remove the unnecessary dispatch in FetchDriver::Fetch()?

  https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp?case=true&from=FetchDriver.cpp#89

And then you can dupe bug 1206079 to here.
Attachment #8675197 - Flags: review?(bkelly) → review+
Comment on attachment 8675198 [details] [diff] [review]
part 2: Remove redundant aCORSFlag argument and instead use mCORSFlagEverSet

Review of attachment 8675198 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/fetch/FetchDriver.h
@@ -126,5 @@
>    // Utility since not all cases need to do any post processing of the filtered
>    // response.
>    nsresult FailWithNetworkError();
>    nsresult SucceedWithResponse();
> -  nsresult DoesNotRequirePreflight(nsIChannel* aChannel);

Removing this seems unrelated to this patch.  Is the intention that AsyncOpen2() will handle this logic for us?  (Or we should put the call to the this back in the redirect callback.)
Attachment #8675198 - Flags: review?(bkelly) → review+
Comment on attachment 8675199 [details] [diff] [review]
part 3: Remove more scheme-specific handling from FetchDriver

Review of attachment 8675199 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/fetch/FetchDriver.cpp
@@ -140,4 @@
>  
> -  // request's current url's origin is request's origin and the CORS flag is unset
> -  // request's current url's scheme is "data" and request's same-origin data-URL flag is set
> -  // request's current url's scheme is "about"

Why drop the spec language?  Its a helpful landmark when trying to trace through both the spec and our implementation.
Attachment #8675199 - Flags: review?(bkelly) → review+
Comment on attachment 8675200 [details] [diff] [review]
part 4: Remove FetchDriver::BasicFetch since it is empty

Review of attachment 8675200 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/fetch/FetchDriver.cpp
@@ +138,5 @@
>    if (!mCORSFlagEverSet &&
>        (NS_IsAboutBlank(requestURI) ||
>         NS_SUCCEEDED(mPrincipal->CheckMayLoad(requestURI, false /* report */,
>                                               true /*allowIfInheritsPrincipal*/)))) {
> +    return MainFetchOp(HTTP_FETCH, false /* cors */, false /* preflight */);

Please add a comment here that what the spec calls "basic fetch" is handled within our necko channel code.  Therefore everything goes through HTTP Fetch.  (Might be less confusing if got rid of the name HTTP_FETCH at some point...)

Also please add the comment at the other BASIC_FETCH you removed in this method.
Attachment #8675200 - Flags: review?(bkelly) → review+
> It seems all the effected protocols (about:, blob:, and data:) are tested in
> here:
> 
>  
> https://dxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/fetch/
> test_fetch_basic.js
> 
> But a test for about: protocol content-length is missing.  Can you add one? 
> It seems there should be no content-length header for about: pages.

All about: pages except about:blank should fail, right? Shouldn't about:blank have a Content-Length: 0 header? I think that's what happens now.

> But it says other protocols should fail.
> 
> Is it worth blocking these other protocol schemes?

I don't think it's worth blocking other protocols. Generally speaking other protocols that addons are explicitly choosing to expose, which I hope that the fetch spec doesn't forbid. And if it does, addons are essentially proxy for the user, and user overrides spec.

> ::: dom/fetch/FetchDriver.cpp
> @@ +209,5 @@
> >  
> >  nsresult
> >  FetchDriver::BasicFetch()
> >  {
> > +  return HttpFetch();
> 
> Since all protocols are going through the channel AsyncOpen(), can you
> please remove the unnecessary dispatch in FetchDriver::Fetch()?

Parts 4 and 6 clean this up, but they do leave the async thread dispatch. Without that we risk synchronously reenter the caller through mObserver. Is that safe?

Maybe we should discuss in bug 1206079?
> > -  nsresult DoesNotRequirePreflight(nsIChannel* aChannel);
> 
> Removing this seems unrelated to this patch.  Is the intention that
> AsyncOpen2() will handle this logic for us?  (Or we should put the call to
> the this back in the redirect callback.)

DoesNotRequirePreflight is never called anywhere. So this was just dead code removal.
(In reply to Ben Kelly [:bkelly] from comment #16)
> Comment on attachment 8675199 [details] [diff] [review]
> part 3: Remove more scheme-specific handling from FetchDriver
> 
> Review of attachment 8675199 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/fetch/FetchDriver.cpp
> @@ -140,4 @@
> >  
> > -  // request's current url's origin is request's origin and the CORS flag is unset
> > -  // request's current url's scheme is "data" and request's same-origin data-URL flag is set
> > -  // request's current url's scheme is "about"
> 
> Why drop the spec language?  Its a helpful landmark when trying to trace
> through both the spec and our implementation.

I generally didn't know what to do with a bunch of the spec related comments when the code that they relate to went away. Generally feels like most of the comments belong inside necko rather than in FetchDriver since this is moving us to using Necko's fetch implementation.

Happy to leave whatever comments you want in whatever place you want though. I'll put this one back for now.
Comment on attachment 8675201 [details] [diff] [review]
part 5: Make FetchDriver use AsyncOpen2

Review of attachment 8675201 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks good, but I think credentials handling is not right for no-cors requests.

Note, I have some unlanded tests in another bug that could be adapted to check this condition.

::: dom/fetch/FetchDriver.cpp
@@ +208,5 @@
>    // new cookies sent by the server from being stored.  This value will
>    // propagate across redirects, which is what we want.
> +  const nsLoadFlags credentialsFlag =
> +    mRequest->GetCredentialsMode() == RequestCredentials::Omit ?
> +    nsIRequest::LOAD_ANONYMOUS : 0;

I think this breaks the "no-cors" mode with "same-origin" credentials case.  If the initial URL is cross-origin we need to set LOAD_ANONYMOUS for that case here.

@@ -791,5 @@
>  
>    // HTTP Fetch Step 9, "redirect status". We only unset this for spec
>    // compatibility. Any actions we take on mRequest here do not affect what the
>    //channel does.
> -  mRequest->UnsetSameOriginDataURL();

Can you amend the comment to indicate where this is handled now?

@@ +810,5 @@
>  
> +  if (RequirePreflight()) {
> +    // We can't handle redirects that require preflight yet.
> +    // This is especially true for no-cors requests, which much always be
> +    // blocked if they require preflight.

This comment is a bit confusing.  Preflights are a CORS thing and don't apply to no-cors.  You should be able to assert that mode == "cors" here.  It should not be possible to generate a no-cors request with a non-simple method or unsafe header.

I'm ok just returning failure here, though.  I'd just like to adjust the comment.

@@ +826,2 @@
>    if (mRequest->GetCredentialsMode() == RequestCredentials::Same_origin &&
> +      mRequest->Mode() == RequestMode::No_cors) {

The makes us set LOAD_ANONYMOUS on a same-origin redirect for no-cors requests.  I don't think thats what we want.  It should only set the LOAD_ANONYMOUS on cross-origin redirect.  I think we lack a test case for no-cors redirects like this.
Attachment #8675201 - Flags: review?(bkelly) → review-
Comment on attachment 8675266 [details] [diff] [review]
part 6: Some code simplification since necko handles fetch recursion

Review of attachment 8675266 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/fetch/FetchDriver.cpp
@@ +82,4 @@
>  
> +  nsCOMPtr<nsIRunnable> r =
> +    NS_NewRunnableMethod(this, &FetchDriver::ContinueFetch);
> +  return NS_DispatchToCurrentThread(r);

Please just call ContinueFetch() or move the code from ContinueFetch() into here.  I don't think we need this async dispatch any more.  See my comment from P1.

@@ +673,4 @@
>      // We proceed as usual here, since we've already created a successful response
>      // from OnStartRequest.
> +  }
> +  else {

nit: style for else on same line as preceding bracket
Attachment #8675266 - Flags: review?(bkelly) → review+
> Is it worth blocking these other protocol schemes?

I do think that we do have a few more protocols that we do actually expose to content right now. Primarily a few of our about: pages are loadable by content I think. But I don't think they are more important to block in the fetch() API than in <img>, <iframe> or XMLHttpRequest. So lets work on them separately, and remove them from being content exposed in general instead.
(In reply to Jonas Sicking (:sicking) from comment #18)
> All about: pages except about:blank should fail, right? Shouldn't
> about:blank have a Content-Length: 0 header? I think that's what happens now.

Maybe its a spec bug, but Basic Fetch currently says this for about:blank:

"return a response whose header list consist of a single header whose name is `Content-Type`"

> Parts 4 and 6 clean this up, but they do leave the async thread dispatch.
> Without that we risk synchronously reenter the caller through mObserver. Is
> that safe?
> 
> Maybe we should discuss in bug 1206079?

Hmm, I wasn't aware of the issue with mObserver.  We can handle later in bug 1206079.
> > +    return MainFetchOp(HTTP_FETCH, false /* cors */, false /* preflight */);
> 
> Please add a comment here that what the spec calls "basic fetch" is handled
> within our necko channel code.  Therefore everything goes through HTTP
> Fetch.  (Might be less confusing if got rid of the name HTTP_FETCH at some
> point...)
> 
> Also please add the comment at the other BASIC_FETCH you removed in this
> method.

I'll do this in Part 5 since this function is dramatically rewritten there.
(In reply to Ben Kelly [:bkelly] from comment #21)
> ::: dom/fetch/FetchDriver.cpp
> @@ +208,5 @@
> >    // new cookies sent by the server from being stored.  This value will
> >    // propagate across redirects, which is what we want.
> > +  const nsLoadFlags credentialsFlag =
> > +    mRequest->GetCredentialsMode() == RequestCredentials::Omit ?
> > +    nsIRequest::LOAD_ANONYMOUS : 0;
> 
> I think this breaks the "no-cors" mode with "same-origin" credentials case. 
> If the initial URL is cross-origin we need to set LOAD_ANONYMOUS for that
> case here.

Great catch!!

> @@ +810,5 @@
> >  
> > +  if (RequirePreflight()) {
> > +    // We can't handle redirects that require preflight yet.
> > +    // This is especially true for no-cors requests, which much always be
> > +    // blocked if they require preflight.
> 
> This comment is a bit confusing.  Preflights are a CORS thing and don't
> apply to no-cors.  You should be able to assert that mode == "cors" here. 
> It should not be possible to generate a no-cors request with a non-simple
> method or unsafe header.
> 
> I'm ok just returning failure here, though.  I'd just like to adjust the
> comment.

Ooh, this is also a really great catch. The code is actually wrong here since RequirePreflight only returns 'true' for mode==cors. But we need to block no-cors requests that are unsafe.

I guess maybe we don't *have* to if code elsewhere ensures that no-cors requests can't use unsafe methods or headers. But it seems like a good idea to enforce that here too.

> @@ +826,2 @@
> >    if (mRequest->GetCredentialsMode() == RequestCredentials::Same_origin &&
> > +      mRequest->Mode() == RequestMode::No_cors) {
> 
> The makes us set LOAD_ANONYMOUS on a same-origin redirect for no-cors
> requests.  I don't think thats what we want.  It should only set the
> LOAD_ANONYMOUS on cross-origin redirect.  I think we lack a test case for
> no-cors redirects like this.

Yup. I forgot to add a check for mHasBeenCrossSite.
Comment on attachment 8675882 [details] [diff] [review]
part 5: Make FetchDriver use AsyncOpen2

Review of attachment 8675882 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: dom/fetch/FetchDriver.cpp
@@ +163,2 @@
>    }
> + 

nit: trailing whitespace

@@ +226,5 @@
> +  const nsLoadFlags credentialsFlag =
> +    (mRequest->GetCredentialsMode() == RequestCredentials::Omit ||
> +    (mHasBeenCrossSite &&
> +     mRequest->GetCredentialsMode() == RequestCredentials::Same_origin &&
> +     mRequest->Mode() == RequestMode::No_cors)) ?

I think checking for opaque tainting:

  mRequest->GetResponseTainting() !=InternalRequest::RESPONSETAINT_OPAQUE)

is simpler than the multi-variable:

  (mHasBeenCrossSite && mRequest->Mode() == RequestMode::No_cors)

But I guess they should be equivalent.

I think it might be nice to one day remove mHasBeenCrossSite, though.  It seems its information is fully subsumed within the tainting value.

@@ +442,5 @@
> +bool
> +FetchDriver::IsUnsafeRequest()
> +{
> +  return mHasBeenCrossSite &&
> +         (mRequest->Mode() == RequestMode::Cors_with_forced_preflight ||

I thought we removed Cors_with_forced_preflight.  Maybe those patches haven't landed yet.

@@ +840,5 @@
>  
> +  // Possibly set the LOAD_ANONYMOUS flag on the channel.
> +  if (mHasBeenCrossSite &&
> +      mRequest->GetCredentialsMode() == RequestCredentials::Same_origin &&
> +      mRequest->Mode() == RequestMode::No_cors) {

Please write a follow-up bug to write tests for no-cors fetch() with credentials set to same-origin.
Attachment #8675882 - Flags: review?(bkelly) → review+
> I think checking for opaque tainting:
> 
>   mRequest->GetResponseTainting() !=InternalRequest::RESPONSETAINT_OPAQUE)
> 
> is simpler than the multi-variable:
> 
>   (mHasBeenCrossSite && mRequest->Mode() == RequestMode::No_cors)
> 
> But I guess they should be equivalent.

I prefer the former since I think that more directly maps to the reason why we're setting the flag. I.e. we're removing cookies because cookies has been requested for same-site only, and this request is going cross origin, and the CORS code isn't clearing the cookies.

To me that was more direct than understanding that that happened to coincide with the request getting opaque tainted.

I agree that it's more code, but since it's security related I favored clarity over anything else.

Hope that's ok?

> I think it might be nice to one day remove mHasBeenCrossSite, though.  It
> seems its information is fully subsumed within the tainting value.

Definitely. I have a plan, but I'd like to understand your plan with regards to tainting before I start writing patches.

> @@ +442,5 @@
> > +bool
> > +FetchDriver::IsUnsafeRequest()
> > +{
> > +  return mHasBeenCrossSite &&
> > +         (mRequest->Mode() == RequestMode::Cors_with_forced_preflight ||
> 
> I thought we removed Cors_with_forced_preflight.  Maybe those patches
> haven't landed yet.

Bug 1215746. The patch there sits on top of these patches, so I'll land them together.
Can you file that follow-up for the no-cors credentials tests?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: