Skip to content

Commit 4a89e72

Browse files
cmb69smalyshev
cmb69
authored and
smalyshev
committed
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] <php@5174de7#commitcomment-45967652>
1 parent 491488d commit 4a89e72

File tree

8 files changed

+38
-28
lines changed

8 files changed

+38
-28
lines changed

‎ext/filter/logical_filters.c

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -532,6 +532,22 @@ void php_filter_validate_domain(PHP_INPUT_FILTER_PARAM_DECL) /* {{{ */
532532
}
533533
/* }}} */
534534

535+
static int is_userinfo_valid(zend_string *str)
536+
{
537+
const char *valid = "-._~!$&'()*+,;=:";
538+
const char *p = ZSTR_VAL(str);
539+
while (p - ZSTR_VAL(str) < ZSTR_LEN(str)) {
540+
if (isalpha(*p) || isdigit(*p) || strchr(valid, *p)) {
541+
p++;
542+
} else if (*p == '%' && p - ZSTR_VAL(str) <= ZSTR_LEN(str) - 3 && isdigit(*(p+1)) && isxdigit(*(p+2))) {
543+
p += 3;
544+
} else {
545+
return 0;
546+
}
547+
}
548+
return 1;
549+
}
550+
535551
void php_filter_validate_url(PHP_INPUT_FILTER_PARAM_DECL) /* {{{ */
536552
{
537553
php_url *url;
@@ -592,6 +608,13 @@ void php_filter_validate_url(PHP_INPUT_FILTER_PARAM_DECL) /* {{{ */
592608
php_url_free(url);
593609
RETURN_VALIDATION_FAILED
594610
}
611+
612+
if (url->user != NULL && !is_userinfo_valid(url->user)) {
613+
php_url_free(url);
614+
RETURN_VALIDATION_FAILED
615+
616+
}
617+
595618
php_url_free(url);
596619
}
597620
/* }}} */

‎ext/standard/tests/url/bug77423.phpt renamed to ‎ext/filter/tests/bug77423.phpt

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -8,23 +8,8 @@ $urls = array(
88
);
99
foreach ($urls as $url) {
1010
var_dump(filter_var($url, FILTER_VALIDATE_URL));
11-
var_dump(parse_url($url));
1211
}
1312
?>
1413
--EXPECT--
1514
bool(false)
16-
array(3) {
17-
["scheme"]=>
18-
string(4) "http"
19-
["host"]=>
20-
string(19) "php.net\@aliyun.com"
21-
["path"]=>
22-
string(7) "/aaa.do"
23-
}
2415
bool(false)
25-
array(2) {
26-
["scheme"]=>
27-
string(5) "https"
28-
["host"]=>
29-
string(26) "example.com\[email protected]"
30-
}

‎ext/standard/tests/strings/url_t.phpt

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -575,13 +575,15 @@ $sample_urls = array (
575575
string(16) "some_page_ref123"
576576
}
577577

578-
--> http://secret@[email protected]:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123: array(6) {
578+
--> http://secret@[email protected]:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123: array(7) {
579579
["scheme"]=>
580580
string(4) "http"
581581
["host"]=>
582-
string(26) "secret@hideout@www.php.net"
582+
string(11) "www.php.net"
583583
["port"]=>
584584
int(80)
585+
["user"]=>
586+
string(14) "secret@hideout"
585587
["path"]=>
586588
string(10) "/index.php"
587589
["query"]=>

‎ext/standard/tests/url/parse_url_basic_001.phpt

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -506,13 +506,15 @@ echo "Done";
506506
string(16) "some_page_ref123"
507507
}
508508

509-
--> http://secret@[email protected]:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123: array(6) {
509+
--> http://secret@[email protected]:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123: array(7) {
510510
["scheme"]=>
511511
string(4) "http"
512512
["host"]=>
513-
string(26) "secret@hideout@www.php.net"
513+
string(11) "www.php.net"
514514
["port"]=>
515515
int(80)
516+
["user"]=>
517+
string(14) "secret@hideout"
516518
["path"]=>
517519
string(10) "/index.php"
518520
["query"]=>

‎ext/standard/tests/url/parse_url_basic_003.phpt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ echo "Done";
6868
--> http://secret:@www.php.net/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(11) "www.php.net"
6969
--> http://:[email protected]:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(11) "www.php.net"
7070
--> http://secret:[email protected]/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(11) "www.php.net"
71-
--> http://secret@[email protected]:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(26) "secret@hideout@www.php.net"
71+
--> http://secret@[email protected]:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(11) "www.php.net"
7272
--> http://secret:hid:[email protected]:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(11) "www.php.net"
7373
--> nntp://news.php.net : string(12) "news.php.net"
7474
--> ftp://ftp.gnu.org/gnu/glic/glibc.tar.gz : string(11) "ftp.gnu.org"

‎ext/standard/tests/url/parse_url_basic_005.phpt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ echo "Done";
6868
--> http://secret:@www.php.net/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(6) "secret"
6969
--> http://:[email protected]:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(0) ""
7070
--> http://secret:[email protected]/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(6) "secret"
71-
--> http://secret@[email protected]:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : NULL
71+
--> http://secret@[email protected]:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(14) "secret@hideout"
7272
--> http://secret:hid:[email protected]:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123 : string(6) "secret"
7373
--> nntp://news.php.net : NULL
7474
--> ftp://ftp.gnu.org/gnu/glic/glibc.tar.gz : NULL

‎ext/standard/tests/url/parse_url_unterminated.phpt

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -508,13 +508,15 @@ echo "Done";
508508
string(16) "some_page_ref123"
509509
}
510510

511-
--> http://secret@[email protected]:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123: array(6) {
511+
--> http://secret@[email protected]:80/index.php?test=1&test2=char&test3=mixesCI#some_page_ref123: array(7) {
512512
["scheme"]=>
513513
string(4) "http"
514514
["host"]=>
515-
string(26) "secret@hideout@www.php.net"
515+
string(11) "www.php.net"
516516
["port"]=>
517517
int(80)
518+
["user"]=>
519+
string(14) "secret@hideout"
518520
["path"]=>
519521
string(10) "/index.php"
520522
["query"]=>

‎ext/standard/url.c

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -260,17 +260,13 @@ PHPAPI php_url *php_url_parse_ex2(char const *str, size_t length, zend_bool *has
260260
ret->pass = zend_string_init(pp, (p-pp), 0);
261261
php_replace_controlchars_ex(ZSTR_VAL(ret->pass), ZSTR_LEN(ret->pass));
262262
} else {
263-
if (!is_userinfo_valid(s, p-s)) {
264-
goto check_port;
265-
}
266-
ret->user = zend_string_init(s, (p-s), 0);
263+
ret->user = zend_string_init(s, (p-s), 0);
267264
php_replace_controlchars_ex(ZSTR_VAL(ret->user), ZSTR_LEN(ret->user));
268265
}
269266

270267
s = p + 1;
271268
}
272269

273-
check_port:
274270
/* check for port */
275271
if (s < ue && *s == '[' && *(e-1) == ']') {
276272
/* Short circuit portscan,

0 commit comments

Comments
 (0)