Closed Bug 591552 Opened 11 years ago Closed 11 years ago

SetupReplacementChannel has bogus cast to nsHttpChannel

Categories

(Core :: Networking: HTTP, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: bzbarsky, Assigned: jduell.mcbugs)

References

Details

(Keywords: regression, Whiteboard: [sg:critical?])

Attachments

(1 file, 1 obsolete file)

Bug 570867 added this code to SetupReplacementChannel:

    1.64 +    nsHttpChannel *httpChannelImpl = static_cast<nsHttpChannel*>(httpChannel.get());
    1.65 +    httpChannelImpl->SetRemoteChannel(mRemoteChannel);

But all you know about httpChannel is that it's some channel that QIs to nsIHttpChannel.  This could be implemented by some extension.  Or it could be an nsViewSourceChannel (for a view-source:http://something URI).

It looks to me offhand that a server returning 3xx with a location that points to such a URI will cause us to write the remote-channel boolean at 676 bytes into the channel (on 64-bit, at least, that's the offset of mRemoteChannel in the HTTP channel).  sizeof(nsViewSourceChannel) there is 128 bits, so we're just writing it.... somewhere.

We need to fix this.
Group: core-security
blocking2.0: --- → final+
Blocks: 570867
Move remoteChannel attribute to nsIHttpChannelInternal or have a new interface for these remote things that is implemented only with MOZ_IPC?

It seems to me that view source doesn't follow redirects, at least after my local test with Fx 3.6.  If SetupRedirectChannel is called still needs to be checked.  Of course, just to check severity of this bug.
This fixes the cast by adding SetRemoteChannel to nsIHttpChannelInternel (which is currently only impl'd by HTTP channels, not view-source).

It's not the prettiest thing ever, but I couldn't think of anything better. (I pondered creating a CID for nsHttpChannel and QI-ing it so we could cast safely, but got confused as to how to actually make a concrete class QI-able: nsDocumentURI does it, but it confuses me--it has two CIDs, for instance).

There was also the underlyingChannel idea bz and I have talked about, but it's not clear that we're going to actually need it for redirects to other protocols, and it seemed the wrong fix for just this.   But if we wind up using underlyingChannel in the future, we might be able to piggyback on it and get rid of servicingRemoteChannel.

> httpInternal->SetServicingRemoteChannel(mRemoteChannel)

mRemoteChannel is a bitfield, but that's fine to pass to an IDL SetBool function right? (i.e. passing by value, so converts to PR_BOOL ok) 

I made HttpChannelChild return an error is Get|Set is called--I could've just always returned PR_FALSE for Get, and have Set drop quietly, but figured I might as well flag it as a logic error.
Attachment #470223 - Flags: review?(bzbarsky)
Honza, view-source does appear to follow redirects:  try 

   view-source:http://people.mozilla.org/~jduell/redir

But this bug is just for redirects *TO* view-source, i.e. see

   http://people.mozilla.org/~jduell/redirvs

[Note that bz pointed out to me that we don't actually handle redirects from HTTP to other protocols correctly in e10s--we always create a new PHttpChannel in HttpChannelParentListener::AsyncOnChannelRedirect.  I'm going to open a bug for this--alas, I think it's going to be a big pain to fix.]
Keywords: regression
Whiteboard: [sg:critical?]
Assignee: nobody → jduell.mcbugs
Comment on attachment 470223 [details] [diff] [review]
Fixes bogus cast by adding IDL attribute

Please rev the iid (and doing this so late in the cycle worries me... I hope we'll be ok, but it might be safer to do an nsIHttpChannelInternal_MOZILLA_2_0_BRANCH, perhaps).

And no need to null-check your out param.

r=me, but we should seriously think about the nsIHttpChannelInternal_MOZILLA_2_0_BRANCH thing.
Attachment #470223 - Flags: review?(bzbarsky) → review+
Yeah, we've had a bunch of trouble resulting from changing nsIChannel in beta5. The likelihood of someone using nsIHttpChannelInternal is lower, but I for one am done with playing with fire. ;)

I'd suggest the branch interface, and file a followup to remove it after.
OK, I've fixed this with a separate IDL interface, nsIHttpChannelParentInternal.  Free bonus:  since only nsHttpChannel needs to implement it, so no chaff methods in HttpChannelChild.   (Perhaps we might even want to keep it around for this reason after the branch?)
Attachment #470223 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/b290cb3e624c
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.