unchecked allocations - potential buffer overrun in windows font code

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

1.9.2 Branch
x86
Windows 7
Points:
---

Firefox Tracking Flags

(blocking1.9.2 needed, status1.9.2 .4-fixed, blocking1.9.1 needed, status1.9.1 .10-fixed)

Details

(Whiteboard: [sg:critical? (possibly)] [qa-noaction-191] [qa-noaction-192])

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
The code for Uniscribe-based text shaping in gfxWindowsFonts.cpp has a number of unchecked uses of nsTArray<T>::SetLength() to resize buffers that will then be passed to Uniscribe APIs. If any of these SetLength() calls fail due to lack of memory (unlikely, but conceivable especially with large textruns), we'd be passing invalid buffers to Uniscribe.

This is being fixed on trunk as part of bug 502906, but that (more extensive) patch is not intended for the 1.9.2 branch.
(Assignee)

Comment 1

8 years ago
Created attachment 431835 [details] [diff] [review]
check array SetLength() calls for allocation failure

This should prevent us passing invalid buffers to Uniscribe.
Assignee: nobody → jfkthame
Attachment #431835 - Flags: review?(jdaggett)

Comment 2

8 years ago
Comment on attachment 431835 [details] [diff] [review]
check array SetLength() calls for allocation failure

Looks good.
Attachment #431835 - Flags: review?(jdaggett) → review+
(Assignee)

Comment 3

8 years ago
Comment on attachment 431835 [details] [diff] [review]
check array SetLength() calls for allocation failure

Requesting approval to land this for 1.9.2.3 - although I don't know of a clear "smoking gun", we have seen issues with pathological pages/scripts that create huge textruns in the past. Allocating several potentially-large arrays for glyph data without checking for failure, and then passing them to Uniscribe, seems like a risk we should not be taking.

So I think we want this on 1.9.2 (and earlier branches?), but I didn't see a "wanted1.9.2?" flag. Should it be blocking? That seems rather extreme for something that is mainly a precautionary measure.
Attachment #431835 - Flags: approval1.9.2.3?
Group: core-security
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
status1.9.1: --- → wanted
status1.9.2: --- → wanted
Whiteboard: [sg:critical?]
Whiteboard: [sg:critical?] → [sg:critical? (possibly)]
blocking1.9.1: ? → needed
blocking1.9.2: ? → needed
Comment on attachment 431835 [details] [diff] [review]
check array SetLength() calls for allocation failure

Approved for 1.9.2.3, a=dveditz for release-drivers

It looks like this is needed on the 1.9.1 branch. Will this patch work?
Attachment #431835 - Flags: approval1.9.2.3? → approval1.9.2.3+
(Assignee)

Comment 5

8 years ago
Pushed to 1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/95c0d61183a1
(Assignee)

Comment 6

8 years ago
Comment on attachment 431835 [details] [diff] [review]
check array SetLength() calls for allocation failure

The same patch applies to 1.9.1 also.
Attachment #431835 - Flags: approval1.9.1.10?
Comment on attachment 431835 [details] [diff] [review]
check array SetLength() calls for allocation failure

Approved for 1.9.1.10, a=dveditz

A checking comment along the lines of a simple "unchecked allocations" would have been better than adding the "potential overflow" part.
Attachment #431835 - Flags: approval1.9.1.10? → approval1.9.1.10+
(Assignee)

Comment 8

8 years ago
(In reply to comment #7)
> A checking comment along the lines of a simple "unchecked allocations" would
> have been better than adding the "potential overflow" part.

Yeah, I thought about that right after I pushed it! Sorry. :(
I've already modified the comment in my 1.9.1 tree.
(Assignee)

Comment 9

8 years ago
Pushed to 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1f3da3f9f51e
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
status1.9.1: wanted → .10-fixed
status1.9.2: wanted → .3-fixed
This doesn't seem to be a scenario that we can exploit on demand. Is there a way for QA to verify this bug as fixed for 1.9.2 or 1.9.1?

Comment 11

8 years ago
It's basically an out-of-memory scenario, so you'd need to make a testcase that fills memory completely, frees a tidbit, then creates enormous text runs.  In short, difficult to hit these cases but not impossible.
Whiteboard: [sg:critical? (possibly)] → [sg:critical? (possibly)] [qa-noaction-191] [qa-noaction-192]
Group: core-security
You need to log in before you can comment on or make changes to this bug.