Last Comment Bug 751858 - location's PUNCTURE policy throws without setting an exception
: location's PUNCTURE policy throws without setting an exception
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla16
Assigned To: Blake Kaplan (:mrbkap)
: Andrew Overholt [:overholt]
Depends on:
Blocks: 667388 CVE-2012-4193
  Show dependency treegraph
Reported: 2012-05-04 05:22 PDT by Blake Kaplan (:mrbkap)
Modified: 2012-06-05 06:14 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Proposed fix (1.76 KB, patch)
2012-05-04 05:26 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Proposed fix v2 (1.81 KB, patch)
2012-05-04 05:58 PDT, Blake Kaplan (:mrbkap)
bobbyholley: review+
mrbkap: checkin+
Details | Diff | Splinter Review

Description Blake Kaplan (:mrbkap) 2012-05-04 05:22:07 PDT
I don't have a testcase right now, but I ran into this working on another bug. Basically, Policy::check functions are responsible for setting their own exceptions if they deny access.
Comment 1 Blake Kaplan (:mrbkap) 2012-05-04 05:26:04 PDT
Created attachment 621016 [details] [diff] [review]
Proposed fix

I think the change to AccessCheck.cpp makes the control flow a little clearer (and it nukes a couple of braces). It's a little unfortunate that this patch actually makes LocationPolicy::check less clear (since the exception for PUNCTURE is "hidden" in a larger boolean expression) but it's the easiest way to share the code that throws the exception.
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2012-05-04 05:46:04 PDT
Comment on attachment 621016 [details] [diff] [review]
Proposed fix

>--- a/js/xpconnect/wrappers/AccessCheck.cpp
>+++ b/js/xpconnect/wrappers/AccessCheck.cpp
>@@ -494,12 +494,10 @@ ExposedPropertiesOnly::check(JSContext *cx, JSObject *wrapper, jsid id, Wrapper:
>         perm = PermitObjectAccess;
>         return true;
>     }
>-    if (act == Wrapper::PUNCTURE) {
>-        perm = DenyAccess;
>-        return false;
>-    }
>     perm = DenyAccess;
>+    if (act == Wrapper::PUNCTURE)
>+        return false;

Doesn't this have the same issue?
Comment 3 Blake Kaplan (:mrbkap) 2012-05-04 05:58:14 PDT
Created attachment 621024 [details] [diff] [review]
Proposed fix v2

Oops, yes.
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2012-05-04 06:18:37 PDT
Comment on attachment 621024 [details] [diff] [review]
Proposed fix v2

Can we add some tests for this? I'm about to touch this code in a couple of ways (fixing __exposedProps__, and removing universalXPConnect), and this seems like the sort of thing I might regress.
Comment 5 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-06-02 15:16:31 PDT
So ... can we land this?
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2012-06-04 03:23:48 PDT
I think so, yes.
Comment 7 Blake Kaplan (:mrbkap) 2012-06-04 12:09:36 PDT
Comment on attachment 621024 [details] [diff] [review]
Proposed fix v2
Comment 8 Geoff Lankow (:darktrojan) 2012-06-05 06:14:56 PDT

Note You need to log in before you can comment on or make changes to this bug.