compiling long XML filtering predicate hangs

VERIFIED FIXED

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: shutdown, Assigned: Igor Bukanov)

Tracking

({hang, verified1.8.0.10, verified1.8.1.2})

1.8 Branch
hang, verified1.8.0.10, verified1.8.1.2
Points:
---
Bug Flags:
blocking1.9 ?
blocking1.8.1.2 +
blocking1.8.0.10 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
$ cat 020-long-filtering-predicate.txt
function exploit() {
  var code = "foo = <x/>.(", obj = {};
  for(var i = 0; i < 0x10000; i++) {
    code += "0, ";
  }
  code += "0);";
  Function(code);
}
exploit();

$ dbg.obj/js 020-long-filtering-predicate.txt
Assertion failure: 0, at jsemit.c:808

$ opt.obj/js 020-long-filtering-predicate.txt
...(never returns)
(Assignee)

Comment 1

11 years ago
The bug is caused by the lack of JSOP_FILTERX. Making as security-sensitive as it is not clear if an exploit is not possible.
Assignee: general → igor.bukanov
Group: security
(Assignee)

Comment 2

11 years ago
Created attachment 250693 [details] [diff] [review]
Fix v1

The fix adds JSOP_FILTERX and adjusts code accordingly. There is no need to change XDR version as the patch does not change the meaning of the current bytecode.
Attachment #250693 - Flags: review?(brendan)
(Assignee)

Comment 3

11 years ago
This fix conflicts in jsopcode.tbl with the patch for bug 365608. Since the fix is much smaller and easier to commit to branches, I suggest to commit it to the trunk first.
Created attachment 250718 [details] [diff] [review]
simpler alterna-patch

My fault again, I'll take this to let Igor get on with more important work.  This is a simpler patch that suffices for 1.8 and 1.8.0 branches, no new opcode needed. An absurdly long XML filtering predicate expression is not something we need to support, and an error beats an exploitable crash any day.

/be
Assignee: igor.bukanov → brendan
Attachment #250693 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #250718 - Flags: review?(igor.bukanov)
Attachment #250718 - Flags: approval1.8.1.2?
Attachment #250718 - Flags: approval1.8.0.10?
Attachment #250693 - Flags: review?(brendan)

Updated

11 years ago
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.10?

Updated

11 years ago
Priority: -- → P1
Version: Trunk → 1.8 Branch
(Assignee)

Updated

11 years ago
Attachment #250718 - Flags: review?(igor.bukanov) → review+
(Assignee)

Comment 5

11 years ago
Comment on attachment 250718 [details] [diff] [review]
simpler alterna-patch

Minusing for now: I saw in the debugger an arbitrary op value.
Attachment #250718 - Flags: review+ → review-
(Assignee)

Comment 6

11 years ago
Comment on attachment 250718 [details] [diff] [review]
simpler alterna-patch

+ again after false alarm, that op=garbage was with partially modified my patch.
Attachment #250718 - Flags: review- → review+

Updated

10 years ago
Flags: blocking1.8.1.2?
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10?
Flags: blocking1.8.0.10+

Comment 7

10 years ago
Comment on attachment 250718 [details] [diff] [review]
simpler alterna-patch

Approved for both branches, a=jay for drivers.
Attachment #250718 - Flags: approval1.8.1.2?
Attachment #250718 - Flags: approval1.8.1.2+
Attachment #250718 - Flags: approval1.8.0.10?
Attachment #250718 - Flags: approval1.8.0.10+
Fixed on the branches:

1.8.0: js/src/jsemit.c rev 3.128.2.3.2.13
1.8:   js/src/jsemit.c rev 3.128.2.62

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: fixed1.8.0.10, fixed1.8.1.2
Resolution: --- → FIXED

Comment 9

10 years ago
Created attachment 251762 [details]
e4x/Expressions/regress-366123.js

Updated

10 years ago
Flags: in-testsuite+
Whiteboard: [sg:critical?]

Comment 10

10 years ago
verified fixed 1.8.0, 1.8.1 2007-01-23 win/mac*/linux but I show this failing on trunk and I can't tell from the comments or lxr that it was checked into the trunk.
Status: RESOLVED → REOPENED
Keywords: fixed1.8.0.10, fixed1.8.1.2 → verified1.8.0.10, verified1.8.1.2
Resolution: FIXED → ---
(Assignee)

Comment 11

10 years ago
(In reply to comment #10)
> verified fixed 1.8.0, 1.8.1 2007-01-23 win/mac*/linux but I show this failing
> on trunk and I can't tell from the comments or lxr that it was checked into the
> trunk.

Of cause the patch for bug 365608 does not fix this. This bug is related to the lack of long-jump support for xml filtering code while bug fixed once and for all the problem of big atom indexes. 

(Assignee)

Comment 12

10 years ago
For the real fix should we just add JSOP_FILTERX? Or perhaps try some generic approach like one for atom indexes? 
The span-dependent extended jump selection is generic, modulo different formats. We don't want a prefix and suffix op just for JSOP_FILTER, and we do not want to re-do and add overhead to the extended jump scheme.

See bug 309894 and please feel free to take it and dup this bug against it.

/be
(Assignee)

Comment 14

10 years ago
(In reply to comment #13)
> See bug 309894 and please feel free to take it and dup this bug against it.

For now I just record that bug as a blocker for this one: with endfilter we will need endfilterx as well.  
Depends on: 309894

Updated

10 years ago
Assignee: brendan → igor
Status: REOPENED → NEW
Flags: blocking1.9?
Group: security
Didn't notice that this was not fixed on trunk.
Group: security

Comment 16

10 years ago
what's needed to get this on the trunk and close out as fixed?
(Assignee)

Comment 17

10 years ago
(In reply to comment #16)
> what's needed to get this on the trunk and close out as fixed?

I think the best is to land the patch already applied to the trunk. The real fix should be done as a part of bug 309894.
(Assignee)

Comment 18

10 years ago
I am going to commit the patch from comment 4 to close the bug.
(Assignee)

Comment 19

10 years ago
I committed Brendan's patch from comment 4 to the trunk:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.241; previous revision: 3.240
done
Status: NEW → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED

Comment 20

10 years ago
verified fixed 1.9.0 20070320 win/mac*/linux
Status: RESOLVED → VERIFIED
Group: security

Comment 21

10 years ago
/cvsroot/mozilla/js/tests/e4x/Expressions/regress-366123.js,v  <--  regress-366123.js
initial revision: 1.1
(Assignee)

Comment 22

9 years ago
The bug should not depend on 309894 as 309894 is not a regression.
No longer depends on: 309894
You need to log in before you can comment on or make changes to this bug.