Closed Bug 793253 Opened 7 years ago Closed 7 years ago

Supposedly infallible TArray can fail to allocate a buffer

Categories

(Core :: XPCOM, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
Tracking Status
firefox17 + fixed
firefox18 + fixed
firefox-esr10 --- wontfix
firefox-esr17 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

(Keywords: sec-moderate, Whiteboard: [adv-track-main17+][qa?])

Attachments

(1 file)

nsTArray_base<Alloc>::EnsureCapacity has this bit:

  if ((uint64_t)capacity * elemSize > size_type(-1)/2) {
    NS_ERROR("Attempting to allocate excessively large array");
    return false;
  }

If Alloc is nsTArrayInfallibleAllocator, we should be aborting here, instead of returning false, I believe.
I'm not convinced about the checkin comment if we want to not make this obvious... suggestions?
Attachment #663499 - Flags: review?
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Attachment #663499 - Flags: review? → review?(justin.lebar+bug)
Comment on attachment 663499 [details] [diff] [review]
Infallible TArrays should really be infallible.

I'm not convinced anyone would have an easy time developing a testcase out of this, and anyway the patch speaks for itself, so I don't feel like we need to obfuscate the commit message, personally.
Attachment #663499 - Flags: review?(justin.lebar+bug) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/415f11a2bb1a
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/415f11a2bb1a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
We should probably get this into Firefox 17, but unless we've found a specific case where this gets abused we don't need to over-worry about esr10 and rushing into Firefox 16 at the last minute.
Let's get this into 17 so it's in the first ESR17 and also nominate for esr10.
sorry, cancel that esr10 request, i see now that we marked wontfix there.
Comment on attachment 663499 [details] [diff] [review]
Infallible TArrays should really be infallible.

OK, requesting approval for 17...

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Long-standing bug
User impact if declined: Probably none, but if there is impact it would be a
   security bug.
Testing completed (on m-c, etc.): On trunk for a while, and seems OK.
Risk to taking this patch (and alternatives if risky): Main risk is if a caller
   was depending on this non-fatal behavior in an OOM situation.  Then this
   fix would introduce a DoS attack.
String or UUID changes made by this patch: None.
Attachment #663499 - Flags: approval-mozilla-beta?
Comment on attachment 663499 [details] [diff] [review]
Infallible TArrays should really be infallible.

Approving for uplift so we get this not only for Beta but for our first ESR17 builds.
Attachment #663499 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [adv-track-main17+]
Is there something QA can do to verify this fix?
Whiteboard: [adv-track-main17+] → [adv-track-main17+][qa?]
Group: core-security
You need to log in before you can comment on or make changes to this bug.