Skip to content

Avoid catastrophic backtracking in the RFC3966 domainname check #4070

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shaggyinsomniac wants to merge 1 commit into
base: master
Choose a base branch
from
+42 −6
Open

Avoid catastrophic backtracking in the RFC3966 domainname check #4070

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions java/libphonenumber/src/com/google/i18n/phonenumbers/PhoneNumberUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,9 +328,9 @@ public class PhoneNumberUtil {
// defined in RFC3966.
private static final String ALPHANUM = VALID_ALPHA + DIGITS;
private static final String RFC3966_DOMAINLABEL =
"[" + ALPHANUM + "]+((\\-)*[" + ALPHANUM + "])*";
"[" + ALPHANUM + "]+(\\-+[" + ALPHANUM + "]+)*";
private static final String RFC3966_TOPLABEL =
"[" + VALID_ALPHA + "]+((\\-)*[" + ALPHANUM + "])*";
"[" + VALID_ALPHA + "]+(\\-+[" + ALPHANUM + "]+)*";
private static final String RFC3966_DOMAINNAME =
"^(" + RFC3966_DOMAINLABEL + "\\.)*" + RFC3966_TOPLABEL + "\\.?$";
static final Pattern RFC3966_DOMAINNAME_PATTERN = Pattern.compile(RFC3966_DOMAINNAME);
Expand Down
36 changes: 36 additions & 0 deletions java/libphonenumber/test/com/google/i18n/phonenumbers/PhoneNumberUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import java.util.regex.Pattern;
import org.junit.Assert;
import org.junit.function.ThrowingRunnable;
import org.mockito.Mockito;
Expand Down Expand Up @@ -2221,6 +2222,41 @@ public void testParseMaliciousInput() throws Exception {
}
}

public void testParseRFC3966PhoneContextDomainname() throws Exception {
// Domain phone-contexts keep parsing, including labels with single or repeated hyphens.
assertEquals(US_LOCAL_NUMBER,
phoneUtil.parse("tel:253-0000;phone-context=www.google.com", RegionCode.US));
assertEquals(US_LOCAL_NUMBER,
phoneUtil.parse("tel:253-0000;phone-context=ex--ample.domain.com", RegionCode.US));

// A long ambiguous domain is rejected. The old label pattern ([alnum]+((\-)*[alnum])*) made
// backtracking engines in the JS and Python ports of this pattern spend minutes on this
// input; Java's own matcher prunes it, this pins the rejection and the accepted language.
StringBuilder evilContext = new StringBuilder(100);
for (int i = 0; i < 25; i++) {
evilContext.append("aa.");
}
evilContext.append("aa!");
try {
phoneUtil.parse("tel:253-0000;phone-context=" + evilContext, RegionCode.US);
fail("This should not parse without throwing an exception.");
} catch (NumberParseException e) {
// Expected this exception.
}

// The rewritten label matches alnum runs separated by one or more hyphens, but not labels
// with leading/trailing hyphens or empty labels.
Pattern domainname = PhoneNumberUtil.RFC3966_DOMAINNAME_PATTERN;
assertTrue(domainname.matcher("a.b.c").matches());
assertTrue(domainname.matcher("ab-cd.ef").matches());
assertTrue(domainname.matcher("a--b.c").matches());
assertTrue(domainname.matcher("example.com.").matches());
assertFalse(domainname.matcher("aa.bb-").matches());
assertFalse(domainname.matcher("aa..bb").matches());
assertFalse(domainname.matcher("-aa.bb").matches());
assertFalse(domainname.matcher("a-.b").matches());
}

public void testParseWithInternationalPrefixes() throws Exception {
assertEquals(US_NUMBER, phoneUtil.parse("+1 (650) 253-0000", RegionCode.NZ));
assertEquals(INTERNATIONAL_TOLL_FREE, phoneUtil.parse("011 800 1234 5678", RegionCode.US));
Expand Down
8 changes: 4 additions & 4 deletions javascript/i18n/phonenumbers/phonenumberutil.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -784,17 +784,17 @@ i18n.phonenumbers.PhoneNumberUtil.ALPHANUM_ =
* @private
*/
i18n.phonenumbers.PhoneNumberUtil.RFC3966_DOMAINLABEL_ = '['
+ i18n.phonenumbers.PhoneNumberUtil.ALPHANUM_ + ']+((\\-)*['
+ i18n.phonenumbers.PhoneNumberUtil.ALPHANUM_ + '])*';
+ i18n.phonenumbers.PhoneNumberUtil.ALPHANUM_ + ']+(\\-+['
+ i18n.phonenumbers.PhoneNumberUtil.ALPHANUM_ + ']+)*';

/**
* @const
* @type {string}
* @private
*/
i18n.phonenumbers.PhoneNumberUtil.RFC3966_TOPLABEL_ = '['
+ i18n.phonenumbers.PhoneNumberUtil.VALID_ALPHA_ + ']+((\\-)*['
+ i18n.phonenumbers.PhoneNumberUtil.ALPHANUM_ + '])*';
+ i18n.phonenumbers.PhoneNumberUtil.VALID_ALPHA_ + ']+(\\-+['
+ i18n.phonenumbers.PhoneNumberUtil.ALPHANUM_ + ']+)*';

/**
* @const
Expand Down