From 31459f94f2780e748e15d5c2951ba20adbba2366 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Wed, 13 May 2020 09:36:52 +0200 Subject: [PATCH 1/2] Fix #77423: parse_url() will deliver a wrong host to user To avoid that `parse_url()` returns an erroneous host, which would be valid for `FILTER_VALIDATE_URL`, we make sure that only userinfo which is valid according to RFC 3986 is treated as such. For consistency with the existing url parsing code, we use ctype functions, although that is not necessarily correct. (cherry picked from commit 2d3d72412a6734e19a38ed10f385227a6238e4a6) --- ext/standard/tests/strings/url_t.phpt | 6 ++-- ext/standard/tests/url/bug77423.phpt | 30 +++++++++++++++++++ .../tests/url/parse_url_basic_001.phpt | 6 ++-- .../tests/url/parse_url_basic_003.phpt | 2 +- .../tests/url/parse_url_basic_005.phpt | 2 +- ext/standard/url.c | 21 +++++++++++++ 6 files changed, 57 insertions(+), 10 deletions(-) create mode 100644 ext/standard/tests/url/bug77423.phpt diff --git a/ext/standard/tests/strings/url_t.phpt b/ext/standard/tests/strings/url_t.phpt index 79ff3bc4a8..f564f59f06 100644 --- a/ext/standard/tests/strings/url_t.phpt +++ b/ext/standard/tests/strings/url_t.phpt @@ -575,15 +575,13 @@ $sample_urls = array ( string(16) "some_page_ref123" } ---> http://secret@hideout@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123: array(7) { +--> http://secret@hideout@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123: array(6) { ["scheme"]=> string(4) "http" ["host"]=> - string(11) "www.php.net" + string(26) "secret@hideout@www.php.net" ["port"]=> int(80) - ["user"]=> - string(14) "secret@hideout" ["path"]=> string(10) "/index.php" ["query"]=> diff --git a/ext/standard/tests/url/bug77423.phpt b/ext/standard/tests/url/bug77423.phpt new file mode 100644 index 0000000000..be03fe95e2 --- /dev/null +++ b/ext/standard/tests/url/bug77423.phpt @@ -0,0 +1,30 @@ +--TEST-- +Bug #77423 (parse_url() will deliver a wrong host to user) +--FILE-- + +--EXPECT-- +bool(false) +array(3) { + ["scheme"]=> + string(4) "http" + ["host"]=> + string(19) "php.net\@aliyun.com" + ["path"]=> + string(7) "/aaa.do" +} +bool(false) +array(2) { + ["scheme"]=> + string(5) "https" + ["host"]=> + string(26) "example.com\uFF03@bing.com" +} diff --git a/ext/standard/tests/url/parse_url_basic_001.phpt b/ext/standard/tests/url/parse_url_basic_001.phpt index 4606849c57..5101099132 100644 --- a/ext/standard/tests/url/parse_url_basic_001.phpt +++ b/ext/standard/tests/url/parse_url_basic_001.phpt @@ -506,15 +506,13 @@ echo "Done"; string(16) "some_page_ref123" } ---> http://secret@hideout@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123: array(7) { +--> http://secret@hideout@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123: array(6) { ["scheme"]=> string(4) "http" ["host"]=> - string(11) "www.php.net" + string(26) "secret@hideout@www.php.net" ["port"]=> int(80) - ["user"]=> - string(14) "secret@hideout" ["path"]=> string(10) "/index.php" ["query"]=> diff --git a/ext/standard/tests/url/parse_url_basic_003.phpt b/ext/standard/tests/url/parse_url_basic_003.phpt index 3d5a4a344a..7968fd3f09 100644 --- a/ext/standard/tests/url/parse_url_basic_003.phpt +++ b/ext/standard/tests/url/parse_url_basic_003.phpt @@ -68,7 +68,7 @@ echo "Done"; --> http://secret:@www.php.net/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(11) "www.php.net" --> http://:hideout@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(11) "www.php.net" --> http://secret:hideout@www.php.net/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(11) "www.php.net" ---> http://secret@hideout@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(11) "www.php.net" +--> http://secret@hideout@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(26) "secret@hideout@www.php.net" --> http://secret:hid:out@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(11) "www.php.net" --> nntp://news.php.net : string(12) "news.php.net" --> ftp://ftp.gnu.org/gnu/glic/glibc.tar.gz : string(11) "ftp.gnu.org" diff --git a/ext/standard/tests/url/parse_url_basic_005.phpt b/ext/standard/tests/url/parse_url_basic_005.phpt index aefb33964b..ba778bf903 100644 --- a/ext/standard/tests/url/parse_url_basic_005.phpt +++ b/ext/standard/tests/url/parse_url_basic_005.phpt @@ -68,7 +68,7 @@ echo "Done"; --> http://secret:@www.php.net/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(6) "secret" --> http://:hideout@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(0) "" --> http://secret:hideout@www.php.net/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(6) "secret" ---> http://secret@hideout@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(14) "secret@hideout" +--> http://secret@hideout@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : NULL --> http://secret:hid:out@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(6) "secret" --> nntp://news.php.net : NULL --> ftp://ftp.gnu.org/gnu/glic/glibc.tar.gz : NULL diff --git a/ext/standard/url.c b/ext/standard/url.c index 4cc1f77aec..d56dc24dc6 100644 --- a/ext/standard/url.c +++ b/ext/standard/url.c @@ -92,6 +92,22 @@ PHPAPI php_url *php_url_parse(char const *str) return php_url_parse_ex(str, strlen(str)); } +static int is_userinfo_valid(const char *str, size_t len) +{ + char *valid = "-._~!$&'()*+,;=:"; + char *p = str; + while (p - str < len) { + if (isalpha(*p) || isdigit(*p) || strchr(valid, *p)) { + p++; + } else if (*p == '%' && p - str <= len - 3 && isdigit(*(p+1)) && isxdigit(*(p+2))) { + p += 3; + } else { + return 0; + } + } + return 1; +} + /* {{{ php_url_parse */ PHPAPI php_url *php_url_parse_ex(char const *str, size_t length) @@ -235,13 +251,18 @@ PHPAPI php_url *php_url_parse_ex(char const *str, size_t length) ret->pass = estrndup(pp, (p-pp)); php_replace_controlchars_ex(ret->pass, (p-pp)); } else { + if (!is_userinfo_valid(s, p-s)) { + goto check_port; + } ret->user = estrndup(s, (p-s)); php_replace_controlchars_ex(ret->user, (p-s)); + } s = p + 1; } +check_port: /* check for port */ if (s < ue && *s == '[' && *(e-1) == ']') { /* Short circuit portscan, -- 2.29.2 From c784479182b92b9b3b96a7be42aa86a6c6d0b693 Mon Sep 17 00:00:00 2001 From: Remi Collet Date: Mon, 4 Jan 2021 14:20:55 +0100 Subject: [PATCH 2/2] NEWS --- NEWS | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/NEWS b/NEWS index bc49257774..1f538b891d 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,12 @@ PHP NEWS ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||| +Backported from 7.3.26 + +- Standard: + . Fixed bug #77423 (FILTER_VALIDATE_URL accepts URLs with invalid userinfo). + (CVE-2020-7071) (cmb) + Backported from 7.2.34 - Core: -- 2.29.2 From b5d4f109bab648c0d07273d2a52a5f2560e7832b Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Tue, 19 Jan 2021 11:23:25 +0100 Subject: [PATCH] Alternative fix for bug 77423 That bug report originally was about `parse_url()` misbehaving, but the security aspect was actually only regarding `FILTER_VALIDATE_URL`. Since the changes to `parse_url_ex()` apparently affect userland code which is relying on the sloppy URL parsing[1], this alternative restores the old parsing behavior, but ensures that the userinfo is checked for correctness for `FILTER_VALIDATE_URL`. [1] (cherry picked from commit 4a89e726bd4d0571991dc22a9a1ad4509e8fe347) (cherry picked from commit 9c673083cd46ee2a954a62156acbe4b6e657c048) (cherry picked from commit 356f7008f36da60ec9794d48c55d117f1dd31903) --- ext/filter/logical_filters.c | 25 +++++++++++++++++++ .../tests/url => filter/tests}/bug77423.phpt | 15 ----------- ext/standard/tests/strings/url_t.phpt | 6 +++-- .../tests/url/parse_url_basic_001.phpt | 6 +++-- .../tests/url/parse_url_basic_003.phpt | 2 +- .../tests/url/parse_url_basic_005.phpt | 2 +- ext/standard/url.c | 21 ---------------- 7 files changed, 35 insertions(+), 42 deletions(-) rename ext/{standard/tests/url => filter/tests}/bug77423.phpt (53%) diff --git a/ext/filter/logical_filters.c b/ext/filter/logical_filters.c index 52c9e285c4..1c51e6d2ef 100644 --- a/ext/filter/logical_filters.c +++ b/ext/filter/logical_filters.c @@ -514,6 +514,24 @@ void php_filter_validate_domain(PHP_INPUT_FILTER_PARAM_DECL) /* {{{ */ } /* }}} */ +static int is_userinfo_valid(char *str) +{ + const char *valid = "-._~!$&'()*+,;=:"; + const char *p = str; + size_t len = strlen(str); + + while (p - str < len) { + if (isalpha(*p) || isdigit(*p) || strchr(valid, *p)) { + p++; + } else if (*p == '%' && p - str <= len - 3 && isdigit(*(p+1)) && isxdigit(*(p+2))) { + p += 3; + } else { + return 0; + } + } + return 1; +} + void php_filter_validate_url(PHP_INPUT_FILTER_PARAM_DECL) /* {{{ */ { php_url *url; @@ -568,6 +586,13 @@ bad_url: php_url_free(url); RETURN_VALIDATION_FAILED } + + if (url->user != NULL && !is_userinfo_valid(url->user)) { + php_url_free(url); + RETURN_VALIDATION_FAILED + + } + php_url_free(url); } /* }}} */ diff --git a/ext/standard/tests/url/bug77423.phpt b/ext/filter/tests/bug77423.phpt similarity index 53% rename from ext/standard/tests/url/bug77423.phpt rename to ext/filter/tests/bug77423.phpt index be03fe95e2..761c7c359a 100644 --- a/ext/standard/tests/url/bug77423.phpt +++ b/ext/filter/tests/bug77423.phpt @@ -8,23 +8,8 @@ $urls = array( ); foreach ($urls as $url) { var_dump(filter_var($url, FILTER_VALIDATE_URL)); - var_dump(parse_url($url)); } ?> --EXPECT-- bool(false) -array(3) { - ["scheme"]=> - string(4) "http" - ["host"]=> - string(19) "php.net\@aliyun.com" - ["path"]=> - string(7) "/aaa.do" -} bool(false) -array(2) { - ["scheme"]=> - string(5) "https" - ["host"]=> - string(26) "example.com\uFF03@bing.com" -} diff --git a/ext/standard/tests/strings/url_t.phpt b/ext/standard/tests/strings/url_t.phpt index f564f59f06..79ff3bc4a8 100644 --- a/ext/standard/tests/strings/url_t.phpt +++ b/ext/standard/tests/strings/url_t.phpt @@ -575,13 +575,15 @@ $sample_urls = array ( string(16) "some_page_ref123" } ---> http://secret@hideout@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123: array(6) { +--> http://secret@hideout@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123: array(7) { ["scheme"]=> string(4) "http" ["host"]=> - string(26) "secret@hideout@www.php.net" + string(11) "www.php.net" ["port"]=> int(80) + ["user"]=> + string(14) "secret@hideout" ["path"]=> string(10) "/index.php" ["query"]=> diff --git a/ext/standard/tests/url/parse_url_basic_001.phpt b/ext/standard/tests/url/parse_url_basic_001.phpt index 5101099132..4606849c57 100644 --- a/ext/standard/tests/url/parse_url_basic_001.phpt +++ b/ext/standard/tests/url/parse_url_basic_001.phpt @@ -506,13 +506,15 @@ echo "Done"; string(16) "some_page_ref123" } ---> http://secret@hideout@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123: array(6) { +--> http://secret@hideout@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123: array(7) { ["scheme"]=> string(4) "http" ["host"]=> - string(26) "secret@hideout@www.php.net" + string(11) "www.php.net" ["port"]=> int(80) + ["user"]=> + string(14) "secret@hideout" ["path"]=> string(10) "/index.php" ["query"]=> diff --git a/ext/standard/tests/url/parse_url_basic_003.phpt b/ext/standard/tests/url/parse_url_basic_003.phpt index 7968fd3f09..3d5a4a344a 100644 --- a/ext/standard/tests/url/parse_url_basic_003.phpt +++ b/ext/standard/tests/url/parse_url_basic_003.phpt @@ -68,7 +68,7 @@ echo "Done"; --> http://secret:@www.php.net/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(11) "www.php.net" --> http://:hideout@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(11) "www.php.net" --> http://secret:hideout@www.php.net/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(11) "www.php.net" ---> http://secret@hideout@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(26) "secret@hideout@www.php.net" +--> http://secret@hideout@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(11) "www.php.net" --> http://secret:hid:out@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(11) "www.php.net" --> nntp://news.php.net : string(12) "news.php.net" --> ftp://ftp.gnu.org/gnu/glic/glibc.tar.gz : string(11) "ftp.gnu.org" diff --git a/ext/standard/tests/url/parse_url_basic_005.phpt b/ext/standard/tests/url/parse_url_basic_005.phpt index ba778bf903..aefb33964b 100644 --- a/ext/standard/tests/url/parse_url_basic_005.phpt +++ b/ext/standard/tests/url/parse_url_basic_005.phpt @@ -68,7 +68,7 @@ echo "Done"; --> http://secret:@www.php.net/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(6) "secret" --> http://:hideout@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(0) "" --> http://secret:hideout@www.php.net/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(6) "secret" ---> http://secret@hideout@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : NULL +--> http://secret@hideout@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(14) "secret@hideout" --> http://secret:hid:out@www.php.net:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(6) "secret" --> nntp://news.php.net : NULL --> ftp://ftp.gnu.org/gnu/glic/glibc.tar.gz : NULL diff --git a/ext/standard/url.c b/ext/standard/url.c index d56dc24dc6..4cc1f77aec 100644 --- a/ext/standard/url.c +++ b/ext/standard/url.c @@ -92,22 +92,6 @@ PHPAPI php_url *php_url_parse(char const *str) return php_url_parse_ex(str, strlen(str)); } -static int is_userinfo_valid(const char *str, size_t len) -{ - char *valid = "-._~!$&'()*+,;=:"; - char *p = str; - while (p - str < len) { - if (isalpha(*p) || isdigit(*p) || strchr(valid, *p)) { - p++; - } else if (*p == '%' && p - str <= len - 3 && isdigit(*(p+1)) && isxdigit(*(p+2))) { - p += 3; - } else { - return 0; - } - } - return 1; -} - /* {{{ php_url_parse */ PHPAPI php_url *php_url_parse_ex(char const *str, size_t length) @@ -251,18 +235,13 @@ PHPAPI php_url *php_url_parse_ex(char const *str, size_t length) ret->pass = estrndup(pp, (p-pp)); php_replace_controlchars_ex(ret->pass, (p-pp)); } else { - if (!is_userinfo_valid(s, p-s)) { - goto check_port; - } ret->user = estrndup(s, (p-s)); php_replace_controlchars_ex(ret->user, (p-s)); - } s = p + 1; } -check_port: /* check for port */ if (s < ue && *s == '[' && *(e-1) == ']') { /* Short circuit portscan, -- 2.29.2