1271037 - Wrong numOperands in RRegExpMatcher, RRegExpSearcher, and RRegExpTester.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Tooru Fujisawa [:arai]]

https://dxr.mozilla.org/mozilla-central/rev/e5a10bc7dac4ee2453d8319165c1f6578203eac7/js/src/jit/Recover.h#568
> class RRegExpMatcher final : public RInstruction
> {
>   public:
>     RINSTRUCTION_HEADER_(RegExpMatcher)
> 
>     virtual uint32_t numOperands() const {
>         return 5;
>     }
> 
>     bool recover(JSContext* cx, SnapshotIterator& iter) const;
> };

It should be 3 (or 4 before bug 1263340).

setting s-s just in case.
(I don't see any case this is used tho)

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Fix numOperands of RRegExpMatcher, RRegExpSearcher, and RRegExpTester.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Statically check that numOperands of the recover instruction and the MIR are consistent.

Added static_assert to catch that case.

Any recover instruction that `numOperands` is a constant is changed to use RINSTRUCTION_HEADER_NUM_OP_ macro instead of RINSTRUCTION_HEADER_.
RINSTRUCTION_HEADER_NUM_OP_ macro also receives `numOperands` value, and assert that it's same as MIR's arity.

Comment 3

[Tooru Fujisawa [:arai]]

this is a regression from bug 1207922 (RegExpMatcher, RegExpTester) from firefox46, and bug 887016 (RegExpSearcher) from firefox49.

Comment 4

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #2)
> Any recover instruction that `numOperands` is a constant is changed to use
> RINSTRUCTION_HEADER_NUM_OP_ macro instead of RINSTRUCTION_HEADER_.
> RINSTRUCTION_HEADER_NUM_OP_ macro also receives `numOperands` value, and
> assert that it's same as MIR's arity.

RINSTRUCTION_HEADER_NUM_OP_ also declares `numOperands` method.

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 8749969 [details] [diff] [review]
Part 1: Fix numOperands of RRegExpMatcher, RRegExpSearcher, and RRegExpTester.

Review of attachment 8749969 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch!

Comment 6

[Hannes Verschore [:h4writer]]

Comment on attachment 8749970 [details] [diff] [review]
Part 2: Statically check that numOperands of the recover instruction and the MIR are consistent.

Review of attachment 8749970 [details] [diff] [review]:
-----------------------------------------------------------------

Good idea!

Comment 7

[Tooru Fujisawa [:arai]]

I'm now wondering about asking sec-approval.
what could happen if the value of numOperands is wrong?

Comment 8

[Hannes Verschore [:h4writer]]

Looking in the source we don't use numOperands of recover instructions? Am I missing something nbp?

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #8)
> Looking in the source we don't use numOperands of recover instructions? Am I
> missing something nbp?

These RInstruction::numOperands are used by the SnapshotIterator to know how many operands are needed to skip a recover instruction, and also to seek in the snapshot while walking inline frames.

https://dxr.mozilla.org/mozilla-central/search?q=%2Bfunction-ref%3A%22js%3A%3Ajit%3A%3ARInstruction%3A%3AnumOperands%28%29+const%22

Comment 10

[Tooru Fujisawa [:arai]]

thanks, so, it will result in using wrong operand in next instruction, right?

Comment 11

[Tooru Fujisawa [:arai]]

Comment on attachment 8749969 [details] [diff] [review]
Part 1: Fix numOperands of RRegExpMatcher, RRegExpSearcher, and RRegExpTester.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
I'm not sure how to exploit this, as we don't yet hit the case.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Commit message says the number is wrong, but there is no testcase describes how to hit it.

> Which older supported branches are affected by this flaw?
firefox46

> If not all supported branches, which bug introduced the flaw?
bug 1207922 (RegExpMatcher, RegExpTester) from firefox46
bug 887016 (RegExpSearcher) from firefox49

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Not yet, but it can be created easily.
(I think only Part 1 should be uplifted)

> How likely is this patch to cause regressions; how much testing does it need?
Less likely.
Part 1 just changes 3 numbers that were wrong.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8749969 [details] [diff] [review]
Part 1: Fix numOperands of RRegExpMatcher, RRegExpSearcher, and RRegExpTester.

sec-approval+ for trunk checkin.

Please nominate Aurora and Beta patches as well to be checked in following trunk. We'll want those very soon after it lands in trunk.

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: (mozilla-aurora and mozilla-beta) Fix numOperands of RRegExpMatcher, and RRegExpTester. r=h4writer

Here's the patch for mozilla-aurora and mozilla-beta.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for beta and up so we don't miss this sec-high issue.

Comment 15

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87b38a0c8543d369ad1dc92d1f55965cb0ad7157
Bug 1271037 - Part 1: Fix numOperands of RRegExpMatcher, RRegExpSearcher, and RRegExpTester. r=h4writer, a=abillings

https://hg.mozilla.org/integration/mozilla-inbound/rev/49f41e6097076c35c92082cb268ea63b9ea5ab03
Bug 1271037 - Part 2: Statically check that numOperands of the recover instruction and the MIR are consistent. r=h4writer, a=abillings

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87b38a0c8543
https://hg.mozilla.org/mozilla-central/rev/49f41e609707

Comment 17

[Tooru Fujisawa [:arai]]

Comment on attachment 8750885 [details] [diff] [review]
(mozilla-aurora and mozilla-beta) Fix numOperands of RRegExpMatcher, and RRegExpTester. r=h4writer

Approval Request Comment
> [Feature/regressing bug #]
bug 1207922

> [User impact if declined]
Possibly exploit, but not yet confirmed this can be exploited.

> [Describe test coverage new/current, TreeHerder]
Statically checked by Part 2, but Part 2 will be overkill to uplift.

> [Risks and why]
Low, this patch is just changing wrong numbers.

> [String/UUID change made/needed]
None

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8750885 [details] [diff] [review]
(mozilla-aurora and mozilla-beta) Fix numOperands of RRegExpMatcher, and RRegExpTester. r=h4writer

Sec-high issue, Aurora48+, Beta47+

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/901b6248af7d

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8b9d08e61f23