Unverified Commit 70157b9c authored by Timothy Gu's avatar Timothy Gu
Browse files

url: forbid certain confusable changes from being introduced by toASCII

The legacy url.parse() function attempts to convert Unicode domains
(IDNs) into their ASCII/Punycode form through the use of the toASCII
function. However, toASCII can introduce or remove various characters
that at best invalidate the parsed URL, and at worst cause hostname
spoofing:

  url.parse('http://bad.c℀.good.com/').href === 'http://bad.ca/c.good.com/'
  (from [1])
  url.parse('http://\u00AD/bad.com').href === 'http:///bad.com/'

While changes to the legacy URL parser are discouraged in general, the
security implications here outweigh the desire for strict compatibility.
This is since this commit only changes behavior when non-ASCII
characters appear in the hostname, an unusual situation for most use
cases. Additionally, despite the availability of the WHATWG URL API,
url.parse remain widely deployed in the Node.js ecosystem, as
exemplified by the recent un-deprecation of the legacy API.

This change is similar in spirit to CPython 3.8's change [2] fixing
bpo-36216 [3] aka CVE-2019-9636, which also occurred despite potential
compatibility concerns.

[1]: https://hackerone.com/reports/678487
[2]: https://github.com/python/cpython/commit/16e6f7dee7f02bb81aa6b385b982dcdda5b99286
[3]: https://bugs.python.org/issue36216

PR-URL: https://github.com/nodejs/node/pull/38631


Reviewed-By: default avatarJames M Snell <jasnell@gmail.com>
Reviewed-By: default avatarRich Trott <rtrott@gmail.com>
Reviewed-By: default avatarMatteo Collina <matteo.collina@gmail.com>
Reviewed-By: default avatarJoyee Cheung <joyeec9h3@gmail.com>
parent 41fab0d6
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment