Closed Bug 1352073 Opened 3 years ago Closed 3 years ago

Off-by-one in Vector::insert

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 + wontfix
firefox54 + fixed
firefox55 + fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main54+])

Attachments

(2 files)

In Vector::insert, the algorithm is the following:

Vector::insert(aPos*, T&& aVal)
  // aPos* gets transformed into an size_t index named pos
  // oldLength == length() before insertion
  } else { // if pos != oldLength
    T oldBack = Move(back());
    if (!append(Move(oldBack))) { /* Dup the last element. */
      return nullptr;
    }
    for (size_t i = oldLength; i > pos; --i) {
      (*this)[i] = Move((*this)[i - 1]);
    }
    (*this)[pos] = Forward<U>(aVal);
  }
  return begin() + pos;

The last element which *was* at oldLength-1 gets move-appended; after the append, it has index oldLength, while oldLength-1 points to moved memory. So the for-loop below should start at i := oldLength - 1, to not replace the last element by just-moved-memory.

I've checked uses in gecko, and none referenced by searchfox involves a movable T, but locking in, just for safety. Found with bug 1340219, which stores UniquePtr into a vector and uses insert() afterwards.
Attached patch offbyone.patchSplinter Review
Patch and test.
Attachment #8852931 - Flags: review?(luke)
Comment on attachment 8852931 [details] [diff] [review]
offbyone.patch

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

Oh wow; not every day you find such a flagrant (6 years old, it would appear: https://hg.mozilla.org/mozilla-central/rev/597254d97174#l46.60) bug in Vector.
Attachment #8852931 - Flags: review?(luke) → review+
Under the premises of copy, this was correct (one copy is redundant, so not a correctness issue but a perf issue there). The real issue appeared when we've started using Move semantics there, which happened in bug 896100.
Comment on attachment 8852931 [details] [diff] [review]
offbyone.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Hard to say: this is a generic Vector<T> class which has many users all over Gecko. The issue happens when:
- T has move semantics that invalidates Move'd objects.
- the insert() function is used with such a T.

In this case, when doing an insertion at any index in the vector, the last element points to invalidated moved memory.

Searchfox doesn't show any use of Vector<T>::insert where T fulfills the first condition: http://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla6Vector6insertEPT_OT_&redirect=false . In all of these cases, Move is equivalent to a plain copy and there's no issue.
That doesn't mean that there aren't other uses on platforms not analyzed by searchfox, or that previous usage on branches was correct.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes: the test case added causes a segfault without the fix. It could be of course added later in a different commit.

Which older supported branches are affected by this flaw? If not all supported branches, which bug introduced the flaw?
It started with bug 896100, so all branches might be affected.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Pretty sure the non-comment one-liner change in mfbt/Vector.h applies everywhere.

How likely is this patch to cause regressions; how much testing does it need?
A try build could confirm it doesn't break anything; a test is included.
Attachment #8852931 - Flags: sec-approval?
It makes sense that most T in a Vector<T> would be POD that isn't affected by this bug.  If we can't see any, it might not be worth it to uplift everywhere.
Critsmash triage: making this a sec-moderate.

As such, it doesn't need sec-approval (which is only for critical or high rated issues) so clearing that request as well.
Attachment #8852931 - Flags: sec-approval?
(In reply to Al Billings [:abillings] from comment #6)
> Critsmash triage: making this a sec-moderate.
> 
> As such, it doesn't need sec-approval (which is only for critical or high
> rated issues) so clearing that request as well.

Can it just land as is on trunk?
(In reply to Benjamin Bouvier [:bbouvier] from comment #7)

> Can it just land as is on trunk?

Yes, per normal bug (non-security) rules. This isn't blocked by the security folks or requiring our approval.
And follow-up for not using reserved() in opt builds (and also fix inconsistent style):
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/110097b36a6d227928db912410b498e69d2a49da
https://hg.mozilla.org/mozilla-central/rev/63c4ff719c83
https://hg.mozilla.org/mozilla-central/rev/110097b36a6d

also need approval requests now i guess
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Per comment 5 and sec rating, I don't think we need to uplift, unless proven otherwise. That being said, the fix is small enough that we shouldn't be lazy here, imo.
Attached patch folded.patchSplinter Review
Two patches folded together, carrying forward r+.

Approval Request Comment
[Feature/Bug causing the regression]: bug 896100
[User impact if declined]: *possible* use-after-free in code that uses Vector::insert. So far, no such case has been detected; this patch is just a very cheap safe net.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not much
[Why is the change risky/not risky?]: literally one line of meaningful change
[String changes made/needed]: none
Attachment #8853402 - Flags: review+
Attachment #8853402 - Flags: approval-mozilla-beta?
Attachment #8853402 - Flags: approval-mozilla-aurora?
Comment on attachment 8853402 [details] [diff] [review]
folded.patch

Too late for beta 53, but we could still take it for 54.
Attachment #8853402 - Flags: approval-mozilla-beta?
Attachment #8853402 - Flags: approval-mozilla-beta-
Attachment #8853402 - Flags: approval-mozilla-aurora?
Attachment #8853402 - Flags: approval-mozilla-aurora+
Group: core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main54+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.