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.
Issue 169685 Missing validation of webkit_base::DataElement across IPC
Starred by 1 user Project Member Reported by aedla@chromium.org, Jan 12 2013 Back to list
Status: Fixed
Owner:
User never visited
Closed: Jan 2013
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment
The webkit_base::DataElement is not validated in ParamTraits::Read() nor in FileAPIMessageFilter::OnAppendBlobDataItem().

The offset and length fields can have crazy values like 2^63, which leads to buffer overflows in BlobURLRequestJob, for example:

int BlobURLRequestJob::ComputeBytesToRead() const {
  int64 current_item_remaining_bytes =
      item_length_list_[current_item_index_] - current_item_offset_;
  int64 remaining_bytes = std::min(current_item_remaining_bytes,
                                   remaining_bytes_);

  return static_cast<int>(std::min(
             static_cast<int64>(read_buf_->BytesRemaining()),
             remaining_bytes));
}

A negative value in item_length_list_ falls through both min()-s and is casted to int. This means that the output of ComputeBytesToRead() is attacker-controlled, leading to an overflow of attacker-controlled length and data in ReadFileItem() or ReadBytesItem().

REPRODUCTION CASE
I created a PoC for linux x64 that displays /etc/passwd.

 - apply webkit.patch
 - compile a release build
 - open http://www.ut.ee/~asd/40cdef8bf7ec8ad87a6e97a26983ede1.html

 
40cdef8bf7ec8ad87a6e97a26983ede1.html
3.8 KB View Download
webkit.patch
1.2 KB View Download
Comment 1 by aedla@chromium.org, Jan 14 2013
A word about the PoC: it doesn't overcome ASLR nor achieve code execution in the browser. It just overwrites a buffer containing a valid path to "/etc/passwd".

So browser buffers with paths are pretty dangerous. Almost as bad as W & X buffers I'd say. You can write something predictable to either to own the browser.
Very nice work, Jüri! :)
Comment 3 by jsc...@chromium.org, Jan 14 2013
@aedla - re comment #1: If you were spending your time bypassing ASLR or DEP we might need to have a discussion about priorities. ;)
Comment 4 by aedla@chromium.org, Jan 21 2013
Cc: kinuko@chromium.org
Project Member Comment 6 by bugdroid1@chromium.org, Jan 28 2013
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=179154

------------------------------------------------------------------------
r179154 | aedla@chromium.org | 2013-01-28T16:24:41.052927Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/blob/blob_url_request_job.cc?r1=179154&r2=179153&pathrev=179154
   M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/blob/blob_url_request_job.h?r1=179154&r2=179153&pathrev=179154

Avoid integer overflows in BlobURLRequestJob.

BUG= 169685 


Review URL: https://chromiumcodereview.appspot.com/12047012
------------------------------------------------------------------------
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify Merge-Approved
Status: Fixed
Comment 8 by aedla@chromium.org, Jan 28 2013
Labels: -Merge-Approved
It will be reverted.

Guess I shouldn't do
const int64 kMaxTotalSize = std::numeric_limits<int64>::max();
Project Member Comment 9 by bugdroid1@chromium.org, Jan 28 2013
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=179158

------------------------------------------------------------------------
r179158 | pkotwicz@chromium.org | 2013-01-28T17:27:33.228869Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/blob/blob_url_request_job.cc?r1=179158&r2=179157&pathrev=179158
   M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/blob/blob_url_request_job.h?r1=179158&r2=179157&pathrev=179158

Revert 179154
Reverting because the CL increased the number of static initializers.
> Avoid integer overflows in BlobURLRequestJob.
> 
> BUG= 169685 
> 
> 
> Review URL: https://chromiumcodereview.appspot.com/12047012

TBR=aedla@chromium.org
Review URL: https://codereview.chromium.org/12093024
------------------------------------------------------------------------
Labels: Merge-Approved
Status: Assigned
Project Member Comment 11 by bugdroid1@chromium.org, Jan 31 2013
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=179901

------------------------------------------------------------------------
r179901 | aedla@chromium.org | 2013-01-31T17:30:39.256657Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/blob/blob_url_request_job.cc?r1=179901&r2=179900&pathrev=179901
   M http://src.chromium.org/viewvc/chrome/trunk/src/webkit/blob/blob_url_request_job.h?r1=179901&r2=179900&pathrev=179901

Avoid integer overflows in BlobURLRequestJob.

BUG= 169685 


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179154

Review URL: https://chromiumcodereview.appspot.com/12047012
------------------------------------------------------------------------
Status: Fixed
Project Member Comment 13 by bugdroid1@chromium.org, Feb 12 2013
Labels: -Merge-Approved merge-merged-1364
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=182045

------------------------------------------------------------------------
r182045 | cevans@chromium.org | 2013-02-12T22:54:07.738685Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1364/src/webkit/blob/blob_url_request_job.cc?r1=182045&r2=182044&pathrev=182045
   M http://src.chromium.org/viewvc/chrome/branches/1364/src/webkit/blob/blob_url_request_job.h?r1=182045&r2=182044&pathrev=182045

Merge 179901
> Avoid integer overflows in BlobURLRequestJob.
> 
> BUG= 169685 
> 
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179154
> 
> Review URL: https://chromiumcodereview.appspot.com/12047012

TBR=aedla@chromium.org
Review URL: https://codereview.chromium.org/12251006
------------------------------------------------------------------------
Labels: -Mstone-24 Mstone-25 Release-0
Labels: CVE-2013-0891
Project Member Comment 16 by bugdroid1@chromium.org, Mar 10 2013
Labels: -Type-Security -SecSeverity-High -Area-Internals -Mstone-25 -SecImpacts-Stable -SecImpacts-Beta Security-Impact-Stable Security-Impact-Beta Cr-Internals Security-Severity-High Type-Bug-Security M-25
Project Member Comment 17 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Severity-High Security_Severity-High
Project Member Comment 18 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Impact-Stable Security_Impact-Stable
Project Member Comment 19 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Impact-Beta Security_Impact-Beta
Labels: -Restrict-View-SecurityNotify
Project Member Comment 21 by sheriffbot@chromium.org, Jun 14 2016
Labels: -security_impact-beta
Project Member Comment 22 by sheriffbot@chromium.org, Oct 1
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 23 by sheriffbot@chromium.org, Oct 2
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