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 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac , Fuchsia
Pri: 0
Type: Bug-Security



Sign in to add a comment
link

Issue 827492: Security: In-memory Cache UaF

Reported by nedwilli...@gmail.com, Mar 30 2018

Issue description

VULNERABILITY DETAILS
Incognito mode uses an in-memory cache to avoid writing to disk like the blockfile and simple caches. This mode suffers from an issue with sparse writes similar to the bug reported in  crbug.com/826626 , but the issue is limited to a single function.

See the following snippet:

```
void MemBackendImpl::EvictIfNeeded() {
  if (current_size_ <= max_size_)
    return;

  int target_size = std::max(0, max_size_ - kDefaultEvictionSize);

  base::LinkNode<MemEntryImpl>* entry = lru_list_.head();
  while (current_size_ > target_size && entry != lru_list_.end()) {
    MemEntryImpl* to_doom = entry->value();
    entry = entry->next();
    if (!to_doom->InUse())
      to_doom->Doom();
  }
}
```

Eviction traverses the list of entries in the lru_list_. The line `entry = entry->next()` keeps a pointer to the next entry to process while we potentially doom the current entry. There's a problem here: if the next entry is a child of the current entry and neither is in use, the entry pointer on the next iteration will be stale, and dooming it again will lead to a double-doom which manifests as use-after-free.

My attached patch supplies a sample fix for the issue and a unit test to demonstrate it. I resolve the issue by resetting the entry to the lru_list_ head when we encounter a doomable parent. It might be improved by also checking if children are present.

VERSION
Chrome Version: 65 Stable
Operating System: All (? wherever in-memory cache is deployed)

REPRODUCTION CASE
See attachments

FOR CRASHES, PLEASE INCLUDE THE FOLLOWING ADDITIONAL INFORMATION
Type of crash: Browser
Crash State: See asan.log
 
inmemory_eviction.patch
2.5 KB Download
asan.log
10.3 KB View Download

Comment 1 by elawrence@chromium.org, Mar 30 2018

Components: Internals>Network>Cache
Thanks, again!

Comment 2 by mmoroz@chromium.org, Mar 30 2018

Cc: jcivelli@chromium.org mmoroz@chromium.org xunji...@chromium.org mmenke@chromium.org morlovich@chromium.org
Labels: Security_Severity-Critical M-66 Security_Impact-Stable OS-Android OS-Chrome OS-Fuchsia OS-iOS OS-Linux OS-Mac OS-Windows Pri-0
Owner: jkarlin@chromium.org
Status: Assigned (was: Unconfirmed)
jkarlin@, could you please take a look? I can't see a better owner using git blame.

morlovich@, might be helpful, as he's been recently fixing a similar bug in a blockfile cache.

------

Hey Ned, how many of those you else have? :)

Comment 3 by mmoroz@chromium.org, Mar 30 2018

Labels: reward-topanel ReleaseBlock-Beta

Comment 4 by morlovich@chromium.org, Mar 30 2018

Josh, feel free to dump it on me if you're too busy (though it's all that similar --- if you want similar, see https://codereview.chromium.org/1725363005)

Comment 5 by jkarlin@chromium.org, Mar 30 2018

Status: Started (was: Assigned)
I can take this one. Thanks for the report Ned!

Comment 6 by nedwilli...@gmail.com, Mar 30 2018

Sure thing guys! Don't forget to add CacheTestFillBuffer in the unittest to avoid an uninitialized read.. security is hard :p

See https://chromium.googlesource.com/chromium/src.git/+/dd44592937e7d78c7c75489d5a6e9bb0b4967f4a

@mmoroz: maybe one more, but I think these two were the worst. I have one with DoomAllEntries as well but I'm trying to figure out if there's a variant without user interaction. I'm not blocking on it though; I'll figure it out as I'm writing the next report.

Comment 7 by nedwilli...@gmail.com, Mar 30 2018

BTW, I thought about a better patch this morning: if the current entry is a parent and next is one of the children, either reset the next head or just move it forward to a non-child. This prevents needlessly rescanning the list and only does extra work in the exact buggy case.

Comment 8 by nedwilli...@gmail.com, Mar 30 2018

"reset the next head" should read "reset the current entry to head" like in the original patch. Moving next forward until it finds a non-child should be better anyways since we always close the children if the parent is to be closed, so skipping those entries shouldn't miss any doomable entries.

Comment 10 by nedwilli...@gmail.com, Mar 30 2018

Ha, great minds... ;)

Thanks for the quick fixes on these! I know you guys have a lot of responsibility and these types of bugs can interfere with daily development.

Comment 11 by bugdroid1@chromium.org, Mar 30 2018

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

commit c9d673b54832afde658f214d7da7d0453fa89774
Author: Josh Karlin <jkarlin@chromium.org>
Date: Fri Mar 30 19:24:28 2018

[MemCache] Fix bug while iterating LRU list in eviction

It was possible to reanalyze a previously doomed entry.

Bug:  827492 
Change-Id: I5d34d2ae87c96e0d2099e926e6eb2c1b30b01d63
Reviewed-on: https://chromium-review.googlesource.com/987919
Commit-Queue: Josh Karlin <jkarlin@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547236}
[modify] https://crrev.com/c9d673b54832afde658f214d7da7d0453fa89774/net/disk_cache/backend_unittest.cc
[modify] https://crrev.com/c9d673b54832afde658f214d7da7d0453fa89774/net/disk_cache/memory/mem_backend_impl.cc

Comment 12 by jkarlin@chromium.org, Apr 2 2018

Labels: Merge-Request-66
Looks good over the weekend. Requesting beta merge.

Comment 13 by sheriffbot@chromium.org, Apr 2 2018

Project Member
Labels: -Merge-Request-66 Merge-Review-66 Hotlist-Merge-Review
This bug requires manual review: Less than 11 days to go before AppStore submit on M66
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 14 by sheriffbot@chromium.org, Apr 2 2018

Project Member
Status: Fixed (was: Started)
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 15 by abdulsyed@chromium.org, Apr 2 2018

Labels: -Merge-Review-66 Merge-Approved-66
Branch:3359

Comment 16 by bugdroid1@chromium.org, Apr 2 2018

Project Member
Labels: -merge-approved-66 merge-merged-3359
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c1c8fd65cc3100148f6a4b2f203312a741d09b9e

commit c1c8fd65cc3100148f6a4b2f203312a741d09b9e
Author: Josh Karlin <jkarlin@chromium.org>
Date: Mon Apr 02 18:06:43 2018

[MemCache] Fix bug while iterating LRU list in eviction

It was possible to reanalyze a previously doomed entry.

Bug:  827492 
Change-Id: I5d34d2ae87c96e0d2099e926e6eb2c1b30b01d63
Reviewed-on: https://chromium-review.googlesource.com/987919
Commit-Queue: Josh Karlin <jkarlin@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#547236}(cherry picked from commit c9d673b54832afde658f214d7da7d0453fa89774)
Reviewed-on: https://chromium-review.googlesource.com/990372
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#531}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/c1c8fd65cc3100148f6a4b2f203312a741d09b9e/net/disk_cache/backend_unittest.cc
[modify] https://crrev.com/c1c8fd65cc3100148f6a4b2f203312a741d09b9e/net/disk_cache/memory/mem_backend_impl.cc

Comment 17 by sheriffbot@chromium.org, Apr 3 2018

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

Comment 18 by awhalley@google.com, Apr 11 2018

Labels: -ReleaseBlock-Beta

Comment 19 by mmoroz@chromium.org, Apr 11 2018

Cc: kcc@chromium.org

Comment 20 by awhalley@google.com, Apr 17 2018

Labels: Release-0-M66

Comment 21 by awhalley@chromium.org, Apr 20 2018

Labels: -reward-topanel reward-unpaid reward-10500
*** Boilerplate reminders! ***
Please do NOT publicly disclose details until a fix has been released to all our users. Early public disclosure may cancel the provisional reward. Also, please be considerate about disclosure when the bug affects a core library that may be used by other products. Please do NOT share this information with third parties who are not directly involved in fixing the bug. Doing so may cancel the provisional reward. Please be honest if you have already disclosed anything publicly or to third parties. Lastly, we understand that some of you are not interested in money. We offer the option to donate your reward to an eligible charity. If you prefer this option, let us know and we will also match your donation - subject to our discretion. Any rewards that are unclaimed after 12 months will be donated to a charity of our choosing.
*********************************

Comment 22 by awhalley@google.com, Apr 20 2018

Thanks Ned, $10,500 for the report :-)

Comment 23 by awhalley@chromium.org, Apr 20 2018

Labels: -reward-unpaid reward-inprocess

Comment 24 by nedwilli...@gmail.com, Apr 20 2018

Thank you!

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

Labels: CVE-2018-6086

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

Labels: CVE_description-missing

Comment 27 by sheriffbot@chromium.org, Jul 10 2018

Project Member
Labels: -Restrict-View-SecurityNotify allpublic
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 awhalley@chromium.org, Dec 4

Labels: -CVE_description-missing CVE_description-submitted

Sign in to add a comment