Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user
Status: Verified
Owner:
Closed: May 2014
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome, Mac
Pri: 2
Type: Bug-Security



Sign in to add a comment
Security: heap write access due to integer overflow on bspatch implementations
Project Member Reported by de...@chromium.org, May 12 2014 Back to list
VULNERABILITY DETAILS
Upstream's bspatch.c implemenation doesn't check for negative values on the number of bytes to read from the "diff" and "extra" streams, allowing an attacker controlling the patch file to write at arbitrary locations in the heap.

bspatch's main loop reads three numbers from the "control" stream in the patch: X, Y and Z. The first two are the number of bytes to read from "diff" and "extra" (and thus only non-negative), while the third one could be positive or negative and moves the oldpos pointer on the source image. These 3 values are 64bits signed ints (encoded somehow on the file) that are later passed the function that reads from the streams, but those values are not verified to be non-negative.

Chrome[OS] has four different implementations of this program, all derived from the same original code by Colin Percival.

VERSION
Chrome Version: ToT / See details
Operating System: Mac / ChromeOS

 * On Mac, src/chrome/installer/mac/third_party/bsdiff/goobspatch.c (although it seems to not be vulnerable due to proper error handling on cfread()). I don't know where the patches come from.

 * On ChromeOS, src/third_party/chromiumos-overlay/dev-util/bsdiff/bsdiff-4.3-r5.ebuild the command is definitively vulnerable, but update_engine checks the hash of the patch *before* running bspatch, so not exploitable on that context.

 * On Chrome/Win, src/third_party/bspatch/mbspatch.cc (from Mozilla) seems to not be vulnerable to this overflow because it reads a struct with X and Y as unsigned values and checks that the resulting pointer is in range.

 * On Chrome/Win, src/courgette/third_party/bsdiff_apply.cc (used by courgette during Chrome updates) it uses a variant of the mbspatch.cc which again reads unsigned values. That impl seems to be ok.


REPRODUCTION CASE
The attached python script creates a patch that exposes the bug for the first two cases.
Essentially, you need to pass negative values for X or Y that BZRead() treats properly.

 
bspatch_bug.py
3.3 KB View Download
Comment 1 by de...@chromium.org, May 12 2014
Cc: mark@chromium.org
+mark@ for goobspatch.c

https://codereview.chromium.org/287483002

Comment 2 by mark@chromium.org, May 12 2014
>  * On Mac, src/chrome/installer/mac/third_party/bsdiff/goobspatch.c (although
> it seems to not be vulnerable due to proper error handling on cfread()). I
> don't know where the patches come from.

I wrote the patches directly in the Chrome source tree, there is no other source for this version than the one you’re patching.

This bug is a very good find.
Comment 3 by de...@chromium.org, May 12 2014
Owner: de...@chromium.org
Status: Started
https://memegen.googleplex.com/4589397022343168

...and the patch for the ChromeOS impl: https://chromium-review.googlesource.com/#/c/199377
Comment 4 by de...@chromium.org, May 12 2014
Cc: aludwig@google.com nnk@google.com
+android security folks
Project Member Comment 5 by bugdroid1@chromium.org, May 13 2014
Project: chromiumos/overlays/chromiumos-overlay
Branch : master
Author : Alex Deymo <deymo@chromium.org>
Commit : cb060bd08959295c992bed27961b982a124d8bd2

Code-Review  0 : Alex Deymo, chrome-internal-fetch
Code-Review  +2: Jorge Lucangeli Obes
Commit-Queue 0 : Jorge Lucangeli Obes, chrome-internal-fetch
Commit-Queue +1: Alex Deymo
Verified     0 : Jorge Lucangeli Obes, chrome-internal-fetch
Verified     +1: Alex Deymo
Change-Id      : I8007b41cf3ab6558a99ffd6085e382f1c32f9bdf
Reviewed-at    : https://chromium-review.googlesource.com/199377

bspatch: Add a sanity-check for malformed patches.

This patch adds an extra sanity-check for malformed patches. A
corrupted patch should be cached by update_engine before reaching
this step anyway.

BUG= chromium:372525 
TEST=emerge-link bsdiff && chroot/build/link/usr/bin/bspatch foo bar malformed_patch
bspatch segfaults without the patch and shows the error message with it.

dev-util/bsdiff/bsdiff-4.3-r6.ebuild
dev-util/bsdiff/files/4.3_sanity_check.patch
Comment 6 by de...@chromium.org, May 14 2014
Status: Fixed
I think all the Chrome* versions requiring it are patched.
Labels: Security_Impact-Head
Based on OP, adding security impact only for ToT. 
Labels: VerifyIn-38
Project Member Comment 9 by clusterf...@chromium.org, Aug 21 2014
Labels: -Restrict-View-SecurityTeam
Bulk update: removing view restriction from closed bugs.
Comment 10 by krisr@chromium.org, Sep 17 2014
Status: Verified
Project Member Comment 11 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 12 by sheriffbot@chromium.org, Oct 1 2016
Labels: Restrict-View-SecurityNotify
Project Member Comment 13 by sheriffbot@chromium.org, Oct 2 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
Labels: allpublic
Sign in to add a comment