Page MenuHomePhabricator

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

Authored by epriestley on Jan 18 2018, 8:05 PM.
Tags
None
Referenced Files
F15467316: D18873.id45264.diff
Thu, Apr 3, 2:03 PM
F15447925: D18873.id45264.diff
Fri, Mar 28, 2:26 AM
F15445061: D18873.id45265.diff
Thu, Mar 27, 12:23 PM
F15441458: D18873.id45264.diff
Wed, Mar 26, 6:19 PM
F15425486: D18873.id45264.diff
Sun, Mar 23, 4:48 AM
F15422711: D18873.id45265.diff
Sat, Mar 22, 10:16 AM
F15415351: D18873.diff
Thu, Mar 20, 5:32 AM
F15397871: D18873.id45265.diff
Sun, Mar 16, 10:43 PM
Subscribers
None

Details

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 of <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".

Diff Detail

Repository
rPHU libphutil
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

I think is probably clearer as-is instead of getting fancy with a regex.

This revision is now accepted and ready to land.Jan 18 2018, 8:09 PM

A vaguely related issue here is how we handle this in remarkup:

[[ http:/\evil.com/ | xyz ]]

We currently treat it as a wiki page, normalize it, and link to /w/evil.com/. This way this works is a little weird, but safe. If we treated it as a URI instead, browsers would accept it, since it seems that all browsers agree that this is a "perfectly valid" URI that has "absolutely nothing wrong with it".

I'm inclined to leave this behavior as-is for now. If we changed it in the future, I'd be inclined to explicitly reject these URIs as malformed.

This revision was automatically updated to reflect the committed changes.