739925 - (CVE-2011-3062) OTS off-by-one may result in arbitrary code execution

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Huzaifa Sidhpurwala]

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.

Comment 1

[Daniel Veditz [:dveditz]]

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.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: patch, port the sanitizer bug-fixes from upstream rev.83

Comment 3

[Jonathan Kew [:jfkthame]]

(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.

Comment 4

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/baf59f769aed

Comment 5

[Jonathan Kew [:jfkthame]]

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

Comment 6

[Al Billings [:abillings - ex-MoCo]]

Do we have a testcase to use for verification of the fix anywhere?

Comment 7

[Jesse Ruderman]

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?

Comment 8

[Al Billings [:abillings - ex-MoCo]]

I have [e-mail].

Comment 9

[Huzaifa Sidhpurwala]

(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

Comment 10

[Huzaifa Sidhpurwala]

This is CVE-2011-3062

Comment 11

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/mozilla-central/rev/baf59f769aed

Comment 12

[Al Billings [:abillings - ex-MoCo]]

We should fix this on all branches since it has been checked into Chromium.

Comment 13

[Alex Keybl [:akeybl]]

Once this has a couple of days to bake, we expect to approve for all branches.

Comment 14

[Alex Keybl [:akeybl]]

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.

Comment 15

[Jonathan Kew [:jfkthame]]

Pushed to branches:
https://hg.mozilla.org/releases/mozilla-aurora/rev/4696c6beca3d
https://hg.mozilla.org/releases/mozilla-beta/rev/63e71ea24257
https://hg.mozilla.org/releases/mozilla-esr10/rev/9c6a6651bc1f

Comment 16

[u279076]

Is there something QA can do to verify this fix?

Comment 17

[Al Billings [:abillings - ex-MoCo]]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #16)
> Is there something QA can do to verify this fix?

Apparently not.