Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 570427 UaF in blink::SearchInputType::didSetValueByUserEdit
Starred by 1 user Project Member Reported by och...@chromium.org, Dec 16 2015 Back to list
Status: Fixed
Owner:
Closed: Dec 2015
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment
in third_party/WebKit/Source/core/html/forms/SearchInputType.cpp:

void SearchInputType::startSearchEventTimer()
{
    ASSERT(element().layoutObject());
    unsigned length = element().innerEditorValue().length();

    if (!length) {
        stopSearchEventTimer();
        element().onSearch(); // <------- can change the type here
        return;
    }

    // After typing the first key, we wait 0.5 seconds.
    // After the second key, 0.4 seconds, then 0.3, then 0.2 from then on.
    m_searchEventTimer.startOneShot(max(0.2, 0.6 - 0.1 * length), BLINK_FROM_HERE); // <--- UAF
}

element().onSearch() could call a user defined handler, changing the type of the |input| and freeing the SearchInputType (this), leading to a use after free when m_searchEventTimer.startOneShot(...) is called.

This can be reached by a search input element with an "incremental" attribute, when either a user clears the input manually, or if input.focus(); input.setRangeText('', start, end) is called.

repro:

<body>                                                                                                                                                         
<script>
input = document.createElement('input');
input.type = 'search'
input.setAttribute('incremental', '');
input.value = '1';
input.onsearch = function() {
  input.type = 'text';
}
input.focus();
input.setRangeText('', 0, 1);
</script>
</body>
 
Comment 1 by och...@chromium.org, Dec 16 2015
Issue 570428 has been merged into this issue.
Project Member Comment 2 by clusterf...@chromium.org, Dec 16 2015
Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6593516991414272

Uploader: ochang@google.com
Job Type: linux_asan_chrome_mp
Platform Id: linux

Crash Type: Heap-use-after-free READ 8
Crash Address: 0x60f000010cb0
Crash State:
  blink::TextFieldInputType::didSetValueByUserEdit
  blink::TextFieldInputType::subtreeHasChanged
  blink::HTMLInputElement::subtreeHasChanged
  

Unminimized Testcase: https://cluster-fuzz.appspot.com/download/AMIfv94vpIVFh8Lxrmtbdtqi4Hvsa_KauEYj1VkA0hwy6oQmsf9-ByQXD9avOMiWACeL_M8mEnltOpZyHKFoYN0lyrSxiNFfrRgNvkdmm2XJvEY9lDVeG0CMJEn4dNiPbyGkpE7ZWaTPRtd7EX6Zjt3_m3-P2i7jxg


Filer: ochang

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
Comment 3 by och...@chromium.org, Dec 16 2015
(small nit, the "input.focus();" isn't actually necessary).

tkent, there has been many past bugs with this pattern, and I'm sure there are/will be more... do you think there's any way that these types of bugs can be killed off once and for all? Can oilpan solve these kinds of use-after-frees where |this| is freed?

see bug 569170, bug 534990, and bug 544020, and bug 536917 for similar past bugs.
Project Member Comment 4 by clusterf...@chromium.org, Dec 16 2015
Labels: Stability-Memory-AddressSanitizer Security_Impact-Beta
Cc: haraken@chromium.org
Labels: -Security_Impact-Beta Security_Impact-Stable Pri-1 Security_Severity-High Cr-Blink-Input
Project Member Comment 6 by clusterf...@chromium.org, Dec 16 2015
Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6593516991414272

Uploader: ochang@google.com
Job Type: linux_asan_chrome_mp
Platform Id: linux

Crash Type: Heap-use-after-free READ 8
Crash Address: 0x60f00003b280
Crash State:
  blink::TextFieldInputType::didSetValueByUserEdit
  blink::TextFieldInputType::subtreeHasChanged
  blink::HTMLInputElement::subtreeHasChanged
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_mp&range=359427:359653

Minimized Testcase (0.20 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv97X6FRYr4Vm7-91ZSaT3n0vwRFujsvZriA1kRwodKZAZwxkpi_aEyw9DkIsiryGDeCCl8qdGGIqUlr6-b8kxMl1cknvOqmp5TxT_sLK90ykrXV61QterZEpSFaiVS7l7Kku1hN8jDdWgO2zok3caOB0mRviNw
<script>
input = document.createElement('input');
input.type = 'search'
input.setAttribute('incremental', '');
input.onsearch = function() {
  input.type = 'text';
}
input.setRangeText('', 0, 1);
</script>


See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
Comment 7 by tkent@chromium.org, Dec 16 2015
Labels: -Cr-Blink-Input Cr-Blink-Forms Cr-Blink-Forms-Search
#3, yes, Oilpan will eliminate this UAF pattern.

Comment 8 by och...@chromium.org, Dec 16 2015
Thanks tkent.

To clarify, if an object is only referenced by the |this| pointer for a method call, that reference will be considered by the GC, right? I can't imagine how that would be implemented, but is oilpan doing some magic here?
Comment 9 by tkent@chromium.org, Dec 16 2015
#8,
Yeah, Oilpan magically retains all objects pointed by CPU registers and stack.  They contains |this|.
Comment 10 by tkent@chromium.org, Dec 17 2015
Status: Started
Re #9: that's pretty cool :)

argh, just realised my mistake in my original comments. Ignore the UAF comment, it happens in a different place that what I pointed out.
(The UAF actually happens when we return from SearchInputType::startSearchEventTimer and call TextFieldInputType::didSetValueByUserEdit(state))
Project Member Comment 13 by clusterf...@chromium.org, Dec 17 2015
Labels: M-47
Comment 14 by tkent@chromium.org, Dec 17 2015
Cc: kochi@chromium.org
Comment 15 by kochi@chromium.org, Dec 17 2015
I'm still wondering why the stale pointer is used in the original bug report.

If the onsearch() handler changes the type, and SearchInputType instance
is deleted by deref'ing the last reference, then the timer is also deleted
and the timer won't fire?

Probably I am misunderstanding, as it is actually happening.
Sorry for the confusing report, the use-after-free is happening here:

void SearchInputType::didSetValueByUserEdit(ValueChangeState state)
{
    updateCancelButtonVisibility();

    // If the incremental attribute is set, then dispatch the search event
    if (searchEventsShouldBeDispatched())
        startSearchEventTimer();

    TextFieldInputType::didSetValueByUserEdit(state);
}

startSearchEventTimer() can free |this| as described in the original report. However, TextFieldInputType::didSetValueByUserEdit is then called, resulting in the UAF on |this|:

void TextFieldInputType::didSetValueByUserEdit(ValueChangeState state)
{
    if (!element().focused())
        return;
    if (ChromeClient* chromeClient = this->chromeClient())
        chromeClient->didChangeValueInTextField(element());
}


Comment 17 by tkent@chromium.org, Dec 17 2015
Summary: UaF in blink::SearchInputType::didSetValueByUserEdit (was: UaF in blink::SearchInputType::startSearchEventTimer)
Comment 18 by kochi@chromium.org, Dec 17 2015
I see, thanks for the explanation!
Project Member Comment 19 by bugdroid1@chromium.org, Dec 17 2015
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dd6acb021e38e8181684026899fdef556099e408

commit dd6acb021e38e8181684026899fdef556099e408
Author: tkent <tkent@chromium.org>
Date: Thu Dec 17 07:30:15 2015

Fix a crash by search event handler for <input type=search incremental>.

We had a crash in SearchInputType::didSetValueByUserEdit().

Dispatching a synchronous event here is dangerous. This CL makes the
non-standard 'search' event dispatching asynchronous in the following cases:
 - The input value becomes empty.
 - The form is submitted implicitly.
Basically 'search' event is triggered by input value changes, and is dispatched
asynchronously by a timer.  So, the behavior change should be acceptable.

BUG= 570427 
TEST=automated

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

Cr-Commit-Position: refs/heads/master@{#365773}

[modify] http://crrev.com/dd6acb021e38e8181684026899fdef556099e408/third_party/WebKit/LayoutTests/fast/events/onsearch-enter.html
[add] http://crrev.com/dd6acb021e38e8181684026899fdef556099e408/third_party/WebKit/LayoutTests/fast/forms/search/search-change-type-onsearch.html
[modify] http://crrev.com/dd6acb021e38e8181684026899fdef556099e408/third_party/WebKit/Source/core/html/HTMLInputElement.cpp
[modify] http://crrev.com/dd6acb021e38e8181684026899fdef556099e408/third_party/WebKit/Source/core/html/forms/SearchInputType.cpp

Status: Fixed
Project Member Comment 21 by clusterf...@chromium.org, Dec 17 2015
Labels: -Restrict-View-SecurityTeam Merge-Triage M-48 Restrict-View-SecurityNotify
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 22 by clusterf...@chromium.org, Dec 18 2015
ClusterFuzz has detected this issue as fixed in range 365683:366004.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6593516991414272

Uploader: ochang@google.com
Job Type: linux_asan_chrome_mp
Platform Id: linux

Crash Type: Heap-use-after-free READ 8
Crash Address: 0x60f00003b280
Crash State:
  blink::TextFieldInputType::didSetValueByUserEdit
  blink::TextFieldInputType::subtreeHasChanged
  blink::HTMLInputElement::subtreeHasChanged
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_mp&range=359427:359653
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_mp&range=365683:366004

Minimized Testcase (0.20 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv97X6FRYr4Vm7-91ZSaT3n0vwRFujsvZriA1kRwodKZAZwxkpi_aEyw9DkIsiryGDeCCl8qdGGIqUlr6-b8kxMl1cknvOqmp5TxT_sLK90ykrXV61QterZEpSFaiVS7l7Kku1hN8jDdWgO2zok3caOB0mRviNw
<script>
input = document.createElement('input');
input.type = 'search'
input.setAttribute('incremental', '');
input.onsearch = function() {
  input.type = 'text';
}
input.setRangeText('', 0, 1);
</script>


See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect,try re-doing that job on the test case report page.
Project Member Comment 23 by clusterf...@chromium.org, Dec 18 2015
Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6593516991414272

Uploader: ochang@google.com
Job Type: linux_asan_chrome_mp
Platform Id: linux

Crash Type: Heap-use-after-free READ 8
Crash Address: 0x60f00003b280
Crash State:
  blink::TextFieldInputType::didSetValueByUserEdit
  blink::TextFieldInputType::subtreeHasChanged
  blink::HTMLInputElement::subtreeHasChanged
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_mp&range=359427:359653
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_mp&range=365683:366004

Minimized Testcase (0.20 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv97X6FRYr4Vm7-91ZSaT3n0vwRFujsvZriA1kRwodKZAZwxkpi_aEyw9DkIsiryGDeCCl8qdGGIqUlr6-b8kxMl1cknvOqmp5TxT_sLK90ykrXV61QterZEpSFaiVS7l7Kku1hN8jDdWgO2zok3caOB0mRviNw
<script>
input = document.createElement('input');
input.type = 'search'
input.setAttribute('incremental', '');
input.onsearch = function() {
  input.type = 'text';
}
input.setRangeText('', 0, 1);
</script>


See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
Comment 24 by tkent@chromium.org, Dec 20 2015
Labels: -Merge-Triage Merge-Request-47 Merge-Request-48
Comment 25 by tin...@google.com, Dec 20 2015
Labels: -Merge-Request-47 Merge-Review-47 Hotlist-Merge-Review
[Automated comment] Request affecting a post-stable build (M47), manual review required.
Comment 26 by tin...@google.com, Dec 20 2015
Labels: -Merge-Request-48 Merge-Approved-48 Hotlist-Merge-Approved
Congrats your change is auto-approved for M48 (branch: 2564)
Project Member Comment 27 by bugdroid1@chromium.org, Dec 21 2015
Labels: -Merge-Approved-48 merge-merged-2564
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/58383a4c517b802a25ccc41b2a2219332a81e5a6

commit 58383a4c517b802a25ccc41b2a2219332a81e5a6
Author: Kent Tamura <tkent@chromium.org>
Date: Mon Dec 21 00:47:52 2015

Fix a crash by search event handler for <input type=search incremental>.

We had a crash in SearchInputType::didSetValueByUserEdit().

Dispatching a synchronous event here is dangerous. This CL makes the
non-standard 'search' event dispatching asynchronous in the following cases:
 - The input value becomes empty.
 - The form is submitted implicitly.
Basically 'search' event is triggered by input value changes, and is dispatched
asynchronously by a timer.  So, the behavior change should be acceptable.

BUG= 570427 
TEST=automated

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

Cr-Commit-Position: refs/heads/master@{#365773}
(cherry picked from commit dd6acb021e38e8181684026899fdef556099e408)

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

Cr-Commit-Position: refs/branch-heads/2564@{#405}
Cr-Branched-From: 1283eca15bd9f772387f75241576cde7bdec7f54-refs/heads/master@{#359700}

[modify] http://crrev.com/58383a4c517b802a25ccc41b2a2219332a81e5a6/third_party/WebKit/LayoutTests/fast/events/onsearch-enter.html
[add] http://crrev.com/58383a4c517b802a25ccc41b2a2219332a81e5a6/third_party/WebKit/LayoutTests/fast/forms/search/search-change-type-onsearch.html
[modify] http://crrev.com/58383a4c517b802a25ccc41b2a2219332a81e5a6/third_party/WebKit/Source/core/html/HTMLInputElement.cpp
[modify] http://crrev.com/58383a4c517b802a25ccc41b2a2219332a81e5a6/third_party/WebKit/Source/core/html/forms/SearchInputType.cpp

Project Member Comment 28 by bugdroid1@chromium.org, Dec 22 2015
The following revision refers to this bug:
  https://chrome-internal.googlesource.com/bling/chromium.git/+/58383a4c517b802a25ccc41b2a2219332a81e5a6

commit 58383a4c517b802a25ccc41b2a2219332a81e5a6
Author: Kent Tamura <tkent@chromium.org>
Date: Mon Dec 21 00:47:52 2015

Labels: -Merge-Review-47 -Hotlist-Merge-Review
Remove a merge-request for M47 because the fix caused another issue.

Cc: yosin@chromium.org
Labels: Release-0-M48
Project Member Comment 32 by sheriffbot@chromium.org, Mar 24 2016
Labels: -Restrict-View-SecurityNotify
This security bug has been closed for more than 14 weeks. Removing view restrictions.

For more details visit https://sites.google.com/a/chromium.org/dev/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member Comment 33 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 34 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