getNationalSignificantNumber robustness improvement (#1576)

Fix for getNationalSignificantNumber to be robust in the face of malicious input (negative number of leading zeros in the phone number proto).

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<char32, char>& normalization_replacements,
+void NormalizeHelper(const std::map<char32, char>& normalization_replacements,
                      bool remove_non_matches,
                      string* number) {
   DCHECK(number);
@@ -249,7 +248,7 @@ void NormalizeHelper(const map<char32, char>& normalization_replacements,
   for (UnicodeText::const_iterator it = number_as_unicode.begin();
        it != number_as_unicode.end();
        ++it) {
-    map<char32, char>::const_iterator found_glyph_pair =
+    std::map<char32, char>::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<int> 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<char32, char> lower_case_mappings;
-    map<char32, char> alpha_letters;
-    for (map<char32, char>::const_iterator it = alpha_mappings_.begin();
+    std::map<char32, char> lower_case_mappings;
+    std::map<char32, char> alpha_letters;
+    for (std::map<char32, char>::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<char32, char> diallable_char_mappings_;
+  std::map<char32, char> diallable_char_mappings_;
   // These mappings map a character (key) to a specific digit that should
   // replace it for normalization purposes.
-  map<char32, char> alpha_mappings_;
+  std::map<char32, char> alpha_mappings_;
   // For performance reasons, store a map of combining alpha_mappings with ASCII
   // digits.
-  map<char32, char> alpha_phone_mappings_;
+  std::map<char32, char> 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<char32, char> all_plus_number_grouping_symbols_;
+  std::map<char32, char> 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<int, char> mobile_token_mappings_;
+  std::map<int, char> 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<int, list<string>* > country_calling_code_to_region_map;
+  std::map<int, list<string>* > country_calling_code_to_region_map;
   for (RepeatedPtrField<PhoneMetadata>::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<int, list<string>* >::iterator calling_code_in_map =
+    std::map<int, list<string>* >::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<string>* list_with_region_code = new list<string>();
+      std::list<string>* list_with_region_code = new std::list<string>();
       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());
 }
 

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);

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));

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));
 

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;
   }

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)));

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.