nsJSURI and nsFileDataURI need specializations of nsIIPCSerializable Read/Write methods taking IPC::Message*

RESOLVED INVALID

Status

()

defect
P5
normal
RESOLVED INVALID
8 years ago
5 months ago

People

(Reporter: dholbert, Unassigned)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Right now* (in cedar as of Bug 647570, soon to be on m-c) nsJSURI overrides one set of Read/Write methods on nsSimpleURI, but not the other set.

Specifically, nsJSURI overrides the nsISerializable versions (which take nsIObjectInputStream*), but _not_ the nsIIPCSerializable versions (which take IPC::Message*)

This triggers these build warnings:

> nsSimpleURI.h:67:60: warning: ‘virtual PRBool nsSimpleURI::Read(const IPC::Message*, void**)’ was hidden
> nsJSProtocolHandler.h:116:62: warning:   by ‘virtual nsresult nsJSURI::Read(nsIObjectInputStream*)’
> nsSimpleURI.h:67:157: warning: ‘virtual void nsSimpleURI::Write(IPC::Message*)’ was hidden
> nsJSProtocolHandler.h:117:62: warning:   by ‘virtual nsresult nsJSURI::Write(nsIObjectOutputStream*)’

...and it looks like this will also cause real bugs, if we try to send a nsJARURI via IPC.  (I have no idea how frequently that happens, if it happens at all.)
Depends on: 647570
I thought we agreed not to serialize URIs (and other XPCOM objects) like that, and all URIs would be passed via UTF8 string. Why are we doing the complex and dangerous thing here?
I don't recall there being any such decision. As far as I recall, the original goal was to only send URIs via the serialization methods, but I think that broke for javascript: ones or something, so we moved to passing as a UTF8 string as a fallback mechanism.
Note that for a javascript: URI the string representation is lossy, by the way.
(In reply to comment #3)
> Note that for a javascript: URI the string representation is lossy, by the
> way.

The same is true for moz-filedata URIs.
(Comment 0 is also entirely true of moz-filedata URIs (nsFileDataURI), FWIW.)
OS: Linux → All
Hardware: x86_64 → All
Summary: nsJSURI needs specialization of nsIIPCSerializable Read/Write methods taking IPC::Message* → nsJSURI and nsFileDataProtocolHandler need specializations of nsIIPCSerializable Read/Write methods taking IPC::Message*
Summary: nsJSURI and nsFileDataProtocolHandler need specializations of nsIIPCSerializable Read/Write methods taking IPC::Message* → nsJSURI and nsFileDataURI need specializations of nsIIPCSerializable Read/Write methods taking IPC::Message*
I see that JS URIs have a concept of a "base URI", although I don't quite understand why we encode that in the URI instead of in the load. But does it really make sense to carry that information across process boundaries?

I am very concerned about the IPC deserialization running lots of code which is going to be difficult to audit for security and safety. I really think we should prefer explicitly passing URI strings, origin charsets if absolutely necessary, and other load data explicitly as IPC arguments (perhaps in a union/struct type with standard handling routines), rather than carrying it along with URI objects.
> although I don't quite understand why we encode that in the URI instead of in
> the load.

Because that information is not available at load time, but is available at URI creation time...  Better solutions quite welcome.

> But does it really make sense to carry that information across process
> boundaries?

If you're sending JS URIs across process boundaries at all, then imo yes.  Otherwise relative URIs in the document produced by the JS URI will point to the wrong place.
We send URIs in the following cases:

http://mxr.mozilla.org/mozilla-central/search?string=uri&find=\.ipdl&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Apart from URIs which are a known protocol in the networking stack, we have:

* referrers
* "doc"
* cookies (are these HTTP only?)
* content permission prompts (geolocation/etc)
* offline cache (HTTP only?)
* global history
* external helper apps

Which should all be safe, I think.

* the parent instructing the child to load a URL

This is the only one where we might require additional context: these should be coming from the frontend (URLbar/bookmarks/session store): and in these cases I think we want to be pretty explicit about context.
Given those use cases, the javascript: base URI issue should not arise.
Duplicate of this bug: 666691
Priority: -- → P5
I think this bug is now INVALID.

The interface whose methods were in question here (nsIIPCSerializable) was renamed to nsIIPCSerializableObsolete here:
 https://hg.mozilla.org/mozilla-central/rev/418b5cbc7cd9#l23.1
...and then was subsequently removed here:
 https://hg.mozilla.org/mozilla-central/rev/c813eeb62b92#l34.24

As you can see, that latter commit's removal included the read/write APIs for the "Message" type that I was talking about in comment 0:
-interface nsIIPCSerializableObsolete : nsISupports
-{
-  [notxpcom] boolean read(in ConstMessage msg, in Iterator iter);
-  [notxpcom] void write(in Message msg);
-};

So there's nothing left to be done here -- the API in question (whose specializations we needed here) has now been removed.
Status: NEW → RESOLVED
Closed: Last year
Depends on: 784726
Resolution: --- → INVALID
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.