Commit f2b4cc12 authored by Vaclav Brozek's avatar Vaclav Brozek Committed by Commit Bot

Bound the length of phone numbers parsed

Autofill's ParsePhoneNumber parses strings to extract valid phone
numbers. This is a considerable effort (including regex matching) and
a potential attack surface for malicious inputs.

Phone numbers are not arbitrarily long, so exceedingly large inputs
for ParsePhoneNumber signal an error or an attack. They are also
expensive to parse.

Thus, this CL introduces an upper bound on the input length, and the
parser will refuse inputs exceeding this bound.

As a side effect, the autofill_phone_number_i18n_fuzzer will stop
timing out on large inputs.

The CL also wraps the phone number unittest in the anonymous
namespace: This is to prevent potential name clashes for the newly
introduced GenerateTooLongString helper method, but is in general
a good thing to do in unittests, which are never meant to be
exported beyond their own file.

Bug: 901675
Change-Id: Ie3b069f846288ccd7f11fbce98c54669a55f980f
Reviewed-on: https://chromium-review.googlesource.com/c/1343007
Commit-Queue: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: default avatarRoger McFarlane <rogerm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609979}
parent 9fed0f06
......@@ -93,6 +93,8 @@ void FormatValidatedNumber(const ::i18n::phonenumbers::PhoneNumber& number,
namespace i18n {
const size_t kMaxPhoneNumberSize = 1000u;
// Returns true if |phone_number| is a possible number.
bool IsPossiblePhoneNumber(
const ::i18n::phonenumbers::PhoneNumber& phone_number) {
......@@ -137,6 +139,9 @@ bool ParsePhoneNumber(const base::string16& value,
number->clear();
*i18n_number = ::i18n::phonenumbers::PhoneNumber();
if (value.size() > kMaxPhoneNumberSize)
return false;
std::string number_text(base::UTF16ToUTF8(value));
// Parse phone number based on the region.
......
......@@ -25,6 +25,11 @@ class AutofillProfile;
// Utilities to process, normalize and compare international phone numbers.
namespace i18n {
// No reasonable phone number should need more than |kMaxPhoneNumberSize|
// characters. Longer inputs might be an error or an attack and processing them
// takes non-trivial time (parsing with regex), so will be ignored.
extern const size_t kMaxPhoneNumberSize;
// Return true if the given |phone_number| object is likely to be a phone number
// This method uses IsPossibleNumber from libphonenumber, instead of
// IsValidNumber. IsPossibleNumber does a less strict check, it will not try to
......
......@@ -61,6 +61,15 @@ struct ParseNumberTestCase {
std::string deduced_region;
};
namespace {
// Returns a string which is too long to be considered a phone number.
std::string GenerateTooLongString() {
return std::string(i18n::kMaxPhoneNumberSize + 1, '7');
}
} // namespace
class ParseNumberTest : public testing::TestWithParam<ParseNumberTestCase> {};
TEST_P(ParseNumberTest, ParsePhoneNumber) {
......@@ -90,6 +99,8 @@ INSTANTIATE_TEST_CASE_P(
// Test for string with less than 7 digits. Should give back empty
// strings.
ParseNumberTestCase{false, "1234", "US"},
// Too long strings should not be parsed.
ParseNumberTestCase{false, GenerateTooLongString(), "US"},
// Test for string with exactly 7 digits.
// Still a possible number with unknown("ZZ") deduced region.
ParseNumberTestCase{true, "17134567", "US", "7134567", "", "1", "ZZ"},
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment