As part of a code review I am conducting for Paul Theriault against the sandbox, the following vulnerability has been discovered. A malicious CompositorBridgeClient may call CompositorBridgeParent::RecvMakeSnapshot and pass in a SurfaceDescriptor which contains an arbitrary pointer value to be used for a memory write during ForceComposeToTarget. This should not be possible as CompositorBridgeClient may not be in the same process as CompositorBridgeParent. In this situation it is intended that a shared memory buffer be used as the write target, however no validation is performed by the CompositorBridgeParent to ensure that this is true. Below we can see that RecvMakeSnapshot will call GetDrawTargetForDescriptor with the SurfaceDescriptor provided by the remote CompositorBridgeClient process. > mozilla::ipc::IPCResult > CompositorBridgeParent::RecvMakeSnapshot(const SurfaceDescriptor& aInSnapshot, > const gfx::IntRect& aRect) > { > RefPtr<DrawTarget> target = GetDrawTargetForDescriptor(aInSnapshot, gfx::BackendType::CAIRO); > MOZ_ASSERT(target); > if (!target) { > // We kill the content process rather than have it continue with an invalid > // snapshot, that may be too harsh and we could decide to return some sort > // of error to the child process and let it deal with it... > return IPC_FAIL_NO_REASON(this); > } > ForceComposeToTarget(target, &aRect); > return IPC_OK(); > } GetDrawTargetForDescriptor will call GetAddressFromDescriptor in order to retrieve a raw pointer value from the SurfaceDescriptor to be used as the data location for the upcoming ForceComposeToTarget call to write to. > already_AddRefed<gfx::DrawTarget> > GetDrawTargetForDescriptor(const SurfaceDescriptor& aDescriptor, gfx::BackendType aBackend) > { > uint8_t* data = GetAddressFromDescriptor(aDescriptor); > auto rgb = aDescriptor.get_SurfaceDescriptorBuffer().desc().get_RGBDescriptor(); > uint32_t stride = ImageDataSerializer::GetRGBStride(rgb); > return gfx::Factory::CreateDrawTargetForData(gfx::BackendType::CAIRO, > data, rgb.size(), > stride, rgb.format()); > } In GetAddressFromDescriptor we can see that if the SurfaceDescriptor (an IPDL structure) data member (An IPDL MemoryOrShmem union) is not a shared memory type, then it defaults to the uintptr_t type. As this is a raw pointer, a malicious CompositorBridgeClient could supply an arbitrary uintptr_t value in order to write to an arbitrary location in the CompositorBridgeParent process. > uint8_t* > GetAddressFromDescriptor(const SurfaceDescriptor& aDescriptor) > { > MOZ_ASSERT(IsSurfaceDescriptorValid(aDescriptor)); > MOZ_RELEASE_ASSERT(aDescriptor.type() == SurfaceDescriptor::TSurfaceDescriptorBuffer, "GFX: surface descriptor is not the right type."); > > auto memOrShmem = aDescriptor.get_SurfaceDescriptorBuffer().data(); > if (memOrShmem.type() == MemoryOrShmem::TShmem) { > return memOrShmem.get_Shmem().get<uint8_t>(); > } else { > return reinterpret_cast<uint8_t*>(memOrShmem.get_uintptr_t()); > } > } For completeness here is the IPDL definitions of SurfaceDescriptorBuffer and MemoryOrShmem (From LayersSurfaces.ipdlh) > union MemoryOrShmem { > uintptr_t; > Shmem; > }; > > struct SurfaceDescriptorBuffer { > BufferDescriptor desc; > MemoryOrShmem data; > }; A malicious CompositorBridgeClient (aka a compromised CompositorBridgeClient process whereby an attacker has achieved arbitrary code execution) could construct such a call as to reach the above scenario using something like the following (Note untested): > // create RGBDescriptor... > const IntSize aSize(1, 1); > > gfx::SurfaceFormat format = gfxPlatform::GetPlatform()->Optimal2DFormatForContent(gfxContentType::COLOR_ALPHA); > > const bool hasIntermediateBuffer = true; > > RGBDescriptor rgbd(aSize, format, hasIntermediateBuffer); > > // create MemoryOrShmem... > MemoryOrShmem bufferDesc; > > uintptr_t evil = 0x4141414141414141; > > bufferDesc.operator=(evil); > > // create SurfaceDescriptorBuffer... > SurfaceDescriptorBuffer sdb = SurfaceDescriptorBuffer(rgbd, bufferDesc); > > // perform SendMakeSnapshot... > SurfaceDescriptor inSnapshot(sdb); > > IntRect bounds(0, 0, 1, 1); > > remoteRenderer->SendMakeSnapshot(inSnapshot, bounds); It is worth pointing out that on the client side, a check is performed in ShadowLayerForwarder::AllocSurfaceDescriptorWithCaps to see if the parent is in the same process or not, and use a shared memory buffer if the parent is not in the same process. However for security we cannot rely on the client side validation of this. The parent must ensure a client from another process cannot not use a raw pointer and must only use a shared memory buffer. > ShadowLayerForwarder::AllocSurfaceDescriptorWithCaps(const gfx::IntSize& aSize, > gfxContentType aContent, > uint32_t aCaps, > SurfaceDescriptor* aBuffer) > { > if (!IPCOpen()) { > return false; > } > gfx::SurfaceFormat format = > gfxPlatform::GetPlatform()->Optimal2DFormatForContent(aContent); > size_t size = ImageDataSerializer::ComputeRGBBufferSize(aSize, format); > if (!size) { > return false; > } > > MemoryOrShmem bufferDesc; > if (GetTextureForwarder()->IsSameProcess()) { > uint8_t* data = new (std::nothrow) uint8_t[size]; > if (!data) { > return false; > } > GfxMemoryImageReporter::DidAlloc(data); > memset(data, 0, size); > bufferDesc = reinterpret_cast<uintptr_t>(data); > } else { > > mozilla::ipc::Shmem shmem; > if (!GetTextureForwarder()->AllocUnsafeShmem(size, OptimalShmemType(), &shmem)) { > return false; > } > > bufferDesc = shmem; > }

Hi Milan, please assign this bug to the developer who can investigate and fix it. Thanks!

Do any callers have a need for |GetAddressFromDescriptor| to support the uintptr_t use case? All the |GetDrawTargetForDescriptor| callers look ok, but I'm less confident about the |GetSurfaceForDescriptor| callers; specifically https://searchfox.org/mozilla-central/source/dom/media/ipc/VideoDecoderManagerChild.cpp#249 Assuming this code doesn't need the uintptr case, just dropping the code entirely from GetAddressFromDescriptor seems best.

The VideoDecoderManagerChild caller is guaranteed to be shmem.

In that case, I think it'd be safe for |GetAddressFromDescriptor| to just drop uintptr_t case.

Simple patch, drops support from |GetAddressFromDescriptor| exclusively, and allows the content process to DoS the parent. Stops the actual vulnerability (and substitutes a much less serious DoS one), hopefully someone from the gfx team can opine if there's a better layer to fix at.

It seems that there is a confusion between CompositorBridgeParent and CrossProcessCompositorBridgeParent. When ShadowLayerForwarder is in parent process, it connect to CompositorBridgeParent. But if ShadowLayerForwarder is in content process, it connects to CrossProcessCompositorBridgeParent. Therefore CompositorBridgeParent::RecvMakeSnapshot() is called only by ClientLayerManager in parent process. If ClientLayerManager is in content process, CrossProcessCompositorBridgeParent::RecvMakeSnapshot() is called instead. CrossProcessCompositorBridgeParent::RecvMakeSnapshot() does nothing. https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CrossProcessCompositorBridgeParent.h#49

attachment 8932241 [details] [diff] [review] causes crash if ClientLayerManager::MakeSnapshotIfRequired() is called in parent process.

(In reply to Stephen Fewer from comment #0) > As part of a code review I am conducting for Paul Theriault against the > sandbox, the following vulnerability has been discovered. > > A malicious CompositorBridgeClient may call > CompositorBridgeParent::RecvMakeSnapshot and pass in a SurfaceDescriptor > which contains an arbitrary pointer value to be used for a memory write > during ForceComposeToTarget. This should not be possible as > CompositorBridgeClient may not be in the same process as > CompositorBridgeParent. In this situation it is intended that a shared > memory buffer be used as the write target, however no validation is > performed by the CompositorBridgeParent to ensure that this is true. > > Below we can see that RecvMakeSnapshot will call GetDrawTargetForDescriptor > with the SurfaceDescriptor provided by the remote CompositorBridgeClient > process. By comment 6, CompositorBridgeParent::RecvMakeSnapshot() does not receive SurfaceDescriptor provided by the remote CompositorBridgeClient process. It only receive SurfaceDescriptor provided by the parent process.

To clarify, are you saying you don't think there's a vulnerability here, because the code isn't reachable from the child process's IPC?

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #9) > To clarify, are you saying you don't think there's a vulnerability here, > because the code isn't reachable from the child process's IPC? Yes, I do not think it is a vulnerability. CompositorBridgeParent::RecvMakeSnapshot() is not reachable from child process. Child process always calls CrossProcessCompositorBridgeParent::RecvMakeSnapshot()

If CompositorBridgeParent really isn't accessible over IPC, this seems like it can be closed.

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #11) > If CompositorBridgeParent really isn't accessible over IPC, this seems like > it can be closed. CompositorBridgeParent is not accessible from content process. ContentProcess always access CrossProcessCompositorBridgeParent. I also think this could be closed.

Is CompositorBridgeParent accessible from the GPU process via IPC?

If GPU process exists, parent process access CompositorBridgeParent via IPC. The following diagram show how it is connected. https://github.com/sotaroikeda/firefox-diagrams/blob/master/gfx/gfx_GPUProcessManager_53.pdf

Ok, so on Windows: CompositorBridgeParent is in the GPU process, and receives messages from the chrome process. This is not exploitable because the chrome side is more privleged. On macOS/Linux: CompositorBridgeParent is in the chrome process, but it doesn't receive messages from the content process, because they go to CrossProcessCompositorBridgeParent. I'm going to go ahead and close this because it sounds like it's not reachable. If any of my analysis is wrong please say so!