Closed Bug 1271037 Opened 3 years ago Closed 3 years ago

Wrong numOperands in RRegExpMatcher, RRegExpSearcher, and RRegExpTester.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox46 --- wontfix
firefox47 + fixed
firefox48 + fixed
firefox49 + fixed
firefox-esr45 --- unaffected

People

(Reporter: arai, Assigned: arai)

References

Details

(Keywords: regression, sec-high, Whiteboard: [post-critsmash-triage][adv-main47+])

Attachments

(3 files)

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)
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.
Attachment #8749970 - Flags: review?(hv1989)
this is a regression from bug 1207922 (RegExpMatcher, RegExpTester) from firefox46, and bug 887016 (RegExpSearcher) from firefox49.
(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 on attachment 8749969 [details] [diff] [review]
Part 1: Fix numOperands of RRegExpMatcher, RRegExpSearcher, and RRegExpTester.

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

Good catch!
Attachment #8749969 - Flags: review?(hv1989) → review+
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!
Attachment #8749970 - Flags: review?(hv1989) → review+
I'm now wondering about asking sec-approval.
what could happen if the value of numOperands is wrong?
Flags: needinfo?(hv1989)
Looking in the source we don't use numOperands of recover instructions? Am I missing something nbp?
Flags: needinfo?(hv1989) → needinfo?(nicolas.b.pierron)
(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
Flags: needinfo?(nicolas.b.pierron)
thanks, so, it will result in using wrong operand in next instruction, right?
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.
Attachment #8749969 - Flags: sec-approval?
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.
Attachment #8749969 - Flags: sec-approval? → sec-approval+
Here's the patch for mozilla-aurora and mozilla-beta.
Attachment #8750885 - Flags: review+
Group: core-security → javascript-core-security
Tracking for beta and up so we don't miss this sec-high issue.
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
https://hg.mozilla.org/mozilla-central/rev/87b38a0c8543
https://hg.mozilla.org/mozilla-central/rev/49f41e609707
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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
Attachment #8750885 - Flags: approval-mozilla-beta?
Attachment #8750885 - Flags: approval-mozilla-aurora?
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+
Attachment #8750885 - Flags: approval-mozilla-beta?
Attachment #8750885 - Flags: approval-mozilla-beta+
Attachment #8750885 - Flags: approval-mozilla-aurora?
Attachment #8750885 - Flags: approval-mozilla-aurora+
Group: javascript-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main47+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.