Page MenuHomePhabricator
Create Task
Maniphest T12526

parse_url() behavior has changed with PHP7, causing libphutil unit tests to fail and possibly creating security concerns
Closed, ResolvedPublic
Actions

Description

Some of the PhutilURITestCase::testURIParsing tests currently fail on PHP7.1 because of this fix:

https://bugs.php.net/bug.php?id=73192

However, it's possible that this fix actually creates danger if cURL has different behavior. See T6755 for context. We should:

  • Fix these tests, possibly by rejecting these URIs on both versions of PHP with an exception.
  • Definitely make PhutilURI start throwing exceptions on this junk if some versions of PHP and some versions of cURL could conspire to behave unsafely.

Revisions and Commits

rPHU libphutil
D18666rPHUf7b0855acf5a (stable) Fix an exception in the hyperlink remarkup rule for unparseable URIs
D18666rPHU20db87e17679 Fix an exception in the hyperlink remarkup rule for unparseable URIs
D17647rPHUfb9e0642c4ea Reject ambiguous URIs with unescaped "#" or "?" in username/password parts

Related Objects
Search...

Event Timeline

epriestley created this task.Apr 10 2017, 1:26 PM
Herald added subscribers: cburroughs, andypuettmann, eadler, revi. · View Herald TranscriptApr 10 2017, 1:26 PM
epriestley added a parent task: T12101: Phabricator PHP 7 Compatibility.Apr 10 2017, 3:46 PM
epriestley added a comment.Apr 10 2017, 4:26 PM

I think this is, to at least some degree, a legitimate security problem. Consider:

http://good.com#u:p@evil.com/
ClientDomain
Safarigood.com
cURL CLIevil.com
PHP < 7.1evil.com
PHP >= 7.1good.com
Chromegood.com
Firefoxgood.com
wgetgood.com

I think we just need to detect anything in this form (and the similar form, http://good.com?u:p@evil.com -- with ? instead of #) and reject it. The unambiguous way to write this is either good.com/? (query string) or good.com%3A (username).

epriestley added a revision: D17647: Reject ambiguous URIs with unescaped "#" or "?" in username/password parts.Apr 10 2017, 4:54 PM
epriestley closed this task as Resolved by committing rPHUfb9e0642c4ea: Reject ambiguous URIs with unescaped "#" or "?" in username/password parts.Apr 10 2017, 5:34 PM
epriestley added a commit: rPHUfb9e0642c4ea: Reject ambiguous URIs with unescaped "#" or "?" in username/password parts.
jcarrillo7 mentioned this in T12867: Youtube remarkup rule fails to parse "ambiguous URI".Jun 23 2017, 6:07 AM
bcooksley added a subscriber: bcooksley.Sep 30 2017, 9:27 PM

This change creates a slight problem for us at KDE as we have some historical commits in our Subversion repository which have URLs in them which are invalid. This exception means that:

  • Herald spins forever trying to process these commits, failing every single time (currently up to 855 failures).
  • The commit can't be viewed in the browser (See https://phabricator.kde.org/R883:271607)

Would it be acceptable to change PhutilRemarkupHyperlinkRule to catch this exception and behave as if it were a non-whitelisted protocol? (Happy to draft the patch, just want to check before I do so)

epriestley added a comment.Sep 30 2017, 10:32 PM

Would it be acceptable to change PhutilRemarkupHyperlinkRule to catch this exception and behave as if it were a non-whitelisted protocol? (Happy to draft the patch, just want to check before I do so)

Yep, see D18149 and D18076 for similar changes.

epriestley added a comment.Sep 30 2017, 10:35 PM

(I'll fix this if you don't get there first, but have used up all my brain energy for today. And thanks for the report!)

epriestley added a revision: D18666: Fix an exception in the hyperlink remarkup rule for unparseable URIs.Oct 2 2017, 2:14 PM
epriestley added a commit: rPHU20db87e17679: Fix an exception in the hyperlink remarkup rule for unparseable URIs.Oct 2 2017, 4:10 PM
epriestley added a commit: rPHUf7b0855acf5a: (stable) Fix an exception in the hyperlink remarkup rule for unparseable URIs.
bcooksley added a comment.Oct 3 2017, 8:40 AM

Not a problem - thanks for fixing that so quickly.
I can confirm that fixed the issue for us.

Phabricator · Built by Phacility