Wrong numOperands in RRegExpMatcher, RRegExpSearcher, and RRegExpTester.

RESOLVED FIXED in Firefox 47

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
a year ago
8 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)

(Assignee)

Description

a year ago
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)
(Assignee)

Comment 1

a year ago
Created attachment 8749969 [details] [diff] [review]
Part 1: Fix numOperands of RRegExpMatcher, RRegExpSearcher, and RRegExpTester.
Attachment #8749969 - Flags: review?(hv1989)
(Assignee)

Comment 2

a year ago
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)
(Assignee)

Comment 3

a year ago
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
(Assignee)

Comment 4

a year ago
(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+
(Assignee)

Comment 7

a year ago
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)
(Assignee)

Comment 10

a year ago
thanks, so, it will result in using wrong operand in next instruction, right?
(Assignee)

Comment 11

a year ago
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?
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.
Attachment #8749969 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 13

a year ago
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+
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: --- → +
(Assignee)

Comment 15

a year ago
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
Last Resolved: a year ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Comment 17

a year ago
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/901b6248af7d
status-firefox48: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/8b9d08e61f23
status-firefox47: affected → fixed
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.