Tor Browser crashes in LogMessageToConsole()
Just tried to search "let's encrypt" on https://github.com. It crashes the Tor Browser Hardened 5.5a5-hardened without any error message.
The error is reproducible (at least on my two machines), but no other search term seems to trigger a crash.
BTW, is there a way to get any logs from the crash?
Trac:
Username: pege
Activity
#17947 (moved) is a duplicate. Disabling JavaScript should at least avoid this issue.
Trac:
Priority: High to Immediate
Severity: Normal to BlockerThe bug here is exposed by an interaction between URL escaping and printf-like format specifiers. Here is what happens:
- The user enter's "let's encrypt" into the github search box.
- Github navigates to the resulting page
https://github.com/search?utf8=%E2%9C%93&q=let%27s+encrypt, which attempts to extract canvas image data. -
CanvasUtils::IsImageExtractionAllowedattempts to log its blocking of the image extraction by callingnsContentUtils::LogMessageToConsole, to which it passes as the first argument a string containing the above URL. - The
%27sfragment in that URL (which is'sescaped) is interpreted byLogMessageToConsoleas a printf-like format specifier for a 27-character string. However, no such char array was passed to LogMessageToConsole, because this format specifier was unintended.
So we have undefined behavior, which manifests as EXC_BAD_ACCESS when I run the debugger.
To avoid this problem, I wrote the following revision to
CanvasUtils::IsImageExtractionAllowedto usensIConsoleService::LogStringMessageinstead ofLogMessageToConsole, as was used in the original Canvas patch. I manually tested this patch and the exception no longer occurs.Here is the patch for review: https://github.com/arthuredelstein/tor-browser/commit/17931
Replying to arthuredelstein:
The bug here is exposed by an interaction between URL escaping and printf-like format specifiers. Here is what happens: ...
Good work finding the root cause of the crash! I have not reviewed your patch yet, but you could reduce its size by continuing to use nsContentUtils::LogMessageToConsole() and just calling it like: nsContentUtils::LogMessageToConsole("%s", message.get()); But maybe that is too ugly and maybe we want to eliminate extra overhead (e.g., a call to PR_vsmprintf() that is not really needed).
I also wonder if the call to nsContentUtils::LogMessageToConsole() in security/sandbox/chromium-shim/sandbox/win/loggingCallbacks.h at line 107 is safe. But maybe Tor Browser does not use that code?
Replying to mcs:
Replying to arthuredelstein:
The bug here is exposed by an interaction between URL escaping and printf-like format specifiers. Here is what happens: ...
Good work finding the root cause of the crash!
To be precise, I am the root cause of the crash. Sorry about that.
I have not reviewed your patch yet, but you could reduce its size by continuing to use nsContentUtils::LogMessageToConsole() and just calling it like: nsContentUtils::LogMessageToConsole("%s", message.get());
Great suggestion! Here's a patch that does that instead: https://github.com/arthuredelstein/tor-browser/commits/17931+1
But maybe that is too ugly and maybe we want to eliminate extra overhead (e.g., a call to PR_vsmprintf() that is not really needed).
I think it's probably better to use this small patch. The extra overhead is pretty inconsequential, I think.
I also wonder if the call to nsContentUtils::LogMessageToConsole() in security/sandbox/chromium-shim/sandbox/win/loggingCallbacks.h at line 107 is safe. But maybe Tor Browser does not use that code?
I added a second patch to this branch, just in case.
I'm suprised to see how little LogMessageToConsole is used in mozilla-central. Perhaps it would be safer to change it to a single-argument call that takes a plain string without format specifiers.
-
Replying to arthuredelstein:
Great suggestion! Here's a patch that does that instead: https://github.com/arthuredelstein/tor-browser/commits/17931+1
r=brade, r=mcs This patch looks good. We also tested it and observed that the crash is fixed on Mac OS.
I also wonder if the call to nsContentUtils::LogMessageToConsole() in security/sandbox/chromium-shim/sandbox/win/loggingCallbacks.h at line 107 is safe. But maybe Tor Browser does not use that code?
I added a second patch to this branch, just in case.
This also looks good.
I'm suprised to see how little LogMessageToConsole is used in mozilla-central. Perhaps it would be safer to change it to a single-argument call that takes a plain string without format specifiers.
Maybe open a Bugzilla bug? The declaration for that method is misleading as well because of the use of aMsg instead of a clearer name such as aFormat: static void LogMessageToConsole(const char* aMsg, ...);
The core problem here is that LogMessageToConsole() is dangerous, undocumented, and borderline deceptive. We should absolutely patch this function to change LogMessageToConsole() to accept only a single non-format argument, to guard against future vulnerabilities coming down from Mozilla or even by new TBB devs in the far future. In fact, it is already misused in Mozilla's own sandboxing code in ./security/sandbox/chromium-shim/sandbox/win/loggingCallbacks.h. If a sandbox violation is able to force a log message there that has a format string, this could also lead to sandbox breakout from the e10s sandbox. We might even be able to claim Mozilla's bug bounty for this. Regardless, a Mozilla bug should be filed.
Also, if Mozilla disagrees with fixing LogMessageToConsole() and instead opts to patch the sandbox directly, we should carry that patch indefinitely, as a built-in reminder to check for more braindamage from them during future rebases.
I hear rumors of an NSS bugfix coming out tomorrow. If that bug affects the NSS in ESR, we should wait to pick that up. Otherwise, we should make a release with a fix for this ASAP.
Trac:
Status: needs_review to needs_revisionHere are two patches to give
nsContentUtils::LogMessageToConsolea single non-format argument, as Mike suggested:https://github.com/arthuredelstein/tor-browser/commits/17931+2
The first patch removes usage of
LogMessageToConsoleinGonkGPSGeolocationProvider.cpp, in favor of a more standard logging method found in other Gonk files.The second patch changes
nsContentUtils::LogMessageToConsoleto a single argument. That makes the existing usage innsCanvasUtils.cppandloggingCallbacks.hsafe.I confirmed that tor-browser.git builds and works with these patches applied. I have also submitted them to Mozilla's try server. The results will be here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71edd495d073
As I mentioned on IRC, I'm very sorry for causing this bug. I do agree with Mike that it will be safer if Mozilla adopts a single-argument signature for
LogMessageToConsole.-
Replying to arthuredelstein:
Here are two patches to give
nsContentUtils::LogMessageToConsolea single non-format argument, as Mike suggested:https://github.com/arthuredelstein/tor-browser/commits/17931+2
The first patch removes usage of
LogMessageToConsoleinGonkGPSGeolocationProvider.cpp, in favor of a more standard logging method found in other Gonk files.The second patch changes
nsContentUtils::LogMessageToConsoleto a single argument. That makes the existing usage innsCanvasUtils.cppandloggingCallbacks.hsafe.I confirmed that tor-browser.git builds and works with these patches applied. I have also submitted them to Mozilla's try server. The results will be here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71edd495d073
r=mcs, r=brade We did not compile or run the code, but it looks good. One question: are the (int) casts in front of the nsresult args needed inside GonkGPSGeolocationProvider.cpp? I assume they are; I am just surprised.
... I do agree with Mike that it will be safer if Mozilla adopts a single-argument signature for
LogMessageToConsole.Have you filed a bugzilla bug yet? I would like to be cc'd on it.
mentioned in issue #17947 (moved)
