Skip to content

Commit

Permalink
[history] Fix unchecked chrome.send param in ForeignSessionHandler
Browse files Browse the repository at this point in the history
The bug describes that a heap overflow can be triggered by using
chrome.send to send an invalid message to the C++ handler.

That was true because the C++ handler did not check the bounds of the
array before using the parameter.

This code is essentially unowned, but I'm fixing it because I'm listed
as a History owner.

Bug: 1408120
Change-Id: Ia8f049ca8bc35bbe63122affcb24b83d7d9cdb62
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4226314
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
Auto-Submit: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1102408}
  • Loading branch information
Tommy C. Li authored and Chromium LUCI CQ committed Feb 7, 2023
1 parent 28947b7 commit 70617a3
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 18 deletions.
28 changes: 12 additions & 16 deletions chrome/browser/ui/webui/history/foreign_session_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -130,15 +130,12 @@ void ForeignSessionHandler::RegisterProfilePrefs(
void ForeignSessionHandler::OpenForeignSessionTab(
content::WebUI* web_ui,
const std::string& session_string_value,
int window_num,
SessionID tab_id,
const WindowOpenDisposition& disposition) {
sync_sessions::OpenTabsUIDelegate* open_tabs = GetOpenTabsUIDelegate(web_ui);
if (!open_tabs)
return;

// We don't actually care about |window_num|, this is just a sanity check.
DCHECK_LE(0, window_num);
const ::sessions::SessionTab* tab;
if (!open_tabs->GetForeignTab(session_string_value, tab_id, &tab)) {
LOG(ERROR) << "Failed to load foreign tab.";
Expand All @@ -156,7 +153,7 @@ void ForeignSessionHandler::OpenForeignSessionTab(
void ForeignSessionHandler::OpenForeignSessionWindows(
content::WebUI* web_ui,
const std::string& session_string_value,
int window_num) {
size_t window_num) {
sync_sessions::OpenTabsUIDelegate* open_tabs = GetOpenTabsUIDelegate(web_ui);
if (!open_tabs)
return;
Expand All @@ -168,14 +165,13 @@ void ForeignSessionHandler::OpenForeignSessionWindows(
"OpenTabsUIDelegate.";
return;
}
std::vector<const ::sessions::SessionWindow*>::const_iterator iter_begin =
windows.begin() + (window_num < 0 ? 0 : window_num);
auto iter_end =
window_num < 0
? std::vector<const ::sessions::SessionWindow*>::const_iterator(
windows.end())
: iter_begin + 1;

// Bounds check `window_num` before using it anywhere. crbug.com/1408120
CHECK_LT(window_num, windows.size());
auto iter_begin = windows.begin() + window_num;
DCHECK(iter_begin != windows.end())
<< "Because we CHECKed that windows_num is less than the size.";
auto iter_end = iter_begin + 1;
SessionRestore::RestoreForeignSessionWindows(Profile::FromWebUI(web_ui),
iter_begin, iter_end);

Expand Down Expand Up @@ -356,9 +352,10 @@ void ForeignSessionHandler::HandleOpenForeignSession(
const std::string& session_string_value = args[0].GetString();

// Extract window number.
int window_num = -1;
if (num_args >= 2 && (!args[1].is_string() ||
!base::StringToInt(args[1].GetString(), &window_num))) {
size_t window_num = 0;
if (num_args >= 2 &&
(!args[1].is_string() ||
!base::StringToSizeT(args[1].GetString(), &window_num))) {
LOG(ERROR) << "Failed to extract window number.";
return;
}
Expand All @@ -375,8 +372,7 @@ void ForeignSessionHandler::HandleOpenForeignSession(
SessionID tab_id = SessionID::FromSerializedValue(tab_id_value);
if (tab_id.is_valid()) {
WindowOpenDisposition disposition = webui::GetDispositionFromClick(args, 3);
OpenForeignSessionTab(web_ui(), session_string_value, window_num, tab_id,
disposition);
OpenForeignSessionTab(web_ui(), session_string_value, tab_id, disposition);
} else {
OpenForeignSessionWindows(web_ui(), session_string_value, window_num);
}
Expand Down
3 changes: 1 addition & 2 deletions chrome/browser/ui/webui/history/foreign_session_handler.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,12 @@ class ForeignSessionHandler : public content::WebUIMessageHandler {

static void OpenForeignSessionTab(content::WebUI* web_ui,
const std::string& session_string_value,
int window_num,
SessionID tab_id,
const WindowOpenDisposition& disposition);

static void OpenForeignSessionWindows(content::WebUI* web_ui,
const std::string& session_string_value,
int window_num);
size_t window_num);

// Returns a pointer to the current session model associator or nullptr.
static sync_sessions::OpenTabsUIDelegate* GetOpenTabsUIDelegate(
Expand Down

0 comments on commit 70617a3

Please sign in to comment.