From 6d88ee38ec98c500c4a596307ce6b3e83becd0e9 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) (cherry picked from commit 31459f94f2780e748e15d5c2951ba20adbba2366) --- 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 e172061ec2..80e164a08e 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 e468066a42..c9e9d32de0 100644 --- a/ext/standard/tests/url/parse_url_basic_001.phpt +++ b/ext/standard/tests/url/parse_url_basic_001.phpt @@ -507,15 +507,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 70dc4bb90b..431de27009 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 b2ca06ff96..b2c1a1d6dd 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 a9cc06b1c0..3bb62c7da3 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 745ba68440670440bdddd6cfb7e0f02eacef0f29 Mon Sep 17 00:00:00 2001 From: Remi Collet Date: Mon, 4 Jan 2021 14:20:55 +0100 Subject: [PATCH 2/2] NEWS (cherry picked from commit c784479182b92b9b3b96a7be42aa86a6c6d0b693) --- NEWS | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/NEWS b/NEWS index 47848d24b7..e328fd39c0 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 efb6c49f08314aca84733b0e83d72cd20c8e0015 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) (cherry picked from commit b5d4f109bab648c0d07273d2a52a5f2560e7832b) --- 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 a0fed76fce..22868fd8c1 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 80e164a08e..e172061ec2 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 c9e9d32de0..e468066a42 100644 --- a/ext/standard/tests/url/parse_url_basic_001.phpt +++ b/ext/standard/tests/url/parse_url_basic_001.phpt @@ -507,13 +507,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 431de27009..70dc4bb90b 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 b2c1a1d6dd..b2ca06ff96 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 3bb62c7da3..a9cc06b1c0 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