Closed Bug 739925 (CVE-2011-3062) Opened 12 years ago Closed 12 years ago

OTS off-by-one may result in arbitrary code execution

Categories

(Firefox :: General, defect)

11 Branch
All
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 12
Tracking Status
firefox11 --- affected
firefox12 + fixed
firefox13 + fixed
firefox14 + fixed
firefox-esr10 12+ fixed

People

(Reporter: huzaifas, Assigned: jfkthame)

References

Details

(Whiteboard: [sg:critical][qa-])

Attachments

(1 file)

This is chromium bug:
https://code.google.com/p/chromium/issues/detail?id=116524
which states that  jruderman and dveditz are cced, but since i did not find an open bug, i decided to open one. (Apologies if this is a dupe.)

Patch available at:
http://code.google.com/p/ots/source/detail?r=83#

snippet from upstream bug:
There's an off-by-one vulnerability in OpenType Sanitiser (http://code.google.com/p/ots/). The bug was found using AddressSanitizer.

See the following code from src/gpos.cc:

--- cut ---
30:   GPOS_TYPE_RESERVED = 10

[...]

60: const ots::LookupSubtableParser::TypeParser kGposTypeParsers[] = {
61:   {GPOS_TYPE_SINGLE_ADJUSTMENT, ParseSingleAdjustment},
62:   {GPOS_TYPE_PAIR_ADJUSTMENT, ParsePairAdjustment},
63:   {GPOS_TYPE_CURSIVE_ATTACHMENT, ParseCursiveAttachment},
64:   {GPOS_TYPE_MARK_TO_BASE_ATTACHMENT, ParseMarkToBaseAttachment},
65:   {GPOS_TYPE_MARK_TO_LIGATURE_ATTACHMENT, ParseMarkToLigatureAttachment},
66:   {GPOS_TYPE_MARK_TO_MARK_ATTACHMENT, ParseMarkToMarkAttachment},
67:   {GPOS_TYPE_CONTEXT_POSITIONING, ParseContextPositioning},
68:   {GPOS_TYPE_CHAINED_CONTEXT_POSITIONING, ParseChainedContextPositioning},
69:   {GPOS_TYPE_EXTENSION_POSITIONING, ParseExtensionPositioning}
70: };
71: 
72: const ots::LookupSubtableParser kGposLookupSubtableParser = {
73:   GPOS_TYPE_RESERVED, GPOS_TYPE_EXTENSION_POSITIONING, kGposTypeParsers
74: };
--- cut ---

The kGposTypeParsers table has 9 items, however a value of 10 (GPOS_TYPE_RESERVED) is used as the num_types value of the kGposLookupSubtableParser structure. This can later lead to an out-of-bounds read and execution of an uninitialized function pointer from kGposTypeParsers[9].parser (see src/layout.cc):

1153: bool LookupSubtableParser::Parse(const OpenTypeFile *file, const uint8_t *data,
1154:                                  const size_t length,
1155:                                  const uint16_t lookup_type) const {
1156:   for (unsigned i = 0; i < num_types; ++i) {
1157:     if (parsers[i].type == lookup_type && parsers[i].parse) {
1158:       if (!parsers[i].parse(file, data, length)) {
1159:         return OTS_FAILURE();
1160:       }
1161:       return true;
1162:     }
1163:   }
1164:   return OTS_FAILURE();
1165: }

The vulnerable behavior can be potentially used to get a direct control over the application's execution flow.
Severity: normal → critical
Hardware: x86_64 → All
critical assuming we use the part of OTS affected by this bug (we do have the code in our tree).

The patch is small and safe, we should try to get it into Firefox 12 since Chrome has already shipped this fix.

We should also plan a general update to a newer OTS to pick up any other unidentified security fixes. That should go in a separate bug and not be slammed into a beta.
Assignee: nobody → jfkthame
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [sg:critical]
(In reply to Daniel Veditz [:dveditz] from comment #1)
> We should also plan a general update to a newer OTS to pick up any other
> unidentified security fixes. That should go in a separate bug and not be
> slammed into a beta.

We do take OTS updates fairly promptly, most recently to rev.81 in bug 730190.
Attachment #610202 - Flags: review?(jdaggett) → review+
Comment on attachment 610202 [details] [diff] [review]
patch, port the sanitizer bug-fixes from upstream rev.83

[Approval Request Comment]
Regression caused by (bug #): n/a - upstream OTS bug

User impact if declined: potentially exploitable security vulnerability

Testing completed (on m-c, etc.): only just landed, but already in upstream OTS/chromium

Risk to taking this patch (and alternatives if risky): minimal risk, just corrects array-size value to avoid out-of-bounds read

String changes made by this patch: none
Attachment #610202 - Flags: approval-mozilla-esr10?
Attachment #610202 - Flags: approval-mozilla-beta?
Attachment #610202 - Flags: approval-mozilla-aurora?
Do we have a testcase to use for verification of the fix anywhere?
There's a testcase in https://code.google.com/p/chromium/issues/detail?id=116524 but I don't know if it works in the browser. Do you have a gmail / google code account that could be CCed on that bug?
(In reply to Jesse Ruderman from comment #7)
> There's a testcase in
> https://code.google.com/p/chromium/issues/detail?id=116524 but I don't know
> if it works in the browser. Do you have a gmail / google code account that
> could be CCed on that bug?

The test cases wont really work in the browser, i can attach them to this bug if needed though
This is CVE-2011-3062
https://hg.mozilla.org/mozilla-central/rev/baf59f769aed
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Alias: CVE-2011-3062
We should fix this on all branches since it has been checked into Chromium.
Once this has a couple of days to bake, we expect to approve for all branches.
Comment on attachment 610202 [details] [diff] [review]
patch, port the sanitizer bug-fixes from upstream rev.83

[Triage Comment]
We haven't heard of any fallout from this change - approved for all branches.
Attachment #610202 - Flags: approval-mozilla-esr10?
Attachment #610202 - Flags: approval-mozilla-esr10+
Attachment #610202 - Flags: approval-mozilla-beta?
Attachment #610202 - Flags: approval-mozilla-beta+
Attachment #610202 - Flags: approval-mozilla-aurora?
Attachment #610202 - Flags: approval-mozilla-aurora+
Is there something QA can do to verify this fix?
Whiteboard: [sg:critical] → [sg:critical][qa?]
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #16)
> Is there something QA can do to verify this fix?

Apparently not.
Whiteboard: [sg:critical][qa?] → [sg:critical][qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.