diff --git a/cpp/src/phonenumbers/phonenumberutil.cc b/cpp/src/phonenumbers/phonenumberutil.cc index 3b969aeb2..f9fba465c 100644 --- a/cpp/src/phonenumbers/phonenumberutil.cc +++ b/cpp/src/phonenumbers/phonenumberutil.cc @@ -52,7 +52,6 @@ namespace i18n { namespace phonenumbers { using google::protobuf::RepeatedField; -using std::find; // static constants const size_t PhoneNumberUtil::kMinLengthForNsn; @@ -238,7 +237,7 @@ string CreateExtnPattern(const string& single_extn_symbols) { // remove_non_matches - indicates whether characters that are not able to be // replaced should be stripped from the number. If this is false, they will be // left unchanged in the number. -void NormalizeHelper(const map& normalization_replacements, +void NormalizeHelper(const std::map& normalization_replacements, bool remove_non_matches, string* number) { DCHECK(number); @@ -249,7 +248,7 @@ void NormalizeHelper(const map& normalization_replacements, for (UnicodeText::const_iterator it = number_as_unicode.begin(); it != number_as_unicode.end(); ++it) { - map::const_iterator found_glyph_pair = + std::map::const_iterator found_glyph_pair = normalization_replacements.find(*it); if (found_glyph_pair != normalization_replacements.end()) { normalized_number.push_back(found_glyph_pair->second); @@ -274,7 +273,7 @@ PhoneNumberUtil::ValidationResult TestNumberLength( RepeatedField local_lengths = phone_number_desc.possible_length_local_only(); int actual_length = number.length(); - if (find(local_lengths.begin(), local_lengths.end(), actual_length) != + if (std::find(local_lengths.begin(), local_lengths.end(), actual_length) != local_lengths.end()) { return PhoneNumberUtil::IS_POSSIBLE; } @@ -295,9 +294,10 @@ PhoneNumberUtil::ValidationResult TestNumberLength( // don't currently have an enum to express this, so we return TOO_LONG in the // short-term. // We skip the first element; we've already checked it. - return find(possible_lengths.begin() + 1, possible_lengths.end(), - actual_length) != possible_lengths.end() - ? PhoneNumberUtil::IS_POSSIBLE : PhoneNumberUtil::TOO_LONG; + return std::find(possible_lengths.begin() + 1, possible_lengths.end(), + actual_length) != possible_lengths.end() + ? PhoneNumberUtil::IS_POSSIBLE + : PhoneNumberUtil::TOO_LONG; } // Returns a new phone number containing only the fields needed to uniquely @@ -394,9 +394,9 @@ class PhoneNumberRegExpsAndMappings { alpha_mappings_.insert(std::make_pair(ToUnicodeCodepoint("X"), '9')); alpha_mappings_.insert(std::make_pair(ToUnicodeCodepoint("Y"), '9')); alpha_mappings_.insert(std::make_pair(ToUnicodeCodepoint("Z"), '9')); - map lower_case_mappings; - map alpha_letters; - for (map::const_iterator it = alpha_mappings_.begin(); + std::map lower_case_mappings; + std::map alpha_letters; + for (std::map::const_iterator it = alpha_mappings_.begin(); it != alpha_mappings_.end(); ++it) { // Convert all the upper-case ASCII letters to lower-case. @@ -474,24 +474,24 @@ class PhoneNumberRegExpsAndMappings { // A map that contains characters that are essential when dialling. That means // any of the characters in this map must not be removed from a number when // dialing, otherwise the call will not reach the intended destination. - map diallable_char_mappings_; + std::map diallable_char_mappings_; // These mappings map a character (key) to a specific digit that should // replace it for normalization purposes. - map alpha_mappings_; + std::map alpha_mappings_; // For performance reasons, store a map of combining alpha_mappings with ASCII // digits. - map alpha_phone_mappings_; + std::map alpha_phone_mappings_; // Separate map of all symbols that we wish to retain when formatting alpha // numbers. This includes digits, ascii letters and number grouping symbols // such as "-" and " ". - map all_plus_number_grouping_symbols_; + std::map all_plus_number_grouping_symbols_; // Map of country calling codes that use a mobile token before the area code. // One example of when this is relevant is when determining the length of the // national destination code, which should be the length of the area code plus // the length of the mobile token. - map mobile_token_mappings_; + std::map mobile_token_mappings_; // Set of country codes that have geographically assigned mobile numbers (see // geo_mobile_countries_ below) which are not based on *area codes*. For @@ -652,7 +652,7 @@ PhoneNumberUtil::PhoneNumberUtil() } // Storing data in a temporary map to make it easier to find other regions // that share a country calling code when inserting data. - map* > country_calling_code_to_region_map; + std::map* > country_calling_code_to_region_map; for (RepeatedPtrField::const_iterator it = metadata_collection.metadata().begin(); it != metadata_collection.metadata().end(); @@ -669,7 +669,7 @@ PhoneNumberUtil::PhoneNumberUtil() } else { region_to_metadata_map_->insert(std::make_pair(region_code, *it)); } - map* >::iterator calling_code_in_map = + std::map* >::iterator calling_code_in_map = country_calling_code_to_region_map.find(country_calling_code); if (calling_code_in_map != country_calling_code_to_region_map.end()) { if (it->main_country_for_code()) { @@ -679,7 +679,7 @@ PhoneNumberUtil::PhoneNumberUtil() } } else { // For most country calling codes, there will be only one region code. - list* list_with_region_code = new list(); + std::list* list_with_region_code = new std::list(); list_with_region_code->push_back(region_code); country_calling_code_to_region_map.insert( std::make_pair(country_calling_code, list_with_region_code)); @@ -2346,9 +2346,10 @@ void PhoneNumberUtil::GetNationalSignificantNumber( string* national_number) const { DCHECK(national_number); // If leading zero(s) have been set, we prefix this now. Note this is not a - // national prefix. + // national prefix. Ensure the number of leading zeros is at least 0 so we + // don't crash in the case of malicious input. StrAppend(national_number, number.italian_leading_zero() ? - string(number.number_of_leading_zeros(), '0') : ""); + string(std::max(number.number_of_leading_zeros(), 0), '0') : ""); StrAppend(national_number, number.national_number()); } diff --git a/cpp/test/phonenumbers/phonenumberutil_test.cc b/cpp/test/phonenumbers/phonenumberutil_test.cc index 571a51105..ba5c81a6f 100644 --- a/cpp/test/phonenumbers/phonenumberutil_test.cc +++ b/cpp/test/phonenumbers/phonenumberutil_test.cc @@ -300,6 +300,26 @@ TEST_F(PhoneNumberUtilTest, GetNationalSignificantNumber) { EXPECT_EQ("12345678", national_significant_number); } +TEST_F(PhoneNumberUtilTest, GetNationalSignificantNumber_ManyLeadingZeros) { + PhoneNumber number; + number.set_country_code(1); + number.set_national_number(650ULL); + number.set_italian_leading_zero(true); + number.set_number_of_leading_zeros(2); + string national_significant_number; + phone_util_.GetNationalSignificantNumber(number, + &national_significant_number); + EXPECT_EQ("00650", national_significant_number); + + // Set a bad value; we shouldn't crash, we shouldn't output any leading zeros + // at all. + number.set_number_of_leading_zeros(-3); + national_significant_number.clear(); + phone_util_.GetNationalSignificantNumber(number, + &national_significant_number); + EXPECT_EQ("650", national_significant_number); +} + TEST_F(PhoneNumberUtilTest, GetExampleNumber) { PhoneNumber de_number; de_number.set_country_code(49); diff --git a/java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java b/java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java index 90a969e25..4ce278100 100644 --- a/java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java +++ b/java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java @@ -1717,7 +1717,7 @@ public class PhoneNumberUtil { public String getNationalSignificantNumber(PhoneNumber number) { // If leading zero(s) have been set, we prefix this now. Note this is not a national prefix. StringBuilder nationalNumber = new StringBuilder(); - if (number.isItalianLeadingZero()) { + if (number.isItalianLeadingZero() && number.getNumberOfLeadingZeros() > 0) { char[] zeros = new char[number.getNumberOfLeadingZeros()]; Arrays.fill(zeros, '0'); nationalNumber.append(new String(zeros)); diff --git a/java/libphonenumber/test/com/google/i18n/phonenumbers/PhoneNumberUtilTest.java b/java/libphonenumber/test/com/google/i18n/phonenumbers/PhoneNumberUtilTest.java index faf11f9d5..706b8fccd 100644 --- a/java/libphonenumber/test/com/google/i18n/phonenumbers/PhoneNumberUtilTest.java +++ b/java/libphonenumber/test/com/google/i18n/phonenumbers/PhoneNumberUtilTest.java @@ -330,6 +330,19 @@ public class PhoneNumberUtilTest extends TestMetadataTestCase { assertEquals("12345678", phoneUtil.getNationalSignificantNumber(INTERNATIONAL_TOLL_FREE)); } + public void testGetNationalSignificantNumber_ManyLeadingZeros() { + PhoneNumber number = new PhoneNumber(); + number.setCountryCode(1); + number.setNationalNumber(650); + number.setItalianLeadingZero(true); + number.setNumberOfLeadingZeros(2); + assertEquals("00650", phoneUtil.getNationalSignificantNumber(number)); + + // Set a bad value; we shouldn't crash, we shouldn't output any leading zeros at all. + number.setNumberOfLeadingZeros(-3); + assertEquals("650", phoneUtil.getNationalSignificantNumber(number)); + } + public void testGetExampleNumber() { assertEquals(DE_NUMBER, phoneUtil.getExampleNumber(RegionCode.DE)); diff --git a/javascript/i18n/phonenumbers/phonenumberutil.js b/javascript/i18n/phonenumbers/phonenumberutil.js index 2ba32a62d..f1681ad41 100644 --- a/javascript/i18n/phonenumbers/phonenumberutil.js +++ b/javascript/i18n/phonenumbers/phonenumberutil.js @@ -2331,7 +2331,8 @@ i18n.phonenumbers.PhoneNumberUtil.prototype.getNationalSignificantNumber = // national prefix. /** @type {string} */ var nationalNumber = '' + number.getNationalNumber(); - if (number.hasItalianLeadingZero() && number.getItalianLeadingZero()) { + if (number.hasItalianLeadingZero() && number.getItalianLeadingZero() && + number.getNumberOfLeadingZerosOrDefault() > 0) { return Array(number.getNumberOfLeadingZerosOrDefault() + 1).join('0') + nationalNumber; } diff --git a/javascript/i18n/phonenumbers/phonenumberutil_test.js b/javascript/i18n/phonenumbers/phonenumberutil_test.js index 22e85205a..4fb8911a7 100644 --- a/javascript/i18n/phonenumbers/phonenumberutil_test.js +++ b/javascript/i18n/phonenumbers/phonenumberutil_test.js @@ -466,6 +466,21 @@ function testGetNationalSignificantNumber() { phoneUtil.getNationalSignificantNumber(INTERNATIONAL_TOLL_FREE)); } +function testGetNationalSignificantNumber_ManyLeadingZeros() { + /** @type {i18n.phonenumbers.PhoneNumber} */ + var number = new i18n.phonenumbers.PhoneNumber(); + number.setCountryCode(1); + number.setNationalNumber(650); + number.setItalianLeadingZero(true); + number.setNumberOfLeadingZeros(2); + assertEquals('00650', phoneUtil.getNationalSignificantNumber(number)); + + // Set a bad value; we shouldn't crash, we shouldn't output any leading zeros + // at all. + number.setNumberOfLeadingZeros(-3); + assertEquals("650", phoneUtil.getNationalSignificantNumber(number)); +} + function testGetExampleNumber() { var PNT = i18n.phonenumbers.PhoneNumberType; assertTrue(DE_NUMBER.equals(phoneUtil.getExampleNumber(RegionCode.DE))); diff --git a/pending_code_changes.txt b/pending_code_changes.txt index 8b1378917..d36789c5c 100644 --- a/pending_code_changes.txt +++ b/pending_code_changes.txt @@ -1 +1,4 @@ - +Code changes: + - Making getNationalSignificantNumber more robust in the face of malicious + input. This now ignores the number_of_leading_zeros if they are less than + zero.