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

Issue metadata

Status: Fixed
Merged: issue 608100
Owner:
hobby only
Closed: May 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment
link

Issue 608101: Security: Heap-use-after-free in autofill components

Reported by rob@robwu.nl, Apr 29 2016 Project Member

Issue description

Chrome version: 50.0.2661.75

There are multiple UAF vulnerabilities in the autofill components, caused by the fact that autofill is implemented by calling WebFormControlElement::setValue with sendEvents=true. As a result, whenever a form field is auto-filled, the input event listeners of the web page are triggered, which can invalidate the frame, destruct the RenderFrameObserver and result in a UAF.

The attached PoC offers simple instructions to reproduce the UAF: Generate autofill data, and choose a suggested autofill item.

 
Vulnerable locations:
- https://chromium.googlesource.com/chromium/src/+/5bc61f8f605564730b06f14e243681e930c7cb88/components/autofill/content/renderer/form_autofill_util.cc#894

- https://chromium.googlesource.com/chromium/src/+/888a550ad227b3546baa480c386640e206f36319/components/autofill/content/renderer/form_cache.cc#215
- https://chromium.googlesource.com/chromium/src/+/888a550ad227b3546baa480c386640e206f36319/components/autofill/content/renderer/form_cache.cc#224
- https://chromium.googlesource.com/chromium/src/+/888a550ad227b3546baa480c386640e206f36319/components/autofill/content/renderer/form_cache.cc#232

- https://chromium.googlesource.com/chromium/src/+/48c8ef0f7d7dccfbf818c289c771ca2ddaece9aa/components/autofill/content/renderer/password_autofill_agent.cc#502
- https://chromium.googlesource.com/chromium/src/+/48c8ef0f7d7dccfbf818c289c771ca2ddaece9aa/components/autofill/content/renderer/password_autofill_agent.cc#608
- https://chromium.googlesource.com/chromium/src/+/48c8ef0f7d7dccfbf818c289c771ca2ddaece9aa/components/autofill/content/renderer/password_autofill_agent.cc#681 (=PoC)
- https://chromium.googlesource.com/chromium/src/+/48c8ef0f7d7dccfbf818c289c771ca2ddaece9aa/components/autofill/content/renderer/password_autofill_agent.cc#807
- https://chromium.googlesource.com/chromium/src/+/48c8ef0f7d7dccfbf818c289c771ca2ddaece9aa/components/autofill/content/renderer/password_autofill_agent.cc#812

- https://chromium.googlesource.com/chromium/src/+/578305d33ed2580610f3cdbb8ca529dfc3faf195/components/autofill/content/renderer/password_generation_agent.cc#96
- https://chromium.googlesource.com/chromium/src/+/578305d33ed2580610f3cdbb8ca529dfc3faf195/components/autofill/content/renderer/password_generation_agent.cc#277
 

Comment 1 by rsesek@chromium.org, May 2 2016

Cc: isherman@chromium.org
Labels: Security_Severity-High Security_Impact-Stable M-50
Owner: vabr@chromium.org
Status: Assigned (was: Unconfirmed)
Seems like a general variant of  issue 608100 .

Comment 2 by rsesek@chromium.org, May 2 2016

Labels: Pri-1

Comment 3 by isherman@chromium.org, May 2 2016

Cc: ma...@chromium.org

Comment 4 by rob@robwu.nl, May 2 2016

@resek (comment 1)
Sorry for the confusion, I attached the test cases and stack traces to the wrong report.

This bug differs from  bug 608100  in that their root causes are different: This one could be fixed by making sure that WebFormControlElement::setValue does not synchronously trigger events in the web page, the other bug has to be resolved in a different way.

I've deleted the test case and log file from the initial report, see  bug 608100  if you want to see it anyway.
asan-autofill-auto-50.0.2661.75.log
19.8 KB View Download
uaf-autofill-auto.html
1.3 KB View Download

Comment 5 by vabr@chromium.org, May 3 2016

Mergedinto: 608100
Status: Duplicate (was: Assigned)
To fix the related  bug 608100  I am postponing the deletion of the agent in https://codereview.chromium.org/1943873002/. It seems to me that this postponing will also fix this issue. Therefore I'll mark this one as a duplicate of  issue 608100 . Please speak up if you disagree.

Comment 6 by rob@robwu.nl, May 3 2016

Status: Assigned (was: Duplicate)
Re-opening because the patch for the other bug did not fix this bug. You only addressed two of the 4 files.


FormCache::ClearFormWithElement [1] is still vulnerable to UAF. I didn't build master with your patch, because the ASAN trace clearly shows that the UAF occurs before control is returned to the classes that you fixed.
See asan-autofill-clear-email-50.0.2661.94.log and uaf-autofill-clear-email.html for a UAF.
See asan-autofill-set-email-50.0.2661.94.log and npe-autofill-set-email.html for a nullptr dereference.

PasswordGenerationAgent also inherits from a RenderFrameObserver, so it is also vulnerable (I didn't bother with a test case yet, feel free to ask).


[1] https://chromium.googlesource.com/chromium/src/+/5bc61f8f605564730b06f14e243681e930c7cb88/components/autofill/content/renderer/form_autofill_util.cc#894
uaf-autofill-clear-email.html
1.1 KB View Download
asan-autofill-clear-email-50.0.2661.94.log
20.0 KB View Download
npe-autofill-set-email.html
876 bytes View Download
asan-autofill-set-email-50.0.2661.94.log
6.6 KB View Download

Comment 7 by bugdroid1@chromium.org, May 4 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/50bf9075cac0e5d69f5adfc0763968d5afa1734b

commit 50bf9075cac0e5d69f5adfc0763968d5afa1734b
Author: vabr <vabr@chromium.org>
Date: Wed May 04 09:13:51 2016

Revert of Postpone deletion of (Password)AutofillAgent (patchset #1 id:1 of https://codereview.chromium.org/1943873002/ )

Reason for revert:
This fix is not complete, I need to also handle disabling the LegacyAutofillAgent.

Because the final fix is going to be merged, I prefer to have the complete solution in one CL. Therefore I am reverting this partial fix, and will reintroduce it in a new CL. Sorry for the noise.

BUG=609010, 609007,  608100 ,  608101 

Original issue's description:
> Postpone deletion of (Password)AutofillAgent
>
> Currently, the two agent classes delete themselves immediately on destruction
> of the RenderFrame they are observing. This CL postpones the deletion to a
> separately posted task, to avoid the situation when the agent is deleted while
> still having methods in progress lower on the stack.
>
> BUG= 608100 
>
> Committed: https://crrev.com/a8755e432460c9412291c0ae4dd887babb3fa506
> Cr-Commit-Position: refs/heads/master@{#391236}

TBR=mathp@chromium.org,dvadym@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 608100 

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

[modify] https://crrev.com/50bf9075cac0e5d69f5adfc0763968d5afa1734b/components/autofill/content/renderer/autofill_agent.cc
[modify] https://crrev.com/50bf9075cac0e5d69f5adfc0763968d5afa1734b/components/autofill/content/renderer/autofill_agent.h
[modify] https://crrev.com/50bf9075cac0e5d69f5adfc0763968d5afa1734b/components/autofill/content/renderer/password_autofill_agent.cc
[modify] https://crrev.com/50bf9075cac0e5d69f5adfc0763968d5afa1734b/components/autofill/content/renderer/password_autofill_agent.h

Comment 8 by bugdroid1@chromium.org, May 4 2016

Project Member

Comment 9 by rob@robwu.nl, May 4 2016

Vaclav, the last version (patchset 4 of https://codereview.chromium.org/1946143002/) does not fix the UAF from #6, at
https://chromium.googlesource.com/chromium/src/+/888a550ad227b3546baa480c386640e206f36319/components/autofill/content/renderer/form_cache.cc#219

It seems that |element| points to invalid memory (assuming that the left hand side of == is evaluated first):

Thread 27 "Chrome_InProcRe" hit Breakpoint 3, ClearFormWithElement () at ../../components/autofill/content/renderer/form_cache.cc:190
.......
215           input_element->setValue(base::string16(), true);
(gdb) 
219           if (element == *input_element) {
(gdb) s
operator== () at ../../third_party/WebKit/public/web/WebNode.h:163
163         return a.equals(b);
(gdb) s
equals () at ../../third_party/WebKit/Source/web/WebNode.cpp:123
123         return m_private.get() == n.m_private.get();
(gdb) s
get () at ../../third_party/WebKit/Source/web/WebNode.cpp:123
123         return m_private.get() == n.m_private.get();
(gdb) s
get () at ../../third_party/WebKit/public/web/../platform/WebPrivatePtr.h:162
162         T* get() const { return m_handle ? m_handle->get() : 0; }
(gdb) s
=================================================================
==5436==ERROR: AddressSanitizer: heap-use-after-free on address 0x6160004a6978 at pc 0x55555c8fb193 bp 0x7fffbcb909e0 sp 0x7fffbcb909d8



And you should also add a check after this line, or the next line is going to have a nullptr deference at best, or a UAF at worst:
https://chromium.googlesource.com/chromium/src/+/5bc61f8f605564730b06f14e243681e930c7cb88/components/autofill/content/renderer/form_autofill_util.cc#903

Comment 10 by bugdroid1@chromium.org, May 4 2016

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

commit d62bc3e6e2c3be6bbb01fa325e3389f089974017
Author: vabr <vabr@chromium.org>
Date: Wed May 04 15:58:52 2016

Destroy (Password)AutofillAgent safely

AutofillAgent and related code often edits field values. Those edits may trigger JavaScript capable of deleting the associated frame. Currently, AutofillAgent and related classes are RenderFrameObservers and delete themselves on the frame deletion. This results in use-after-free if the deletion happens up in the stack and there is still the method which changed the field value down on the stack.

Therefore this CL postpones deletion by sending a DeleteSoon task on the frame destruction. The CL also changes a couple of places relying on render frame being alive if the observer is alive to handle a null frame gratefully.

R=dvadym@chromium.org
BUG=609010,609007, 608100 , 608101 

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

[modify] https://crrev.com/d62bc3e6e2c3be6bbb01fa325e3389f089974017/components/autofill/content/renderer/autofill_agent.cc
[modify] https://crrev.com/d62bc3e6e2c3be6bbb01fa325e3389f089974017/components/autofill/content/renderer/autofill_agent.h
[modify] https://crrev.com/d62bc3e6e2c3be6bbb01fa325e3389f089974017/components/autofill/content/renderer/form_autofill_util.cc
[modify] https://crrev.com/d62bc3e6e2c3be6bbb01fa325e3389f089974017/components/autofill/content/renderer/password_autofill_agent.cc
[modify] https://crrev.com/d62bc3e6e2c3be6bbb01fa325e3389f089974017/components/autofill/content/renderer/password_autofill_agent.h
[modify] https://crrev.com/d62bc3e6e2c3be6bbb01fa325e3389f089974017/components/autofill/content/renderer/password_generation_agent.cc
[modify] https://crrev.com/d62bc3e6e2c3be6bbb01fa325e3389f089974017/components/autofill/content/renderer/password_generation_agent.h

Comment 11 by bugdroid1@chromium.org, May 9 2016

Project Member
Labels: merge-merged-2704
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7e112c1563632f57cfa1c4fa964987f823da17fa

commit 7e112c1563632f57cfa1c4fa964987f823da17fa
Author: Vaclav Brozek <vabr@chromium.org>
Date: Mon May 09 07:18:24 2016

Destroy (Password)AutofillAgent safely

AutofillAgent and related code often edits field values. Those edits may trigger JavaScript capable of deleting the associated frame. Currently, AutofillAgent and related classes are RenderFrameObservers and delete themselves on the frame deletion. This results in use-after-free if the deletion happens up in the stack and there is still the method which changed the field value down on the stack.

Therefore this CL postpones deletion by sending a DeleteSoon task on the frame destruction. The CL also changes a couple of places relying on render frame being alive if the observer is alive to handle a null frame gratefully.

R=dvadym@chromium.org
BUG=609010,609007, 608100 , 608101 

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

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

Cr-Commit-Position: refs/branch-heads/2704@{#441}
Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251}

[modify] https://crrev.com/7e112c1563632f57cfa1c4fa964987f823da17fa/components/autofill/content/renderer/autofill_agent.cc
[modify] https://crrev.com/7e112c1563632f57cfa1c4fa964987f823da17fa/components/autofill/content/renderer/autofill_agent.h
[modify] https://crrev.com/7e112c1563632f57cfa1c4fa964987f823da17fa/components/autofill/content/renderer/form_autofill_util.cc
[modify] https://crrev.com/7e112c1563632f57cfa1c4fa964987f823da17fa/components/autofill/content/renderer/password_autofill_agent.cc
[modify] https://crrev.com/7e112c1563632f57cfa1c4fa964987f823da17fa/components/autofill/content/renderer/password_autofill_agent.h
[modify] https://crrev.com/7e112c1563632f57cfa1c4fa964987f823da17fa/components/autofill/content/renderer/password_generation_agent.cc
[modify] https://crrev.com/7e112c1563632f57cfa1c4fa964987f823da17fa/components/autofill/content/renderer/password_generation_agent.h

Comment 12 by ClusterFuzz, May 9 2016

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

- Your friendly ClusterFuzz

Comment 13 by rob@robwu.nl, May 9 2016

Status: Started (was: Fixed)
Re-opening since there are still unpatched vulnerabilities, see comment 6 and 9.

Comment 14 by vabr@chromium.org, May 9 2016

Status: Assigned (was: Started)
(I will be looking into this more, am just swamped at the moment.)

Comment 15 by rob@robwu.nl, May 9 2016

Status: Fixed (was: Assigned)
I just tested with ASAN on HEAD (52.0.2730.0) and the test case from comment 6 does not crash any more.

In the test case in comment 6 I overlooked that |element| in FormCache::ClearFormWithElement is owned by AutofillAgent. Your fix of deferrring the AutofillAgent destruction means that |element| is no longer a dangling pointer. so the issue as a whole has been fixed.

Comment 16 by vabr@chromium.org, May 10 2016

Thanks for the update!

I still think you are correct with the last sentence in #9 -- it is apparently possible to listen to selection changes, so the existence of the frame on line 905 should be checked. https://chromium.googlesource.com/chromium/src/+/5bc61f8f605564730b06f14e243681e930c7cb88/components/autofill/content/renderer/form_autofill_util.cc#905

I'll try to find time to put up a fix and ideally a test (I'm still owing to supply the tests for the already landed patch as well). It is still a bit fragile, though, anybody can introduce this kind of vulnerability in the future, I wonder what to do to prevent such interleaving of JavaScript and C++ code execution.

Comment 17 by rob@robwu.nl, May 10 2016

#16
At first I thought that setSelectionRange would trigger JS, but when I took a closer look it appears that WebFormControlElement::setSelectionRange does not dispatch events (and even if it did, then not synchronously), so there should not be any issue at that line.

You did a pretty good job at fixing it.


About preventing this kind of vulnerability: Blink has a (non-public) ScriptForbiddenScope object, which prevents script from running while the ScriptForbiddenScope is active. This could be used to eagerly detect such issues.

A general way to avoid side effects due to unexpected events is to not dispatch them synchronously, but schedule them for later (like setSelectionRange does).

Comment 18 by sheriffbot@chromium.org, May 10 2016

Project Member
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 19 by vabr@chromium.org, May 10 2016

Thanks for the helpful answer (and for the great reports, by the way!).

Asynchronous dispatching looks indeed like a good option. But setSelectionRange actually seems to dispatch no events at all? I guess I'll need to understand more of how Blink works here before being able to fix this properly.

Comment 20 by timwillis@google.com, May 24 2016

Labels: -merge-merged-2704 Merge-Triage reward-topanel M-51
Status: Started (was: Fixed)
Marking this as "started" as it appears as though there is more work to be done here.

Comment 21 by ClusterFuzz, May 24 2016

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

- Your friendly ClusterFuzz

Comment 22 by vabr@chromium.org, May 25 2016

Labels: -Merge-Triage merge-merged-2704
#20 -- No, sorry for my confusing phrasing. There is no more work to be done to fix this issue. The note in #19 is about preventing similar stuff in the future.

Comment 23 by timwillis@google.com, May 31 2016

Labels: Release-1-M51

Comment 24 by timwillis@google.com, Jun 6 2016

Labels: -Security_Severity-High Security_Severity-Medium
Updating severity due to required interaction.

Comment 25 by timwillis@google.com, Jun 6 2016

Labels: -reward-topanel reward-1000 reward-unpaid CVE-2016-1701
$1,000 here as noted in the release notes. Panel provided a reduced amount due to the amount of interaction required.

I'll put this in the system this week for payment.

Comment 26 by timwillis@google.com, Jun 8 2016

Labels: -reward-unpaid reward-inprocess

Comment 27 by sheriffbot@chromium.org, Aug 31 2016

Project Member
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

Comment 28 by sheriffbot@chromium.org, Oct 1 2016

Project Member
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

Comment 29 by sheriffbot@chromium.org, Oct 2 2016

Project Member
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

Comment 30 by mbarbe...@chromium.org, Oct 2 2016

Labels: allpublic

Comment 31 by awhalley@chromium.org, Apr 25 2018

Labels: CVE_description-submitted

Sign in to add a comment