1191645 - Use channel->ascynOpen2 in dom/base/nsSyncLoadService.cpp

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1191645_asyncOpen2_syncloadservice.patch

I suppose that should work if we pass the CORS flag to the new channel. Right?

Comment 2

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

Comment on attachment 8644122 [details] [diff] [review]
bug_1191645_asyncOpen2_syncloadservice.patch

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

You need to also remove contentpolicy checks at the callsites which call into this code. For example

http://mxr.mozilla.org/mozilla-central/source/dom/xslt/xslt/txMozillaStylesheetCompiler.cpp#669

Comment 3

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

http://mxr.mozilla.org/mozilla-central/search?string=nsSyncLoadService

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1191645_asyncOpen2_syncloadservice.patch

Jonas, it seems that TYPE_OTHER is not appropriate when creating a channel within LoadDocument. We could extend LoadDocument by an additional argument if we have different contentPolicyTypes on each callsite. At the moment I am not sure I have found the most matching one on each callsite. What do you think?

Once we have figured out what contentPolicyTypes to use we can update nsContentSecuritymanager to pass the right mime-type-guess, node, etc to contentPolicies.

Comment 5

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

I'm not sure what the question to me here is. Adding additional parameters so that we can get the "correct" contentpolicy type sounds good.

Where "correct" means "same as we currently use".

You also need to pass in the system principal rather than null for the loadingPrincipal if aLoaderPrincipal is null.

Comment 6

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

You'll also have to create a new internal type for XSLT stylesheets :(

Comment 7

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

Looking at the code, it looks like there will always be a principal, so you can assert that and then rely on it. And you can use CORS mode always since that allows cross-origin loads when the loadingprincipal is the system principal.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1191645_asyncOpen2_syncloadservice.patch

Two things that are important to notice:
1) LoadDocument(nsIChannel* ...) already uses 'aLoderPrincipal' as the loadingPrincipal. Hence we can drop that argument and query that information from the Loadinfo. In fact the triggeringPrincipal and the loadingPrincipal match at the moment. To be more future proof we should query the triggeringPrincipal from the Loadinfo instead of the loadingPrincipal and use the triggeringPrincipal to query the referrer.

2) We can not *always* use nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS on the channel. If we would do that we would hit the following assertion. So, we should check if the load 'isSync' or not and change the security flag based on that information.

Jonas, are we missing something?

Assertion failure: aInAndOutListener (can not perform CORS checks without a listener), at /home/ckerschb/moz/mc/dom/security/nsContentSecurityManager.cpp:96
#01: DoCORSChecks(nsIChannel*, nsILoadInfo*, nsCOMPtr<nsIStreamListener>&) (/home/ckerschb/moz/mc/dom/security/nsContentSecurityManager.cpp:96)
#02: nsContentSecurityManager::doContentSecurityCheck(nsIChannel*, nsCOMPtr<nsIStreamListener>&) (/home/ckerschb/moz/mc/dom/security/nsContentSecurityManager.cpp:347)
#03: nsBaseChannel::Open2(nsIInputStream**) (/home/ckerschb/moz/mc/netwerk/base/nsBaseChannel.cpp:621)
#04: non-virtual thunk to nsBaseChannel::Open2(nsIInputStream**) (/home/ckerschb/moz/mc-obj-dbg/netwerk/base/Unified_cpp_netwerk_base1.cpp:624)
#05: nsSyncLoader::PushSyncStream(nsIStreamListener*) (/home/ckerschb/moz/mc/dom/base/nsSyncLoadService.cpp:248)
#06: nsSyncLoader::LoadDocument(nsIChannel*, bool, bool, mozilla::net::ReferrerPolicy, nsIDOMDocument**) (/home/ckerschb/moz/mc/dom/base/nsSyncLoadService.cpp:189)
#07: nsSyncLoadService::LoadDocument(nsIURI*, unsigned int, nsIPrincipal*, nsILoadGroup*, bool, mozilla::net::ReferrerPolicy, nsIDOMDocument**) (/home/ckerschb/moz/mc/dom/base/nsSyncLoadService.cpp:329)
#08: nsXMLPrettyPrinter::PrettyPrint(nsIDocument*, bool*) (/home/ckerschb/moz/mc/dom/xml/nsXMLPrettyPrinter.cpp:104)
#09: nsXMLContentSink::MaybePrettyPrint() (/home/ckerschb/moz/mc/dom/xml/nsXMLContentSink.cpp:212)
#10: nsXMLContentSink::DidBuildModel(bool) (/home/ckerschb/moz/mc/dom/xml/nsXMLContentSink.cpp:306)
#11: non-virtual thunk to nsXMLContentSink::DidBuildModel(bool) (/home/ckerschb/moz/mc-obj-dbg/dom/xml/Unified_cpp_dom_xml0.cpp:335)
#12: nsParser::DidBuildModel(nsresult) (/home/ckerschb/moz/mc/parser/htmlparser/nsParser.cpp:901)
#13: nsParser::ResumeParse(bool, bool, bool) (/home/ckerschb/moz/mc/parser/htmlparser/nsParser.cpp:1508)
#14: nsParser::OnStopRequest(nsIRequest*, nsISupports*, nsresult) (/home/ckerschb/moz/mc/parser/htmlparser/nsParser.cpp:1878)
#15: non-virtual thunk to nsParser::OnStopRequest(nsIRequest*, nsISupports*, nsresult) (/home/ckerschb/moz/mc-obj-dbg/parser/htmlparser/Unified_cpp_parser_htmlparser0.cpp:1893)

Comment 9

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

Comment on attachment 8672792 [details] [diff] [review]
bug_1191645_asyncOpen2_syncloadservice.patch

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

::: dom/base/nsSyncLoadService.cpp
@@ +299,5 @@
>  }
>  
>  /* static */
>  nsresult
> +nsSyncLoadService::LoadDocument(nsIURI *aURI, nsContentPolicyType aContentPolicyType,

too long line

@@ +316,5 @@
>                                  aURI,
>                                  aLoaderPrincipal,
> +                                isSync
> +                                  ? nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS
> +                                  : nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS,

We shouldn't determine policy based on what URL is being loaded. That defeats the purpose of having a policy.

Do different callers do different security checks before calling into this code? If so we might need to pass in security flags.

What security checks do the various callsites do right now before calling the syncloader code? The goal is to remove those checks and replace them with using the appropriate flags here.

::: dom/base/nsSyncLoadService.h
@@ +37,5 @@
>       *                    content type.
>       * @param referrerPolicy Referrer policy.
>       * @param aResult [out] The document loaded from the URI.
>       */
> +    static nsresult LoadDocument(nsIURI *aURI, nsContentPolicyType aContentPolicyType,

Too long line.

::: dom/xslt/xml/txXMLParser.cpp
@@ +35,5 @@
>      // the document.
>      nsIDOMDocument* theDocument = nullptr;
>      nsAutoSyncOperation sync(loaderDocument);
>      rv = nsSyncLoadService::LoadDocument(documentURI,
> +                                         nsIContentPolicy::TYPE_XSLT,

This can actually be either TYPE_XSLT or TYPE_XMLHTTPREQUEST.

Mind filing a followup to make sure all callers of this function passes in a type?

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1191645_asyncOpen2_syncloadservice.patch

(In reply to Jonas Sicking (:sicking) from comment #9)
 
> @@ +316,5 @@
> >                                  aURI,
> >                                  aLoaderPrincipal,
> > +                                isSync
> > +                                  ? nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS
> > +                                  : nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS,
> 
> We shouldn't determine policy based on what URL is being loaded. That
> defeats the purpose of having a policy.
> 
> Do different callers do different security checks before calling into this
> code? If so we might need to pass in security flags.

The problem is that in the current code all callers of LoadDocument pass 'aLoaderPrincipal'. Within LoadDocument the code set up CORS if 'aLoaderPrincipal' was not null. In other words, the CORSListeners were always set up. Now let's have a closer look; there are 3 callsites to LoadDocument:

1) dom/xml/nsXMLPrettyPrinter.cpp
* passed aLoaderPrincipal, and hence set up CORS
* uses nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS
* if we would use nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS then test dom/xml/test/test_bug691215.html would crash (see stacktrace in one of the previous comments).

2) dom/xslt/xml/txXMLParser.cpp
* passed aLoaderPrincipal, and hence set up CORS
* uses nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS

3) dom/xslt/xslt/txMozillaStylesheetCompiler.cpp
* passed aLoaderPrincipal, and hence set up CORS
* uses nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS

So there was already a problem in the old code, but I am not sure if we are passing the right flags here for each callsite. Please look closely!

> ::: dom/xslt/xml/txXMLParser.cpp
> @@ +35,5 @@
> >      // the document.
> >      nsIDOMDocument* theDocument = nullptr;
> >      nsAutoSyncOperation sync(loaderDocument);
> >      rv = nsSyncLoadService::LoadDocument(documentURI,
> > +                                         nsIContentPolicy::TYPE_XSLT,
> 
> This can actually be either TYPE_XSLT or TYPE_XMLHTTPREQUEST.
> 
> Mind filing a followup to make sure all callers of this function passes in a
> type?

In fact I would like to try finding the right contentType within that bug. I already tried to set that up, but I can't see when it would be something other than TYPE_XSLT. If you could guide me to the right code I am going to update. Also, do you really mean TYPE_XMLHTTPREQUEST or TYPE_INTERNAL_STYLESHEET?

Comment 11

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

Comment on attachment 8676010 [details] [diff] [review]
bug_1191645_asyncOpen2_syncloadservice.patch

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

::: dom/base/nsSyncLoadService.cpp
@@ +244,5 @@
>  nsresult
>  nsSyncLoader::PushSyncStream(nsIStreamListener* aListener)
>  {
>      nsCOMPtr<nsIInputStream> in;
> +    nsresult rv = mChannel->Open2(getter_AddRefs(in));

Could you add an NS_ENSURE_TRUE here as well: 
http://mxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#103

Or change the assertion to MOZ_RELEASE_ASSERT.

Just so that if we make a mistake somehow and end up with trying to use CORS with Open2, that it fails in release builds rather than silently has a security bug.

::: dom/xml/nsXMLPrettyPrinter.cpp
@@ +102,5 @@
>  
>      nsCOMPtr<nsIDOMDocument> xslDocument;
> +    rv = nsSyncLoadService::LoadDocument(xslUri, nsIContentPolicy::TYPE_XSLT,
> +                                         nsContentUtils::GetSystemPrincipal(),
> +                                         nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS,

Nit: Might as well use SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL since that's effectively how we'll behave since we're using the system principal.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1191645_asyncOpen2_syncloadservice.patch

Addressed Jonas suggestions. Also, as discussed over IRC we should always use TYPE_INTERNAL_XMLHTTPREQUEST within txXMLParser.cpp.

Carrying over r+ from Jonas.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=435eb8168c4a

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=435eb8168c4a

This looks good to me, setting checkin-needed.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5c5ddddff1

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5d5c5ddddff1