The default bug view has changed. See this FAQ.

Wrong numOperands in RRegExpMatcher, RRegExpSearcher, and RRegExpTester.

RESOLVED FIXED in Firefox 47

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
11 months ago
3 months ago

People

(Reporter: arai, Assigned: arai)

Tracking

({regression, sec-high})

Trunk
mozilla49
regression, sec-high
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox46 wontfix, firefox47+ fixed, firefox48+ fixed, firefox49+ fixed, firefox-esr45 unaffected)

Details

(Whiteboard: [post-critsmash-triage][adv-main47+])

Attachments

(3 attachments)

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

Updated

11 months ago
Blocks: 1207922, 887016
Keywords: regression, sec-high
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.

Updated

11 months ago
Attachment #8749969 - Flags: sec-approval? → sec-approval+
Created attachment 8750885 [details] [diff] [review]
(mozilla-aurora and mozilla-beta) Fix numOperands of RRegExpMatcher, and RRegExpTester. r=h4writer

Here's the patch for mozilla-aurora and mozilla-beta.
Attachment #8750885 - Flags: review+

Updated

11 months ago
Group: core-security → javascript-core-security
Tracking for beta and up so we don't miss this sec-high issue.
status-firefox46: affected → wontfix
tracking-firefox47: --- → +
tracking-firefox48: --- → +
tracking-firefox49: --- → +
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

11 months ago
https://hg.mozilla.org/mozilla-central/rev/87b38a0c8543
https://hg.mozilla.org/mozilla-central/rev/49f41e609707
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox49: affected → fixed
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 18

11 months ago
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+

Comment 19

11 months ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/901b6248af7d
status-firefox48: affected → fixed

Comment 20

11 months ago
https://hg.mozilla.org/releases/mozilla-beta/rev/8b9d08e61f23
status-firefox47: affected → fixed

Updated

10 months ago
Group: javascript-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

Updated

10 months ago
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.