Prevent USSD codes via Click to Call
Click to Call allows users to send a phone number from their Chrome
desktop instance to their Android phone. This number either comes from a
user's selection and sent via the context menu, or by clicking on a link
with a "tel:" href.
Sending from the context menu is gated by a regular expression and will
not allow any special characters like '#' or '*' to be contained in the
phone number.
Sending link hrefs does not go through that check as we assume the link
is a valid phone number. We do call GURL::GetContent() to get the number
which should discard anything after a (and including the) '#' character.
However, we also URL-decoded the resulting string before then sending it
over to Android, where we URL-decoded it again when constructing the
Dialer intent. This allows sending double-URL-encoded USSD tel links
which will be sent straight to the Dialer on certain Android versions
and device states.
The fix here is on both desktop and Android side:
Desktop:
- URL-decode the number and ignore if it contains '#', '*' or '%'.
- Send the raw number (URL-encoded) to Android
Android:
- Verify that URL-decoding the received raw number is valid as above
- Show the decoded number in the notification
- Parse the raw number in Java into a Uri object for the Dialer
Together this makes sure that we only URL-decode tel: links once and
verify it on both sender and receiver side before passing it on to the
Android Dialer.
Bug: 1180510
Test: updated unit_tests and browser_tests to check for conversion
Change-Id: Idf380b629cdf00155ecab054398af69f37ec2ef9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2825704
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: David Jacobo <djacobo@chromium.org>
Reviewed-by: Gayane Petrosyan <gayane@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Richard Knoll <knollr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#875572}
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/sharing/click_to_call/ClickToCallMessageHandler.java b/chrome/android/java/src/org/chromium/chrome/browser/sharing/click_to_call/ClickToCallMessageHandler.java
index 5bec93b..ef5b10f 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/sharing/click_to_call/ClickToCallMessageHandler.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/sharing/click_to_call/ClickToCallMessageHandler.java
@@ -103,11 +103,12 @@
*/
private static void displayNotification(String phoneNumber) {
Context context = ContextUtils.getApplicationContext();
+ String contentTitle = Uri.decode(phoneNumber);
SharingNotificationUtil.showNotification(
NotificationUmaTracker.SystemNotificationType.CLICK_TO_CALL,
NotificationConstants.GROUP_CLICK_TO_CALL,
NotificationConstants.NOTIFICATION_ID_CLICK_TO_CALL,
- getContentIntentProvider(phoneNumber), /*deleteIntent=*/null, phoneNumber,
+ getContentIntentProvider(phoneNumber), /*deleteIntent=*/null, contentTitle,
context.getResources().getString(R.string.click_to_call_notification_text),
R.drawable.ic_devices_16dp, R.drawable.ic_dialer_icon_blue_40dp,
R.color.default_icon_color_blue, /*startsActivity=*/true);
diff --git a/chrome/android/junit/src/org/chromium/chrome/browser/sharing/click_to_call/ClickToCallMessageHandlerTest.java b/chrome/android/junit/src/org/chromium/chrome/browser/sharing/click_to_call/ClickToCallMessageHandlerTest.java
index 7753e3c7a8..2c7a059 100644
--- a/chrome/android/junit/src/org/chromium/chrome/browser/sharing/click_to_call/ClickToCallMessageHandlerTest.java
+++ b/chrome/android/junit/src/org/chromium/chrome/browser/sharing/click_to_call/ClickToCallMessageHandlerTest.java
@@ -5,22 +5,33 @@
package org.chromium.chrome.browser.sharing.click_to_call;
import static org.junit.Assert.assertEquals;
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
import static org.robolectric.Shadows.shadowOf;
+import android.app.Notification;
import android.app.NotificationManager;
import android.content.Context;
+import android.content.Intent;
import android.os.Build;
+import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
+import org.mockito.MockitoAnnotations;
+import org.mockito.Spy;
import org.robolectric.RuntimeEnvironment;
import org.robolectric.annotation.Config;
+import org.robolectric.shadows.ShadowNotification;
import org.robolectric.shadows.ShadowNotificationManager;
+import org.chromium.base.ContextUtils;
import org.chromium.base.test.BaseRobolectricTestRunner;
import org.chromium.base.test.util.Feature;
import org.chromium.chrome.browser.device.DeviceConditions;
import org.chromium.chrome.browser.device.ShadowDeviceConditions;
+import org.chromium.chrome.browser.notifications.NotificationConstants;
import org.chromium.net.ConnectionType;
/**
@@ -30,6 +41,15 @@
@RunWith(BaseRobolectricTestRunner.class)
@Config(manifest = Config.NONE, shadows = {ShadowDeviceConditions.class})
public class ClickToCallMessageHandlerTest {
+ @Spy
+ private Context mContext = RuntimeEnvironment.application.getApplicationContext();
+
+ @Before
+ public void setUp() {
+ MockitoAnnotations.initMocks(this);
+ ContextUtils.initApplicationContextForTests(mContext);
+ }
+
/**
* Android Q+ should always display a notification to open the dialer.
*/
@@ -71,6 +91,25 @@
ClickToCallMessageHandler.handleMessage("18004444444");
assertEquals(0, getShadowNotificationManager().size());
+ verify(mContext, times(1)).startActivity(any(Intent.class));
+ }
+
+ @Test
+ @Feature({"Browser", "Sharing", "ClickToCall"})
+ @Config(sdk = Build.VERSION_CODES.Q)
+ public void testHandleMessage_decodesUrlForNotification() {
+ setIsScreenOnAndUnlocked(true);
+
+ ClickToCallMessageHandler.handleMessage("%2B44%201234");
+
+ ShadowNotificationManager manager = getShadowNotificationManager();
+ assertEquals(1, manager.size());
+
+ Notification notification =
+ manager.getNotification(NotificationConstants.GROUP_CLICK_TO_CALL,
+ NotificationConstants.NOTIFICATION_ID_CLICK_TO_CALL);
+ ShadowNotification shadowNotification = shadowOf(notification);
+ assertEquals("+44 1234", shadowNotification.getContentTitle());
}
private void setIsScreenOnAndUnlocked(boolean isScreenOnAndUnlocked) {
diff --git a/chrome/browser/BUILD.gn b/chrome/browser/BUILD.gn
index 066d87c..8f4b4ec 100644
--- a/chrome/browser/BUILD.gn
+++ b/chrome/browser/BUILD.gn
@@ -1536,6 +1536,10 @@
"sharesheet/sharesheet_types.h",
"sharing/ack_message_handler.cc",
"sharing/ack_message_handler.h",
+ "sharing/click_to_call/click_to_call_utils.cc",
+ "sharing/click_to_call/click_to_call_utils.h",
+ "sharing/click_to_call/phone_number_regex.cc",
+ "sharing/click_to_call/phone_number_regex.h",
"sharing/features.cc",
"sharing/features.h",
"sharing/ping_message_handler.cc",
@@ -3970,10 +3974,6 @@
"sharing/click_to_call/click_to_call_metrics.h",
"sharing/click_to_call/click_to_call_ui_controller.cc",
"sharing/click_to_call/click_to_call_ui_controller.h",
- "sharing/click_to_call/click_to_call_utils.cc",
- "sharing/click_to_call/click_to_call_utils.h",
- "sharing/click_to_call/phone_number_regex.cc",
- "sharing/click_to_call/phone_number_regex.h",
"sharing/shared_clipboard/remote_copy_message_handler.cc",
"sharing/shared_clipboard/remote_copy_message_handler.h",
"sharing/shared_clipboard/shared_clipboard_context_menu_observer.cc",
diff --git a/chrome/browser/ash/arc/intent_helper/arc_external_protocol_dialog.cc b/chrome/browser/ash/arc/intent_helper/arc_external_protocol_dialog.cc
index 7f4b9af..e87e9cf1 100644
--- a/chrome/browser/ash/arc/intent_helper/arc_external_protocol_dialog.cc
+++ b/chrome/browser/ash/arc/intent_helper/arc_external_protocol_dialog.cc
@@ -380,7 +380,7 @@
auto* device = it->get();
ClickToCallUiController::GetOrCreateFromWebContents(web_contents)
- ->OnDeviceSelected(GetUnescapedURLContent(url), *device,
+ ->OnDeviceSelected(url.GetContent(), *device,
SharingClickToCallEntryPoint::kLeftClickLink);
}
diff --git a/chrome/browser/ash/arc/intent_helper/arc_external_protocol_dialog_unittest.cc b/chrome/browser/ash/arc/intent_helper/arc_external_protocol_dialog_unittest.cc
index c137956..73437cd 100644
--- a/chrome/browser/ash/arc/intent_helper/arc_external_protocol_dialog_unittest.cc
+++ b/chrome/browser/ash/arc/intent_helper/arc_external_protocol_dialog_unittest.cc
@@ -9,6 +9,8 @@
#include "chrome/browser/sharing/click_to_call/click_to_call_ui_controller.h"
#include "chrome/browser/sharing/fake_device_info.h"
#include "chrome/browser/sharing/mock_sharing_service.h"
+#include "chrome/browser/sharing/proto/click_to_call_message.pb.h"
+#include "chrome/browser/sharing/proto/sharing_message.pb.h"
#include "chrome/browser/sharing/sharing_service_factory.h"
#include "chrome/browser/ui/app_list/arc/arc_app_test.h"
#include "chrome/test/base/browser_with_test_window_test.h"
@@ -993,6 +995,13 @@
EXPECT_TRUE(in_out_safe_to_bypass_ui);
}
+MATCHER_P(ProtoEquals, message, "") {
+ std::string expected_serialized, actual_serialized;
+ message.SerializeToString(&expected_serialized);
+ arg.SerializeToString(&actual_serialized);
+ return expected_serialized == actual_serialized;
+}
+
// Tests that clicking on a device calls through to SharingService.
TEST_F(ArcExternalProtocolDialogTestUtils, TestSelectDeviceForTelLink) {
CreateTab(/*started_from_arc=*/false);
@@ -1004,13 +1013,18 @@
std::vector<std::unique_ptr<syncer::DeviceInfo>> devices;
devices.push_back(CreateFakeDeviceInfo(device_guid));
- EXPECT_CALL(
- *sharing_service,
- SendMessageToDevice(Property(&syncer::DeviceInfo::guid, device_guid),
- testing::_, testing::_, testing::_));
+ GURL phone_number("tel:073%2099%209999%2099");
+
+ chrome_browser_sharing::SharingMessage sharing_message;
+ sharing_message.mutable_click_to_call_message()->set_phone_number(
+ phone_number.GetContent());
+ EXPECT_CALL(*sharing_service,
+ SendMessageToDevice(
+ Property(&syncer::DeviceInfo::guid, device_guid), testing::_,
+ ProtoEquals(sharing_message), testing::_));
OnIntentPickerClosedForTesting(
- rvh->GetProcess()->GetID(), rvh->GetRoutingID(), GURL("tel:0123456789"),
+ rvh->GetProcess()->GetID(), rvh->GetRoutingID(), phone_number,
/*safe_to_bypass_ui=*/true, std::move(handlers), std::move(devices),
/*selected_app_package=*/device_guid, apps::PickerEntryType::kDevice,
apps::IntentPickerCloseReason::OPEN_APP, /*should_persist=*/false);
diff --git a/chrome/browser/renderer_context_menu/render_view_context_menu.cc b/chrome/browser/renderer_context_menu/render_view_context_menu.cc
index 9c2d8b04..1928e8d 100644
--- a/chrome/browser/renderer_context_menu/render_view_context_menu.cc
+++ b/chrome/browser/renderer_context_menu/render_view_context_menu.cc
@@ -1927,7 +1927,7 @@
std::string selection_text;
if (ShouldOfferClickToCallForURL(browser_context_, params_.link_url)) {
entry_point = SharingClickToCallEntryPoint::kRightClickLink;
- phone_number = GetUnescapedURLContent(params_.link_url);
+ phone_number = params_.link_url.GetContent();
} else if (!params_.selection_text.empty()) {
entry_point = SharingClickToCallEntryPoint::kRightClickSelection;
selection_text = base::UTF16ToUTF8(params_.selection_text);
diff --git a/chrome/browser/send_tab_to_self/send_tab_to_self_util_unittest.cc b/chrome/browser/send_tab_to_self/send_tab_to_self_util_unittest.cc
index a3e9d36..c53623e 100644
--- a/chrome/browser/send_tab_to_self/send_tab_to_self_util_unittest.cc
+++ b/chrome/browser/send_tab_to_self/send_tab_to_self_util_unittest.cc
@@ -110,7 +110,7 @@
}
TEST_F(SendTabToSelfUtilTest, ShouldNotOfferFeatureForTelephoneLink) {
- url_ = GURL("tel:07387252578");
+ url_ = GURL("tel:07399999999");
AddTab(browser(), url_);
SendTabToSelfSyncServiceFactory::GetInstance()->SetTestingFactory(
diff --git a/chrome/browser/sharing/click_to_call/click_to_call_message_handler_android.cc b/chrome/browser/sharing/click_to_call/click_to_call_message_handler_android.cc
index 74a2a38..1949120 100644
--- a/chrome/browser/sharing/click_to_call/click_to_call_message_handler_android.cc
+++ b/chrome/browser/sharing/click_to_call/click_to_call_message_handler_android.cc
@@ -5,11 +5,16 @@
#include "chrome/browser/sharing/click_to_call/click_to_call_message_handler_android.h"
#include "base/android/jni_string.h"
+#include "base/callback_helpers.h"
#include "base/check.h"
+#include "base/metrics/histogram_functions.h"
+#include "base/strings/strcat.h"
#include "base/trace_event/trace_event.h"
#include "chrome/android/chrome_jni_headers/ClickToCallMessageHandler_jni.h"
+#include "chrome/browser/sharing/click_to_call/click_to_call_utils.h"
#include "chrome/browser/sharing/proto/click_to_call_message.pb.h"
#include "chrome/browser/sharing/proto/sharing_message.pb.h"
+#include "url/gurl.h"
ClickToCallMessageHandler::ClickToCallMessageHandler() = default;
@@ -20,12 +25,32 @@
SharingMessageHandler::DoneCallback done_callback) {
DCHECK(message.has_click_to_call_message());
TRACE_EVENT0("sharing", "ClickToCallMessageHandler::OnMessage");
+ base::ScopedClosureRunner runner(
+ base::BindOnce(std::move(done_callback), /*response=*/nullptr));
std::string phone_number = message.click_to_call_message().phone_number();
- JNIEnv* env = base::android::AttachCurrentThread();
+ GURL phone_url(base::StrCat({"tel:", phone_number}));
+ bool is_valid_phone_number = IsUrlSafeForClickToCall(phone_url) &&
+ phone_url.GetContent() == phone_number;
+ base::UmaHistogramBoolean("Sharing.ClickToCallPhoneNumberValid",
+ is_valid_phone_number);
+
+ // This can happen if a user on an older version of Chrome on their desktop
+ // clicks on a tel link that contains url-escaped unsafe characters like #.
+ // Another reason might be if the remote sender is using a custom or
+ // compromised version of Chrome. In either case, ignoring the message is the
+ // safest option as we don't want to pass along this number to the Android
+ // Dialer.
+ if (!is_valid_phone_number)
+ return;
+
+ HandlePhoneNumber(phone_number);
+}
+
+void ClickToCallMessageHandler::HandlePhoneNumber(
+ const std::string& phone_number) {
+ JNIEnv* env = base::android::AttachCurrentThread();
Java_ClickToCallMessageHandler_handleMessage(
env, base::android::ConvertUTF8ToJavaString(env, phone_number));
-
- std::move(done_callback).Run(/*response=*/nullptr);
}
diff --git a/chrome/browser/sharing/click_to_call/click_to_call_message_handler_android.h b/chrome/browser/sharing/click_to_call/click_to_call_message_handler_android.h
index 6b9f0af..577f258 100644
--- a/chrome/browser/sharing/click_to_call/click_to_call_message_handler_android.h
+++ b/chrome/browser/sharing/click_to_call/click_to_call_message_handler_android.h
@@ -18,6 +18,10 @@
void OnMessage(chrome_browser_sharing::SharingMessage message,
SharingMessageHandler::DoneCallback done_callback) override;
+ protected:
+ // Calls into Java to handle a |phone_number|. Virtual for testing.
+ virtual void HandlePhoneNumber(const std::string& phone_number);
+
private:
DISALLOW_COPY_AND_ASSIGN(ClickToCallMessageHandler);
};
diff --git a/chrome/browser/sharing/click_to_call/click_to_call_message_handler_android_unittest.cc b/chrome/browser/sharing/click_to_call/click_to_call_message_handler_android_unittest.cc
new file mode 100644
index 0000000..fc4e62a
--- /dev/null
+++ b/chrome/browser/sharing/click_to_call/click_to_call_message_handler_android_unittest.cc
@@ -0,0 +1,63 @@
+// Copyright 2021 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/sharing/click_to_call/click_to_call_message_handler_android.h"
+
+#include <memory>
+#include <string>
+
+#include "base/callback_helpers.h"
+#include "base/optional.h"
+#include "chrome/browser/sharing/proto/click_to_call_message.pb.h"
+#include "chrome/browser/sharing/proto/sharing_message.pb.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+// Test implementation of ClickToCallMessageHandler that does not call out to
+// Java via JNI but records the last phone number it would have sent over.
+class TestClickToCallMessageHandler : public ClickToCallMessageHandler {
+ public:
+ TestClickToCallMessageHandler() = default;
+ ~TestClickToCallMessageHandler() override = default;
+
+ base::Optional<std::string> last_phone_number() { return last_phone_number_; }
+
+ protected:
+ void HandlePhoneNumber(const std::string& phone_number) override {
+ last_phone_number_ = phone_number;
+ }
+
+ private:
+ base::Optional<std::string> last_phone_number_;
+};
+
+} // namespace
+
+TEST(ClickToCallMessageHandlerTest, HandlesValidPhoneNumber) {
+ TestClickToCallMessageHandler handler;
+ chrome_browser_sharing::SharingMessage message;
+
+ message.mutable_click_to_call_message()->set_phone_number("12345678");
+ handler.OnMessage(std::move(message), base::DoNothing());
+ EXPECT_EQ("12345678", handler.last_phone_number());
+}
+
+TEST(ClickToCallMessageHandlerTest, IgnoresInvalidPhoneNumbers) {
+ TestClickToCallMessageHandler handler;
+ chrome_browser_sharing::SharingMessage message;
+
+ message.mutable_click_to_call_message()->set_phone_number("*#06#");
+ handler.OnMessage(std::move(message), base::DoNothing());
+ EXPECT_FALSE(handler.last_phone_number().has_value());
+
+ message.mutable_click_to_call_message()->set_phone_number("%2A%2306%23");
+ handler.OnMessage(std::move(message), base::DoNothing());
+ EXPECT_FALSE(handler.last_phone_number().has_value());
+
+ message.mutable_click_to_call_message()->set_phone_number(
+ "%252A%252306%2523");
+ handler.OnMessage(std::move(message), base::DoNothing());
+ EXPECT_FALSE(handler.last_phone_number().has_value());
+}
diff --git a/chrome/browser/sharing/click_to_call/click_to_call_ui_controller.cc b/chrome/browser/sharing/click_to_call/click_to_call_ui_controller.cc
index 9e7b653..b8b9469 100644
--- a/chrome/browser/sharing/click_to_call/click_to_call_ui_controller.cc
+++ b/chrome/browser/sharing/click_to_call/click_to_call_ui_controller.cc
@@ -119,7 +119,7 @@
if (ukm_recorder_)
std::move(ukm_recorder_).Run(SharingClickToCallSelection::kDevice);
- SendNumberToDevice(device, GetUnescapedURLContent(phone_url_),
+ SendNumberToDevice(device, phone_url_.GetContent(),
SharingClickToCallEntryPoint::kLeftClickLink);
}
diff --git a/chrome/browser/sharing/click_to_call/click_to_call_ui_controller_unittest.cc b/chrome/browser/sharing/click_to_call/click_to_call_ui_controller_unittest.cc
index 84dc0e8..e881aba 100644
--- a/chrome/browser/sharing/click_to_call/click_to_call_ui_controller_unittest.cc
+++ b/chrome/browser/sharing/click_to_call/click_to_call_ui_controller_unittest.cc
@@ -33,8 +33,7 @@
namespace {
-const char kPhoneNumber[] = "073%2087%202525%2078";
-const char kExpectedPhoneNumber[] = "073 87 2525 78";
+const char kPhoneNumber[] = "073%2099%209999%2099";
const char kReceiverGuid[] = "test_receiver_guid";
const char kReceiverName[] = "test_receiver_name";
@@ -85,7 +84,7 @@
chrome_browser_sharing::SharingMessage sharing_message;
sharing_message.mutable_click_to_call_message()->set_phone_number(
- kExpectedPhoneNumber);
+ kPhoneNumber);
EXPECT_CALL(
*service(),
SendMessageToDevice(
diff --git a/chrome/browser/sharing/click_to_call/click_to_call_utils.cc b/chrome/browser/sharing/click_to_call/click_to_call_utils.cc
index f9709ab..a52c792 100644
--- a/chrome/browser/sharing/click_to_call/click_to_call_utils.cc
+++ b/chrome/browser/sharing/click_to_call/click_to_call_utils.cc
@@ -10,6 +10,7 @@
#include "base/optional.h"
#include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h"
+#include "build/build_config.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/sharing/click_to_call/phone_number_regex.h"
#include "chrome/browser/sharing/sharing_service.h"
@@ -34,6 +35,10 @@
constexpr int kSelectionTextMaxDigits = 15;
bool IsClickToCallEnabled(content::BrowserContext* browser_context) {
+#if defined(OS_ANDROID)
+ // We don't support sending phone numbers from Android.
+ return false;
+#else // defined(OS_ANDROID)
// Check Chrome enterprise policy for Click to Call.
Profile* profile = Profile::FromBrowserContext(browser_context);
if (profile && !profile->GetPrefs()->GetBoolean(prefs::kClickToCallEnabled))
@@ -42,6 +47,32 @@
SharingService* sharing_service =
SharingServiceFactory::GetForBrowserContext(browser_context);
return sharing_service != nullptr;
+#endif // defined(OS_ANDROID)
+}
+
+// Returns the first possible phone number in |selection_text| given the
+// |regex_variant| to be used or base::nullopt if the regex did not match.
+base::Optional<std::string> ExtractPhoneNumber(
+ const std::string& selection_text) {
+ std::string parsed_number;
+
+ const re2::RE2& regex = GetPhoneNumberRegex();
+ if (!re2::RE2::PartialMatch(selection_text, regex, &parsed_number))
+ return base::nullopt;
+
+ return base::UTF16ToUTF8(
+ base::TrimWhitespace(base::UTF8ToUTF16(parsed_number), base::TRIM_ALL));
+}
+
+// Unescapes and returns the URL contents.
+std::string GetUnescapedURLContent(const GURL& url) {
+ std::string content_string(url.GetContent());
+ url::RawCanonOutputT<char16_t> unescaped_content;
+ url::DecodeURLEscapeSequences(content_string.data(), content_string.size(),
+ url::DecodeURLMode::kUTF8OrIsomorphic,
+ &unescaped_content);
+ return base::UTF16ToUTF8(
+ std::u16string(unescaped_content.data(), unescaped_content.length()));
}
} // namespace
@@ -49,7 +80,7 @@
bool ShouldOfferClickToCallForURL(content::BrowserContext* browser_context,
const GURL& url) {
return !url.is_empty() && url.SchemeIs(url::kTelScheme) &&
- !url.GetContent().empty() && IsClickToCallEnabled(browser_context);
+ IsUrlSafeForClickToCall(url) && IsClickToCallEnabled(browser_context);
}
base::Optional<std::string> ExtractPhoneNumberForClickToCall(
@@ -71,24 +102,13 @@
return ExtractPhoneNumber(selection_text);
}
-base::Optional<std::string> ExtractPhoneNumber(
- const std::string& selection_text) {
- std::string parsed_number;
-
- const re2::RE2& regex = GetPhoneNumberRegex();
- if (!re2::RE2::PartialMatch(selection_text, regex, &parsed_number))
- return base::nullopt;
-
- return base::UTF16ToUTF8(
- base::TrimWhitespace(base::UTF8ToUTF16(parsed_number), base::TRIM_ALL));
-}
-
-std::string GetUnescapedURLContent(const GURL& url) {
- std::string content_string(url.GetContent());
- url::RawCanonOutputT<char16_t> unescaped_content;
- url::DecodeURLEscapeSequences(content_string.data(), content_string.size(),
- url::DecodeURLMode::kUTF8OrIsomorphic,
- &unescaped_content);
- return base::UTF16ToUTF8(
- std::u16string(unescaped_content.data(), unescaped_content.length()));
+bool IsUrlSafeForClickToCall(const GURL& url) {
+ // Get the unescaped content as this is what we'll end up sending to the
+ // Android dialer.
+ std::string unescaped = GetUnescapedURLContent(url);
+ // We don't allow any number that contains any of these characters as they
+ // might be used to create USSD codes.
+ return !unescaped.empty() &&
+ std::none_of(unescaped.begin(), unescaped.end(),
+ [](char c) { return c == '#' || c == '*' || c == '%'; });
}
diff --git a/chrome/browser/sharing/click_to_call/click_to_call_utils.h b/chrome/browser/sharing/click_to_call/click_to_call_utils.h
index 194b00f..2edaadf 100644
--- a/chrome/browser/sharing/click_to_call/click_to_call_utils.h
+++ b/chrome/browser/sharing/click_to_call/click_to_call_utils.h
@@ -24,12 +24,9 @@
content::BrowserContext* browser_context,
const std::string& selection_text);
-// Returns the first possible phone number in |selection_text| given the
-// |regex_variant| to be used or base::nullopt if the regex did not match.
-base::Optional<std::string> ExtractPhoneNumber(
- const std::string& selection_text);
-
-// Unescapes and returns the URL contents.
-std::string GetUnescapedURLContent(const GURL& url);
+// Checks if the given |url| is safe to be used by Click to Call to be sent to
+// remote Android devices. Note that the remote device might open the dialer
+// immediately with the given |url| so any USSD codes should return false here.
+bool IsUrlSafeForClickToCall(const GURL& url);
#endif // CHROME_BROWSER_SHARING_CLICK_TO_CALL_CLICK_TO_CALL_UTILS_H_
diff --git a/chrome/browser/sharing/click_to_call/click_to_call_utils_unittest.cc b/chrome/browser/sharing/click_to_call/click_to_call_utils_unittest.cc
index c5c7e17..41dba07e 100644
--- a/chrome/browser/sharing/click_to_call/click_to_call_utils_unittest.cc
+++ b/chrome/browser/sharing/click_to_call/click_to_call_utils_unittest.cc
@@ -112,6 +112,26 @@
EXPECT_FALSE(ShouldOfferClickToCallForURL(&profile_, GURL(kNonTelUrl)));
}
+TEST_F(ClickToCallUtilsTest, TelLinkWithFragment) {
+ GURL fragment("tel:123#456");
+ EXPECT_TRUE(ShouldOfferClickToCallForURL(&profile_, fragment));
+ EXPECT_EQ("123", fragment.GetContent());
+}
+
+TEST_F(ClickToCallUtilsTest, TelLinkWithEncodedCharacters) {
+ // %23 == '#'
+ EXPECT_FALSE(ShouldOfferClickToCallForURL(&profile_, GURL("tel:123%23456")));
+ // %2A == '*'
+ EXPECT_FALSE(ShouldOfferClickToCallForURL(&profile_, GURL("tel:123%2A456")));
+ EXPECT_FALSE(ShouldOfferClickToCallForURL(&profile_, GURL("tel:123*456")));
+ // %25 == '%'
+ EXPECT_FALSE(ShouldOfferClickToCallForURL(&profile_, GURL("tel:123%25456")));
+
+ // %2B == '+'
+ EXPECT_TRUE(ShouldOfferClickToCallForURL(&profile_, GURL("tel:%2B44123")));
+ EXPECT_TRUE(ShouldOfferClickToCallForURL(&profile_, GURL("tel:+44123")));
+}
+
TEST_F(ClickToCallUtilsTest,
SelectionText_ValidPhoneNumberRegex_OfferForSelection) {
// Stores a mapping of selected text to expected phone number parsed.
@@ -187,3 +207,13 @@
EXPECT_EQ(base::nullopt,
ExtractPhoneNumberForClickToCall(&profile_, "+1234567890123456"));
}
+
+TEST_F(ClickToCallUtilsTest, IsUrlSafeForClickToCall) {
+ EXPECT_FALSE(IsUrlSafeForClickToCall(GURL("tel:123%23456")));
+ EXPECT_FALSE(IsUrlSafeForClickToCall(GURL("tel:123%2A456")));
+ EXPECT_FALSE(IsUrlSafeForClickToCall(GURL("tel:123*456")));
+ EXPECT_FALSE(IsUrlSafeForClickToCall(GURL("tel:123%25456")));
+
+ EXPECT_TRUE(IsUrlSafeForClickToCall(GURL("tel:%2B44123")));
+ EXPECT_TRUE(IsUrlSafeForClickToCall(GURL("tel:+44123")));
+}
diff --git a/chrome/browser/ui/views/sharing/click_to_call_browsertest.cc b/chrome/browser/ui/views/sharing/click_to_call_browsertest.cc
index 5e6cbaf..5c40bd5 100644
--- a/chrome/browser/ui/views/sharing/click_to_call_browsertest.cc
+++ b/chrome/browser/ui/views/sharing/click_to_call_browsertest.cc
@@ -109,7 +109,7 @@
menu->ExecuteCommand(IDC_CONTENT_CONTEXT_SHARING_CLICK_TO_CALL_SINGLE_DEVICE,
0);
CheckLastReceiver(*devices[0]);
- CheckLastSharingMessageSent(GetUnescapedURLContent(GURL(kTelUrl)));
+ CheckLastSharingMessageSent(GURL(kTelUrl).GetContent());
}
IN_PROC_BROWSER_TEST_F(ClickToCallBrowserTest, ContextMenu_NoDevicesAvailable) {
@@ -127,6 +127,42 @@
IDC_CONTENT_CONTEXT_SHARING_CLICK_TO_CALL_MULTIPLE_DEVICES));
}
+IN_PROC_BROWSER_TEST_F(ClickToCallBrowserTest, ContextMenu_UnsafeTelLink) {
+ Init(sync_pb::SharingSpecificFields::CLICK_TO_CALL_V2,
+ sync_pb::SharingSpecificFields::UNKNOWN);
+ auto devices = sharing_service()->GetDeviceCandidates(
+ sync_pb::SharingSpecificFields::CLICK_TO_CALL_V2);
+ ASSERT_EQ(1u, devices.size());
+
+ std::unique_ptr<TestRenderViewContextMenu> menu = InitContextMenu(
+ GURL("tel:%23*999%23"), kLinkText, kTextWithoutPhoneNumber);
+ EXPECT_FALSE(menu->IsItemPresent(
+ IDC_CONTENT_CONTEXT_SHARING_CLICK_TO_CALL_SINGLE_DEVICE));
+ EXPECT_FALSE(menu->IsItemPresent(
+ IDC_CONTENT_CONTEXT_SHARING_CLICK_TO_CALL_MULTIPLE_DEVICES));
+}
+
+IN_PROC_BROWSER_TEST_F(ClickToCallBrowserTest, ContextMenu_EscapedCharacters) {
+ Init(sync_pb::SharingSpecificFields::CLICK_TO_CALL_V2,
+ sync_pb::SharingSpecificFields::UNKNOWN);
+ auto devices = sharing_service()->GetDeviceCandidates(
+ sync_pb::SharingSpecificFields::CLICK_TO_CALL_V2);
+ ASSERT_EQ(1u, devices.size());
+
+ GURL phone_number("tel:%2B44%20123");
+ std::unique_ptr<TestRenderViewContextMenu> menu =
+ InitContextMenu(phone_number, kLinkText, kTextWithoutPhoneNumber);
+ ASSERT_TRUE(menu->IsItemPresent(
+ IDC_CONTENT_CONTEXT_SHARING_CLICK_TO_CALL_SINGLE_DEVICE));
+ EXPECT_FALSE(menu->IsItemPresent(
+ IDC_CONTENT_CONTEXT_SHARING_CLICK_TO_CALL_MULTIPLE_DEVICES));
+
+ menu->ExecuteCommand(IDC_CONTENT_CONTEXT_SHARING_CLICK_TO_CALL_SINGLE_DEVICE,
+ 0);
+ CheckLastReceiver(*devices[0]);
+ CheckLastSharingMessageSent(phone_number.GetContent());
+}
+
IN_PROC_BROWSER_TEST_F(ClickToCallBrowserTest,
ContextMenu_DevicesAvailable_SyncTurnedOff) {
if (base::FeatureList::IsEnabled(kSharingSendViaSync)) {
@@ -181,7 +217,7 @@
sub_menu_model->ActivatedAt(device_id);
CheckLastReceiver(*device);
- CheckLastSharingMessageSent(GetUnescapedURLContent(GURL(kTelUrl)));
+ CheckLastSharingMessageSent(GURL(kTelUrl).GetContent());
device_id++;
}
}
diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn
index 9011ef81..33a028d 100644
--- a/chrome/test/BUILD.gn
+++ b/chrome/test/BUILD.gn
@@ -4451,6 +4451,7 @@
"../browser/password_manager/android/update_password_infobar_delegate_android_unittest.cc",
"../browser/permissions/permission_prompt_android_unittest.cc",
"../browser/profiles/android/profile_resolver_unittest.cc",
+ "../browser/sharing/click_to_call/click_to_call_message_handler_android_unittest.cc",
"../browser/sharing/sms/sms_fetch_request_handler_unittest.cc",
"../browser/touch_to_fill/touch_to_fill_controller_unittest.cc",
"../browser/translate/translate_manager_render_view_host_android_unittest.cc",
diff --git a/tools/metrics/histograms/histograms_xml/sharing/histograms.xml b/tools/metrics/histograms/histograms_xml/sharing/histograms.xml
index 0491b0a..62a740a 100644
--- a/tools/metrics/histograms/histograms_xml/sharing/histograms.xml
+++ b/tools/metrics/histograms/histograms_xml/sharing/histograms.xml
@@ -79,6 +79,17 @@
</summary>
</histogram>
+<histogram name="Sharing.ClickToCallPhoneNumberValid" units="BooleanValid"
+ expires_after="M95">
+ <owner>knollr@chromium.org</owner>
+ <owner>peter@chromium.org</owner>
+ <summary>
+ Records if a received phone number is valid. Invalid numbers might suggest
+ that the remote device tried to send malicious data. Logged when handling a
+ Click to Call message on Android received from a Chrome desktop instance.
+ </summary>
+</histogram>
+
<histogram name="Sharing.ClickToCallSelectedAppIndex" units="index"
expires_after="2021-08-22">
<!-- Name completed by histogram_suffixes name="SharingClickToCallUi" -->
