New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 116524 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment

Security: Off-by-one in OTS resulting in arbitrary code execution

Project Member Reported by mjurczyk@google.com, Mar 2 2012

Issue description

VULNERABILITY DETAILS

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.

Log from ASAN:

=================================================================
==1119== ERROR: AddressSanitizer global-buffer-overflow on address 0x000000754d70 at pc 0x51447a bp 0x7fff67fe8db0 sp 0x7fff67fe8da8
READ of size 2 at 0x000000754d70 thread T0
  #0 0x51447a ots::LookupSubtableParser::Parse()
  #1 0x5161fe (anonymous namespace)::ParseLookupTable()
  #2 0x515ab8 ots::ParseLookupListTable()
  #3 0x4f9f7d ots::ots_gpos_parse()
  #4 0x4b5e6f (anonymous namespace)::ProcessGeneric()
  #5 0x4b5384 (anonymous namespace)::ProcessTTF()
  #6 0x4b4107 ots::Process()
  #7 0x4b344b main
  #8 0x7f2927a2cd5d __libc_start_main
0x000000754d70 is located 0 bytes to the right of global variable '_ZN12_GLOBAL__N_1L16kGposTypeParsersE (ots/src/gpos.cc)' (0x754ce0) of size 144
==1119== ABORTING
Stats: 0M malloced (0M for red zones) by 710 calls
Stats: 0M realloced by 0 calls
Stats: 0M freed by 29 calls
Stats: 0M really freed by 0 calls
Stats: 20M (5121 full pages) mmaped in 5 calls
  mmaps   by size class: 8:16383; 9:8191; 10:4095; 11:2047; 17:32;
  mallocs by size class: 8:696; 9:9; 10:3; 11:1; 17:1;
  frees   by size class: 8:26; 9:2; 10:1;
  rfrees  by size class:
Stats: malloc large: 1 small slow: 5
Shadow byte and word:
  0x1000000ea9ae: f9
  0x1000000ea9a8: 00 00 00 00 00 00 f9 f9
More shadow bytes:
  0x1000000ea988: 00 00 00 00 f9 f9 f9 f9
  0x1000000ea990: 00 00 00 00 00 00 00 f9
  0x1000000ea998: f9 f9 f9 f9 00 00 00 00
  0x1000000ea9a0: 00 00 00 00 00 00 00 00
=>0x1000000ea9a8: 00 00 00 00 00 00 f9 f9
  0x1000000ea9b0: f9 f9 f9 f9 00 00 00 00
  0x1000000ea9b8: 00 00 00 00 00 00 f9 f9
  0x1000000ea9c0: f9 f9 f9 f9 00 00 f9 f9
  0x1000000ea9c8: f9 f9 f9 f9 00 00 f9 f9


VERSION

OpenType Sanitiser r82, any operating system or Chrome version.


REPRODUCTION CASE

See attachments. They reproduce when used with ot-sanitise:

./ot-sanitise >/dev/null 1015sn.otf.asan.5a.2

 
1015sn.otf.asan.5a.2
44.0 KB Download
Kinnari.otf.asan.5a.172
125 KB Download
24248.8683.ttf.asan.5a.55
10.2 KB Download
Upstream tracking bug to get some attention:
http://code.google.com/p/ots/issues/detail?id=3
Cc: bashi@chromium.org
@bashi: it looks like you may be the right guy to look at this bug.

Comment 3 by bashi@chromium.org, Mar 3 2012

Cc: yusukes@chromium.org agl@chromium.org
Owner: bashi@chromium.org
Status: Started
Thank you for finding the bug. It's obviously my fault. Attached a patch that will fix the bug. Could you confirm it? 
fix.patch
863 bytes View Download
Technically it is fine, as long as GPOS_TYPE_RESERVED isn't (erroneously or not) changed to something different from number of the array size + 1 at some point in time. I think it would be safer to directly use the number of cells in the table (i.e. sizeof(kGposTypeParsers)/sizeof(kGposTypeParsers[0]) in this context, especially if there's a macro for that in Chromium.

If nothing else, this patch is correct and fixes the issue.

Comment 5 by bashi@chromium.org, Mar 3 2012

Thanks. GPOS_TYPE_RESERVED is unlikely changed, but I agree with you for using the number of cells in the table. I uploaded the patch for review.
Project Member

Comment 6 by bugdroid1@chromium.org, Mar 3 2012

Commit: be8c6381b93a634d6c67a6e3d52c00df864ed461
 Email: bashi@chromium.org@a4e77c2c-9104-11de-800e-5b313e0d2bf3

[OTS] Fix off-by-one in GSUB/GPOS parser

Use the number of cells in the sub-parser table for number of lookup type.

BUG= chromium:116524 
TEST=ran test_{un,}malicious_fonts.sh
Review URL: http://codereview.chromium.org/9581044

git-svn-id: http://ots.googlecode.com/svn/trunk@83 a4e77c2c-9104-11de-800e-5b313e0d2bf3

M	src/gpos.cc
M	src/gsub.cc
Labels: SecSeverity-High
Assigning SecSeverity-High, assuming OTS is in the renderer? Please let me know if it runs in the browser.
@skylined It runs in the renderer.

The fix has been landed in ots repo http://code.google.com/p/ots/source/list (r83). I'll update Chrome DEPS once the tree is opened (since bashi@ does not have corp network access right now).

Comment 9 by bashi@chromium.org, Mar 3 2012

OTS is in the renderer. I couldn't roll the fix to Chromium now. yusukes@ is going to roll the fix.
@yusukes -- thank you so much for your help!
Project Member

Comment 11 by bugdroid1@chromium.org, Mar 3 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=124860

------------------------------------------------------------------------
r124860 | yusukes@google.com | Sat Mar 03 04:56:40 PST 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/DEPS?r1=124860&r2=124859&pathrev=124860

Update ots to r83.

BUG= 116524 
TEST=none
TBR=bashi@chromium.org

Review URL: https://chromiumcodereview.appspot.com/9584048
------------------------------------------------------------------------
Status: Fixed
Labels: -Restrict-View-SecurityTeam -Pri-0 -Area-Undefined Restrict-View-SecurityNotify Pri-1 Area-Internals Merge-Approved OS-All
Status: FixUnreleased
Is this a recent regression ? Did this affect m17 stable ?
This affects m17 stable. Can I merge the change to m17/m18?
Labels: Mstone-18
Too late for M17 but please do go ahead and merge to M18.
I see. It's difficult to me to merge the change right now because I don't have VPN access and I don't remember my SVN password.. Is it ok to merge on Monday? Or could I ask someone to merge instead?
Monday is fine. Anything merged by 7PM PST Tuesday will make it into the Wednesday Beta.
Project Member

Comment 18 by bugdroid1@chromium.org, Mar 5 2012

Labels: merge-merged-1025
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=124913

------------------------------------------------------------------------
r124913 | bashi@chromium.org | Sun Mar 04 16:32:29 PST 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/1025/src/DEPS?r1=124913&r2=124912&pathrev=124913

Merge 124860 - Update ots to r83.

BUG= 116524 
TEST=none
TBR=bashi@chromium.org

Review URL: https://chromiumcodereview.appspot.com/9584048

TBR=yusukes@google.com
Review URL: https://chromiumcodereview.appspot.com/9578009
------------------------------------------------------------------------
Labels: -Merge-Approved Merge-Merged SecImpacts-Stable SecImpacts-Beta
Cc: jruder...@gmail.com dved...@gmail.com
Firefox uses OTS. Courtesy cc: to jruderman, dveditz.

Guys, we'll have the fix to stable in about two weeks. You can go sooner if you like but don't drop details too obviously.
ots revved for M18 on src-branch DEPS, r23125

I don't think the change in comment #c18 updated DEPS in the right place.
Labels: CVE-2011-3062
Cc: alb...@gmail.com

Comment 24 by cdn@chromium.org, May 15 2012

Status: Fixed
Marking old security bugs Fixed..
Project Member

Comment 25 by bugdroid1@chromium.org, Oct 13 2012

Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.
Project Member

Comment 26 by bugdroid1@chromium.org, Nov 14 2012

The following revision refers to this bug:
    http://goto.ext.google.com/viewvc/chrome-internal?view=rev&revision=23125

------------------------------------------------------------------------
r23125 | cevans@google.com | 2012-03-21T20:48:54.210408Z

------------------------------------------------------------------------
Project Member

Comment 27 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Type-Security -Area-Internals -SecSeverity-High -Mstone-18 -SecImpacts-Stable -SecImpacts-Beta Security-Impact-Stable Security-Impact-Beta M-18 Cr-Internals Security-Severity-High Type-Bug-Security
Project Member

Comment 28 by bugdroid1@chromium.org, Mar 13 2013

Labels: Restrict-View-EditIssue
Project Member

Comment 29 by bugdroid1@chromium.org, Mar 13 2013

Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue
Labels: -Restrict-View-SecurityNotify -Restrict-View-EditIssue
Project Member

Comment 31 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Severity-High Security_Severity-High
Project Member

Comment 32 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Impact-Stable Security_Impact-Stable
Project Member

Comment 33 by bugdroid1@chromium.org, Mar 21 2013

Labels: -Security-Impact-Beta Security_Impact-Beta
Project Member

Comment 34 by sheriffbot@chromium.org, Jun 14 2016

Labels: -security_impact-beta
Project Member

Comment 35 by sheriffbot@chromium.org, Oct 1 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 36 by sheriffbot@chromium.org, Oct 2 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: allpublic
Labels: CVE_description-submitted

Sign in to add a comment