Hi, thanks for your report. unfortunately we cannot decrypt the GPG attachment because it seems to be encrypted to the wrong key.

please feel free to re-encrypt with the security@google.com or security@chromium.org (we can handle either/both)

https://pgp.mit.edu/pks/lookup?op=get&search=0xB8E4105CC9DEDC77 (security@google.com)
https://pgp.mit.edu/pks/lookup?op=get&search=0xA73851D532C206B6 (security@chromium.org)

Ned has kindly offered to help with triage of this bug while we wait for the report to come through decrypted.

Attaching it in unencrypted form

Deleted: chromesbx.zip 80.2 KB

chromesbx.zip 80.2 KB Download

This is a critical security issue. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

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

jsbell@, could you please take a look or help to find an owner?

I suppose this affects all platforms.

Here is the writeup from inside the ZIP. Please address this pri-0 critical bug as soon as possible.

Vulnerability:

The vulnerability exists in the AppCache subsystem in Chrome.
This code is located in the privileged browser process outside
of the sandbox. The renderer interacts with this subsystem by
sending IPC messages from the renderer to the browser process.
These messages can cause the browser to make network requests,
which are also attacker-controlled and influence the behavior
of the code.

See the following snippet from AppCacheGroup::RemoveCache:

```
void AppCacheGroup::RemoveCache(AppCache* cache) {
  DCHECK(cache->associated_hosts().empty());
  if (cache == newest_complete_cache_) {
    CancelUpdate(); // buggy call here
    AppCache* tmp_cache = newest_complete_cache_;
    newest_complete_cache_ = nullptr;
    tmp_cache->set_owning_group(nullptr);  // may cause this group to be deleted
  } else {
```

Notice the `AppCache* cache` paramter. This is a reference-counted
object, and this `RemoveCache` function can be called during the
`AppCache` destructor. The `CancelUpdate` can re-entrantly add a
reference to the AppCache that's passed in which is currently being
destroyed. When that pointer is later accessed, a use after free
occurs.

Here's how the reference count be incremented; recall newest_complete_cache_
has 0 references and is in the process of destruction.

`CancelUpdate` deletes an in progress `AppCacheUpdateJob`.

```
void AppCacheGroup::CancelUpdate() {
  if (update_job_) {
    delete update_job_;
    // The update_job_ destructor calls AppCacheGroup::SetUpdateAppCacheStatus
    // which zeroes update_job_.
    DCHECK(!update_job_);
    DCHECK_EQ(IDLE, update_status_);
  }
}
```

The AppCacheUpdateJob destructor calls `SetUpdateAppCacheStatus`.

```
AppCacheUpdateJob::~AppCacheUpdateJob() {
  if (service_)
    service_->RemoveObserver(this);
  if (internal_state_ != COMPLETED)
    Cancel();

  DCHECK(!inprogress_cache_.get());
  DCHECK(pending_master_entries_.empty());

  // The job must not outlive any of its fetchers.
  CHECK(!manifest_fetcher_);
  CHECK(pending_url_fetches_.empty());
  CHECK(master_entry_fetches_.empty());

  if (group_)
    group_->SetUpdateAppCacheStatus(AppCacheGroup::IDLE);
}
```

In `SetUpdateAppCacheStatus`, if the job was not idle, we call
`observer.OnUpdateComplete`, on every `AppCacheHost` currently
observing the `AppCacheGroup`.

```
void AppCacheGroup::SetUpdateAppCacheStatus(UpdateAppCacheStatus status) {
  if (status == update_status_)
    return;

  update_status_ = status;

  if (status != IDLE) {
    DCHECK(update_job_);
  } else {
    update_job_ = nullptr;

    // Observers may release us in these callbacks, so we protect against
    // deletion by adding an extra ref in this scope (but only if we're not
    // in our destructor).
    scoped_refptr<AppCacheGroup> protect(is_in_dtor_ ? nullptr : this);
    for (auto& observer : observers_)
      observer.OnUpdateComplete(this);
```

Now finally, recall that `newest_complete_cache_` is currently being
destroyed, but is not yet NULL. This means for as many hosts as we
add as observers for this AppCacheGroup we can add an arbitrary number
of references to the destroyed object.

```
void AppCacheHost::OnUpdateComplete(AppCacheGroup* group) {
  DCHECK_EQ(group, group_being_updated_.get());
  group->RemoveUpdateObserver(this);

  // Add a reference to the newest complete cache.
  SetSwappableCache(group);
```

```
void AppCacheHost::SetSwappableCache(AppCacheGroup* group) {
  if (!group) {
    swappable_cache_ = nullptr;
  } else {
    AppCache* new_cache = group->newest_complete_cache();
    if (new_cache != associated_cache_.get())
      swappable_cache_ = new_cache;
```

Reproducing the Issue:

To reproduce the issue, apply the attached `renderer-271eaf.patch`,
build with ASAN, run the server located at `server/.py`, and browse
to `localhost:8000`. I believe this bug can be fixed by modifying
`AppCacheGroup::RemoveCache` to move the `CancelUpdate()` call to
after setting `newest_complete_cache_ = nullptr;`. This way the
invalid newest_complete_cache_ won't be accessed when setting a new
swappable cache.

Exploit Technique:

This bug provides us two essential primitives: use-after-free
decrement-by-N of the first dword of the freed object, where
N is controlled. If in the process of decrementing, the first
dword reaches 0, the AppCache destructor is called and the
pointer is freed.

We use these primitives in two stages: first, to construct a leak,
and second, to trigger code execution.

The freed AppCache object has size 0xA0 bytes. We found that
`net::CanonicalCookie` has the same size, so we can spray cookies
in the browser process by making a network request and including
cookies in the reseponse.

`std::string name` is the first object in the CanonicalCookie. This
name is the key from the key/value pair `name=value` from the cookie
string. On Windows STL, the first qword of a std::string object is a
pointer to the string data. By using decrement-by-N, we leak a number
of bytes by reading the cookie back from the browser and scanning the
`name` field. This leak gives us a heap address, which allows us to
spray the heap and predictably place controlled data at a now-known
address.

To achieve code execution, we produce a single dangling reference to
a freed AppCache via the described vulnerability. We reclaim it with
a blob of the same size, forging a reference count of 1 and a fake
AppCacheGroup with reference count 0. Once we remove the dangling
reference and enter the AppCache destructor, the else branch of the
RemoveCache method will cause the AppCacheGroup to be freed due to its
reference count going from 0 to 1 and back to 0.

```
void AppCacheGroup::RemoveCache(AppCache* cache) {
  DCHECK(cache->associated_hosts().empty());
  if (cache == newest_complete_cache_) {
    // ...
  } else {
    scoped_refptr<AppCacheGroup> protect(this);
    // ...
  }
}
```

The AppCacheGroup destructor in turn performs a virtual call, which
we fully control.

```
AppCacheGroup::~AppCacheGroup() {
  // ...
  if (update_job_)
    delete update_job_; // <- code execution here
}
```

Due to the once-per-boot ASLR approach of Windows, all modules are loaded
at the same address in the renderer and broker process. We use a gadget
from __longjmp_internal to bootstrap the ROP. From there we can either
jump to shellcode or open notepad.


Credit:
Ned Williamson (Bug, Exploit)
Niklas Baumstark (Exploit)

I think this was missing from the writeup, but I think the bug was introduced here : https://chromium.googlesource.com/chromium/src/+/22e5bbdd259f9ca5393dfd32a6f8fc02f7c600a0

Moving the CancelUpdate call to after `newest_complete_cache_ = nullptr;` fixed the bug for me but I didn't check whether or not that introduces a regression.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9d2ead1650a1c901754dd1a68705006a6934cffc

commit 9d2ead1650a1c901754dd1a68705006a6934cffc
Author: Chris Palmer <palmer@chromium.org>
Date: Wed Sep 26 21:21:29 2018

Refcount AppCacheGroup correctly.

Bug:  888926 
Change-Id: Iab0d82d272e2f24a5e91180d64bc8e2aa8a8238d
Reviewed-on: https://chromium-review.googlesource.com/1246827
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Commit-Queue: Chris Palmer <palmer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594475}
[modify] https://crrev.com/9d2ead1650a1c901754dd1a68705006a6934cffc/content/browser/appcache/appcache_group.cc

Is this Fixed? Does it need a merge?

Oh sorry, yes and yes.

This bug requires manual review: Less than 14 days to go before AppStore submit on M70
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

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

abdulsyed@ - good for 70

Approving for M70. branch:3538

There are no respins planned for M69 yet.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8b3539517f8a751cdba9f5276fb060a23b05053e

commit 8b3539517f8a751cdba9f5276fb060a23b05053e
Author: Marijn Kruisselbrink <mek@chromium.org>
Date: Fri Sep 28 16:49:07 2018

Refcount AppCacheGroup correctly.

TBR=palmer@chromium.org

(cherry picked from commit 9d2ead1650a1c901754dd1a68705006a6934cffc)

Bug:  888926 
Change-Id: Iab0d82d272e2f24a5e91180d64bc8e2aa8a8238d
Reviewed-on: https://chromium-review.googlesource.com/1246827
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Commit-Queue: Chris Palmer <palmer@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#594475}
Reviewed-on: https://chromium-review.googlesource.com/1252004
Cr-Commit-Position: refs/branch-heads/3538@{#733}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/8b3539517f8a751cdba9f5276fb060a23b05053e/content/browser/appcache/appcache_group.cc

The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/8b3539517f8a751cdba9f5276fb060a23b05053e

Commit: 8b3539517f8a751cdba9f5276fb060a23b05053e
Author: mek@chromium.org
Commiter: mek@chromium.org
Date: 2018-09-28 16:49:07 +0000 UTC

Refcount AppCacheGroup correctly.

TBR=palmer@chromium.org

(cherry picked from commit 9d2ead1650a1c901754dd1a68705006a6934cffc)

Bug:  888926 
Change-Id: Iab0d82d272e2f24a5e91180d64bc8e2aa8a8238d
Reviewed-on: https://chromium-review.googlesource.com/1246827
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Commit-Queue: Chris Palmer <palmer@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#594475}
Reviewed-on: https://chromium-review.googlesource.com/1252004
Cr-Commit-Position: refs/branch-heads/3538@{#733}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}

We're not planning any further M69 releases, Rejecting merge to M69.

(CCing Chromium embedder)

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

commit f83007304ba201e3a0ee9eaf198d369aa48f6090
Author: Max Moroz <mmoroz@chromium.org>
Date: Tue Nov 06 16:39:35 2018

Add appcache_fuzzer

Integration fuzzer for appcache, focusing on logical behavior,
not parsing.

Bug:  888926 
Change-Id: I5871940f39d191f0b75fad49952961bd312d8a0b
Reviewed-on: https://chromium-review.googlesource.com/c/1302101
Commit-Queue: Max Moroz <mmoroz@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Oliver Chang <ochang@chromium.org>
Reviewed-by: Max Moroz <mmoroz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605711}
[modify] https://crrev.com/f83007304ba201e3a0ee9eaf198d369aa48f6090/content/browser/BUILD.gn
[modify] https://crrev.com/f83007304ba201e3a0ee9eaf198d369aa48f6090/content/browser/appcache/DEPS
[add] https://crrev.com/f83007304ba201e3a0ee9eaf198d369aa48f6090/content/browser/appcache/appcache_fuzzer.cc
[add] https://crrev.com/f83007304ba201e3a0ee9eaf198d369aa48f6090/content/browser/appcache/appcache_fuzzer.proto
[modify] https://crrev.com/f83007304ba201e3a0ee9eaf198d369aa48f6090/content/test/fuzzer/BUILD.gn

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