Invalid read in js_PopStatement

RESOLVED DUPLICATE of bug 410852

Status

()

RESOLVED DUPLICATE of bug 410852
11 years ago
11 years ago

People

(Reporter: romaxa, Assigned: igor)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

11 years ago
Created attachment 270474 [details]
Full Valgrind log

Open Trunk ff on URL with valgrind

==30698== Invalid read of size 4
==30698==    at 0x4F007FA: js_PopStatement (jsemit.c:1495)
==30698==    by 0x4F43BFA: FunctionBody (jsparse.c:770)
==30698==    by 0x4F43ED3: js_CompileFunctionBody (jsparse.c:850)
==30698==    by 0x4EE6BF6: JS_CompileUCFunctionForPrincipals (jsapi.c:4598)

See full memcheck log in attachment
(Assignee)

Comment 1

11 years ago
I will look at it later today.
Assignee: general → igor
(Assignee)

Comment 2

11 years ago
The problem is that FunctionBody calls js_PopStatement even in case of errors when the statement stack can be invalid and should be touched.

The consequence of the bug is assignments in js_PopStatement to JSTokenStream fields of values read through a pointer chain originated in the already released stack frame. AFAICS due to the error propogation the results of these asignments are never accessed since JSTokenStream is released itself. Thus on the first impression this is not exploitable.

Still I mark the bug as sensitive pending extended review of PopStatement usages.
Group: security
Status: NEW → ASSIGNED
(Assignee)

Comment 3

11 years ago
Created attachment 270545 [details] [diff] [review]
fix v1

For the fix I changed FunctionBody to use "goto bad" on errors to avoid js_PopStatement.
Attachment #270545 - Flags: review?(brendan)
(Assignee)

Comment 4

11 years ago
Created attachment 270546 [details] [diff] [review]
diff-b version of fix v1
(Assignee)

Comment 5

11 years ago
The bug is safe as in the worst case it can lead to a crash. But in reality it is rather harmless. When the code accesses the value allocated on already returned C stack frame that value remains valid as at that moment the stack slots for the returned frame has not yet been overwritten. This is a consequence of immediate-return-on-failure code style used in jsparse.c.  
Group: security
Comment on attachment 270545 [details] [diff] [review]
fix v1

It's good to restore cx->fp correctly, but this patch no longer always arranges to call js_PopStatement after it calls js_PushStatement. Or so it appears from the diffs.

/be
Attachment #270545 - Flags: review?(brendan) → review-
(Assignee)

Comment 7

11 years ago
(In reply to comment #6)
> ... this patch no longer always arranges
> to call js_PopStatement after it calls js_PushStatement. 

But this is precisely the point of the patch! It makes sure that js_PopStatement is never called on errors. The rest of the code never does that so on errors JSTreeContext.topStmt may point to JSStmtInfo allocated on already returned C stack frame.

Here for references patched FunctionBody:

static JSParseNode *
FunctionBody(JSContext *cx, JSTokenStream *ts, JSFunction *fun,
             JSTreeContext *tc)
{
    JSStackFrame *fp, frame;
    JSObject *funobj;
    JSStmtInfo stmtInfo;
    uintN oldflags, firstLine;
    JSParseNode *pn;

    fp = cx->fp;
    funobj = fun->object;
    if (!fp || fp->fun != fun || fp->varobj != funobj ||
        fp->scopeChain != funobj) {
        memset(&frame, 0, sizeof frame);
        frame.fun = fun;
        frame.varobj = frame.scopeChain = funobj;
        frame.down = fp;
        if (fp)
            frame.flags = fp->flags & JSFRAME_COMPILE_N_GO;
        cx->fp = &frame;
    }

    /*
     * Set interpreted early so js_EmitTree can test it to decide whether to
     * eliminate useless expressions.
     */
    fun->flags |= JSFUN_INTERPRETED;

    oldflags = tc->flags;
    tc->flags &= ~(TCF_RETURN_EXPR | TCF_RETURN_VOID);
    tc->flags |= TCF_IN_FUNCTION;

    /* From this point the control must flow through out: or bad:. */
    js_PushStatement(tc, &stmtInfo, STMT_BLOCK, -1);
    stmtInfo.flags = SIF_BODY_BLOCK;

    /*
     * Save the body's first line, and store it in pn->pn_pos.begin.lineno
     * later, because we may have not peeked in ts yet, so Statements won't
     * acquire a valid pn->pn_pos.begin from the current token.
     */
    firstLine = ts->lineno;
#if JS_HAS_EXPR_CLOSURES
    if (CURRENT_TOKEN(ts).type == TOK_LC) {
        pn = Statements(cx, ts, tc);
        if (!pn)
            goto bad;
    } else {
        pn = NewParseNode(cx, ts, PN_UNARY, tc);
        if (!pn)
            goto bad;
        pn->pn_kid = AssignExpr(cx, ts, tc);
        if (!pn->pn_kid)
            goto bad;
        if (tc->flags & TCF_FUN_IS_GENERATOR) {
            ReportBadReturn(cx, ts, JSREPORT_ERROR, JSMSG_BAD_GENERATOR_RETURN,
                            JSMSG_BAD_ANON_GENERATOR_RETURN);
            goto bad;
        }
        pn->pn_type = TOK_RETURN;
        pn->pn_op = JSOP_RETURN;
        pn->pn_pos.end = pn->pn_kid->pn_pos.end;
    }
#else
    pn = Statements(cx, ts, tc);
    if (!pn)
        goto bad;
#endif

    JS_ASSERT(pn);
    js_PopStatement(tc);

    /* Check for falling off the end of a function that returns a value. */
    if (JS_HAS_STRICT_OPTION(cx) && (tc->flags & TCF_RETURN_EXPR)) {
        if (!CheckFinalReturn(cx, ts, pn))
            goto bad;
    }

    /*
     * We have a parse tree in pn and a code generator in tc, emit this
     * function's code.  We must do this here, not in js_CompileFunctionBody,
     * in order to detect TCF_IN_FUNCTION among tc->flags.
     */
    pn->pn_pos.begin.lineno = firstLine;
    if ((tc->flags & TCF_COMPILING)) {
        JSCodeGenerator *cg = (JSCodeGenerator *) tc;

        if (!js_FoldConstants(cx, pn, tc) ||
            !js_EmitFunctionBytecode(cx, cg, pn)) {
            goto bad;
        }
     }

  out:
    cx->fp = fp;
    tc->flags = oldflags | (tc->flags & (TCF_FUN_FLAGS | TCF_HAS_DEFXMLNS));
    return pn;

  bad:
    pn = NULL;
    goto out;
}
Is this still an issue?
Blake, I thought you patched this recently -- in another bug?

/be
Argh, yes. See bug 410852; the two approaches are fundamentally the same if executed slightly differently. I prefer the version without gotos, but I'll leave it up to you which patch you want to take.
(Assignee)

Comment 11

11 years ago
The patch here would require to sync with CVS so lets go with bug 410852.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 410852
You need to log in before you can comment on or make changes to this bug.