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

Issue 614934 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

Security: sfntly font parsing heap-buffer-overflow

Reported by chen...@topsec.com.cn, May 26 2016

Issue description

VULNERABILITY DETAILS
sfntly fonts parsing heap-buffer-overflow or heap-use-after-free

two cases are index overflow.
date used by memcpy later.


VERSION
latest=git-repo

REPRODUCTION CASE

sources:

chromium/src/third_party/sfntly/src/cpp/src/sfntly/data/growable_memory_byte_array.cc:63
chromium/src/third_party/sfntly/src/cpp/src/sfntly/data/memory_byte_array.cc:46

two cases are index overflow.
date used by memcpy later.

can be reproduced with processes like this:
1.compile.

2.run.
./bin/subsetter GrowableMemoryByteArray_InternalGet_heap_overflow 1.bin
./bin/subsetter MemoryByteArray_InternalGet_heap_over_flow 1.bin

==========================================================================
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/sfntly/


 
GrowableMemoryByteArray_InternalGet_heap_overflow
72.0 KB View Download
MemoryByteArray_InternalGet_heap_over_flow
36.5 KB View Download

Comment 1 by mea...@chromium.org, May 26 2016

Owner: arthurhsu@chromium.org
Status: Assigned (was: Unconfirmed)
arthurhsu: Can you please take a look and reassign as appropriate? Thanks.
Cc: arthurhsu@chromium.org
Owner: thestig@chromium.org
Lei can you help?

growable_memory_byte_array are more like C-style things. I did not use STL initially because STL at that time throws exceptions. I don't know if it's okay to use STL now.

If so these shall be handled nicely by STL.


Cc: behdad@chromium.org
Let me try to figure out how to build the subsetter test binary and take a look.
Project Member

Comment 4 by ClusterFuzz, May 31 2016

Labels: Missing_Severity-1 Missing_Impact-1
Labels: M-51 Pri-2
GrowableMemoryByteArray_InternalGet_heap_overflow repro'd. ByteVector in third_party/sfntly/src/cpp/src/sfntly/port/type.h is typedef'd to std::vector, so in a Linux debug build, there is bounds checking and the program aborts.

MemoryByteArray_InternalGet_heap_over_flow repro'd with Valgrind and ASAN.

Similarly, uaf repo'd with Valgrind and ASAN.

Comment 6 by mea...@chromium.org, May 31 2016

Labels: -Missing_Severity-1 -Missing_Impact-1 Security_Severity-High Security_Impact-Stable
Adding labels based on comment #5 (I'm assuming this reproes on stable).
It looks like if we just do the bounds check in ByteArray::Get() and return -1 on error, then all 3 errors stop occurring. Basically:

------
diff --git a/cpp/src/sfntly/data/byte_array.cc b/cpp/src/sfntly/data/byte_array.cc
index 915a40c..57f9eed 100644
--- a/cpp/src/sfntly/data/byte_array.cc
+++ b/cpp/src/sfntly/data/byte_array.cc
@@ -35,6 +35,8 @@ int32_t ByteArray::SetFilledLength(int32_t filled_length) {
 }
 
 int32_t ByteArray::Get(int32_t index) {
+  if (index < 0 || index >= Length())
+    return -1;
   return InternalGet(index) & 0xff;
 }
------

I'll put together a pull request with my GitHub account later today.
Project Member

Comment 8 by sheriffbot@chromium.org, Jun 1 2016

Labels: -Pri-2 Pri-1

Comment 9 by f...@chromium.org, Jun 2 2016

Components: Blink>Fonts
Cc: mea...@chromium.org
Status: Started (was: Assigned)
Github pull request merged, Chromium DEPS roll is next: https://codereview.chromium.org/2060023003/

I'm not 100% sure if this affects mobile platforms. Just going to mark it OS-All anyway.

meacer: Can you help decide how far back we should merge this? Definitely M-52. How about M-51? This bug has been around for a long time.
Project Member

Comment 12 by bugdroid1@chromium.org, Jun 11 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e40502b71c9bd4f548118550952afd5d6a158bc4

commit e40502b71c9bd4f548118550952afd5d6a158bc4
Author: thestig <thestig@chromium.org>
Date: Sat Jun 11 04:11:08 2016

Roll DEPS for sfntly to 468cad5.

TBR=behdad@chromium.org
BUG= 614934 

Review-Url: https://codereview.chromium.org/2060023003
Cr-Commit-Position: refs/heads/master@{#399363}

[modify] https://crrev.com/e40502b71c9bd4f548118550952afd5d6a158bc4/DEPS

Project Member

Comment 13 by bugdroid1@chromium.org, Jun 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e40502b71c9bd4f548118550952afd5d6a158bc4

commit e40502b71c9bd4f548118550952afd5d6a158bc4
Author: thestig <thestig@chromium.org>
Date: Sat Jun 11 04:11:08 2016

Roll DEPS for sfntly to 468cad5.

TBR=behdad@chromium.org
BUG= 614934 

Review-Url: https://codereview.chromium.org/2060023003
Cr-Commit-Position: refs/heads/master@{#399363}

[modify] https://crrev.com/e40502b71c9bd4f548118550952afd5d6a158bc4/DEPS

Cc: mbarbe...@chromium.org
thestig: Sorry I missed your comment. Not sure about the merge policy for DEPS rolls to stable channel.

mbarbella: Can you please advise?
FWIW, we use sfntly in Skia when generating PDFs for printing. I don't know if Skia is ever used in the browser process, but if it is, sfntly is at least not in the normal execution path.
Cc: timwillis@chromium.org
At a glance it looks like there's not much in the roll so I think it would probably be safe in this case. In general we're usually reluctant to merge a roll to a stable branch, though.

+timwillis in case he disagrees, but I think this is probably reasonable for M51. M52 definitely seems fine.
Labels: -M-51 Merge-Request-52 M-52 OS-All
Ya, it's not much of a roll. The other commits in the roll are Java and does not affect us.

Comment 18 by tin...@google.com, Jun 15 2016

Labels: -Merge-Request-52 Merge-Review-52 Hotlist-Merge-Review
[Automated comment] DEPS changes referenced in bugdroid comments, needs manual review.
Project Member

Comment 19 by ClusterFuzz, Jun 16 2016

Status: Fixed (was: Started)
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges.

- Your friendly ClusterFuzz
Project Member

Comment 20 by sheriffbot@chromium.org, Jun 16 2016

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Before we approve merge to M52, Could you please confirm whether this change is baked/verified in Canary and safe to merge?

It's a very small change and there hasn't been any reports of printing failures.
Labels: -Merge-Review-52 Merge-Approved-52
Approving merge to M52 branch 2743 based on comment #22. Please merge ASAP possibly before 5:00 PM PST today so we can take it for this week Beta release. Thank you.
Project Member

Comment 24 by bugdroid1@chromium.org, Jun 20 2016

Labels: -merge-approved-52 merge-merged-2743
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a5694a5ba90056b8323afaab9c4b6b1032f54a72

commit a5694a5ba90056b8323afaab9c4b6b1032f54a72
Author: Lei Zhang <thestig@chromium.org>
Date: Mon Jun 20 17:41:39 2016

M52: Roll DEPS for sfntly to 468cad5.

TBR=behdad@chromium.org
BUG= 614934 

Review-Url: https://codereview.chromium.org/2060023003
Cr-Commit-Position: refs/heads/master@{#399363}
(cherry picked from commit e40502b71c9bd4f548118550952afd5d6a158bc4)

Review URL: https://codereview.chromium.org/2082643003 .

Cr-Commit-Position: refs/branch-heads/2743@{#405}
Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939}

[modify] https://crrev.com/a5694a5ba90056b8323afaab9c4b6b1032f54a72/DEPS

Labels: reward-topanel
Labels: Release-0-M52
Labels: CVE-2016-1709
Labels: reward-500 reward-unpaid
Cc: -timwillis@chromium.org
Congratulations! The panel has awarded $500 for this bug.  A member of our finance team will be in touch in the next few weeks.
Labels: -reward-topanel
Labels: -reward-unpaid reward-inprocess
Project Member

Comment 32 by sheriffbot@chromium.org, Sep 22 2016

Labels: -Restrict-View-SecurityNotify
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 33 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 34 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
Components: Internals>Skia>PDF

Sign in to add a comment