Closed Bug 1192945 Opened 4 years ago Closed 4 years ago

Use channel->asyncOpen2 in widget/windows/nsDataObj.cpp

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed
firefox44 --- affected
firefox45 --- affected
firefox46 --- affected
firefox50 --- fixed

People

(Reporter: ckerschb, Assigned: jkt)

References

(Depends on 1 open bug)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 3 obsolete files)

No description provided.
Assignee: nobody → mozilla
Blocks: 1182535
In case you can't recall what this code is doing, i copied over the comment from
https://bugzilla.mozilla.org/show_bug.cgi?id=1038756#c160

> This is used in drag and drop on Windows. nsDataObj is a COM object that
> windows speaks two when the user is dragging things like images out of
> content. Since nsIRequest::LOAD_FROM_CACHE doesn't guarantee cache only
> hits, it sounds like the right principal needs to be set. The object gets
> created when the drag session is invoked in nsDragService here - 
> 
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsDragService.
> cpp#174
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsClipboard.
> cpp#115
Attachment #8645906 - Flags: review?(jonas)
Comment on attachment 8645906 [details] [diff] [review]
bug_1192945_asyncopen2_dataobj.patch

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

So there's no security checks right now at all? Might the URI that's being loaded here be a file:// URI if you drag an image from the desktop? If so this will prevent those since we'll now do a checkLoadURI check.
Comment on attachment 8645906 [details] [diff] [review]
bug_1192945_asyncopen2_dataobj.patch

(In reply to Jonas Sicking (:sicking) from comment #2)
> So there's no security checks right now at all?

At least no obvious checks.

> Might the URI that's being
> loaded here be a file:// URI if you drag an image from the desktop? If so
> this will prevent those since we'll now do a checkLoadURI check.

Jim, can you help us review those bits since you also helped us review the initial bits [1] to attach a loadInfo on all channels?

A bit of background: the idea is to remove all security checks from the callsite and then ::AsyncOpen(2) calls into the newly added contentSecurityManager [2] which performs all the necessary security checks before opening the channel. What we have to do is to find out what security checks we need to perform and hence what arguments we should pass to newChannel.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1038756#c160
[2] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#243
Attachment #8645906 - Flags: review?(jmathies)
Blocks: 664717
Attachment #8645906 - Flags: review?(jmathies) → review+
So what is SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS?
(In reply to Jim Mathies [:jimm] from comment #5)
> no docs - 
> http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsILoadInfo.idl#40

Ah, sorry found it lower down in the source - 

http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsILoadInfo.idl#61

This sounds about right but I'm not sure. The code here does what people understand it to do, when you drag something visible out of content we need to have the image data representation to display for the drag. Windows wants this interface to be sync and expects us to have that data immediately but we need some time to retrieve it. Hence this event loop. It's unwanted but necessary.

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsDataObj.cpp#202

Note this is a non-critical feature, if something fails here we can live without having the resource.
Ah, so this code is used when you drag something *from* a webpage? It is used to generate the preview while we're dragging?

If so I think this new code is correct and in fact better than the old code that we had.

My concern is that if a webpage from http:// contains a resource from file://, that we will no longer load it. However if a page from the web contains an <img src="file://...">, then we won't be rendering it in the normal view, so it's good that we don't load it when generating the preview.
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/7bd651ae54dd6b090555d4420f34525de3fda931
changeset:  7bd651ae54dd6b090555d4420f34525de3fda931
user:       Christoph Kerschbaumer <mozilla@christophkerschbaumer.com>
date:       Thu Aug 13 18:35:34 2015 -0700
description:
Bug 1192945 - Use channel->ascynOpen2 in widget/windows/nsDataObj.cpp (r=sicking,jimm)
https://hg.mozilla.org/mozilla-central/rev/7bd651ae54dd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Depends on: 1231429
Kwierso, can you please back out that change on beta, aurora and nightly? We want to fix the regression from Bug 1231429, which will be fixed by that backout.

Ritu, can you confirm?
Flags: needinfo?(wkocher)
Flags: needinfo?(rkothari)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)
> Kwierso, can you please back out that change on beta, aurora and nightly? We
> want to fix the regression from Bug 1231429, which will be fixed by that
> backout.
> 
> Ritu, can you confirm?

Christoph, which specific patch needs to be backed out? And could you please do some local testing with that patch backed out to make sure there are no regressions? I will gtb b9 without this but can consider taking it in RC1. Thanks!
Flags: needinfo?(rkothari)
(In reply to Ritu Kothari (:ritu) from comment #11)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)
> > Kwierso, can you please back out that change on beta, aurora and nightly? We
> > want to fix the regression from Bug 1231429, which will be fixed by that
> > backout.
> > 
> > Ritu, can you confirm?
> 
> Christoph, which specific patch needs to be backed out? And could you please
> do some local testing with that patch backed out to make sure there are no
> regressions? I will gtb b9 without this but can consider taking it in RC1.
> Thanks!

It's this patch:
https://bugzilla.mozilla.org/attachment.cgi?id=8645906&action=diff

Jimm, can you please verify that it works for you as well?
Flags: needinfo?(jmathies)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> (In reply to Ritu Kothari (:ritu) from comment #11)
> > (In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)
> > > Kwierso, can you please back out that change on beta, aurora and nightly? We
> > > want to fix the regression from Bug 1231429, which will be fixed by that
> > > backout.
> > > 
> > > Ritu, can you confirm?
> > 
> > Christoph, which specific patch needs to be backed out? And could you please
> > do some local testing with that patch backed out to make sure there are no
> > regressions? I will gtb b9 without this but can consider taking it in RC1.
> > Thanks!
> 
> It's this patch:
> https://bugzilla.mozilla.org/attachment.cgi?id=8645906&action=diff
> 
> Jimm, can you please verify that it works for you as well?

confirmed, this fixes twitter drag bug 1231429 and I also tried dragging some images from other sites out, no issues seen.
Flags: needinfo?(jmathies)
Given that the fix (backout) was verified by two people, let's take it in 44.0b9. Thanks Christoph, Jim and Wes!
Jimm, now that we have backed out those changes, what should be the policy for nsDataObj?

As described in detail within [1] we are facing the problem that we create a new channel using 'TYPE_OTHER' which is *not* the actual content type and hence CSP might incorrectly block the load. 

Ideally we would pass the right contentPolicyType to NS_NewChannel(). I looked into the problem, but it seems we can't query the content type within nsDragService.cpp. Any suggestions on how we could accomplish to query the right contentPolicyType or what we should do to convert this callsite to use asyncOpen2?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1231429#c16
[2] http://hg.mozilla.org/mozilla-central/rev/4afe0fb6dd86
Flags: needinfo?(jmathies)
Summary: Use channel->ascynOpen2 in widget/windows/nsDataObj.cpp → Use channel->asyncOpen2 in widget/windows/nsDataObj.cpp
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #17)
> Jimm, now that we have backed out those changes, what should be the policy
> for nsDataObj?
> 
> As described in detail within [1] we are facing the problem that we create a
> new channel using 'TYPE_OTHER' which is *not* the actual content type and
> hence CSP might incorrectly block the load. 
> 
> Ideally we would pass the right contentPolicyType to NS_NewChannel(). I
> looked into the problem, but it seems we can't query the content type within
> nsDragService.cpp. Any suggestions on how we could accomplish to query the
> right contentPolicyType or what we should do to convert this callsite to use
> asyncOpen2?
> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1231429#c16
> [2] http://hg.mozilla.org/mozilla-central/rev/4afe0fb6dd86

I don't see why the web site has the ability to override what we want to do in the browser. Can we associate this with some sort of system operation such that it will always succeed?
Flags: needinfo?(jmathies)
> I don't see why the web site has the ability to override what we want to do
> in the browser. Can we associate this with some sort of system operation
> such that it will always succeed?

I would rather not use some kind of system operation - it really sounds scary from a security perspective, I would rather try to get the right contentPolicyType.

Here is a WIP patch, which covers:
a) I think we should rather use |trans->SetRequestingNode(mSourceDocument);| as the loadingNode. Mostly based on the code I found here [1].
b) We should try to pass a contentPolicyType to Init(), potentially we can actually figure out what type we are loading. Big question to me is, how does GetDownloadDetails() work? If we can figure that, then potentially we also know the contentPolicyType.

Any thoughts?

[1] http://mxr.mozilla.org/mozilla-central/source/widget/nsBaseDragService.cpp#220
Attachment #8645906 - Attachment is obsolete: true
Flags: needinfo?(jmathies)
This bug has been lying around for some time by now. Jim can we help together and keep that bug going and finally out of the way?
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #20)
> This bug has been lying around for some time by now. Jim can we help
> together and keep that bug going and finally out of the way?

I am totally swamped right now, I really don't have the time to go spelunking in our dom code to figure this out. If this is a serious issue we should get it tracked to a release. Otherwise it'll have to wait until someone has some time to work on it.
Flags: needinfo?(jmathies)
(In reply to Jim Mathies [:jimm] from comment #21)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #20)
> > This bug has been lying around for some time by now. Jim can we help
> > together and keep that bug going and finally out of the way?
> 
> I am totally swamped right now, I really don't have the time to go
> spelunking in our dom code to figure this out. If this is a serious issue we
> should get it tracked to a release. Otherwise it'll have to wait until
> someone has some time to work on it.

Thanks for the update Jim - I'll figure something!
Whiteboard: [domsecurity-active]
Could someone explain to me what makes this hard?

I assume that somewhere there is code that looks at what is being dragged and decide that we should start a download. If the user drags text we don't initiate a download, but if you drag an image we initiate download. Hence somewhere we need to decide to start a download.

Likewise, somewhere there needs to be code that determine what URL to download. And digs that URL out of the <img> element, or out of the layout frame, or some such.

It would seem to me, though I could of course be entirely wrong, that somewhere there is code that determine that the user is dragging an image, gets the URL of that image, and kicks off a download.

Couldn't that code also pass along the information that we're downloading an image?
Assignee: ckerschb → jkingston
Hey Jimm, this bug has been lying around for quite some time. Luckily Jonathan is going to help us out and drive the bug. What if we make it really simple and just pass the contentPolicyType an addition to the domNode (pretty similar to what we did for the initial patch [1]).

As of now, I couldn't find any other use cases than TYPE_IMAGE and fallback to TYPE_OTHER, but potentially we should investigate the callsites of InvokeDragSession [2] a little closer.

Anyway, main question is: What do you think, could we land such a patch?

[1] https://hg.mozilla.org/mozilla-central/rev/4afe0fb6dd86
[2] http://mxr.mozilla.org/mozilla-central/search?string=InvokeDragSession%28
Attachment #8712451 - Attachment is obsolete: true
Attachment #8755811 - Flags: feedback?(jmathies)
Comment on attachment 8755811 [details] [diff] [review]
bug_1192945_asyncopen2_dataobj.patch

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

::: widget/nsTransferable.cpp
@@ +654,5 @@
> +
> +NS_IMETHODIMP
> +nsTransferable::SetContentPolicyType(nsContentPolicyType aContentPolicyType)
> +{
> +  mContentPolicyType = aContentPolicyType;

Initialize this to nsIContentPolicy::TYPE_OTHER.

http://mxr.mozilla.org/mozilla-central/source/widget/nsTransferable.cpp#219

::: widget/windows/nsDataObj.cpp
@@ +74,5 @@
>    nsresult rv;
>    rv = NS_NewChannel(getter_AddRefs(mChannel),
>                       pSourceURI,
>                       aRequestingNode,
> +                     nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS,

I think this is ok looking at other uses (any origin)? But I am not sure, this is outside my area of expertise.

@@ +346,5 @@
>    mTransferable->GetRequestingNode(getter_AddRefs(requestingDomNode));
>    nsCOMPtr<nsINode> requestingNode = do_QueryInterface(requestingDomNode);
>    MOZ_ASSERT(requestingNode, "can not create channel without a node");
> +  uint32_t contentPolicyType = nsIContentPolicy::TYPE_OTHER;
> +  mTransferable->GetContentPolicyType(&contentPolicyType);

add a comment here noting the default is TYPE_OTHER too.
Attachment #8755811 - Flags: feedback?(jmathies) → feedback+
Comment on attachment 8761216 [details]
Bug 1192945 - Use channel->asyncOpen2 in widget/windows/nsDataObj.cpp

Removing review as still testing myself. Thanks
Attachment #8761216 - Flags: review?(jonas)
https://reviewboard.mozilla.org/r/58510/#review55388

Please address my nits and flag jonas again (btw, I know he doesn't like mozreview that much)

::: widget/nsBaseDragService.cpp:224
(Diff revision 1)
>    NS_ENSURE_TRUE(mSuppressLevel == 0, NS_ERROR_FAILURE);
>  
>    // stash the document of the dom node
>    aDOMNode->GetOwnerDocument(getter_AddRefs(mSourceDocument));
>    mSourceNode = aDOMNode;
> +  mContentPolicyType = aContentPolicyType;

Please also initialize mContentPolicType to TYPE_OTHER within the initialization list of the constructor.

::: widget/nsTransferable.cpp:654
(Diff revision 1)
> +nsTransferable::GetContentPolicyType(nsContentPolicyType* outContentPolicyType)
> +{
> +  NS_ENSURE_ARG_POINTER(outContentPolicyType);
> +  if ( !mContentPolicyType ) {
> +    mContentPolicyType = nsIContentPolicy::TYPE_OTHER;
> +  }

Please initialize mContentPolicyType to TYPE_OTHER here [1] then you can also remove that if-branch.

[1] http://mxr.mozilla.org/mozilla-central/source/widget/nsTransferable.cpp#219
Jonathan, can you please also verify that dragging and dropping an image (where the toplevel page uses CSP) onto the desktop works? Have a look at this STR:
https://bugzilla.mozilla.org/show_bug.cgi?id=1231429#c0
Flags: needinfo?(jkingston)
Comment on attachment 8761216 [details]
Bug 1192945 - Use channel->asyncOpen2 in widget/windows/nsDataObj.cpp

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58510/diff/1-2/
Attachment #8761216 - Attachment description: Bug 1192945 - Use channel->ascynOpen2 in widget/windows/nsDataObj.cpp () → Bug 1192945 - Use channel->ascynOpen2 in widget/windows/nsDataObj.cpp
Attachment #8761216 - Flags: review?(jonas)
Comment on attachment 8761216 [details]
Bug 1192945 - Use channel->asyncOpen2 in widget/windows/nsDataObj.cpp

Again removing review
Flags: needinfo?(jkingston)
Attachment #8761216 - Flags: review?(jonas)
Attachment #8761216 - Flags: review?(ckerschb)
Comment on attachment 8755811 [details] [diff] [review]
bug_1192945_asyncopen2_dataobj.patch

Marking this one as obsolete since Jonathan's new patch is the most current one.
Attachment #8755811 - Attachment is obsolete: true
Comment on attachment 8761216 [details]
Bug 1192945 - Use channel->asyncOpen2 in widget/windows/nsDataObj.cpp

https://reviewboard.mozilla.org/r/58510/#review55470

Jonathan, just fix up my nits and you can flag Jonas for review. Thanks for the speedy turnaround time!

::: widget/nsBaseDragService.cpp:63
(Diff revision 2)
>      mDragAction(DRAGDROP_ACTION_NONE),
>      mDragActionFromChildProcess(DRAGDROP_ACTION_UNINITIALIZED), mTargetSize(0,0),
>      mScreenX(-1), mScreenY(-1), mSuppressLevel(0),
>      mInputSource(nsIDOMMouseEvent::MOZ_SOURCE_MOUSE)
>  {
> +  mContentPolicyType = nsIContentPolicy::TYPE_OTHER;

please put that in the initialization list.

...
mDragActionFromChildProcess(DRAGDROP_ACTION_UNINITIALIZED),
mTargetSize(0,0),
mContentPolicyType(nsIContentPolicy::TYPE_OTHER),
mScreenX(...)
...

::: widget/nsBaseDragService.cpp:272
(Diff revision 2)
>    aDragEvent->GetScreenY(&mScreenY);
>    aDragEvent->GetMozInputSource(&mInputSource);
>  
>    nsresult rv = InvokeDragSession(aDOMNode, aTransferableArray,
> -                                  aRegion, aActionType);
> +                                  aRegion, aActionType,
> +                                  nsIContentPolicy::TYPE_IMAGE);

Sorry, I missed that the first time, in fact we need to pass: TYPE_INTERNAL_IMAGE

::: widget/nsTransferable.cpp:226
(Diff revision 2)
>    : mPrivateData(false)
>  #ifdef DEBUG
>    , mInitialized(false)
>  #endif
>  {
> +  mContentPolicyType = nsIContentPolicy::TYPE_OTHER;

also here, please put it in the initialization list, right after
: mPrivateData(false)
, mContentPolicyType(nsIContentPolicy::TYPE_OTHER)
Attachment #8761216 - Flags: review?(ckerschb)
Comment on attachment 8761216 [details]
Bug 1192945 - Use channel->asyncOpen2 in widget/windows/nsDataObj.cpp

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58510/diff/2-3/
Attachment #8761216 - Flags: review?(jonas)
Comment on attachment 8761216 [details]
Bug 1192945 - Use channel->asyncOpen2 in widget/windows/nsDataObj.cpp

Sorry keeps asking for review automatically.
This has the review comments however unable to test at the moment. Will push for review tomorrow :ckerschb.
Attachment #8761216 - Flags: review?(jonas)
Comment on attachment 8761216 [details]
Bug 1192945 - Use channel->asyncOpen2 in widget/windows/nsDataObj.cpp

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58510/diff/3-4/
Attachment #8761216 - Flags: review?(jonas)
Comment on attachment 8761216 [details]
Bug 1192945 - Use channel->asyncOpen2 in widget/windows/nsDataObj.cpp

Rebased on windows, passes the test mentioned.

Thanks for your help!
Attachment #8761216 - Flags: review?(jonas) → review?(ckerschb)
Comment on attachment 8761216 [details]
Bug 1192945 - Use channel->asyncOpen2 in widget/windows/nsDataObj.cpp

Jim, from a security perspective |nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS| is what we want. Are you fine with the other changes within this changeset?
Attachment #8761216 - Flags: review?(jmathies)
Attachment #8761216 - Flags: review?(ckerschb)
Attachment #8761216 - Flags: review+
Attachment #8761216 - Flags: review?(jmathies) → review+
Comment on attachment 8761216 [details]
Bug 1192945 - Use channel->asyncOpen2 in widget/windows/nsDataObj.cpp

https://reviewboard.mozilla.org/r/58510/#review55754

lgtm!
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/36e71bcb0266
Use channel->ascynOpen2 in widget/windows/nsDataObj.cpp . r=ckerschb, r=jmathies
Keywords: checkin-needed
Backed out for treewide reorder failures like
https://treeherder.mozilla.org/logviewer.html#?job_id=29953768&repo=mozilla-inbound
widget/nsBaseDragService.h:184:12: error: 'nsBaseDragService::mSuppressLevel' will be initialized after [-Werror=reorder]
etc
Comment on attachment 8761216 [details]
Bug 1192945 - Use channel->asyncOpen2 in widget/windows/nsDataObj.cpp

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58510/diff/4-5/
Attachment #8761216 - Flags: review?(jonas)
Attachment #8761216 - Flags: review+
Attachment #8761216 - Flags: review?(jonas) → review?(ckerschb)
Comment on attachment 8761216 [details]
Bug 1192945 - Use channel->asyncOpen2 in widget/windows/nsDataObj.cpp

Initialization list needs to be in order - thanks!
Attachment #8761216 - Flags: review?(ckerschb) → review+
Comment on attachment 8761216 [details]
Bug 1192945 - Use channel->asyncOpen2 in widget/windows/nsDataObj.cpp

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58510/diff/5-6/
Attachment #8761216 - Attachment description: Bug 1192945 - Use channel->ascynOpen2 in widget/windows/nsDataObj.cpp → Bug 1192945 - Use channel->asyncOpen2 in widget/windows/nsDataObj.cpp
Attachment #8761216 - Flags: review+ → review?(jonas)
Comment on attachment 8761216 [details]
Bug 1192945 - Use channel->asyncOpen2 in widget/windows/nsDataObj.cpp

So in central the issue we were looking at fails too, I think this is good to go https://treeherder.mozilla.org/#/jobs?repo=try&revision=121df1315b30&selectedJob=22323863
Attachment #8761216 - Flags: review?(jonas) → review?(ckerschb)
Comment on attachment 8761216 [details]
Bug 1192945 - Use channel->asyncOpen2 in widget/windows/nsDataObj.cpp

https://reviewboard.mozilla.org/r/58510/#review56088

This patch already got an r+ from me. If it's ready to go, then go ahead and land it - thanks!
Attachment #8761216 - Flags: review?(ckerschb) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bbe8829e424
Use channel->asyncOpen2 in widget/windows/nsDataObj.cpp. r=ckerschb
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3bbe8829e424
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Depends on: 1281579
You need to log in before you can comment on or make changes to this bug.