Opened 2 years ago

Closed 23 months ago

#17931 closed defect (fixed)

Tor Browser crashes in LogMessageToConsole()

Reported by: pege Owned by: tbb-team
Priority: Immediate Milestone:
Component: Applications/Tor Browser Version:
Severity: Blocker Keywords: tbb-hardened, tbb-crash, TorBrowserTeam201601R
Cc: arthuredelstein, brade, mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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?

Child Tickets

Change History (18)

comment:1 Changed 23 months ago by cypherpunks_backup

Keywords: tbb-hardened tbb-crash added

comment:2 Changed 23 months ago by arthuredelstein

Cc: arthuredelstein added

comment:3 Changed 23 months ago by arthuredelstein

Priority: MediumHigh

comment:4 Changed 23 months ago by gk

Priority: HighImmediate
Severity: NormalBlocker

#17947 is a duplicate. Disabling JavaScript should at least avoid this issue.

comment:5 Changed 23 months ago by mcs

Cc: brade mcs added

comment:6 Changed 23 months ago by arthuredelstein

The bug here is exposed by an interaction between URL escaping and printf-like format specifiers. Here is what happens:

  1. The user enter's "let's encrypt" into the github search box.
  2. 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.
  3. CanvasUtils::IsImageExtractionAllowed attempts to log its blocking of the image extraction by calling nsContentUtils::LogMessageToConsole, to which it passes as the first argument a string containing the above URL.
  4. The %27s fragment in that URL (which is 's escaped) is interpreted by LogMessageToConsole as 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::IsImageExtractionAllowed to use nsIConsoleService::LogStringMessage instead of LogMessageToConsole, 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

Last edited 23 months ago by arthuredelstein (previous) (diff)

comment:7 Changed 23 months ago by arthuredelstein

Keywords: TorBrowserTeam201512R added
Status: newneeds_review

comment:8 in reply to:  6 ; Changed 23 months ago by 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!
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?

comment:9 in reply to:  8 ; Changed 23 months ago by arthuredelstein

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.

comment:10 Changed 23 months ago by cypherpunks

TorBrowser is Turing complete computer! Congrats!, jk

Last edited 23 months ago by cypherpunks (previous) (diff)

comment:11 in reply to:  9 Changed 23 months ago by mcs

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, ...);

comment:12 Changed 23 months ago by cypherpunks

The %27s fragment in that URL (which is 's escaped) is interpreted by LogMessageToConsole as a printf-like format specifier for a 27-character string.

Format-string vunnerabilities in my TBB? Who the fuck is working for you, it is an obvious BACKDOOR!

comment:13 Changed 23 months ago by mikeperry

Status: needs_reviewneeds_revision

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.

Last edited 23 months ago by mikeperry (previous) (diff)

comment:14 in reply to:  13 ; Changed 23 months ago by arthuredelstein

Here are two patches to give nsContentUtils::LogMessageToConsole a single non-format argument, as Mike suggested:

https://github.com/arthuredelstein/tor-browser/commits/17931+2

The first patch removes usage of LogMessageToConsole in GonkGPSGeolocationProvider.cpp, in favor of a more standard logging method found in other Gonk files.

The second patch changes nsContentUtils::LogMessageToConsole to a single argument. That makes the existing usage in nsCanvasUtils.cpp and loggingCallbacks.h safe.

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.

Last edited 23 months ago by arthuredelstein (previous) (diff)

comment:15 Changed 23 months ago by arthuredelstein

Keywords: TorBrowserTeam201601R added; TorBrowserTeam201512R removed
Status: needs_revisionneeds_review

comment:16 Changed 23 months ago by gk

Summary: Tor Browser Hardened CrashTor Browser crashes in LogMessageToConsole()

comment:17 in reply to:  14 Changed 23 months ago by mcs

Replying to arthuredelstein:

Here are two patches to give nsContentUtils::LogMessageToConsole a single non-format argument, as Mike suggested:

https://github.com/arthuredelstein/tor-browser/commits/17931+2

The first patch removes usage of LogMessageToConsole in GonkGPSGeolocationProvider.cpp, in favor of a more standard logging method found in other Gonk files.

The second patch changes nsContentUtils::LogMessageToConsole to a single argument. That makes the existing usage in nsCanvasUtils.cpp and loggingCallbacks.h safe.

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.

comment:18 Changed 23 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looks good to me, too. This will be fixed in 5.0.7, 5.5a6 and 5.5a6-hardened. Applied to tor-browser-38.5.0esr-5.0-2 and tor-browser-38.5.0esr-5.5-2 with slightly modified commit messages.

Note: See TracTickets for help on using tickets.