Closed Bug 223041 Opened 19 years ago Closed 19 years ago

eval (and possibly other natives) "thunk" should not run with chrome privs

Categories

(Core :: Security: CAPS, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.6beta

People

(Reporter: brendan, Assigned: caillon)

References

Details

(Keywords: js1.5, Whiteboard: [sg:mustfix])

Attachments

(3 files, 4 obsolete files)

See bug 217195.  When content defines eval as a setter for a property in a
content DOM object that chrome calls, when eval runs it should use its "object
principals", which come unalterably from its global object via the parent link.

See bug 213946 for a typedef,

+typedef JSPrincipals *
+(* JS_DLL_CALLBACK JSGetObjectPrincipals)(JSContext *cx, JSObject *obj);

that seems helpful here, if used to declare a JSRuntime member set by caps code.
If set, obj_eval (jsobj.c) would call that hook on the parent of the eval funobj
being called (argv[-2]), and prefer the result to the caller's principals.

Can anyone think of native functions other than eval that need such treatment? 
Ah -- Script.prototype.compile and the Script constructor, of course.

/be
Function, setTimeout, setInterval?
Taking.

/be
Assignee: security-bugs → brendan
Priority: -- → P1
Target Milestone: --- → mozilla1.6beta
So we need a getObjectPrincipals callback, probably per-context (so privileged
contexts don't need to pay a price).

If caps code sets this for DOM contexts, then we can kill this bird and the
other one (bug 213946) with the same stone.

There are several places in the JS engine that pass caller->script->principals
on to callee.  That's wrong on two counts: one, a native may be calling the
callee, so it's necessary to loop (see jsfun.c, Function, which gets this
right); two, the caller->script->principals should be trumped by caller function
object principals in the case of a cloned function object (e.g. brutal sharing).

Patch for 1.6b coming soon.

/be
Status: NEW → ASSIGNED
Keywords: js1.5
Attached patch proposed fix (obsolete) — Splinter Review
Note one difference from the patch in bug 213946: the callback to find an
object principal from an object on a given cx is JSObjectPrincipalsFinder (the
function pointer typedef), and it returns a weak ref to the JSPrincipals (so
the impl. must not JSPRINCIPALS_HOLD).

I took this opportunity to fix JSPRINCIPALS_HOLD and JSPRINCIPALS_DROP to use
atomic ops #ifdef JS_THREADSAFE.

diff -w version in a few seconds (I got rid of some trailing whitespace, and
the indentation changed in a few places).

/be
Attached patch diff -w version of last patch (obsolete) — Splinter Review
Attachment #134083 - Flags: superreview?(shaver)
Attachment #134083 - Flags: review?(caillon)
Comment on attachment 134083 [details] [diff] [review]
diff -w version of last patch

>+JS_PUBLIC_API(JSPrincipals *)
>+JS_StackFramePrincipals(JSContext *cx, JSStackFrame *fp)
>+{
>+    if (fp->fun && cx->findObjectPrincipals) {
>+        JSObject *callee = JSVAL_TO_OBJECT(fp->argv[-2]);
>+
>+        if (fp->fun->object != callee)
>+            return cx->findObjectPrincipals(cx, callee);
>+        /* FALL THROUGH */
>+    }

Should we sample cx->fOP to a local, and then test that local, to protect
against a race to unset it?

>+/*
>+ * Return a weak reference to the fp's principals.  A null return does not
>+ * mean error, it means no principals.
>+ */
>+extern JS_PUBLIC_API(JSPrincipals *)
>+JS_StackFramePrincipals(JSContext *cx, JSStackFrame *fp);

Is it safe to return a weak reference from JS_StackFramePrincipals?  Can
another thread not race to destroy it, leaving the JS_SFP caller with a
dangling pointer?  (Or does the frame pointer, whose lifetime is guaranteed by
the caller, hold a ref to the prins?  I forget.)

>@@ -1048,17 +1049,17 @@ obj_eval(JSContext *cx, JSObject *obj, u
>         ok = JS_FALSE;
>         goto out;
>     }
> 
> #if !JS_BUG_EVAL_THIS_SCOPE
> #if JS_HAS_SCRIPT_OBJECT
>     if (argc < 2)
> #endif
>-    {
>+    if (caller) {
>         /* Execute using caller's new scope object (might be a Call object). */
>         scopeobj = caller->scopeChain;
>     }
> #endif

Let's give |if (caller)| its very own set of braces, and leave the existing set
to optionally bound the EVAL_THIS_SCOPE/SCRIPT_OBJECT argc check.  (I had to
think about how an else appended to the caller-check's then would bind, and I
don't like having to think about that sort of thing.)

>+ * Return a weak reference to the principals associated with obj, possibly via
>+ * the immutable parent chain leading from obj to a top-level container (e.g.,
>+ * a window object in the DOM level 0).  If there are no principals associated
>+ * with obj.  Null does not mean an error was reported; in no event should an
>+ * error or exception be thrown by this callback's implementation.
>+ */
>+typedef JSPrincipals *
>+(* JS_DLL_CALLBACK JSObjectPrincipalsFinder)(JSContext *cx, JSObject *obj);

Again, should we ask cx->fOP implementors to JS_HOLD_PRINCIPALS the return
value, so that it's not a weak ref?

sr=shaver with the braces added, and the weak refs either explained away or
made strong.
Attachment #134083 - Flags: superreview?(shaver) → superreview+
> Should we sample cx->fOP to a local, and then test that local, to protect
> against a race to unset it?

Nope, cx is per-thread, no racey.

> Is it safe to return a weak reference from JS_StackFramePrincipals?  Can
> another thread not race to destroy it, leaving the JS_SFP caller with a
> dangling pointer?  (Or does the frame pointer, whose lifetime is guaranteed by
> the caller, hold a ref to the prins?  I forget.)

The stack frame is still active, so its script can't be GC'd, so the script's
strong ref to its principals lives till the stack unwinds.  In the cloned
function object case, the function object's object-principals trump the
script's, but again those live as long as the function object (fp->argv[-2])
lives, because the object principals are owned by the parent of the funobj (the
window object, in the DOM embedding).

So refcounting sucks, and should be avoided where not needed. Just ask bryner,
who deCOMtaminated nsIDocument recently.  There is no need for a strong ref from
the object-principals finder, and making it return one adds not only needless
bumping and dropping (threadsafe, yet), but it cruds up the callers in jsfun.c,
jsobj.c, and jsscript.c, who must remember to drop on all exit paths from the
function, once the held ref is returned (were we to change fOP to hold).  No C++
auto-class helpers!

I'll fix the bracing gaffe, thanks for pointing that out.

/be
Comment on attachment 134083 [details] [diff] [review]
diff -w version of last patch

>Index: jsapi.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsapi.c,v
>retrieving revision 3.152
>diff -p -u -8 -w -r3.152 jsapi.c
>--- jsapi.c	22 Oct 2003 06:26:03 -0000	3.152
>+++ jsapi.c	25 Oct 2003 01:32:55 -0000
>@@ -2869,24 +2869,48 @@ JS_SetReservedSlot(JSContext *cx, JSObje
>                              JSMSG_RESERVED_SLOT_RANGE);
>         return JS_FALSE;
>     }
>     slot = JSSLOT_START(clasp) + index;
>     OBJ_SET_REQUIRED_SLOT(cx, obj, slot, v);
>     return JS_TRUE;
> }
> 
>+#ifdef JS_THREADSAFE
>+JS_PUBLIC_API(uintN)


It seems that jsrefcount is signed, so this probably should match.  Why is
jsrefcount signed though?  Would it make more sense to change that to unsigned?



>+JS_HoldPrincipals(JSContext *cx, JSPrincipals *principals)
>+{
>+    return JS_ATOMIC_INCREMENT(&principals->refcount);
>+}
>+
>+JS_PUBLIC_API(uintN)


Same as above.



>+JS_DropPrincipals(JSContext *cx, JSPrincipals *principals)
>+{
>+    return JS_ATOMIC_DECREMENT(&principals->refcount);
>+}
>+#endif
>+



>Index: jsapi.h
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsapi.h,v
>retrieving revision 3.80
>diff -p -u -8 -w -r3.80 jsapi.h
>--- jsapi.h	21 Oct 2003 18:03:04 -0000	3.80
>+++ jsapi.h	25 Oct 2003 01:32:56 -0000
>@@ -1097,29 +1097,43 @@ JS_SetReservedSlot(JSContext *cx, JSObje
>  * Security protocol.
>  */
> struct JSPrincipals {
>     char *codebase;
>     void * (* JS_DLL_CALLBACK getPrincipalArray)(JSContext *cx, JSPrincipals *);
>     JSBool (* JS_DLL_CALLBACK globalPrivilegesEnabled)(JSContext *cx, JSPrincipals *);
> 
>     /* Don't call "destroy"; use reference counting macros below. */
>-    uintN refcount;
>+    jsrefcount refcount;
>     void (* JS_DLL_CALLBACK destroy)(JSContext *cx, struct JSPrincipals *);
> };
> 
>-#define JSPRINCIPALS_HOLD(cx, principals)               \
>-    ((principals)->refcount++)
>+#ifdef JS_THREADSAFE
>+#define JSPRINCIPALS_HOLD(cx, principals)   JS_HoldPrincipals(cx,principals)
>+#define JSPRINCIPALS_DROP(cx, principals)   JS_DropPrincipals(cx,principals)
>+
>+extern JS_PUBLIC_API(uintN)
>+JS_HoldPrincipals(JSContext *cx, JSPrincipals *principals);
>+
>+extern JS_PUBLIC_API(uintN)
>+JS_DropPrincipals(JSContext *cx, JSPrincipals *principals);
>+
>+#else
>+#define JSPRINCIPALS_HOLD(cx, principals)   ((principals)->refcount++)
> #define JSPRINCIPALS_DROP(cx, principals)               \
>-    ((--((principals)->refcount) == 0)                  \
>+    ((--(principals)->refcount == 0)                                          \
>         ? (*(principals)->destroy)((cx), (principals))  \
>         : (void) 0)


Do we want to make it possible to do

  jsrefcount foo = JSPRINCIPALS_DROP(cx, principals)

and get the expected refcnt?

It looks like it is currently possible to do this for JSPRINCIPALS_HOLD.  I
think that the addref/release stuff in caps which shares the nsIPrincipal
refcnt with the jsprincipals would be able to make good use of this
functionality.


>+#endif
> 
> extern JS_PUBLIC_API(JSPrincipalsTranscoder)
> JS_SetPrincipalsTranscoder(JSRuntime *rt, JSPrincipalsTranscoder px);
>+
>+extern JS_PUBLIC_API(JSObjectPrincipalsFinder)
>+JS_SetObjectPrincipalsFinder(JSContext *cx, JSObjectPrincipalsFinder fop);
> 
> /************************************************************************/
> 
> /*
>  * Functions and scripts.
>  */
> extern JS_PUBLIC_API(JSFunction *)
> JS_NewFunction(JSContext *cx, JSNative call, uintN nargs, uintN flags,




>Index: jsdbgapi.h
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsdbgapi.h,v
>retrieving revision 3.17
>diff -p -u -8 -w -r3.17 jsdbgapi.h
>--- jsdbgapi.h	19 Mar 2002 04:28:20 -0000	3.17
>+++ jsdbgapi.h	25 Oct 2003 01:32:56 -0000
>@@ -140,28 +140,41 @@ extern JS_PUBLIC_API(JSStackFrame *)
> JS_FrameIterator(JSContext *cx, JSStackFrame **iteratorp);
> 
> extern JS_PUBLIC_API(JSScript *)
> JS_GetFrameScript(JSContext *cx, JSStackFrame *fp);
> 
> extern JS_PUBLIC_API(jsbytecode *)
> JS_GetFramePC(JSContext *cx, JSStackFrame *fp);
> 
>-extern JS_PUBLIC_API(JSBool)
>-JS_IsNativeFrame(JSContext *cx, JSStackFrame *fp);
>+/*
>+ * Get the closest scripted frame below fp.  If fp is null, start from cx->fp.
>+ */
>+extern JS_PUBLIC_API(JSStackFrame *)
>+JS_GetScriptedCaller(JSContext *cx, JSStackFrame *fp);
>+
>+/*
>+ * Return a weak reference to the fp's principals.  A null return does not
>+ * mean error, it means no principals.


I think you have a gratuitous "the" in the above comment's first sentence, and
I think the second could probably be formed a little better.

"A null return does not denote an error; it means there are no principals."
perhaps?


>+ */
>+extern JS_PUBLIC_API(JSPrincipals *)
>+JS_StackFramePrincipals(JSContext *cx, JSStackFrame *fp);



>Index: jsobj.c

>@@ -1019,25 +1019,26 @@ obj_eval(JSContext *cx, JSObject *obj, u
>         /* From here on, control must exit through label out with ok set. */
> #endif
> 
> #if JS_BUG_EVAL_THIS_SCOPE
>         /* An old version used the object in which eval was found for scope. */
>         scopeobj = obj;
> #else
>         /* Compile using caller's current scope object. */
>+        if (caller)
>         scopeobj = caller->scopeChain;
> #endif
>     }
> 
>     str = JSVAL_TO_STRING(argv[0]);
>-    if (caller->script) {
>+    if (caller) {
>         file = caller->script->filename;



Are we guaranteed that caller->script is non-null here?



>         line = js_PCToLineNumber(cx, caller->script, caller->pc);
>-        principals = caller->script->principals;
>+        principals = JS_StackFramePrincipals(cx, caller);
>     } else {
>         file = NULL;
>         line = 0;
>         principals = NULL;
>     }
> 
>     fp->flags |= JSFRAME_EVAL;
>     script = JS_CompileUCScriptForPrincipals(cx, scopeobj, principals,
>@@ -1048,17 +1049,17 @@ obj_eval(JSContext *cx, JSObject *obj, u
>         ok = JS_FALSE;
>         goto out;
>     }
> 
> #if !JS_BUG_EVAL_THIS_SCOPE
> #if JS_HAS_SCRIPT_OBJECT
>     if (argc < 2)
> #endif
>-    {
>+    if (caller) {
>         /* Execute using caller's new scope object (might be a Call object). */
>         scopeobj = caller->scopeChain;
>     }
> #endif
>     ok = js_Execute(cx, scopeobj, script, caller, JSFRAME_EVAL, rval);
>     JS_DestroyScript(cx, script);
> 
> out:
>Index: jspubtd.h
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jspubtd.h,v
>retrieving revision 3.37
>diff -p -u -8 -w -r3.37 jspubtd.h
>--- jspubtd.h	21 Oct 2003 18:03:04 -0000	3.37
>+++ jspubtd.h	25 Oct 2003 01:32:58 -0000
>@@ -540,12 +540,22 @@ typedef struct JSPrincipals JSPrincipals
>  * JSXDR_ENCODE, in which case *principalsp should be encoded; or JSXDR_DECODE,
>  * in which case implementations must return a held (via JSPRINCIPALS_HOLD),
>  * non-null *principalsp out parameter.  Return true on success, false on any
>  * error, which the implementation must have reported.
>  */
> typedef JSBool
> (* JS_DLL_CALLBACK JSPrincipalsTranscoder)(JSXDRState *xdr,
>                                            JSPrincipals **principalsp);
>+
>+/*
>+ * Return a weak reference to the principals associated with obj, possibly via
>+ * the immutable parent chain leading from obj to a top-level container (e.g.,
>+ * a window object in the DOM level 0).  If there are no principals associated
>+ * with obj.


Huh?   :-)



  Null does not mean an error was reported; in no event should an
>+ * error or exception be thrown by this callback's implementation.
>+ */
>+typedef JSPrincipals *
>+(* JS_DLL_CALLBACK JSObjectPrincipalsFinder)(JSContext *cx, JSObject *obj);
> 
> JS_END_EXTERN_C
> 
> #endif /* jspubtd_h___ */
> It seems that jsrefcount is signed, so this probably should match.  Why is
> jsrefcount signed though?  Would it make more sense to change that to unsigned?


jsrefcount is int32 (aka PRInt32) because that's what PR_AtomicIncrement takes a
pointer to, same for PR_AtomicDecrement.  I fixed the return types for the new
JS API entry points.

> Do we want to make it possible to do
> 
>   jsrefcount foo = JSPRINCIPALS_DROP(cx, principals)
> 
> and get the expected refcnt?

Oops.  And I blew JSPRINCIPALS_HOLD in the !JS_THREADSAFE case -- it needs to
preincrement, to match PR_AtomicIncrement.  Here are the new non-threadsafe
versions of the macros:

#define JSPRINCIPALS_HOLD(cx, principals)   (++(principals)->refcount)
#define JSPRINCIPALS_DROP(cx, principals)                                     \
    ((--(principals)->refcount == 0)                                          \
     ? ((*(principals)->destroy)((cx), (principals)), 0)                      \
     : (principals)->refcount)

> I think you have a gratuitous "the" in the above comment's first sentence,
> and I think the second could probably be formed a little better.

Fixed, thanks.

> Are we guaranteed that caller->script is non-null here?

Yes, because caller was set to JS_GetScriptedCaller's return value.  If it's
non-null, then caller->script must be non-null.

> Huh?   :-)

Fixed that too -- thanks!

/be
> >-    if (caller->script) {
> >+    if (caller) {
> >         file = caller->script->filename;
>Are we guaranteed that caller->script is non-null here?

Yes we are, by way of JS_GetScriptedCaller above.
Attachment #134083 - Flags: superreview+
Attachment #134083 - Flags: review?(caillon)
Attached patch fixed per caillon's comments (obsolete) — Splinter Review
This is the diff -w, as I'm just looking for review stamps.

/be
Attachment #134082 - Attachment is obsolete: true
Attachment #134083 - Attachment is obsolete: true
Comment on attachment 134287 [details] [diff] [review]
fixed per caillon's comments

So close -- dazzled by the minimal and beauteous changes and API extensions
here, we all seem to have forgotten that the point of this bug is to make sure
eval's native function object principals trump its caller's principals, so that
content can't stick eval as the setter for a link href and cause a javascript:
url to be eval'ed as a labeled expression statement that runs with chrome
privs, due to chrome setting href naively (as in hyatt's patch for
instantaneous visited link coloring).

Whew.  I will have a new patch later today.  What's here is good, it just
doesn't go the final distance.

/be
Attachment #134287 - Attachment is obsolete: true
Comment on attachment 134287 [details] [diff] [review]
fixed per caillon's comments

Looks better to me, but I think we all know that caillon's review is where it's
at for this patch. r=shaver
Attachment #134287 - Attachment is obsolete: false
Attachment #134287 - Flags: review+
Oops. Yeah, I got caught up in jsapi land studying the means and forgot about
the way.  Thanks for bringing us all back into check!
I added JS_EvalFramePrincipals to jsdbgapi.h, used by eval and friends in
conjunction with JS_GetScriptedCaller.	JS_EvalFramePrincipals prefers the
object principals for fp->argv[-2] (which is the "callee" jsval for eval,
Function, or Script) to the caller's JS_StackFramePrincipals if
cx->findObjectPrincipals is non-null.

Embeddings that don't use principals, or that don't set cx->fOP, work as
before. Mozilla, once we change caps to set cx->fOP, will run eval with *that
particular eval method's* object principals (for example, w.eval for content
window w loading evil content, where content script has set its window's eval
method as the setter of a link.href, in order to run a javascript: URL as a
chrome script -- bug 217195).

/be
Attachment #134287 - Attachment is obsolete: true
Attachment #134434 - Flags: superreview?(shaver)
Attachment #134434 - Flags: review?(caillon)
Attachment #134287 - Flags: review+
Passes the js testsuite, btw.

/be
Comment on attachment 134434 [details] [diff] [review]
diff -w version of last patch

*stamp*; sr=shaver
Attachment #134434 - Flags: superreview?(shaver) → superreview+
Comment on attachment 134434 [details] [diff] [review]
diff -w version of last patch

Looks good from where I stand.	r=caillon.
Attachment #134434 - Flags: review?(caillon) → review+
Checked in.  Caillon, can you patch caps to provide the needed fOP callback? 
Thanks,

/be
Assignee: brendan → caillon
Status: ASSIGNED → NEW
This patch broke the build, thanks to the following caller of JSPRINCIPALS_HOLD
which is now passing a variable that doesn't exist to a function instead of just
a macro:

NS_IMETHODIMP
nsPrincipal::GetJsPrincipals(JSPrincipals **jsprin)
{
  NS_PRECONDITION(mJSPrincipals.nsIPrincipalPtr, "mJSPrincipals is uninitalized!");
                                                                               
         
  *jsprin = &mJSPrincipals;
                                                                               
         
  // JSPRINCIPALS_HOLD does not use its first argument.
  // Just use a dummy cx to save the codesize.
  JSPRINCIPALS_HOLD(cx, *jsprin);
                                                                               
         
  return NS_OK;
}
Comment on attachment 134434 [details] [diff] [review]
diff -w version of last patch

>+JS_PUBLIC_API(jsrefcount)
>+JS_DropPrincipals(JSContext *cx, JSPrincipals *principals)
>+{
>+    return JS_ATOMIC_DECREMENT(&principals->refcount);
>+}

Doesn't this (or the macro) need to do something if the refcount hits zero?
I temporarily changed the dummy variables to null to fix the bustage, but I'm
not happy with that fix, and I hope somebody changes it soon.
Sorry, flu still upon me, thanks to dbaron for cleaning up the mess.  I fixed
JS_DropPrincipals, but left it to caillon to re-parameterize GetJsPrincipals to
take a JSContext* argument.

/be
Since JSPRINCIPALS_HOLD and JSPRINCIPALS_DROP have been in jsapi.h since 1997 or
so, I don't think we can change them to drop the cx parameter.  So I'm hoping
it's not to hard to change

nsIPrincipal.idl:    readonly attribute JSPrincipals jsPrincipals;

to be a method.  The somewhat odd "Js" spelling can be rectified.  Aw heck, I
should help.  The files to change, from
http://lxr.mozilla.org/mozilla/search?string=GetJSPrincipal, are

caps/src/nsJSPrincipals.cpp
caps/src/nsPrincipal.cpp
caps/src/nsSystemPrincipal.cpp
dom/src/base/nsJSEnvironment.cpp
embedding/browser/activex/src/plugin/LegacyPlugin.cpp
js/src/liveconnect/nsCLiveconnect.cpp
js/src/xpconnect/loader/mozJSComponentLoader.cpp
js/src/xpconnect/loader/mozJSSubScriptLoader.cpp
modules/oji/src/lcglue.cpp
security/manager/ssl/src/nsCrypto.cpp

Patch anon.

/be
Comment on attachment 134637 [details] [diff] [review]
better fix for the missing cx  param problem

-    readonly attribute JSPrincipals jsPrincipals;
+    JSPrincipals GetJSPrincipals(in JSContext cx);

Pretend I spelled that method name "getJSPrincipals", which is how it's spelled
now in my tree.  Just for IDL interCaps (vs. C++ InterCaps) purity and symmetry
with the rest of the methods in this non-scriptable interface.

/be
Attachment #134637 - Flags: superreview?(shaver)
Attachment #134637 - Flags: review?(caillon)
Comment on attachment 134637 [details] [diff] [review]
better fix for the missing cx  param problem

Forgot a struct JSContext; in nsIPrincipal.idl's %{C++ section.

/be
Attachment #134637 - Attachment is obsolete: true
Attachment #134637 - Flags: superreview?(shaver)
Attachment #134637 - Flags: review?(caillon)
This patch also fixes nsSystemPrincipal.cpp's JSPRINCIPALS_HOLD call to pass
cx.

/be
Attachment #134672 - Flags: superreview?(shaver)
Attachment #134672 - Flags: review?(caillon)
Comment on attachment 134672 [details] [diff] [review]
working fix for the missing cx param problem

Thanks for the quick turnaround on the patch.  r=caillon.
Attachment #134672 - Flags: review?(caillon) → review+
Attachment #134672 - Flags: superreview?(shaver) → superreview+
we should mark this fixed, and open a new bug for continuing work, right?  that
is the way to get things tested....    or is this all done?
Caillon, can you close, filing any followup bugs?  I don't know of any, but I'm
not thinking hard ATM.

/be
This is fixed, no one has thought of any follow-on issues (which should go in a
new bug, besides)
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: [sg:mustfix]
I've been asked to verify this on the 1.7 branch.  Is there test case(s) available?
So we forgot to call JS_SetObjectPrincipalsFinder anywhere!  Fortunately, jst
did that in patching bug 289074.  D'oh!  Oh well, finally fixed for real.

/be
Depends on: 289074
Group: security
You need to log in before you can comment on or make changes to this bug.