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

Issue 349898 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Email to this user bounced
Closed: Apr 2014
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security
Nag



Sign in to add a comment

Security: Integer Overflows in CharacterData::deleteData & CharacterData::replaceData

Reported by jbutler@chromium.org, Mar 6 2014

Issue description

VULNERABILITY DETAILS
Both CharacterData::deleteData and CharacterData::replaceData do not properly check for integer overflows. This is shown in the following code from deleteData, where offset and count are both user-specified values.

unsigned realCount;
if (offset + count > length())
    realCount = length() - offset;
else
    realCount = count;
...
document().didRemoveText(this, offset, realCount);

didRemoveText eventually calls Range::didRemoveText, which updates the start and end offsets of the range with the invalid values. Very similar code is also present in replaceData.

The patches should be trivial, so I've assigned it to myself to work on.

VERSION
Confirmed on stable (33.0.1750.146) on Windows and Linux, also confirmed on a LKGR (35.0.1867.0) build on Linux.

REPRODUCTION CASE
A minimised testcase is attached. With assertions enabled, it triggers an assertion. Without assertions enabled, it doesn't crash, and the endOffset property of the range object can be observed to be above the length of the underlying text.
 
range_oob.html
430 bytes View Download
Thanks Jon. Please make sure to change that assertion into ASSERT_WITH_SECURITY_IMPLICATION as part of your patch. Which assertion are we talking about here ?
Ok, will do. The assertion in question is in editing/FrameSelection.cpp:386.

I'm just in the process of submitting another patch, once that one is done I'll submit this one too.
Labels: reward-topanel
Labels: M-33 Security_Impact-Beta Security_Impact-Stable Security_Severity-High
https://codereview.chromium.org/188693007/

Yet to update with layout tests, will wait to assign until I've finished that.
Labels: Cr-Blink-DOM
Layout tests uploaded, patch out for review.
Labels: Hotlist-Webkit
Project Member

Comment 9 by ClusterFuzz, Mar 15 2014

Labels: Nag
Status: Assigned
jbutler@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member

Comment 10 by ClusterFuzz, Mar 24 2014

jbutler@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Labels: Launch-Status-WIP
Project Member

Comment 12 by ClusterFuzz, Mar 31 2014

Labels: -M-33 M-34
jbutler: any updates on this?
Cc: jbutler@chromium.org
Labels: -Launch-Status-WIP
Owner: sigbjo...@opera.com
Sigbjornf@, can you please drive this to completion. Jbutler@ is an external contributor (ignore the @chromium account) and unfamiliar with the codebase and spec requirements. This is a high severity security vulnerability that has been open for 1.5 months, please help to close this soon.
certainly, i can round off jbutler's CL & work.
Created a separate CL, https://codereview.chromium.org/229793004/
Project Member

Comment 17 by bugdroid1@chromium.org, Apr 9 2014

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

------------------------------------------------------------------
r171165 | sigbjornf@opera.com | 2014-04-09T19:00:32.975283Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/editing/FrameSelection.cpp?r1=171165&r2=171164&pathrev=171165
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/CharacterData.cpp?r1=171165&r2=171164&pathrev=171165
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/dom/Range/deleteData-replaceData-count-overflow.html?r1=171165&r2=171164&pathrev=171165
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/dom/Range/deleteData-replaceData-count-overflow-expected.txt?r1=171165&r2=171164&pathrev=171165

Add CharacterData.deleteData()/replaceData() overflow handling.

If the offset and count exceed the underlying length, the spec tells us

  http://dom.spec.whatwg.org/#concept-cd-replace (step 3)

to use a count that is equal to length minus the offset. Perform that
check in an overflow-sensitive manner.

(Change based on https://codereview.chromium.org/188693007/ )

R=
BUG= 349898 

Review URL: https://codereview.chromium.org/229793004
-----------------------------------------------------------------
Status: Fixed
Project Member

Comment 19 by ClusterFuzz, Apr 9 2014

Labels: -Restrict-View-SecurityTeam Merge-Triage Restrict-View-SecurityNotify M-35
Adding Merge-Triage label for tracking purposes.

Once your fix had sufficient bake time (on canary, dev as appropriate), please nominate your fix for merge by adding the Merge-Requested label.

When your merge is approved by the release manager, please start merging with higher milestone label first. Make sure to re-request merge for every milestone in the label list. You can get branch information on omahaproxy.appspot.com.

- Your friendly ClusterFuzz
Project Member

Comment 20 by bugdroid1@chromium.org, Apr 10 2014

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

------------------------------------------------------------------
r171240 | yurys@chromium.org | 2014-04-10T07:41:11.808687Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/dom/CharacterData.cpp?r1=171240&r2=171239&pathrev=171240

Fix compilation on Chromium OS after r171165

r171165 broke compilation on Chromium OS bot and we had to revert Blink roll.
...
../../../../../../../home/chrome-bot/chrome_root/src/third_party/WebKit/Source/core/dom/CharacterData.cpp:146:36:
error: 'realCount' may be used uninitialized in this function
...
I simply added initialization for realCount variables to make the compiler happy.

[1] http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28x86%29/builds/20955

BUG= 349898 
TBR=sigbjornf@opera.com

Review URL: https://codereview.chromium.org/232243004
-----------------------------------------------------------------
Cc: -jbutler@chromium.org
Labels: -Merge-Triage Merge-Requested
Merge Requested for M35. This hasn't baked enough for M34 Patch 1, so let's see if we can take it in M34 Patch 2 (providing there is one). 

Also removing jbutler@chromium.org from cc as account is disabled.

Comment 22 by kareng@google.com, Apr 21 2014

Labels: -Merge-Requested Merge-Approved
approved for m35
Yury - please merge into M35 (branch 1916)
Labels: -Merge-Approved -M-35 Merge-Requested
merged to m35 in r172583, r172584, requesting for m34.
Project Member

Comment 25 by bugdroid1@chromium.org, Apr 25 2014

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

------------------------------------------------------------------
r172583 | inferno@chromium.org | 2014-04-25T04:08:15.336489Z

Changed paths:
   A http://src.chromium.org/viewvc/blink/branches/chromium/1916/LayoutTests/fast/dom/Range/deleteData-replaceData-count-overflow.html?r1=172583&r2=172582&pathrev=172583
   A http://src.chromium.org/viewvc/blink/branches/chromium/1916/LayoutTests/fast/dom/Range/deleteData-replaceData-count-overflow-expected.txt?r1=172583&r2=172582&pathrev=172583
   M http://src.chromium.org/viewvc/blink/branches/chromium/1916/Source/core/editing/FrameSelection.cpp?r1=172583&r2=172582&pathrev=172583
   M http://src.chromium.org/viewvc/blink/branches/chromium/1916/Source/core/dom/CharacterData.cpp?r1=172583&r2=172582&pathrev=172583

Merge 171165 "Add CharacterData.deleteData()/replaceData() overf..."

> Add CharacterData.deleteData()/replaceData() overflow handling.
> 
> If the offset and count exceed the underlying length, the spec tells us
> 
>   http://dom.spec.whatwg.org/#concept-cd-replace (step 3)
> 
> to use a count that is equal to length minus the offset. Perform that
> check in an overflow-sensitive manner.
> 
> (Change based on https://codereview.chromium.org/188693007/ )
> 
> R=
> BUG= 349898 
> 
> Review URL: https://codereview.chromium.org/229793004

TBR=sigbjornf@opera.com

Review URL: https://codereview.chromium.org/255803002
-----------------------------------------------------------------
Project Member

Comment 26 by bugdroid1@chromium.org, Apr 25 2014

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

------------------------------------------------------------------
r172584 | inferno@chromium.org | 2014-04-25T04:09:22.905450Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/branches/chromium/1916/Source/core/dom/CharacterData.cpp?r1=172584&r2=172583&pathrev=172584

Merge 171240 "Fix compilation on Chromium OS after r171165"

> Fix compilation on Chromium OS after r171165
> 
> r171165 broke compilation on Chromium OS bot and we had to revert Blink roll.
> ...
> ../../../../../../../home/chrome-bot/chrome_root/src/third_party/WebKit/Source/core/dom/CharacterData.cpp:146:36:
> error: 'realCount' may be used uninitialized in this function
> ...
> I simply added initialization for realCount variables to make the compiler happy.
> 
> [1] http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28x86%29/builds/20955
> 
> BUG= 349898 
> TBR=sigbjornf@opera.com
> 
> Review URL: https://codereview.chromium.org/232243004

TBR=yurys@chromium.org

Review URL: https://codereview.chromium.org/259783002
-----------------------------------------------------------------
Merge Requested for m34 patch 2.
ping dxie@ - merge requested for M34 (in case patch 2 happens).

Comment 29 by dxie@chromium.org, Apr 30 2014

Labels: -Merge-Requested Merge-Approved
Cc: infe...@chromium.org
inferno@ - please merge into M34 (branch 1847)
Labels: -Merge-Approved Merge-Merged merge-merged-1847
merged in r173028, 173029.
Project Member

Comment 32 by bugdroid1@chromium.org, Apr 30 2014

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

------------------------------------------------------------------
r173028 | inferno@chromium.org | 2014-04-30T21:16:33.887009Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/branches/chromium/1847/Source/core/dom/CharacterData.cpp?r1=173028&r2=173027&pathrev=173028
   A http://src.chromium.org/viewvc/blink/branches/chromium/1847/LayoutTests/fast/dom/Range/deleteData-replaceData-count-overflow.html?r1=173028&r2=173027&pathrev=173028
   A http://src.chromium.org/viewvc/blink/branches/chromium/1847/LayoutTests/fast/dom/Range/deleteData-replaceData-count-overflow-expected.txt?r1=173028&r2=173027&pathrev=173028
   M http://src.chromium.org/viewvc/blink/branches/chromium/1847/Source/core/editing/FrameSelection.cpp?r1=173028&r2=173027&pathrev=173028

Merge 171165 "Add CharacterData.deleteData()/replaceData() overf..."

> Add CharacterData.deleteData()/replaceData() overflow handling.
> 
> If the offset and count exceed the underlying length, the spec tells us
> 
>   http://dom.spec.whatwg.org/#concept-cd-replace (step 3)
> 
> to use a count that is equal to length minus the offset. Perform that
> check in an overflow-sensitive manner.
> 
> (Change based on https://codereview.chromium.org/188693007/ )
> 
> R=
> BUG= 349898 
> 
> Review URL: https://codereview.chromium.org/229793004

TBR=sigbjornf@opera.com

Review URL: https://codereview.chromium.org/268523002
-----------------------------------------------------------------
Project Member

Comment 33 by bugdroid1@chromium.org, Apr 30 2014

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

------------------------------------------------------------------
r173029 | inferno@chromium.org | 2014-04-30T21:17:10.046840Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/branches/chromium/1847/Source/core/dom/CharacterData.cpp?r1=173029&r2=173028&pathrev=173029

Merge 171240 "Fix compilation on Chromium OS after r171165"

> Fix compilation on Chromium OS after r171165
> 
> r171165 broke compilation on Chromium OS bot and we had to revert Blink roll.
> ...
> ../../../../../../../home/chrome-bot/chrome_root/src/third_party/WebKit/Source/core/dom/CharacterData.cpp:146:36:
> error: 'realCount' may be used uninitialized in this function
> ...
> I simply added initialization for realCount variables to make the compiler happy.
> 
> [1] http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28x86%29/builds/20955
> 
> BUG= 349898 
> TBR=sigbjornf@opera.com
> 
> Review URL: https://codereview.chromium.org/232243004

TBR=yurys@chromium.org

Review URL: https://codereview.chromium.org/267573002
-----------------------------------------------------------------
Labels: Release-2-M34
Labels: -reward-topanel reward-unpaid reward-1500 CVE-2014-1741
Congrats - $1500 for this one (the bump in amount was for the patch).
Project Member

Comment 36 by ClusterFuzz, Jul 20 2014

Labels: -Restrict-View-SecurityNotify
Bulk update: removing view restriction from closed bugs.
Labels: -reward-unpaid reward-inprocess
Cc: jonbutle...@gmail.com
Labels: -reward-inprocess
Processing via our e-payment system can take a few weeks, but reward should be on its way to you. Thanks again for your help!

Project Member

Comment 40 by ClusterFuzz, Feb 2 2016

Labels: -Security_Impact-Beta
Project Member

Comment 41 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 42 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