HomePhabricator

Expand detection of protocol-relative hrefs to cover "/\evil.com" and variants

Description

Expand detection of protocol-relative hrefs to cover "/\evil.com" and variants

Summary:
Via HackerOne. See https://hackerone.com/reports/306414.

All browsers seem to accept <a href="/\evil.com" ...> as equivalent to //evil.com, i.e. a protocol-relative URI (uses "http" or "https" depending on the protocol for the current page).

They're equally accepting fo <a href="/\/\\\\///\/\evil.com" ...> and so on, too.

I assume this is because normal users don't know the difference between slash and backslash, and someone in the 1990's thought this was a reaonable usability affordance and we're stuck with it forever?

Note that we've encountered this problem previously in PhabricatorEnv->isValidLocalURIForLink(), although the scope and context are a little different there. In that case, we simply reject URIs with backslashes anywhere.

The security risk associated with this -- "tabnabbing" -- is fairly low, and amounts to a targeted phishing attack, so I don't plan to take any exceptional security measures here.

Test Plan:

  • Pre-change: wrote a [[ /\evil.com | xyz ]] link in Remarkup, saw it fail to get "noreferrer". Saw Chrome, Firefox and Safari all dutifully follow the link to evil.com.
  • Made change; added test coverage.
  • Tests pass.
  • Post-change: Verified /\evil.com link (and other similar links, e.g., ///evil.com, /\\evil.com) now get "noreferrer".

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18873

Details

Provenance
epriestleyAuthored on Jan 18 2018, 7:58 PM
epriestleyPushed on Jan 18 2018, 8:17 PM
Reviewer
amckinley
Differential Revision
D18873: Expand detection of protocol-relative hrefs to cover "/\evil.com" and variants
Parents
rPHU59642f236822: Mark `ParseError` as a PHP7 builtin class
Branches
Unknown
Tags
Unknown
Build Status
Buildable 19048
Build 25702: Run Core Tests