1182539 - Use channel->ascynOpen2 in dom/base/nsDocument.cpp

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1182539_asyncopen2_nsdocument.patch

Jonas, isn't CheckLoadURIWithPrincipal() and CheckMayLoad() mutually exlusive? One would need the flag SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS and the other SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS.

Comment 2

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

Comment on attachment 8640301 [details] [diff] [review]
bug_1182539_asyncopen2_nsdocument.patch

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

::: dom/base/nsDocument.cpp
@@ +1292,1 @@
>    nsCOMPtr<nsIInterfaceRequestor> req = nsContentUtils::SameOriginChecker();

You don't need the same-origin checker since AsyncOpen2 will enforce same-origin if you pass it the right flags.

Comment 3

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

CheckMayLoad is strictly more restrictive than CheckLoadURI. I wouldn't call them mutually exclusive, but rather that the CheckLoadURI check is redundant and unnecessary.

What you want here though is a same-origin policy since the old code enforces same-origin.

Comment 4

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1182539_asyncopen2_nsdocument.patch

(In reply to Jonas Sicking (:sicking) from comment #3)
> What you want here though is a same-origin policy since the old code
> enforces same-origin.

Alrighty, that should do it then!

Comment 5

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

Comment on attachment 8640657 [details] [diff] [review]
bug_1182539_asyncopen2_nsdocument.patch

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

This looks good to me. Though IIRC there's enough special stuff involved with subresources that it might be good to get bz's review.

Comment 6

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

Boris, one difference with this code is that a load from data: will return a resource whose GetChannelResponsePrincipal will be the principal of the loading document. I.e. we'll inherit the principal.

That seems desired, but it's not actually what the old code does. I'm not sure if it makes a difference though?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8640657 [details] [diff] [review]
bug_1182539_asyncopen2_nsdocument.patch

Yeah, the data: change there looks reasonable.  r=me

Comment 8

[Christoph Kerschbaumer [:ckerschb}]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ea13be4e3a1

Comment 9

[Christoph Kerschbaumer [:ckerschb}]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/0fbb2ec4b4a84968764a678474fa2ce35a8b2085
changeset:  0fbb2ec4b4a84968764a678474fa2ce35a8b2085
user:       Christoph Kerschbaumer <[e-mail]>
date:       Fri Jul 31 08:58:14 2015 -0700
description:
Bug 1182539 - Use channel->ascynOpen2 in dom/base/nsDocument.cpp (r=sicking)

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/0fbb2ec4b4a8