Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.

This is a serious security regression. 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

lizeb: I suspect this is intended fallout from your change https://chromium.googlesource.com/chromium/src/+/1ec5add49bbd0dbd4a15c0f1d675fec151aad57a ? Can you help route?

Yes, this is indeed a consequence from the CL above. Looking into it, but I suspect it's a false positive.

This is not a security issue, so removing the labels and dropping to P2.

Rationale: the poisoning is meant to catch acesses we don't want to ParkableString, to be able to move the underlying storage. There is no moving done currently, and the poisoning logic is a bit over-eager, so there is no illegal access possible here in the code as is. Though we have to either conclude that it's a false positive (due to the over-eager poisoning) or fix the illegal access before actually moving the content.

As a consequence, removing the various security labels, marking as blocking for 877044, and dropping to P2.

So, from the stack trace, what's happening is:

1. String ParkableString::ToString() returns a string.
3. This string is atomized, hence added to AtomicStringTable. This table only holds a pointer to the String and doesn't increase its reference count.
4. Later, ParkableString::Unlock() or ParkableString::Park() is called. At that point, the underlying string has a reference count of 1 (since AtomicStringTable doesn't take a reference). As a consequence, we poison the memory.
5. At some point, a String is converted into an AtomicString from blink::CharacterData::Atomize(). This calls AtomicStringTable::Add(), and there is a bucket collision (possibly because the strings are equal). Then the slow comparison is done in https://chromium.googlesource.com/chromium/src/+/f9b78f71666284a5ad5a632763043349ead0eecf/third_party/blink/renderer/platform/wtf/hash_table.h#1326, which reads the poisoned data.


Now, this is a false positive, because we poison the String while keeping it alive. This means that the string is still in AtomicStringTable, and the scenario above can happen. If we were to really "park" the string, then StringImpl::~StringImpl() would call AtomicStringTable::Remove(), and step (5) would not happen.

There are a couple of ways we can fix this:
- Don't poison AtomicStrings
- Really "park" the data, since ASAN will not reuse the address for a while, and still warn about use-after-free, so we keep the same coverage.

I am more inclined to do the second one, but only when ASAN is enabled. This way we keep ClusterFuzz coverage and don't create mysterious crashes if there are legitimate issues.

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

commit 9aa76aede80b2e9317f02dccd00010ea2967d2c8
Author: Benoit Lize <lizeb@chromium.org>
Date: Mon Oct 22 10:55:24 2018

blink/bindings: Fix false-positive ASAN check.

A ParkableString underlying String may be atomic. In this case, as long
as it it alive, there is a raw pointer reference to it in a per-thread
table. This can lead to a use-after-poison as the string gets poisoned
whereas it is still in the table.

This is due to not freeing string_ in ParkableStringImpl. To fix that,
don't poison AtomicStrings (which are not the majority of
ParkableString).

This is a false positive as when real parking happens the underlying
string would be freed, hence removed from the AtomicStringTable.

Bug:  883344 ,877044
Change-Id: I685260eafe31da4cafed150b74870a08aa61ed40
Reviewed-on: https://chromium-review.googlesource.com/c/1228057
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Benoit L <lizeb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601522}
[modify] https://crrev.com/9aa76aede80b2e9317f02dccd00010ea2967d2c8/third_party/blink/renderer/platform/bindings/parkable_string.cc

ClusterFuzz has detected this issue as fixed in range 601521:601522.

Detailed report: https://clusterfuzz.com/testcase?key=6654396610641920

Fuzzer: inferno_twister
Job Type: linux_asan_chrome_media
Platform Id: linux

Crash Type: Use-after-poison READ {*}
Crash Address: 0x7fad7e16480c
Crash State:
  blink::CharacterData::Atomize
  blink::Element::TextFromChildren
  blink::HTMLScriptElement::TextFromChildren
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_media&range=590275:590277
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_media&range=601521:601522

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6654396610641920

See https://github.com/google/clusterfuzz-tools for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase 6654396610641920 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.