The default bug view has changed. See this FAQ.

potential overflow in xml_setNamespace

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: bhackett, Assigned: gal)

Tracking

unspecified
mozilla2.0b7
Points:
---

Firefox Tracking Flags

(blocking2.0 beta8+, blocking1.9.2 .13+, status1.9.2 .13-fixed, blocking1.9.1 .16+, status1.9.1 .16-fixed)

Details

(Whiteboard: [sg:critical])

Attachments

(1 attachment)

Fast native xml_setNamespace can write to vp[2] when argc == 0

Here is the relevant code:

static JSBool
xml_setNamespace(JSContext *cx, uintN argc, jsval *vp)
{
    JSObject *qn;
    JSObject *ns;
    jsval qnargv[2];
    JSXML *nsowner;

    NON_LIST_XML_METHOD_PROLOG;
    if (!JSXML_HAS_NAME(xml))
        return JS_TRUE;

    xml = CHECK_COPY_ON_WRITE(cx, xml, obj);
    if (!xml)
        return JS_FALSE;

    ns = js_ConstructObject(cx, &js_NamespaceClass, NULL, obj,
                            argc == 0 ? 0 : 1, Valueify(vp + 2));
    if (!ns)
        return JS_FALSE;
    vp[0] = OBJECT_TO_JSVAL(ns);
    ns->setNamespaceDeclared(JSVAL_TRUE);

    qnargv[0] = vp[2] = OBJECT_TO_JSVAL(ns);
    qnargv[1] = OBJECT_TO_JSVAL(xml->name);
(Assignee)

Comment 1

7 years ago
Potentially exploitable if we reach the end of the 2MB stack chunk (free memory write).
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
Whiteboard: [sg:critical]
(Assignee)

Comment 2

7 years ago
Looks like vp[2] is used here for rooting. That's no longer necessary (conservative stack scanner!).
(Assignee)

Comment 3

7 years ago
Created attachment 478107 [details] [diff] [review]
patch
Assignee: general → gal
Attachment #478107 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #478107 - Flags: review? → review?(brendan)
Attachment #478107 - Flags: review?(brendan) → review+
We'll take it on the branches as soon as it's ready to go for trunk. If it makes next week's code-freeze that's great, otherwise next time.
blocking1.9.1: ? → needed
blocking1.9.2: ? → needed
status1.9.1: --- → wanted
status1.9.2: --- → wanted

Updated

7 years ago
blocking2.0: ? → beta8+

Comment 5

7 years ago
http://hg.mozilla.org/mozilla-central/rev/76b7f615bab9
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
blocking1.9.1: needed → .15+
blocking1.9.2: needed → .12+
(Assignee)

Comment 6

7 years ago
sayrer, do you mind landing this on the branches? should be trivial. I don't have decent connectivity here
(Assignee)

Updated

7 years ago
Assignee: gal → sayrer
(In reply to comment #6)
> sayrer, do you mind landing this on the branches? should be trivial. I don't
> have decent connectivity here

It doesn't have approval yet, so it can't land on the branches anyway.
Assignee: sayrer → gal
OS: Mac OS X → All
Hardware: x86 → All
Attachment #478107 - Flags: approval1.9.2.13?
Attachment #478107 - Flags: approval1.9.1.16?
(Assignee)

Comment 8

7 years ago
reed, I would appreciate if you don't reassign bugs without consent of the assigning or receiving party.

clegnitto emailed me about this bug and I was reacting to that email, trying to make sure this bug is being handled in my absence.

If sayrer wants me to hold onto a bug while I am on PTO and a release is pending, he will assign it back to me.

Please do not change the assignee of this bug again without consent from sayrer, a JS peer or a release driver.
Assignee: gal → sayrer
The bug assignee is always the patch writer. That's how things work. You don't get a choice in the matter. There are other ways to track when a bug needs work, and those ways should be used rather than misusing the assignee field.
(Assignee)

Comment 10

7 years ago
Please point me to the policy that supports your claim that contributors cannot hand off bugs in their own module to each other. I am highly doubtful that such a policy exists.

We do, however, have a binding Bugzilla Etiquette:

https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

"2. Changing Fields

    No messing with other people's bugs. Unless you are the bug assignee, or have some say over the use of their time, never change the Priority or Target Milestone fields. If in doubt, do not change the fields of bugs you do not own - add a comment instead, suggesting the change."

You are not the assignee, nor do you have say over the use of my time, so please keep your hands of my bugs.
Comment on attachment 478107 [details] [diff] [review]
patch

So is this the right branch patch, or is reed just guessing with the approval requests? One interpretation of Andreas reassigning without requesting approval is that some porting work needs to be done.
Attachment #478107 - Flags: approval1.9.2.13?
Attachment #478107 - Flags: approval1.9.2.13+
Attachment #478107 - Flags: approval1.9.1.16?
Attachment #478107 - Flags: approval1.9.1.16+
Comment on attachment 478107 [details] [diff] [review]
patch

oops, didn't mean to approve yet. It's certainly approvable if this is the right branch patch.
Attachment #478107 - Flags: approval1.9.2.13?
Attachment #478107 - Flags: approval1.9.2.13+
Attachment #478107 - Flags: approval1.9.1.16?
Attachment #478107 - Flags: approval1.9.1.16+
(Assignee)

Comment 13

7 years ago
#11: From a quick check the patch needs a trivial merge. I will be back Tuesday night, if sayrer didn't get to this until then I will do it.
Comment on attachment 478107 [details] [diff] [review]
patch

Approved for 1.9.2.13 and 1.9.1.16, a=dveditz for release-drivers
Attachment #478107 - Flags: approval1.9.2.13?
Attachment #478107 - Flags: approval1.9.2.13+
Attachment #478107 - Flags: approval1.9.1.16?
Attachment #478107 - Flags: approval1.9.1.16+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/def535668c41
Assignee: sayrer → gal
status1.9.2: wanted → .13-fixed
Target Milestone: --- → mozilla2.0b7
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b0216c5e5f4a
status1.9.1: wanted → .16-fixed
Considering comment 2, and the fact that if I'm not mistaken, conservative stack scanner is only available on m-c, is it really the right fix for branches ?

Comment 18

6 years ago
Will leaving in this (potentially incomplete) fix cause crashing/correctness issues for the release?
Keywords: qawanted
I know that Reed is wanting this verified for branches but I see not STR or testcase available. How exactly is QA supposed to verify that this one line change is fixed?
This is difficult to verify without valgrind. The patch is trivially correct, though.

(In reply to comment #17)
> Considering comment 2, and the fact that if I'm not mistaken, conservative
> stack scanner is only available on m-c, is it really the right fix for branches
> ?

We actually root ns in vp[0] (which is guaranteed to exist) right after its creation. Its assignment into vp[2] was extra rooting that had the additional property of being wrong some of the time. So this doesn't introduce any GC hazards and it fixes a potential out-of-bounds write.
Group: core-security
Issue is resolved - clearing old keywords - qa-wanted clean-up
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.