Closed Bug 386635 Opened 12 years ago Closed 10 years ago

Return XPCSafeJSObjectWrappers from evalInSandbox

Categories

(Core :: XPConnect, defect, P2)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- ?

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

Attachments

(2 files, 8 obsolete files)

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.
Attached patch Proof of concept patch (obsolete) — Splinter Review
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.
Attached patch With that implemented (obsolete) — Splinter Review
jst, what do you think?
Attachment #270632 - Attachment is obsolete: true
Attachment #270637 - Flags: review?(jst)
Flags: blocking1.9?
Attached patch Updated fix (obsolete) — Splinter Review
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.
Attachment #270637 - Attachment is obsolete: true
Attachment #270801 - Flags: review?(jst)
Attachment #270637 - Flags: review?(jst)
Attachment #270801 - Flags: review?(jst) → review+
Attachment #270801 - Flags: superreview?(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
Attached patch Addressing those comments (obsolete) — Splinter Review
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.
Attachment #270801 - Attachment is obsolete: true
Attachment #271106 - Flags: superreview?(brendan)
Attachment #271106 - Flags: review+
Attachment #270801 - Flags: superreview?(brendan)
Attachment #271106 - Flags: superreview?(brendan) → superreview+
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
I think this checkin may be causing bug 387859
Should this solve the potential security problems described in http://developer.mozilla.org/en/docs/Components.utils.evalInSandbox#Security ?
I tested a local backout of this patch, and it did indeed cause bug 387859, so I backed out the patch for this bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Please make sure to land tests for whatever that problem was?
Bug 387859 comment 9 explains why this broke session store.
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

Flags: in-testsuite?
Greasemonkey, too?  Bug 387848
Flags: blocking1.9? → blocking1.9+
This also caused a problem with copy-and-paste of bookmarks in places.
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.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.6?
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.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Flags: blocking1.9+
Resolution: --- → INVALID
Flags: wanted1.8.1.x-
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.7?
Flags: blocking1.8.1.7-
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.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Status: REOPENED → ASSIGNED
Flags: blocking1.9?
Attached patch Updated to trunk (obsolete) — Splinter Review
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.
Attachment #271106 - Attachment is obsolete: true
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.
+'ing with P2.   Blake please adjust priority as needed.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Attached patch Updated to trunk (obsolete) — Splinter Review
Attachment #294770 - Attachment is obsolete: true
The Greasemonkey scripts I use still work with this patch.
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
Moving this to wanted-Next and clearing priority.  Triage w/ jst and sicking, decided this won't block the release.
Flags: tracking1.9+ → wanted1.9.0.x+
Priority: P2 → --
Could this be what's causing eval'd Greasemonkey API calls to fail?

http://greasemonkey.devjavu.com/ticket/137
No, this was backed out and never checked back in.
Attached patch Updated again (obsolete) — Splinter Review
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!).
Attachment #304533 - Attachment is obsolete: true
Attached patch Compatibility fixes (obsolete) — Splinter Review
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).
Attachment #434766 - Flags: review?(jst)
Attachment #434766 - Flags: review?(jst) → review+
Attached patch UpdatedSplinter Review
This fixes a leak due to picking the wrong scope for the wrappers returned from evalInSandbox.
Attachment #397432 - Attachment is obsolete: true
Attachment #437450 - Flags: review?(jst)
Attachment #434766 - Attachment is obsolete: true
Attachment #437454 - Flags: review?(jst)
Attachment #437450 - Flags: review?(jst) → review+
Attachment #437454 - Flags: review?(jst) → review+
http://hg.mozilla.org/mozilla-central/rev/0a8b08d8f142 and
http://hg.mozilla.org/mozilla-central/rev/55a7fb78d78b
Status: ASSIGNED → RESOLVED
Closed: 12 years ago10 years ago
Resolution: --- → FIXED
blocking2.0: --- → ?
Please don't nominate for tracking without justification about why this bug needs specific attention in Firefox 5.
You need to log in before you can comment on or make changes to this bug.