Security: buffer overflow in AudioSyncReader |
||||||||||||||||||
Issue descriptionVULNERABILITY DETAILS I think this vulnerability would require a compromised renderer. AudioSyncReader receives audio data from the renderer in shared memory. For normal streams, the shared memory is completely filled with audio every callback. This is not the case for compressed streams (since the actual size of the audio data may vary); for compressed streams, the renderer tells the browser how much data was actually written to the shared memory (read at [1]). The browser then memcpys it to another buffer[2]. Since there is no validation that the size is actually reasonable, this may lead to a buffer overflow. It may be possible to trigger this without a compromised renderer by using a poorly compressed stream, I didn't bother investigating that though. OS might be Android only (compressed streams are only supported on Android), but the code isn't very explicit about disallowing compressed streams for other platforms, so maybe there's some way to trigger this code regardless of platform. VERSION Chrome Version: ToT, since https://chromium-review.googlesource.com/c/chromium/src/+/619624. REPRODUCTION CASE The following is a unit test: const int kSampleRate = 44100; const int kBitsPerSample = 32; const int kFramesPerBuffer = 1; media::AudioParameters params(media::AudioParameters::AUDIO_BITSTREAM_AC3, media::CHANNEL_LAYOUT_STEREO, kSampleRate, kBitsPerSample, kFramesPerBuffer); auto socket = std::make_unique<base::CancelableSyncSocket>(); std::unique_ptr<media::AudioBus> output_bus = media::AudioBus::Create(params); std::unique_ptr<AudioSyncReader> reader = AudioSyncReader::Create(params, socket.get()); const base::SharedMemory* shmem = reader->shared_memory(); reader->RequestMoreData(base::TimeDelta(), base::TimeTicks(), 0); /* Code executed in the renderer starts here */ media::AudioOutputBuffer* buffer = reinterpret_cast<media::AudioOutputBuffer*>(shmem->memory()); uint32_t signal; EXPECT_EQ(socket->Receive(&signal, sizeof(signal)), sizeof(signal)); // So far, this is an ordinary stream. // Now |reader| expects data to be writted to the shared memory. The renderer // says how much data was written. Let's say it's one more than what's allocated. buffer->params.bitstream_data_size = shmem->mapped_size() - sizeof(media::AudioOutputBufferParameters) + 1; ++signal; EXPECT_EQ(socket->Send(&signal, sizeof(signal)), sizeof(signal)); /* Code executed in the renderer ends here */ reader->Read(output_bus.get()); [1] https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audio_sync_reader.cc?type=cs&l=212 [2] https://cs.chromium.org/chromium/src/media/base/audio_bus.cc?type=cs&l=370
,
Dec 6 2017
,
Dec 6 2017
Nice catch; sorry I missed this in review. It was a concern in the original combined patch set but I forgot about it when it got split up. We should merge this all the way back, it's a low cost fix and a huge security issue since it allows compromised renderer to OOB read/write controllably.
,
Dec 6 2017
+awhalley@ for M63 review. Please note M63 already went out for Android and Desktop.
,
Dec 6 2017
The change listed at #1 is not yet baked in canary/dev/beta.
,
Dec 6 2017
Stable is being released today, we shouldn't block that. Once it's backed a bit, let's get it into 63 to get picked up in any stable update.
,
Dec 7 2017
Merges are tracked by merge flags, marking it as fixed.
,
Dec 8 2017
,
Dec 11 2017
Requesting merge of CL in c1 to M63-64. It's been in canary for a few days. CL is straightforward and should be safe to merge.
,
Dec 11 2017
This bug requires manual review: Request affecting a post-stable build Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4f3cf4101794b5e85a259eac42ece845a3bcf4b0 commit 4f3cf4101794b5e85a259eac42ece845a3bcf4b0 Author: Max Morin <maxmorin@chromium.org> Date: Mon Dec 11 09:36:00 2017 Stop bitstream audio in IPC layer if not supported This reduces the attack surface of the browser process. Bug: 792422 Change-Id: Ie50557d0bc3ee9ace3aad234501bc390d73ab476 Reviewed-on: https://chromium-review.googlesource.com/811265 Reviewed-by: Sergey Volk <servolk@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Commit-Queue: Max Morin <maxmorin@chromium.org> Cr-Commit-Position: refs/heads/master@{#523065} [modify] https://crrev.com/4f3cf4101794b5e85a259eac42ece845a3bcf4b0/content/browser/bad_message.h [modify] https://crrev.com/4f3cf4101794b5e85a259eac42ece845a3bcf4b0/content/browser/renderer_host/media/audio_input_renderer_host.cc [modify] https://crrev.com/4f3cf4101794b5e85a259eac42ece845a3bcf4b0/content/browser/renderer_host/media/audio_renderer_host.cc [modify] https://crrev.com/4f3cf4101794b5e85a259eac42ece845a3bcf4b0/media/mojo/DEPS [modify] https://crrev.com/4f3cf4101794b5e85a259eac42ece845a3bcf4b0/media/mojo/services/BUILD.gn [modify] https://crrev.com/4f3cf4101794b5e85a259eac42ece845a3bcf4b0/media/mojo/services/mojo_audio_output_stream_provider.cc [modify] https://crrev.com/4f3cf4101794b5e85a259eac42ece845a3bcf4b0/media/mojo/services/mojo_audio_output_stream_provider.h [add] https://crrev.com/4f3cf4101794b5e85a259eac42ece845a3bcf4b0/media/mojo/services/mojo_audio_output_stream_provider_unittest.cc [modify] https://crrev.com/4f3cf4101794b5e85a259eac42ece845a3bcf4b0/tools/metrics/histograms/enums.xml
,
Dec 11 2017
This bug requires manual review: DEPS changes referenced in bugdroid comments. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 11 2017
Note: CL in #11 doesn't fix any issue per se, but limits the impact of further security issues related to compressed audio streams to Android. Not sure if it should be merged anywhere.
,
Dec 11 2017
After few baking days in Canary with good results, we can take it in M64.
,
Dec 12 2017
please confirm once you've verified this in Canary and results are looking good. Seems like it's been about 2 days on Canary.
,
Dec 13 2017
I'm requesting a merge of https://chromium.googlesource.com/chromium/src.git/+/7ca0390e00cc6d7e9948aa4f7ab1fa4213b295a0 from comment 1, which has been in canary for 6 days. I checked crash/, and didn't see anything related to it.
,
Dec 13 2017
Approving merge to M64, based on #16. Branch:3282
,
Dec 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5ed4257ef11ec56f54c08a823aedda55bc93b054 commit 5ed4257ef11ec56f54c08a823aedda55bc93b054 Author: Max Morin <maxmorin@chromium.org> Date: Thu Dec 14 13:18:18 2017 [M64] Validate bitstream_data_size in AudioSyncReader. In case the compressed data that is sent to the browser is larger than the allocated buffer, we shouldn't try to use the buffer. We should also be a bit careful with the bitstream_frames field, since it comes from an IPC call and thus isn't trustworthy. Bug: 792422 Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Change-Id: I1f6c02a4947b593375575da659573b1a93b93bd3 Reviewed-on: https://chromium-review.googlesource.com/810604 Reviewed-by: Olga Sharonova <olka@chromium.org> Commit-Queue: Max Morin <maxmorin@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#522107}(cherry picked from commit 7ca0390e00cc6d7e9948aa4f7ab1fa4213b295a0) Reviewed-on: https://chromium-review.googlesource.com/826223 Reviewed-by: Max Morin <maxmorin@chromium.org> Cr-Commit-Position: refs/branch-heads/3282@{#224} Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840} [modify] https://crrev.com/5ed4257ef11ec56f54c08a823aedda55bc93b054/content/browser/renderer_host/media/audio_sync_reader.cc [modify] https://crrev.com/5ed4257ef11ec56f54c08a823aedda55bc93b054/content/browser/renderer_host/media/audio_sync_reader.h [add] https://crrev.com/5ed4257ef11ec56f54c08a823aedda55bc93b054/content/browser/renderer_host/media/audio_sync_reader_unittest.cc [modify] https://crrev.com/5ed4257ef11ec56f54c08a823aedda55bc93b054/content/test/BUILD.gn [modify] https://crrev.com/5ed4257ef11ec56f54c08a823aedda55bc93b054/media/audio/audio_output_controller.cc
,
Jan 8 2018
,
Jan 22 2018
,
Mar 2 2018
,
Mar 16 2018
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
,
Mar 27 2018
|
||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||
Comment 1 by bugdroid1@chromium.org
, Dec 6 2017