Closed Bug 646968 Opened 10 years ago Closed 10 years ago

let-block variable initializers are statically outside the let-scope but dynamically inside it


(Core :: JavaScript Engine, defect)

Other Branch
Not set



Tracking Status
firefox5 - wontfix
firefox6 - wontfix
firefox7 - wontfix
firefox8 + fixed
firefox9 + fixed
firefox10 + fixed
blocking1.9.2 --- -
status1.9.2 --- wontfix


(Reporter: jorendorff, Assigned: jorendorff)



(Whiteboard: [sg:critical?] [landed m-c 7/15][qa-] not exploitable on 1.9.2?)


(1 file, 2 obsolete files)

The EXPR in `let (x = EXPR) foo();` is supposed to be outside the scope of the lexical variable x being declared.

The compiler gets this right for names it can bind statically, but it still emits the bytecode for the expression *after* the JSOP_ENTERBLOCK instruction. That means the scope is wrong for plain old JSOP_NAME opcodes:

    var x = 5;
    let (x = x)
        assertEq(x, 5);                    // PASS
    let (x = eval("x"))
        assertEq(x, 5);                    // FAIL
    let (x = function () { with ({}) return x; })
        assertEq(x(), 5);                  // FAIL

    (let (x = x) assertEq(x, 5));          // PASS
    (let (x = eval("x")) assertEq(x, 5));  // FAIL
    (let (x = function () { with ({}) return x; }) assertEq(x(), 5)); // FAIL

I can't think of a clever way to fix this offhand.
Group: core-security
dvander points out that

  - you don't need with/eval to emit NAME opcodes; an escaping function
    with upvars is enough

  - this could cause a JSOP_NAME/JSOP_SETNAME to read or write an inner
    variable that the static analysis thinks isn't being read/written.

He thinks it can probably be exploited in the JIT.
We could emit the right-hand sides first, then ENTERBLOCK. We would modify ENTERBLOCK to use the already-populated operand stack slots as storage for the varibles in the new scope, instead of populating new slots with undefined.

    Disassembly for `let (x = EXPR1, y = EXPR2) EXPR3` would look like this:
      EXPR1                           # v1
      EXPR2                           # v1 v2
      enterblock (x, y)               # x=v1 y=v2
      EXPR3                           # x y v3
      leaveblockexpr 2                # v3

This might be a little tricky for destructuring let. More on that in a minute.
It's not so bad.

`let ([x, y] = obj) ...`

    push        # set aside a slot for x
    push        # and one for y
    getgname obj
    setlocalpop 0  # set x = obj[0]
    setlocalpop 1  # set y = obj[1]
    enterblock (x, y)

  The only difficulty I anticipate here is getting the decompiler to
  understand which names those setlocalpop instructions are assigning to.
  Worst-case, we add a JSOP_NOP with a magic source note.

`let ([x, y] = [a, b]) ...`

  Using the existing group destructuring code with this new approach, we
  would get:
    push        # set aside a slot for x
    push        # and one for y
    getgname a
    getgname b
    getlocal 2
    setlocal 0  # store x = a
    getlocal 3
    setlocal 1  # store y = b
    popn 2
    enterblock (x, y)

  (Of course all that amounts to:
    getgname a
    getgname b
    enterblock (x, y)
  which the decompiler would happily decompile to `let (x=a, y=b)`. Cute, but
  not worth implementing, at least not in this bug.)
(In reply to comment #1)
>   - this could cause a JSOP_NAME/JSOP_SETNAME to read or write an inner
>     variable that the static analysis thinks isn't being read/written.

Is the variable potentially written to still a "variable"? or could that bit of memory now be used for something else entirely where now an attacker can cause us to execute the wrong stuff. I guess even as a variable it could be a reference to some object on which you call methods, replaced with a value that points at a fake object the attacker lays out in a typedarray or something like that?
David, per comment 1 you're suggesting that this is a potentially exploitable bug. Can you please suggest a sg: rating per
Assignee: general → dvander
David says that while it would be tricky to get the jit to generate exploitable code here he thinks it's possible, and suggests sg:critical? for this bug. He also thought Jason would be a good owner here, so reassigning to Jason for now.
Assignee: dvander → jorendorff
Whiteboard: [sg:critical?]
can we get a testcase or js shell script that exhibits the problem? We'll need this to verify the bug and create a regression test.

Does this affect the 1.9.2 branch as well, or is this a problem that crept in with the method JIT?
Keywords: testcase-wanted
Jason, do you think you're the right owner here? And if so, when do you think you can get to this?
Jason: Only a couple weeks left before mozilla6 goes to beta channel, will this be done by then?
Yeah, this is on my list. I'll patch it this week.
FWIW, I believe Jason is right that the intended scoping semantics of let expressions and let statements was that in the form:

    let (x1[= e1], ..., xn[= en]) body

all the expressions e1 ... en are in the outer scope, and the x1 ... xn variables are only in scope in the body. (This corresponds to Scheme's LET, as opposed to LET* or LETREC.)

But this is from ES4 and is not in Harmony, at least not currently. So there isn't really a definitive spec.

Scoping should be like Scheme let, not let* or letrec.

I was always partial to let* allowing easy abbreviation of intermediate steps, myself.  But I imagine let is easiest and least complicated to implement, because you don't have a gazillion different possible scope-object-munges.  Is simplicity the reason we would want let rather than let*?
Jeff: I pretty much agree with you. I like let* for the same reason. I think I originally proposed let instead of let* for ES4 without really thinking it through. And now it looks like we're schedule-bound to get this patched quickly.

But we might actually be able to get away with changing this down the road, if we decided we preferred the let* semantics; I think people use let-declarations far more often than let-expressions and let-statements, so it might not break much. And we could easily do some simple scope analysis of Firefox and addon code to see how much bustage it would cause.

FWIW, we could probably do scope analysis to merge frames in many cases, so let* would often devolve to being no more expensive than let.

The code for emitting switch statements contains this:

	/* Emit JSOP_ENTERBLOCK before code to evaluate the discriminant. */
        if (!EmitEnterBlock(cx, pn2, cg))
            return JS_FALSE;

That seems like the same kind of bug.
Heh, after all that noise in comments 2 and 3, I think all I had to do was emit the block chain in a couple extra places.
Whoops, posted that prematurely. I guess I might as well explain.

What I mean is, there's already code in EmitVariables to handle let-heads and other similar places specially. My plan: in the places where that code hacks the CodeGenerator, also emit JSOP_BLOCKCHAIN/NULLBLOCKCHAIN annotations for GetBlockChain's benefit.
Attached patch WIP 1 (obsolete) — Splinter Review
This passes the tests with let-expressions and let-blocks, but it still fails
for(let x = eval('x');;) and switch (eval('x')) { let x; }.
Attached patch v2 (obsolete) — Splinter Review
Phew. This patch was harder to write than it looks. :-|

Unfortunately there is still this:

  var s = '', x = {a: 1, b: 2, c: 3};
  for (let x in let (y = 0) x)
      s += x;
  assertEq(s, 'abc');

  typein:4: Error: Assertion failed: got "", expected "abc"

I think this is a bug too, but it seems to be a bug in the jsparse code that builds the def-use chains, not jsemit. Assuming this patch gets review, I'll file a follow-up bug for that.

Maybe I should remove the for-in part of this patch, though, so that I at least leave things consistent. Your thoughts?
Attachment #543048 - Attachment is obsolete: true
Attachment #543428 - Flags: review?(brendan)
Attached patch v3Splinter Review
Identical to v2 except for bumping the bytecode version.
Attachment #543428 - Attachment is obsolete: true
Attachment #543428 - Flags: review?(brendan)
Attachment #543429 - Flags: review?(brendan)
Went ahead and filed followup bug 668848.
Duplicate of this bug: 647695
Jason and Brendan, we're considering this for Firefox 6, which is already in beta. Any chance of getting this reviewed and landed reasonably soon to get plenty of testing during beta for this?
Comment on attachment 543429 [details] [diff] [review]

Why the JSOP_NOP emits before the EmitBlockChain calls?

Sorry I didn't write a nice comment about that.

The current block chain is *not* always computed by looking at the most recent JSOP_BLOCKCHAIN/NULLBLOCKCHAIN/ENTERBLOCK instruction. Sometimes we call GetBlockChainFast. It looks at the op immediately *following* the op at pc. If it is JSOP_BLOCKCHAIN or JSOP_NULLBLOCKCHAIN, then we don't bother looking at instructions before the pc.

So in this patch, I emit JSOP_NOPs in order to avoid accidentally confusing GetBlockChainFast when pc points to the preceding instruction. (!)

I think it would be better to separate these two competely different meanings of JSOP_BLOCKCHAIN/NULLBLOCKCHAIN (modifying the immediately preceding opcode vs. modifying only subsequent opcodes), but I thought it best not to refactor all that in this bug.
It might also be better if GetBlockChainFast could assert that the looked-for annotation is there. We should know that statically at each call site, right?
Comment on attachment 543429 [details] [diff] [review]

Please FIXME-comment the NOPs citing the followup bug. Thanks,

Attachment #543429 - Flags: review?(brendan) → review+
Can we get this landed on the trunk so that we can tell whether this is something we can take late in a beta cycle for 6?
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Comment on attachment 543429 [details] [diff] [review]

I assume I still need to get these bits even though this is tracking-firefox6+ and tracking-firefox7+.
Attachment #543429 - Flags: approval-mozilla-beta?
Attachment #543429 - Flags: approval-mozilla-aurora?
(btw, followup bug 671360.)
jorendorff, yes, and we'll get to these approval requests on Monday's 2pm PT triage.
Whiteboard: [sg:critical?] → [sg:critical?] [landed m-c 7/15]
Is this the kind of thing we'd expect to see independently discovered in the wild? If not, can we just let it climb up the release chain normally?
Hasn't this been in since the start of let, i.e. 2006 or so?  Waiting's always a gamble, but five years of gambling has, er, not bitten us yet.  I'd at least lean toward waiting.
Comment on attachment 543429 [details] [diff] [review]

We're not willing to take the risk into Aurora and Beta -- worried especially about add-on compatibility. Given that this is internally found, we'd prefer to take the extra 6 weeks to let this climb the normal rout through the channels.
Attachment #543429 - Flags: approval-mozilla-beta?
Attachment #543429 - Flags: approval-mozilla-beta-
Attachment #543429 - Flags: approval-mozilla-aurora?
Attachment #543429 - Flags: approval-mozilla-aurora-
Tracking for 8 since there were regressions from this.
qa- as nothing to do for QA verification -- please reset to qa+ if a testcase can be provided.
Whiteboard: [sg:critical?] [landed m-c 7/15] → [sg:critical?] [landed m-c 7/15][qa-]
Does this bug affect 1.9.2? at least part of the patch would apply to jsemit
blocking1.9.2: --- → ?
status1.9.2: --- → ?
I think 1.9.2 is affected, but we never found a way to exploit this. The way we imagined it might be exploitable involved the methodjit, which does not exist in 1.9.2.

Combine that with the size of the patch and the number of regressions it introduced, and I can confidently say that 1.9.2 is better off without it.
Whiteboard: [sg:critical?] [landed m-c 7/15][qa-] → [sg:critical?] [landed m-c 7/15][qa-] not exploitable on 1.9.2?
blocking1.9.2: ? → -
Group: core-security
You need to log in before you can comment on or make changes to this bug.