Summary

Incomplete fix of the vulnerabilities reported in GHSL-2020-035 and GHSL-2020-038

Product

Chrome

CVE

CVE-2020-6450

Tested Version

Chrome version: master branch build 3bdff94, asan build 80.3987.132 Operating System: Ubuntu 18.04

Details

The fix suggested for GHSL-2020-035 and GHSL-2020-038 did not fix the problem completely and it is still possible to trigger UaP in those cases, although the fix did prevent UaF.

The fix in those issues prevents UaF of BaseAudioContext by making the AudioHandlers in the relevant callbacks (AudioScheduledSourceHandler::NotifyEnded and IIRFilterHandler::NotifyBadState) weak pointers. Take IIRFilterHandler::NotifyBadState for example:

@@ -105,9 +105,9 @@
     if (HasNonFiniteOutput()) {
       did_warn_bad_filter_state_ = true;
 
-      PostCrossThreadTask(*task_runner_, FROM_HERE,
-                          CrossThreadBindOnce(&IIRFilterHandler::NotifyBadState,
-                                              WrapRefCounted(this)));
+      PostCrossThreadTask(
+          *task_runner_, FROM_HERE,
+          CrossThreadBindOnce(&IIRFilterHandler::NotifyBadState, AsWeakPtr()));
     }
   }
 }

This prevents IIRFilterHandler from outliving BaseAudioContext as a cross thread task. As explained in 1055788, in order to destroy BaseAudioContext while IIRFilterHandler::NotifyBadState is waiting in the task queue, the IIRFilterNode that owns the IIRFilterHandler first needs to be destroyed, and this can only happen while the graph is being pulled, otherwise you won’t arrive at the code that posts IIRFilterHandler::NotifyBadState. This will cause the ownership of IIRFilterHandler to be transferred to rendering_orphan_handlers_ in DeferredTaskHandlers. [1]

When BaseAudioContext is destroyed, it clears itself out of the handlers in rendering_orphan_handlers_ by calling DeferredTaskHandler::ContextWillBeDestroyed in the destructor [2].

void DeferredTaskHandler::ContextWillBeDestroyed() {
  for (auto& handler : rendering_orphan_handlers_)
    handler->ClearContext();
  for (auto& handler : deletable_orphan_handlers_)
    handler->ClearContext();
  ClearHandlersToBeDeleted();
  // Some handlers might live because of their cross thread tasks.
}

This means that to cause a UaF of AudioHandler using BAC after it is destroyed, the AudioHandler needs to be removed from rendering_orphan_handlers_ at this point to prevent context_ from being cleared. In the test cases of 1055788 and 1057627, this was done by first destroying the ExecutionContext and cause BaseAudioContext::Uninitialize to run, which calls ClearHandlersToBeDeleted to be called to clear out rendering_orphan_handlers_.

By making the callbacks holding weak pointers instead of scoped_refptr, the fix ensures that rendering_orphan_handlers_ is the sole owner of the AudioHandlers after AudioNode is disposed of and hence the AudioHandlers cannot outlive BaseAudioContext. (because when BaseAudioContext is destroyed, it clears rendering_orphan_handlers_, causing the handlers to be destroyed) This prevents UaF of BAC from happening.

Use-after-poison, however, can still happen after the object is GCed and before it is destroyed (when destructor is called). As the BAC only cleans itself up from rendering_orphan_handlers_ and deletable_orphan_handlers_ when it is destroyed, by triggering the callbacks in the window after BAC is garbage collected and before it is destroyed (destructor called), it is still possible to cause UaP. At this point, rendering_orphan_handlers_ will still be keeping the AudioHandler alive while the AudioHandler still holds BAC as an UntraceMember, but because BAC is already garbage collected, a UaP will happen when the callback is run and trying to access BAC by calling Context()->GetExecutionContext():

void IIRFilterHandler::NotifyBadState() const {
  DCHECK(IsMainThread());
  if (!Context() || !Context()->GetExecutionContext()) //<-- UaP can still happen when Context()->GetExecutionContext is accessed.
    return;
  ....

To prevent this, and other cases that are fixed here: https://source.chromium.org/chromium/chromium/src/+/b75436e554d54b2d8d3590d7e61607e1ce67a2fe?originalUrl=https:%2F%2Fcs.chromium.org%2F I’d suggest adding a prefinalizer to BaseAudioContext and clean itself up from rendering_orphan_handlers_ and deletable_orphan_handlers_ there:

void BaseAudioContext::Dispose() {
  for (auto& handler : rendering_orphan_handlers_)
    handler->ClearContext();
  for (auto& handler : deletable_orphan_handlers_)
    handler->ClearContext();
}

as AudioHandler should not hold on to a reference of BaseAudioContext after it is garbage collected, this should have little side effect. The previous fixes in those issues, however, are still necessary to prevent UaF and should not be removed.

  1. https://source.chromium.org/chromium/chromium/src/+/129460b86794115e96b5ec4ee724f7ac971d1f41:third_party/blink/renderer/modules/webaudio/audio_node.cc;l=619;bpv=1;bpt=1?originalUrl=https:%2F%2Fcs.chromium.org%2F
  2. https://source.chromium.org/chromium/chromium/src/+/129460b86794115e96b5ec4ee724f7ac971d1f41:third_party/blink/renderer/modules/webaudio/base_audio_context.cc;l=112;bpv=1;bpt=1?originalUrl=https:%2F%2Fcs.chromium.org%2F

Impact

Use-after-free in renderer.

Coordinated Disclosure Timeline

Credit

This issue was discovered and reported by GHSL team member @m-y-mo (Man Yue Mo).

Contact

You can contact the GHSL team at securitylab@github.com, please include the GHSL-2020-053 in any communication regarding this issue.