Remove unused WebRTC event logging API |
||||||||
Issue descriptionI was commenting on https://chromium-review.googlesource.com/c/chromium/src/+/760816/19/content/browser/webrtc/webrtc_event_log_manager.cc#122, but I realized it's potentially more of a vulnerability report than a CL comment. :] > There are only two places from which this call can come (and that's by design, > so it shouldn't change): > > 1. Direct user input (chrome://webrtc-internals -> create dump -> RTC event > logging). In that case, I think it's reasonable to let the user save those > files anywhere he wishes. Or do we suspect Chrome might have permissions that > the logged-in user doesn't? > > 2. Outdates JS hooks that are only accessible from Google domains (pinging > @terelius for confirmation), that's nobody currently using. We intend to > remove these. We can land the CL that removes this before, if you think it's a > security issue, but otherwise, I'd prefer to do it after. 3. A compromised web renderer. This risk would be significantly mitigated if we are sure that this call site can only be reached from a blessed WebUI renderer process. Are we sure of that? (2) suggests no. It's a better design for the callee to validate the inputs from its callers, rather than to assume its callers are always trustworthy. Callers change, assumptions go unchallenged... Also, how are you ensuring that the calling domains in (2) are really Google domains? You'd also want to check for key pinning, to ensure that a MITM using a Google domain name can't make the call to this function. It might be that investigating the answers to these questions results in a severity downgrade for this bug, or maybe even closing it. But at the moment it seems like a compromised renderer can call this vulnerable function and write to an arbitrary file.
,
Nov 29 2017
,
Nov 29 2017
My bad, this is a High, not a Critical.
,
Nov 29 2017
I commented on the CL as well, but when I first reviewed these IPCs in https://codereview.chromium.org/1855193002/, I was under the impression this was strictly a WebUI thing. So (2) is extremely surprising to me.
,
Nov 29 2017
Could you clarify what vulnerability you are worried about? Are you worried about the existing code or the new code? Also, I don't understand why it is classified as high severity. That is afaik normally used for things like memory corruption, sandbox-escapes and arbitrary code execution with special requirements or for origin impersonations. The current code has two ways to enable it: 1) Using chrome://webrtc-internals, the user can specify any filename in any location. The file is opened by the browser process and transferred to the render process via IPC. The render process writes to the file. 2) Logging can also be started using the webrtc private API (https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc?l=531) in the browser process. I don't know how the generated IDL code works, and whether this can be triggered by a compromised renderer. However, the API cannot control filename or location in any way. The file is placed in the default location for WebRTC logs with a filename derived from the render process id and peer connection id. Moreover, this API only works if a command line flag has been provided, so by default it has no effect. The rest is the same as for webrtc-internals; the browser process opens the file and passes it to the render process. Please note that this code mirrors what is done for audio debug dumps. If you have concerns about this, it will likely affect these dumps as well. The CL in question moves ownership of the open files from the render process to the browser process. The render process will send encoded strings to the browser process and the browser process will (subject to file size limits) write the string to file. This might make it easier for a compromised renderer to determine exactly what data is written, but this only affects the integrity of the logs and not the security of chrome.
,
Nov 29 2017
It seems to me, possibly due to a misunderstanding, that the existing code allows a compromised web renderer to write arbitrary contents to a file with an arbitrary pathname. If that is the case, I consider that a High severity bug; it's basically a sandbox escape. (https://chromium.googlesource.com/chromium/src/+/master/docs/security/severity-guidelines.md#High-severity) (1) Is OK, if and only if the pathname comes from the browser/the chooser that the browser spawns. (2) If there is really no path from a renderer to the browser-side function that opens whatever pathname it's given, then OK, there's no bug. But I'm not sure of that yet; the discussion in the CL made it sound like renderers rendering Google-property pages can hit it. The command line option would be an excellent severity-reducing mitigation, but does not necessarily make the code inherently safe.
,
Nov 30 2017
,
Nov 30 2017
It might bear mentioning that, AFAICT: * Both before my CL as well as after, the user had very limited control over the file path - Chrome suffixes the renderer and PeerConnection IDs, and sets the file extension. After my CL, a timestamp is also part of the name. This is for the webrtc-internals codepath. For the deprecated JS hooks, if I understood Björn correctly, the path is pre-set. * The user has no control over the file permissions. Specifically, the compromised "log" would never be executable. * At least after my CL, it's impossible to use this to overwrite an existing file.
,
Nov 30 2017
I don't think there is any vulnerability. WebrtcLoggingPrivateApi is enabled through the Hangouts services extension. https://cs.chromium.org/chromium/src/chrome/browser/resources/hangout_services/manifest.json?l=12 It allows connections from *.google.com and localhost. I have not looked into this from a security perspective, but the same manifest is used to restrict all other webrtc logging. Suppose that an attacker somehow bypasses this and is able to make the JS call. The first thing that happens when we receive a JS method call is checking the command line flags of the browser process https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc?l=531 There are separate flags for audio debug dumps and event logs, the flags only control this API and they are off by default so no normal user would have them turned on. Further, even if the command line flag has been given, the JS api only passes in a log duration in seconds (and some automatically generated values related to origin of the call) https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc?l=551 Chrome selects a directory by appending "WebRTC Logs" to the user's profile path https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/webrtc_event_log_handler.cc?l=79 Chrome then generates file names in this directory as "WebRtcEventLog.<log_number>.<render_process_id.<peer_connection_id> https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/webrtc_event_log_handler.cc?l=27 https://cs.chromium.org/chromium/src/content/browser/webrtc/webrtc_eventlog_host.cc?l=39 for each peer connection in the relevant render process. Note that the IDs are just integers stored in the browser process, so they can't be used for any fancy directory traversal.
,
Nov 30 2017
By the way, I've now uploaded this relevant CL: https://chromium-review.googlesource.com/c/chromium/src/+/800470
,
Nov 30 2017
OK, #9 sounds OK to me. Thank you.
,
Dec 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/488c1cb2954d69d5a4a0c73354cd7aac16b32038 commit 488c1cb2954d69d5a4a0c73354cd7aac16b32038 Author: Elad Alon <eladalon@chromium.org> Date: Fri Dec 01 09:15:55 2017 Remove deprecated WebRTC event logging JS hooks. Bug: 789351 , 775415 Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation;master.tryserver.chromium.linux:linux_mojo Change-Id: I356d41e09491fc4dc70bff7192b6abe6e0187dc1 Reviewed-on: https://chromium-review.googlesource.com/800470 Reviewed-by: Tommi <tommi@chromium.org> Reviewed-by: Devlin <rdevlin.cronin@chromium.org> Reviewed-by: Chris Palmer <palmer@chromium.org> Reviewed-by: Mark Pearson <mpearson@chromium.org> Reviewed-by: Henrik Grunell <grunell@chromium.org> Commit-Queue: Elad Alon <eladalon@chromium.org> Cr-Commit-Position: refs/heads/master@{#520894} [modify] https://crrev.com/488c1cb2954d69d5a4a0c73354cd7aac16b32038/chrome/browser/BUILD.gn [delete] https://crrev.com/041dca89c23713dcebdc72c3f247f626f105e7a2/chrome/browser/extensions/api/webrtc_logging_private/webrtc_event_log_apitest.cc [modify] https://crrev.com/488c1cb2954d69d5a4a0c73354cd7aac16b32038/chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.cc [modify] https://crrev.com/488c1cb2954d69d5a4a0c73354cd7aac16b32038/chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api.h [modify] https://crrev.com/488c1cb2954d69d5a4a0c73354cd7aac16b32038/chrome/browser/extensions/api/webrtc_logging_private/webrtc_logging_private_api_stub.cc [delete] https://crrev.com/041dca89c23713dcebdc72c3f247f626f105e7a2/chrome/browser/media/webrtc/webrtc_event_log_handler.cc [delete] https://crrev.com/041dca89c23713dcebdc72c3f247f626f105e7a2/chrome/browser/media/webrtc/webrtc_event_log_handler.h [modify] https://crrev.com/488c1cb2954d69d5a4a0c73354cd7aac16b32038/chrome/browser/media/webrtc/webrtc_logging_handler_host.cc [modify] https://crrev.com/488c1cb2954d69d5a4a0c73354cd7aac16b32038/chrome/browser/media/webrtc/webrtc_logging_handler_host.h [modify] https://crrev.com/488c1cb2954d69d5a4a0c73354cd7aac16b32038/chrome/browser/resources/hangout_services/manifest.json [modify] https://crrev.com/488c1cb2954d69d5a4a0c73354cd7aac16b32038/chrome/browser/resources/hangout_services/thunk.js [modify] https://crrev.com/488c1cb2954d69d5a4a0c73354cd7aac16b32038/chrome/common/chrome_switches.cc [modify] https://crrev.com/488c1cb2954d69d5a4a0c73354cd7aac16b32038/chrome/common/chrome_switches.h [modify] https://crrev.com/488c1cb2954d69d5a4a0c73354cd7aac16b32038/chrome/common/extensions/api/webrtc_logging_private.idl [modify] https://crrev.com/488c1cb2954d69d5a4a0c73354cd7aac16b32038/chrome/test/BUILD.gn [modify] https://crrev.com/488c1cb2954d69d5a4a0c73354cd7aac16b32038/extensions/browser/extension_function_histogram_value.h [modify] https://crrev.com/488c1cb2954d69d5a4a0c73354cd7aac16b32038/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter [modify] https://crrev.com/488c1cb2954d69d5a4a0c73354cd7aac16b32038/tools/metrics/histograms/enums.xml
,
Dec 1 2017
,
Dec 4 2017
,
Dec 4 2017
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by palmer@chromium.org
, Nov 29 2017