In order for editor embedding to be complete, we need to complete this task: * We need to come up with a scheme for sandboxing the editor.

what needs to be done here, do we need to have a security review?

We are going to use the editor embedding APIs, probably via XBL, to implement <htmlarea> (or, if you prefer, <textarea type="html">). That means that some subset of the emebedding APIs, and the commands that are executable via nsICommandManger, need to be able to run in the context of page content (or somehow proxied from content). That means that we'll need to have some kind of security API support in there.

-->brade

This is a first attempt at a patch. In short, it compares nsCommandManager's mWindow with the focused window and returns an error if they aren't the same. (GetControllerForCommand is called by: DoCommand, GetCommandState, IsCommandEnabled and IsCommandSupported. None of nsContentUtils' checks seem appropriate to me. nsDOMSerializer.cpp changes (r1.15.4.2 vs r1.15.4.2.12.1) don't seem right either. Mitch--can you point me to the right api if this check is insufficient?

I think this patch will break the dispatching of commands to subframes in a frameset, which is bad. I think you should check to see if you're in chrome or content, and only do this check in content.

+PRBool +nsCommandManager::IsCallerChrome() Paranoia. I think this should return an nsresult, and PRBool* out param. If this call fails, I don't think you want to go any further. Defaulting to assuming content may seem OK, but I'd rather be paranoid. I also don't understand how this method checks the type of the nsIDOMWindow on which the nsCommandManger lives. Isn't that what we care about? +nsCommandManager::GetControllerForCommand(const char *aCommand, + nsIDOMWindow *aTargetWindow, + nsIController** outController) ... if (aTargetWindow) + { + // check if we're in content or chrome + // if we're not in chrome the windows must be the same From the patch it looks like the ischrome check only occurs if you supply a target window. I think it should always be done. The logic here, and in docommand, should be: if (targetWindow) destWindow = targetWindow else destWindow = focussedWindow if (nsCommandManager lives in content) check that destWindow is the same as the command manager's window (or a subframe, in which case check that destWindow is content) do stuff. Somewhere in here we'll need to check a command whitelist too, if we're in content. + if (!url.Equals("about:blank")) { + // If we're 'about:blank' then we don't care who can edit us. + // If we're not about:blank, then we need to check sameOrigin. This one isn't clear to me. Why not always do the sameOrigin check?

This patch looks OK, securitywise. The use of IsCallerChrome looks correct, and excepting about:blank is OK. Georgi, when this lands, could you please pound on it and see if it opens any holes? brade will post a testcase that you can use as a starting point.

1. If paste works, it may be used to accessing the content of the clipbard. Suggest at least a pref for disabling it. 2. Can't make selectall work from script. There is some chance selectall may be used for selecting nested frames. 3. saveas is #ifdef'ed out which is fine. But if saveas becomes scriptable in the future, this opens the possibility for exploits - saving a file in the user's startup folder, etc. What about making a pref which disables the whole scripting of editor for people who don't want it?

Ooops, I hit internal bugzilla error, so not all of my previous comment was included in the resubmitting. The patch seems to work fine in respect to DOM security. My previous comment explains some potential vulnerabilities if additional functionality is added.

Comment on attachment 105656 [details] [diff] [review] address feedback from smfr and mstoltz sr=sfraser. Does any JS/XBL need to change when this goes in?

Need to revisit mozilla/embedding/tests/mfcembed/EditorFrm.cpp I put in a temporary fix to add the additional parameter to the call. Probably need to change the calling function as well.

I'm going to resolve this bug as fixed to increase the testing it may get in this area. Please reopen if any issues are found.

vrfy'd fixed --sample url works nicely. (separate bugs for this feature, where applicable, have already been logged.)