386635 - Return XPCSafeJSObjectWrappers from evalInSandbox

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Description

[Blake Kaplan (:mrbkap) (inactive)]

From all of the recent PAC bugs, it's become apparent that using evalInSandbox is difficult to do safely. I think that if evalInSandbox returned XPCSafeJSObjectWrappers instead of raw objects, we'd mitigate a lot of the potential problems. I thought I filed a bug on this a while ago, but apparently I forgot, so this one will do.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proof of concept patch

The only thing left here is the #if 0 in SandboxImport. The problem is that by the time we get into SandboxImport now, we're in the function generated from the safe JSObject wrapper, so when we do the security check, it fails.

My proposed fix is to move importFunction to Components.utils.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With that implemented

jst, what do you think?

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated fix

With this patch, we are now strict about passing in an XPCSafeJSObjectWrapper around a sandbox. I've removed the security check in ImportFunction, since that now lives on Components.utils, which is chrome-only anyway.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 270801 [details] [diff] [review]
Updated fix


>+    /*
>+     * importIntoSandbox must only be called from JavaScript.

s/only be called/be called only/

You can actually check, via XPCContext::GetCallingLangType() IIRC, and enforce this restriction in the ImportIntoSandbox method itself.

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Addressing those comments

Since GetCallerLanguage lives on XPCCallContext, I couldn't go through nsXPConnect to get an nsIXPCCallContext. Since I was making that change, I also switched to using the non-virtual accessor methods, which are a lot nicer to use.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 7

[aja+bugzilla]

I think this checkin may be causing bug 387859

Comment 8

[Sylvain Pasche]

Should this solve the potential security problems described in http://developer.mozilla.org/en/docs/Components.utils.evalInSandbox#Security ?

Comment 9

[Jeff Walden [:Waldo]]

I tested a local backout of this patch, and it did indeed cause bug 387859, so I backed out the patch for this bug.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Please make sure to land tests for whatever that problem was?

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Bug 387859 comment 9 explains why this broke session store.

Comment 12

[aja+bugzilla]

This apparently caused two other issues:
1) Search suggestions not working
2) missing History->Recently closed tabs. 

FWIW, a couple other consumers to consider when creating tests

Comment 13

[Brian Crowder]

Greasemonkey, too?  Bug 387848

Comment 14

[(not reading, please use [e-mail] instead)]

This also caused a problem with copy-and-paste of bookmarks in places.

Comment 15

[Daniel Veditz [:dveditz]]

Seth: is there a bug on the copy-paste issue? If not please file one and mark it "blocking" this one so we remember to re-test when this one lands again.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

I looked into the bustage that this caused. The problem is some code that jst added in SafeJSObjectWrappers to deal with sites using allAccess properties through them. We discussed a couple of options for fixing this, but in the end, decided to not bother following through with this patch. Hopefully, once cross origin wrappers land, PAC will be safe without this.

Marking INVALID, since we're never going to do this.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

I backed off too easily here. I'm going to revive this bug and fix SJOWs so that they can be used by arbitrary chrome.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to trunk

I'm not going to request review until the dependencies have landed. This updates the patch to trunk and also adds a missing request in ImportIntoSandbox.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Oh, and until I remember to remove the #if 0 stuff -- I might get rid of that option and just rely on safe JSObject wrappers to do the right thing.

Comment 20

[Damon Sicore (:damons)]

+'ing with P2.   Blake please adjust priority as needed.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to trunk

Comment 22

[Jesse Ruderman]

The Greasemonkey scripts I use still work with this patch.

Comment 23

[Jesse Ruderman]

I think this breaks suggestions in Firefox's search bar.  If I type "mozilla fi" into the search bar, I don't get any suggestions, and I see

###!!! ASSERTION: toString failed us: 'type != JSTYPE_STRING', file /Users/jruderman/trunk/mozilla/js/src/xpconnect/src/XPCSafeJSObjectWrapper.cpp, line 663

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Search suggest uses JSOM.jsm's fromString, which uses a sandbox.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/loader/JSON.jsm&rev=1.2&mark=156#156
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/search/nsSearchSuggestions.js&rev=1.19&mark=526#503

Comment 25

[Damon Sicore (:damons)]

Moving this to wanted-Next and clearing priority.  Triage w/ jst and sicking, decided this won't block the release.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

I filed bug 420647 on comment 23.

Comment 27

[Dan Phiffer]

Could this be what's causing eval'd Greasemonkey API calls to fail?

http://greasemonkey.devjavu.com/ticket/137

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

No, this was backed out and never checked back in.

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated again

This is the bug that just won't go away! This patch is refreshed. I've tested all reported regressions from the previous patches (and filed bug 513428 to fix some more bustage).

One important note: C.u.importIntoSandbox is not included in this patch. We're going to end up relying on SJOWs to do all of the proper wrapping (go, go gadget COW!).

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Compatibility fixes

This applies on top of the main patch and implements compatibility fixes until we come up with a better API for common evalInSandbox uses (e.g. how GreaseMonkey uses evalInSandbox).

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated

This fixes a leak due to picking the wrong scope for the wrappers returned from evalInSandbox.

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Compatibility fixes

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/0a8b08d8f142 and
http://hg.mozilla.org/mozilla-central/rev/55a7fb78d78b

Comment 34

[Johnathan Nightingale [:johnath]]

Please don't nominate for tracking without justification about why this bug needs specific attention in Firefox 5.