Skip to content

Commit

Permalink
[CrOS HaTS] Fix UAF issue with HatsDialog
Browse files Browse the repository at this point in the history 
When switching users, the current implementation of HatsDialog always
passes in the active user sessions, which may have changed since the
original HaTS notification was clicked. Since the UI context that is
tied to that user may no longer be available, a UAF can occur in this
situation.

This change checks to see if the current user is still the same user
that activated the notification, ensuring that the UI context will
exist before creating the dialog.

It also moves the triggering logic for the HatsDialog into the
HatsNotificationController. This allows the controller to properly
handle the life-cycle of the dialog, and prevents the UAF issue that
originally occurred.

There are three different scenarios that were manually tested:
1. Normal path: The user stays logged in and sees the dialog as
expected.
2. Switch path: The user clicks the notification and switches
to a different account before the dialog is displayed. In this case,
the dialog will not be displayed since a different user is using the
device.
3. Switch back path: The user clicks the notification, switches to a
different account, and then switches back to the original account before
the dialog is displayed. In this case, the dialog is displayed since the
original user has a valid UI context.

These tests were conducted by adding a `sleep(20);` call to the
beginning of `HatsDialog::GetFormattedSiteContext` to allow the tester
time to manually switch between accounts. The following arguments were
supplied to the built chrome binary invocation: `--login-manager --force-happiness-tracking-system --enable-features=HappinessTrackingSystem:prob/1.0/trigger_id/test`

Note: This CL is not unit tested due to the current design of the class,
which does not have a unittest file. A refactor would be required to
add a test, which is tracked by b/232329702.

LOW_COVERAGE_REASON=Only moved logic/tests, did not change tests

Bug: 1320139, 1319229
Change-Id: I73b52623a47a2f63ee961326a59ae94168aff0e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3828048
Reviewed-by: Miriam Zimmerman <mutexlox@chromium.org>
Commit-Queue: Jack Shira <jackshira@google.com>
Cr-Commit-Position: refs/heads/main@{#1043675}
  • Loading branch information
Jack Shira authored and Chromium LUCI CQ committed Sep 6, 2022
1 parent b4d2da1 commit 209ccf9
Show file tree
Hide file tree
Showing 6 changed files with 179 additions and 183 deletions.
103 changes: 1 addition & 102 deletions chrome/browser/ash/hats/hats_dialog.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,80 +54,8 @@ const char kClientSmileySelected[] = "smiley-selected-";
constexpr char kCrOSHaTSURL[] =
"https://storage.googleapis.com/chromeos-hats-web-stable/index.html";

// Delimiters used to join the separate device info elements into a single
// string to be used as site context.
const char kDeviceInfoStopKeyword[] = "&";
const char kDeviceInfoKeyValueDelimiter[] = "=";
const char kDefaultProfileLocale[] = "en-US";

enum class DeviceInfoKey : unsigned int {
BROWSER = 0,
PLATFORM,
FIRMWARE,
LOCALE,
};

// Maps the given DeviceInfoKey |key| enum to the corresponding string value
// that can be used as a key when creating a URL parameter.
const std::string KeyEnumToString(DeviceInfoKey key) {
switch (key) {
case DeviceInfoKey::BROWSER:
return "browser";
case DeviceInfoKey::PLATFORM:
return "platform";
case DeviceInfoKey::FIRMWARE:
return "firmware";
case DeviceInfoKey::LOCALE:
return "locale";
default:
NOTREACHED();
return std::string();
}
}

} // namespace

// static
std::string HatsDialog::GetFormattedSiteContext(
const std::string& user_locale,
const base::flat_map<std::string, std::string>& product_specific_data) {
base::flat_map<std::string, std::string> context;

context[KeyEnumToString(DeviceInfoKey::BROWSER)] =
version_info::GetVersionNumber();

context[KeyEnumToString(DeviceInfoKey::PLATFORM)] =
chromeos::version_loader::GetVersion(
chromeos::version_loader::VERSION_FULL);

context[KeyEnumToString(DeviceInfoKey::FIRMWARE)] =
chromeos::version_loader::GetFirmware();

context[KeyEnumToString(DeviceInfoKey::LOCALE)] = user_locale;

for (const auto& pair : context) {
if (product_specific_data.contains(pair.first)) {
LOG(WARNING) << "Product specific data contains reserved key "
<< pair.first << ". Value will be overwritten.";
}
}
context.insert(product_specific_data.begin(), product_specific_data.end());

std::stringstream stream;
bool first_iteration = true;
for (const auto& pair : context) {
if (!first_iteration)
stream << kDeviceInfoStopKeyword;

stream << base::EscapeQueryParamValue(pair.first, /*use_plus=*/false)
<< kDeviceInfoKeyValueDelimiter
<< base::EscapeQueryParamValue(pair.second, /*use_plus=*/false);

first_iteration = false;
}
return stream.str();
}

// static
bool HatsDialog::HandleClientTriggeredAction(
const std::string& action,
Expand Down Expand Up @@ -160,43 +88,14 @@ bool HatsDialog::HandleClientTriggeredAction(
return false;
}

// static
std::unique_ptr<HatsDialog> HatsDialog::CreateAndShow(
const HatsConfig& hats_config,
const base::flat_map<std::string, std::string>& product_specific_data) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

Profile* profile = ProfileManager::GetActiveUserProfile();
std::string user_locale =
profile->GetPrefs()->GetString(language::prefs::kApplicationLocale);
language::ConvertToActualUILocale(&user_locale);
if (!user_locale.length())
user_locale = kDefaultProfileLocale;

std::unique_ptr<HatsDialog> hats_dialog(
new HatsDialog(HatsFinchHelper::GetTriggerID(hats_config), profile,
hats_config.histogram_name));

// Raw pointer is used here since the dialog is owned by the hats
// notification controller which lives until the end of the user session. The
// dialog will always be closed before that time instant.
base::ThreadPool::PostTaskAndReplyWithResult(
FROM_HERE, {base::MayBlock(), base::TaskPriority::BEST_EFFORT},
base::BindOnce(&GetFormattedSiteContext, user_locale,
product_specific_data),
base::BindOnce(&HatsDialog::Show, base::Unretained(hats_dialog.get())));

return hats_dialog;
}

void HatsDialog::Show(const std::string& site_context) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

// Link the trigger ID to fetch the correct survey.
url_ = std::string(kCrOSHaTSURL) + "?" + site_context +
"&trigger=" + trigger_id_;

chrome::ShowWebDialog(nullptr, ProfileManager::GetActiveUserProfile(), this);
chrome::ShowWebDialog(nullptr, user_profile_, this);
}

HatsDialog::HatsDialog(const std::string& trigger_id,
Expand Down
31 changes: 5 additions & 26 deletions chrome/browser/ash/hats/hats_dialog.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,55 +7,34 @@

#include <string>

#include "base/containers/flat_map.h"
#include "base/gtest_prod_util.h"
#include "base/strings/string_piece.h"
#include "ui/web_dialogs/web_dialog_delegate.h"

class Profile;

namespace ash {
struct HatsConfig;

// Happiness tracking survey dialog. Sometimes appears after login to ask the
// user how satisfied they are with their Chromebook.
// This class lives on the UI thread.
class HatsDialog : public ui::WebDialogDelegate {
public:
// Creates an instance of HatsDialog and posts a task to load all the relevant
// device info before displaying the dialog. If |product_specific_data| is
// provided, the key-value pairs will be attached to the survey results.
static std::unique_ptr<HatsDialog> CreateAndShow(
const HatsConfig& hats_config,
const base::flat_map<std::string, std::string>& product_specific_data =
base::flat_map<std::string, std::string>());
HatsDialog(const std::string& trigger_id,
Profile* user_profile,
const std::string& histogram_name);

HatsDialog(const HatsDialog&) = delete;
HatsDialog& operator=(const HatsDialog&) = delete;

~HatsDialog() override;

void Show(const std::string& site_context);

private:
FRIEND_TEST_ALL_PREFIXES(HatsDialogTest, GetFormattedSiteContext);
FRIEND_TEST_ALL_PREFIXES(HatsDialogTest, HandleClientTriggeredAction);

void Show(const std::string& site_context);

HatsDialog(const std::string& trigger_id,
Profile* user_profile,
const std::string& histogram_name);

// Must be run on a blocking thread pool.
// Gathers the browser version info, firmware info and platform info and
// returns them in a single encoded string, in the format
// "<key>=<value>&<key>=<value>&<key>=<value>" where the keys and values are
// url-escaped. Any key-value pairs in |product_specific_data| are also
// encoded and appended to the string, unless the keys collide with existing
// device info keys.
static std::string GetFormattedSiteContext(
const std::string& user_locale,
const base::flat_map<std::string, std::string>& product_specific_data);

// Based on the supplied |action|, returns true if the client should be
// closed. Handling the action could imply logging or incrementing a survey
// specific UMA metric (using |histogram_name|).
Expand Down
53 changes: 1 addition & 52 deletions chrome/browser/ash/hats/hats_dialog_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,62 +8,10 @@

#include "base/test/metrics/histogram_tester.h"
#include "hats_dialog.h"
#include "net/base/url_util.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "url/gurl.h"

namespace {

const char kLocaleKey[] = "locale";
const char kBrowserKey[] = "browser";
const char kPlatformKey[] = "platform";
const char kFirmwareKey[] = "firmware";
const char kPsdKey1[] = "psd1";
const char kPsdKey2[] = "psd2";
const char kPsdValue1[] = "psdValue1";
const char kPsdValue2[] = "psdValue2 =%^*$#&";
const char kLocaleValue1[] = "locale1";
const char kBrowserValue1[] = "browser1";

bool GetQueryParameter(const std::string& query,
const std::string& key,
std::string* value) {
// Name and scheme actually don't matter, but are required to get a valid URL
// for parsing.
GURL query_url("http://localhost?" + query);
return net::GetValueForKeyInQuery(query_url, key, value);
}

} // namespace

namespace ash {

TEST(HatsDialogTest, GetFormattedSiteContext) {
base::flat_map<std::string, std::string> product_specific_data = {
{kPsdKey1, kPsdValue1},
{kPsdKey2, kPsdValue2},
{kBrowserKey, kBrowserValue1}};

std::string context =
HatsDialog::GetFormattedSiteContext(kLocaleValue1, product_specific_data);

std::string value;
EXPECT_TRUE(GetQueryParameter(context, kLocaleKey, &value));
EXPECT_EQ(kLocaleValue1, value);
EXPECT_TRUE(GetQueryParameter(context, kBrowserKey, &value));
EXPECT_NE(kBrowserValue1, value);
EXPECT_TRUE(GetQueryParameter(context, kPlatformKey, &value));
EXPECT_TRUE(GetQueryParameter(context, kFirmwareKey, &value));

EXPECT_TRUE(GetQueryParameter(context, kPsdKey1, &value));
EXPECT_EQ(kPsdValue1, value);
EXPECT_TRUE(GetQueryParameter(context, kPsdKey2, &value));
EXPECT_EQ(kPsdValue2, value);

// Confirm that the values are properly url escaped.
EXPECT_NE(std::string::npos, context.find("psdValue2%20%3D%25%5E*%24%23%26"));
}

TEST(HatsDialogTest, HandleClientTriggeredAction) {
// Client asks to close the window
EXPECT_TRUE(HatsDialog::HandleClientTriggeredAction("close", "hist-name"));
Expand All @@ -87,4 +35,5 @@ TEST(HatsDialogTest, HandleClientTriggeredAction) {
EXPECT_EQ(histogram_tester.GetAllSamples("full-histogram-name"),
expected_buckets);
}

} // namespace ash

0 comments on commit 209ccf9

Please sign in to comment.