New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 633460 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

TranslateHelper needs to filter some parameters in a translate request from browser side

Project Member Reported by leon....@intel.com, Aug 2 2016

Issue description

Currently when TranslateHelper(renderer side) got a translate request from browser side, it uses the parameters directly to concatenate out a JS string to execute. 
'
std::string script = "cr.googleTranslate.translate('" + source_lang_ + "','" + target_lang_ + "')";
'

Although this is invoked in response to a browser message, and the message flows in a less secure direction, we might want to protect ourselves if source_lang contains unexpected punctuation, in case souce_lang is somehow taken by the browser from some user or page input.

We need to add some guard mechanism here. 
WDYT? Thanks.
 
Browser process can not trust renderer processes because it may be out of our control, but on the other hand, when an attacker get control of the browser process, that would mean the attacker already can do more powerful things than just running arbitrary JavaScript. So, generally speaking, I do not think chrome code doesn't care about such cases.

tsepez@ may have a better answer for this.
It's a nice to have second line of defense against an unexpected change in the browser.  Definitely lo priority.
In other words, its not a bug per-se but an enhancement.
Labels: -Type-Bug Type-Feature

Comment 5 by groby@chromium.org, Aug 2 2016

Yeah, it's a nice-to-have. base::EscapeJSONString is probably the right answer here.

SStringPrintF(&string, "cr.google.Translate.translate(%s, %s)", GetQuotedJSONString(source_lang_), GetQuotedJSONString(target_lang_));

or something to similar effect. (content::WebUI::GetJavascriptCall works too, but would need to move elsewhere, and requires base::Value objects. Probably overkill :)
Components: -UI>Browser>Translate UI>Browser>Language>Translate

Comment 7 by napper@chromium.org, May 29 2017

Owner: napper@chromium.org
Status: assigned (was: Untriaged)

Comment 8 by napper@chromium.org, Jun 26 2017

Status: Started (was: Assigned)
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a7dc1b0092a81e03e2b432a2e1f2f972d6388ccd

commit a7dc1b0092a81e03e2b432a2e1f2f972d6388ccd
Author: Jon Napper <napper@chromium.org>
Date: Tue Jun 27 23:51:15 2017

Escape the language strings when building the translation JS script

TranslateHelper uses the source and target languages to build a JS script
that is executed to perform translation. However the source and target
languages are used directly and without escaping, which means that if
the source language contained punctuation it could execute unintended
code.

This patch escapes the source and target language strings when building
the translation JS.

Bug:  633460 
Change-Id: Iad8e88305eb35fea5e09b16226fba9fe393145ba
Reviewed-on: https://chromium-review.googlesource.com/544481
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Christopher Lam <calamity@chromium.org>
Commit-Queue: Jon Napper <napper@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482799}
[modify] https://crrev.com/a7dc1b0092a81e03e2b432a2e1f2f972d6388ccd/components/BUILD.gn
[modify] https://crrev.com/a7dc1b0092a81e03e2b432a2e1f2f972d6388ccd/components/translate/content/renderer/BUILD.gn
[modify] https://crrev.com/a7dc1b0092a81e03e2b432a2e1f2f972d6388ccd/components/translate/content/renderer/translate_helper.cc
[modify] https://crrev.com/a7dc1b0092a81e03e2b432a2e1f2f972d6388ccd/components/translate/content/renderer/translate_helper.h
[add] https://crrev.com/a7dc1b0092a81e03e2b432a2e1f2f972d6388ccd/components/translate/content/renderer/translate_helper_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment