Closed
Bug 1206952
Opened 9 years ago
Closed 8 years ago
Use channel->asyncOpen2 in dom/plugins/base/nsPluginStreamListenerPeer.cpp
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: ckerschb, Assigned: ckerschb)
References
Details
(Whiteboard: [domsecurity-active])
Attachments
(2 files, 2 obsolete files)
5.92 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
5.97 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee: mozilla → franziskuskiefer
Comment 1•9 years ago
|
||
here a first draft. There don't seem to be any security checks, but the magic number needs some attention. Since all tests run just fine without using the magic number (using AsyncOpen(converter, nullptr) for example), I'm not sure what it's actually used for. But you could check if everything else looks ok.
Attachment #8680694 -
Flags: feedback?(mozilla)
Assignee | ||
Comment 2•9 years ago
|
||
Comment on attachment 8680694 [details] [diff] [review] using asyncopen2 v0.1 Review of attachment 8680694 [details] [diff] [review]: ----------------------------------------------------------------- I don't know that this magic number does, maybe Jonas knows, but I would leave it untouched. ::: dom/plugins/base/nsPluginStreamListenerPeer.cpp @@ +706,5 @@ > else { > // in this else branch we really don't know where the load is coming > // from and in fact should use something better than just using > // a nullPrincipal as the loadingPrincipal. > nsCOMPtr<nsIPrincipal> principal = nsNullPrincipal::Create(); We have to figure out when this else branch is actually executed. Using the nullPrincipal will definitely fail security checks. @@ +756,2 @@ > > + rv = channel->AsyncOpen2(converter); What happens to the container? You have to create some kind of proxy class to mediate the aContext argument for asyncOpen, similar to what nsScriptLoader does (see Bug 1218433).
Attachment #8680694 -
Flags: feedback?(mozilla)
Assignee | ||
Comment 3•9 years ago
|
||
Jonas, any idea what that magic number does?
Flags: needinfo?(jonas)
No idea what the magic number is
Flags: needinfo?(jonas)
Comment 5•9 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2) > Comment on attachment 8680694 [details] [diff] [review] > using asyncopen2 v0.1 > > Review of attachment 8680694 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/plugins/base/nsPluginStreamListenerPeer.cpp > @@ +706,5 @@ > > else { > > // in this else branch we really don't know where the load is coming > > // from and in fact should use something better than just using > > // a nullPrincipal as the loadingPrincipal. > > nsCOMPtr<nsIPrincipal> principal = nsNullPrincipal::Create(); > > We have to figure out when this else branch is actually executed. Using the > nullPrincipal will definitely fail security checks. > Indeed, I hope for more infos here as well as I can't seem to find tests or anything else that triggers this. > @@ +756,2 @@ > > > > + rv = channel->AsyncOpen2(converter); > > What happens to the container? You have to create some kind of proxy class > to mediate the aContext argument for asyncOpen, similar to what > nsScriptLoader does (see Bug 1218433). The container holds only the magic number, which I handle manually now. It's not as straightforward as nsScriptLoader because we have different requesters here that don't implement the same interfaces. But here we need more info I regarding this magic number think (everything I tested works just fine without even using the magic number). Georg can you have a look?
Flags: needinfo?(gfritzsche)
Updated•9 years ago
|
Summary: Use channel->ascynOpen2 in dom/plugins/base/nsPluginStreamListenerPeer.cpp → Use channel->asyncOpen2 in dom/plugins/base/nsPluginStreamListenerPeer.cpp
Comment 6•9 years ago
|
||
Benjamin, would you mind having a look at this? See comment 5 for details.
Flags: needinfo?(gfritzsche) → needinfo?(benjamin)
Updated•9 years ago
|
Flags: needinfo?(gfritzsche)
Comment 7•9 years ago
|
||
Sorry it took so long to get here - i don't know offhand about this. Josh, do you still remember about this code? See comment 5 for details.
Flags: needinfo?(gfritzsche) → needinfo?(jaas)
Comment 9•8 years ago
|
||
I'm not exactly sure what info the NEEDINFO is for, but here's some context at least? The multitude of intermediate plugin stream listeners is relic of having non-NPAPI plugin types, among other things. It is possible (likely?0 that byte-range requests (NPN_RequestRead) don't have either any in-tree tests, or at least not useful ones. IIRC the only popular plugin that uses byte-range requests is the Acrobat plugin, but I don't know that for certain. The purpose of the separate byte-range listener is to consolidate multiple byte-range requests back to a single stream passed back to NPP_Write. Doing anything here will require some manual testing and perhaps writing some more testcases.
Flags: needinfo?(benjamin)
Comment 10•8 years ago
|
||
I have no idea what this code is doing unfortunately :( It looks terrifying, though!
Flags: needinfo?(john)
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8680694 [details] [diff] [review] using asyncopen2 v0.1 Josh, do you recall what this dubious mMagicNumber is and if we still need to keep it around? Probably you can recall since you have touched code within that file a while ago [1]. Thanks for your help! [1] http://hg.mozilla.org/mozilla-central/rev/702850944422
Attachment #8680694 -
Flags: feedback?(jaas)
Comment 12•8 years ago
|
||
I sat down here with Franziskus and we found: MAGIC_REQUEST_CONTEXT allows to tell nsPluginStreamListenerPeer about requests being byte range requests. It allows skipping some code that doesnt apply to those: https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/dom/plugins/base/nsPluginStreamListenerPeer.cpp#848 https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/dom/plugins/base/nsPluginStreamListenerPeer.cpp#983 It is set up here: https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/dom/plugins/base/nsPluginStreamListenerPeer.cpp#746 There is special logic to remove the magic marker: https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/dom/plugins/base/nsPluginStreamListenerPeer.cpp#176 ... which is triggered when the server falls back from serving byte ranges to serving as a file: https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/dom/plugins/base/nsPluginStreamListenerPeer.cpp#152 This seems fragile logic and we probably shouldn't mess with it. It's much less risky to keep to the logic as is and only adapt it a little. Next steps: * We should start with writing a test-case that exercises this behavior (and make sure we cover the normal byte range request code path) * Then we can confirm this is working now as commented... * Figure out how to pass the magic marker around without the context argument for AsyncOpen2 * $$$ Bonus steps: * we could rename MAGIC_REQUEST_CONTEXT to something actually useful * some other variable names (like calling a "listener" a "converter") could be improved We should also have some QA verification on streaming cases: * Flash * Netflix with Silverlight * Unity? (not sure if/where they are using it) * Java? (ditto)
Flags: needinfo?(jaas)
Assignee | ||
Comment 13•8 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #12) > Next steps: > * We should start with writing a test-case that exercises this behavior (and > make sure we cover the normal byte range request code path) John, given that you are a peer [1] and already commented on the bug - any chance we could convince you to help us out writing that testcase? We are not experts within the plugin/ code, so any help in writing that test is highly appreciated. [1] https://wiki.mozilla.org/Modules/Core#Plugins
Flags: needinfo?(john)
Comment 14•8 years ago
|
||
It's not clear to me what test would even need to be written here without some serious code archaeology. I'm not even sure what this bug is trying to accomplish, other than changing some ancient fragile code that only one plugin anywhere is known to use. The fact that only Acrobat uses this API makes it worse -- acrobat already has existing hacks in the plugin code to work around its bugs, and there's every reason to believe their current behavior is bug-for-bug compatible with whatever gecko currently does, regardless of any spec. Writing a test that this API works "right" doesn't mean we wont break acrobat anyway. The proper way forward is probably to make the minimal change here to accomplish whatever we're trying to accomplish, and do some manual testing of Acrobat.
Flags: needinfo?(john)
Assignee | ||
Comment 15•8 years ago
|
||
(In reply to John Schoenick [:johns] from comment #14) > It's not clear to me what test would even need to be written here without > some serious code archaeology. I'm not even sure what this bug is trying to > accomplish, other than changing some ancient fragile code that only one > plugin anywhere is known to use. > > The fact that only Acrobat uses this API makes it worse -- acrobat already > has existing hacks in the plugin code to work around its bugs, and there's > every reason to believe their current behavior is bug-for-bug compatible > with whatever gecko currently does, regardless of any spec. Writing a test > that this API works "right" doesn't mean we wont break acrobat anyway. > > The proper way forward is probably to make the minimal change here to > accomplish whatever we're trying to accomplish, and do some manual testing > of Acrobat. Thanks John, but that basically means we end up with the patch (or some slight modification) of what Franziskus already attached here. Georg, do you wanna review those bits?
Assignee | ||
Updated•8 years ago
|
Attachment #8680694 -
Flags: feedback?(jaas) → review?(gfritzsche)
Comment 16•8 years ago
|
||
Comment on attachment 8680694 [details] [diff] [review] using asyncopen2 v0.1 Review of attachment 8680694 [details] [diff] [review]: ----------------------------------------------------------------- Per comment 12, we were concluding that this is fragile logic and that it's hard to verify that the new behavior is equivalent. For that, we need to figure out how to pass the magic marker along the same paths without the context argument for AsyncOpen2(). Thanks to comment 14 (thanks John!) we should be good with skipping test-coverage and defer that to manual QA of Acrobat. We should rename MAGIC_REQUEST_CONTEXT to something meaningful or at least comment on what it's there for.
Attachment #8680694 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 17•8 years ago
|
||
Franziskus, is this bug still on your radar?
Flags: needinfo?(franziskuskiefer)
Assignee | ||
Comment 18•8 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #17) > Franziskus, is this bug still on your radar? Chattet with Franziskus in person; taking this one back.
Assignee: franziskuskiefer → mozilla
Flags: needinfo?(franziskuskiefer)
Assignee | ||
Comment 19•8 years ago
|
||
Hey Georg, first of all: thanks for all your help within this bug. Now, let's see if we have the same view on things. I think the 'container' [Second Arg of AsyncOpen] is not needed at all, and in fact is only a detour (workaround) of what should actually happen. To make sure I am not missing anything, let me guide you through: 1) Before calling AsyncOpen2(converter) the converter either becomes a nsPluginStreamListenerPeer or a nsPluginByteRangeStreamListener. 2) If it becomes nsPluginStreamListenerPeer then there is basically nothing for us to do, because the only time the variable mUseByteRangeRequest gets updated is within nsPluginByteRangeStreamListener::OnStopRequest(). 3) Now within nsPluginByteRangeStreamListener::OnStopRequest() we have access to the original nsPluginStreamListenerPeer, because we pass it as argument when initially onstructing nsPluginByteRangeStreamListener. In my opinion, we can just access that member variable directly and update the mediated listener by calling UpdateUseByteRangeRequest(false); 4) Using a nullPrincipal as the loadingPrincipal within NS_NewChannel() will definitely fail security checks. My hope is that we should always have a node available, but we should double check - probably with bsmedberg. I think that's it, or am I missing something? All mochi-tests work, but i don't know how to actually test the byteRangeRequest. Any suggestions what I can do to verify it's actually working correctly?
Attachment #8680694 -
Attachment is obsolete: true
Attachment #8728570 -
Flags: review?(gfritzsche)
Comment 20•8 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #19) > Hey Georg, first of all: thanks for all your help within this bug. Now, > let's see if we have the same view on things. I think the 'container' > [Second Arg of AsyncOpen] is not needed at all, and in fact is only a detour > (workaround) of what should actually happen. Yes, i think so too, but i find the existing logic hard to trace and verify. Some background here is that: * plugin code is fragile * minor behavior changes can break plugins * we only want to touch plugin code as far as needed to keep it working The safest path forward would seem to just keep the existing behavior and just do what is needed to move your project forward. Is that feasible? To test the byte range request behaviour, we could find out either if and how Acrobat still uses them or do what we think the code is good for with our test plugin. But we would probably better served by keeping the existing logic as is, test manually with Acrobat that it behaves ok in the usual use cases and move on.
Updated•8 years ago
|
Flags: needinfo?(mozilla)
Assignee | ||
Comment 21•8 years ago
|
||
Georg, thanks for the feedback. As discussed, let's discussion in person again to make sure we are not missing anything important. (In reply to Georg Fritzsche [:gfritzsche] from comment #20) > (In reply to Christoph Kerschbaumer [:ckerschb] from comment #19) > > Hey Georg, first of all: thanks for all your help within this bug. Now, > > let's see if we have the same view on things. I think the 'container' > > [Second Arg of AsyncOpen] is not needed at all, and in fact is only a detour > > (workaround) of what should actually happen. > > Yes, i think so too, but i find the existing logic hard to trace and verify. > Some background here is that: > * plugin code is fragile > * minor behavior changes can break plugins > * we only want to touch plugin code as far as needed to keep it working I agree, that code is fragile, but I am pretty confident that my changes replicate existing logic. In fact simplify it! > The safest path forward would seem to just keep the existing behavior and > just do what is needed to move your project forward. > Is that feasible? That's what I tried in the patch. I didn't touch anything else, besides renaming the MagicNumber and passing the byteRangeRequest between the nsPluginByteRangeStreamListener and the underlying nsPluginStreamListenerPeer. > To test the byte range request behaviour, we could find out either if and > how Acrobat still uses them or do what we think the code is good for with > our test plugin. > But we would probably better served by keeping the existing logic as is, > test manually with Acrobat that it behaves ok in the usual use cases and > move on. I agree, that we should test carefully. Probably even contact QA to help us out.
Flags: needinfo?(mozilla)
Comment 22•8 years ago
|
||
Comment on attachment 8728570 [details] [diff] [review] bug_1206952_asyncopen2_pluginstreamlistenerpeer.patch Review of attachment 8728570 [details] [diff] [review]: ----------------------------------------------------------------- We quickly talked through this and it sounds like keeping exactly the original logic would be bigger effort then a simplification of this code. However, due to the simplification there are some corner-cases that could lead to behavior changes - specifically if we ever get data from unexpected request instances. Also assuming that we always have a requestingNode is a risk for regression. On the upper hand all the changes seem to be limited to byte range requests, which limits the impact here. ::: dom/plugins/base/nsPluginStreamListenerPeer.cpp @@ +158,5 @@ > NS_IMETHODIMP > nsPluginByteRangeStreamListener::OnStopRequest(nsIRequest *request, nsISupports *ctxt, > nsresult status) > { > + NS_ENSURE_TRUE(mStreamConverter, NS_ERROR_FAILURE); In general we want to stay away from the |NS_ENSURE_*| macros because they hide return statements. We shouldn't add new ones here - ditto for the other places below. @@ +174,5 @@ > + if (mRemoveByteRangeRequest) { > + // update the underlying mWeakPtrPluginStreamListenerPeer to not handle the > + // request as a byteRangeRequest. > + nsCOMPtr<nsIStreamListener> listener = do_QueryReferent(mWeakPtrPluginStreamListenerPeer); > + NS_ENSURE_TRUE(listener, NS_ERROR_UNEXPECTED); This potentially changes behavior. Should we just stay with a warning here as before? @@ +680,4 @@ > nsCOMPtr<nsIChannel> channel; > + rv = NS_NewChannel(getter_AddRefs(channel), > + mURL, > + requestingNode, Always expecting node is definitely speculative and would require broader testing across the relevant plugins, both before landing and with QA. You should needinfo? or feedback? bsmedberg on this specific part. Is this unavoidable? Do we have no alternatives that give us the previous behavior? @@ -846,5 @@ > - nsCOMPtr<nsISupportsPRUint32> container = do_QueryInterface(aContext); > - if (container) > - container->GetData(&magicNumber); > - > - if (magicNumber != MAGIC_REQUEST_CONTEXT) { I'm not sure this is equivalent: If nsPluginStreamListenerPeer::OnDataAvailable() is called for any requests other than the ones from nsPluginStreamListenerPeer, this can change behavior. Specifically i'm thinking of |mUseByteRangeRequest == true| and a request originating from a different source. @@ +940,5 @@ > > // we keep our connections around... > + if (mUseByteRangeRequest) { > + // this is one of our range requests > + return NS_OK; This also depends on only our expected requests to come in... otherwise it would change behavior.
Attachment #8728570 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 23•8 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #20) > To test the byte range request behaviour, we could find out either if and > how Acrobat still uses them or do what we think the code is good for with > our test plugin. Benjamin, Georg, I tried to reproduce for quite some time now. It seems Acrobat is not using the byte range request anymore. Can you think of any other plugins that might still use the byte range request? It would be good to verify at least with one plugin that the code we have still works correctly.
Flags: needinfo?(gfritzsche)
Flags: needinfo?(benjamin)
Comment 24•8 years ago
|
||
I don't really know where else this gets actively used. Benjamin, do you have an idea? If not, maybe we should just do simple manual testing of byte range requests with the test plugin, then land it and hand it off to QA to take through common plugin scenarios.
Flags: needinfo?(gfritzsche)
Assignee | ||
Updated•8 years ago
|
Whiteboard: [domsecurity-active]
Assignee | ||
Comment 25•8 years ago
|
||
As discussed in the meeting today, we probably should not touch the fragile logic of byte range requests and we should just introduce a lightweight wrapper for the context. I am doing this in the following two patches. Within the first one I am just replacing the |magicNumber| with byteRangeRequests because a variable name called |magicNumber| doesn't really tell you anything about what's going on :-(
Attachment #8728570 -
Attachment is obsolete: true
Flags: needinfo?(benjamin)
Attachment #8734240 -
Flags: review?(jonas)
Assignee | ||
Comment 26•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=02964d0a3df6
Attachment #8734241 -
Flags: review?(jonas)
Attachment #8734240 -
Flags: review?(jonas) → review+
Attachment #8734241 -
Flags: review?(jonas) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 27•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc1666f91ba7 https://hg.mozilla.org/integration/mozilla-inbound/rev/cd324ce321b1
Keywords: checkin-needed
Comment 28•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fc1666f91ba7 https://hg.mozilla.org/mozilla-central/rev/cd324ce321b1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•